Simple sketch and simple question: where does this sketch have undefined behavior?
Note: I don't need a solution to fix the code. I already have code that works. I really need to understand where my thinking is wrong.
Background: some kids were writing a program for a line follower robot which has 3 digital sensors. Many of them bricked their Arduino. I reduced the affected sketches to quite a minimum so that it still reproduces the problem. The sketch is pretty self-explanatory I hope.
It gives no warnings in Arduino IDE 1.8.9, 1.8.15 and 2.0.4 even with all warnings enabled.
Still, this code bricks all the Arduino clones (9 instances of Elegoo Smart Robot Car Kit 3.0 Plus) and I spent several hours to find a reliable procedure to unbrick them. Therefore I have strong belief that this code must have undefined behavior - except that I can't see it.
The unbrick procedure is:
- Remove the shield, if not removed yet
- Disconnect Arduino from USB
- Connect Arduino to USB
- Press Reset button on the Arduino board
- Disconnect Arduino from USB again (!)
- Connect Arduino to USB
- Compile and upload an empty sketch
long PIN_LINE_LEFT = 10;
long PIN_LINE_MID = 4;
long PIN_LINE_RIGHT = 2;
long DATARATE = 115200;
void setup() {
pinMode(PIN_LINE_LEFT, INPUT);
pinMode(PIN_LINE_MID, INPUT);
pinMode(PIN_LINE_RIGHT, INPUT);
Serial.begin(DATARATE);
}
void loop() {
byte leftValue = digitalRead(PIN_LINE_LEFT);
Serial.println(leftValue);
}
Here's my analysis:
IMHO, the sketch is unusual just in one way: instead of #define
ing the pin numbers and other stuff, it declares them as global variables of type long
.
According to the Arduino reference about data types a long
is 32 bit. Printing sizeof(long)
confirms that. Therefore, 115200 will not overflow at 65536 (16 Bit).
For Serial.begin()
, the DATARATE
will be converted into an unsigned long
, which IMHO should not be a problem here, since the number is positive.
Sure enough, the pin numbers needn't be long
and should be declared as uint8_t
to better match the pinMode()
and digitalRead()
function. But I still think the implicit conversion down to uint8_t
should be fine, since there is no overflow either.
Next, digitalRead()
officially returns an int
. The result is assigned to a byte
which is an alias for uint8_t
. It seems that this also gets converted implicitly. However, the values should only be HIGH
or LOW
, i.e. 1 or 0, which does not generate a signed overflow either.
What I tried:
Changing DATARATE
to the more appropriate unsigned long DATARATE = 115200UL;
doesn't change anything.
Changing byte leftValue = digitalRead(PIN_LINE_LEFT);
to the more appropriate int leftValue = digitalRead(PIN_LINE_LEFT);
also didn't change anything.
Changing long PIN_LINE_MID = 4;
and all others to the more appropriate uint8_t PIN_LINE_xxx = xxx;
also doesn't change anything.
IMHO with these changes, there's basically nothing left to fix and this should be a super valid C/C++ program without any implicit dangerous casts.
A Serial.flush();
in the loop is not needed according to the documentation of Serial.write()
. So there will not be a buffer overflow.