Posted 30 May 2024

While looking through the code for another reason, I discovered that I have committed the mortal sins of using the same name for a global variable, a local variable and a function definition parameter. Originally I defined global variables gl_Leftspeednum & gl_Rightspeednum thusly:

But then some years later in my code I see:

They’re everywhere! yikes!

So, what to do? The original (bad) idea was to have these variables ‘global’ so any part of the code could ‘see’ the current motor speeds. This was BAD because that also meant that any part of the code could change the motor speed (even if it shouldn’t) , and figuring out who did that would be a nightmare. This is where I should have started thinking about building a ‘motor’ class to hide all this – but I didn’t, so….

Also, using a global symbol name in a function definition is at least moronic if not suicidally stupid – does that overwrite the original declaration? To add insult to injury, the function definitions above use ‘int’ as the type rather than ‘uint_16’, so does that mean that motor speed can be negative, but just inside that function – ouch, my head hurts!

Alright – since I didn’t do the right thing and encapsulate this stuff in a motor class, and I don’t want to have to rewrite the entire 7K+ line program (at least not yet), I need to figure out a short-term non-idiotic fix (or maybe just close my eyes and have another beer?)

OK, so the functions involved in this debacle are:

• void SetLeftMotorDirAndSpeed(bool bIsFwd, int speed)
• void SetRightMotorDirAndSpeed(bool bIsFwd, int speed)
• void RunBothMotors(bool bisFwd, int gl_Leftspeednum, int gl_Rightspeednum)
• RunBothMotorsBidirectional(int leftspeed, int rightspeed)
• void RunBothMotorsMsec(bool bisFwd, int timeMsec = 500, int gl_Leftspeednum = MOTOR_SPEED_HALF, int gl_Rightspeednum = MOTOR_SPEED_HALF)
• void RunBothMotorsMsec(bool bisFwd, int timeMsec, int gl_Leftspeednum, int gl_Rightspeednum)
• void MoveReverse(int gl_Leftspeednum, int gl_Rightspeednum)
• void MoveAhead(int gl_Leftspeednum, int gl_Rightspeednum)

SetLeft/RightMotorDirAndSpeed(bool bIsFwd, int speed):

This declaration should probably be (bool, uint16_t) as negative speed values aren’t allowed. I changed the speed declaration from ‘int’ to ‘uint16_t’ and the program still compiles OK. The ‘speed’ argument gets passed to ‘AnalogWrite’ which is declared as AnalogWrite(int pin, int value).

RunBothMotors(bool bisFwd, int gl_Leftspeednum, int gl_Rightspeednum):

RunBothMotors() is called just once in the code, by RunBothMotorsMsec(). RunBothMotorsMsec() in turn is called just four times – three times by HandleExcessSteervalCase() and once by RunToDaylight(). In all four cases the speed arguments are positive constant integers <= 1000 (Teensy analog output resolution is set at 12 bits –>4096). It looks like RunBothMotors() and RunBothMotorsMsec() should declare their speed arguments to be uint16_t

RunBothMotorsBidirectional(int leftspeed, int rightspeed)

RunBothMotorsBidirectional(int leftspeed, int rightspeed) just calls SetLeftMotorDirAndSpeed() however, the speed arguments can be positive or negative, so the ‘int’ declaration is required in this case. The sign of the speed input argument is converted to the appropriate direction flag value and a negative input speed is converted to a positive value for the SetLeftMotorDirAndSpeed() call.

RunBothMotorsMsec(bool bisFwd, int timeMsec, int gl_Leftspeednum, int gl_Rightspeednum)

All this function does is call RunBothMotors(), then delay for the requested amount of time, then stop the motors. Note that RunBothMotors() does not check the speed arguments for range or sign.

MoveReverse(int gl_Leftspeednum, int gl_Rightspeednum):

MoveReverse() is used extensively in ‘CheckForUserInput()’, but only twice elsewhere ( both times in IRHomeToChgStn()).

MoveAhead(int gl_Leftspeednum, int gl_Rightspeednum):

Similar to MoveReverse(), but used more outside ‘CheckForUserInput()’. Once in ExecuteRearObstacleRecovery(), once in TrackLeftWallOffset(), once in TrackRightWallOffset(), once in IRHomeToChgStnNoPings(), once in IRHomeToChgStnNoPingsPID(), twice in IRHomeToChgStn().

int gl_Leftspeednum, int gl_Rightspeednum:

These symbols are everywhere in the code, in a global variable declaration, in the signature of many of the motor functions, and in the code itself as local variables in the functions that have those symbols in the signature.

As an experiment I commented the global uint16_t definitions out and re-compiled. I got a bunch of ‘was not declared in this scope’ errors, but they were all like the following snippit:

in the above code a local int16_t variable is declared because the result could be negative. Then the local variables are constrained into the range (0-255) and then loaded into the gl_Left/Rightspeednum global vars, and also passed to MoveAhead(). This only occurs in the two TrackLeft/RightWallOffset() functions.

gl_Left/Rightspeednum global vars are also used in the ‘OutputTelemetryLine()’ function

So it looks like the usage in the above snippet is actually OK. The global vars wind up being loaded with the latest left/right speed values just before those values are sent to the motor driver. The usage in the telemetry output functions are also OK, as they just print the current left/right speed value

gl_Left/Rightspeednum used in function declarations:

I re-educated myself on the fact that formal function declarations don’t actually need parameter names – just the type declarations, so:

could just as easily be written:

so maybe my use of the gl_Left/Rightspeednum names for these parameters wasn’t quite so scary bad as I thought. Still, defining the same symbol name in two different contexts as two different types (uint16_t and int) is demonstrably a bad idea, even if one of the symbol usages is ignored by the compiler (after all, this usage is what resulted in my current freakout). I changed these to ‘uint16_t leftspeednum’ and ‘uint16_t rightspeednum, in both the formal declaration at the top of the program (reqd for default parameter declaration) and the ‘inline’ declaration.

I wound up changing the following lines:

In addition, there are a number of places where the output from the PIDCalcs() function is added to or subtracted from the current speed to produce the next speed value, but the initial adjustment is to a ‘uint16_t’ variable. This is problematic because the initial adjustment can result in a negative value being loaded into a uint16_t variable, with unexpected (if still well-defined) behavior. The fix for this is to change the type of the ‘local’ variable to ‘int’ vs ‘uint16_t’ to accommodate the potential for negative values, and only load the result into the global ‘uint16_t’ variable when it is certain the result is positive. This resulted in the following changes:

After all these edits, the program still compiles cleanly. As to whether or not it behaves cleanly, that is still a very open questions. Only time will tell!

Stay tuned,

Frank