Fw_ver_13.ino: some potential issues

line 119: the variable “flag” is set but is never used
line 292: the comment about the dangers of cmd[0] should be completed :slight_smile:
line 386: should set payload[1]=0; before sending it back
line 412: this will never set OUTPUT mode
line 414: probably should have “cmd[0] = 0;” here
line 417: the PID logic will get skipped if the user issues another cmd after the motors are started e.g. read encoders or change the LEDs. Should not cmd[0] since it is volatile.
lines 723,755: When tst_flg==1, hitting the encoder target value will clobber cmd[0]. This could break the enc_read and the three trim_xxx commands which haven’t been tested for yet.
lines 859,873: buffer overrun is possible if byteCount is anything except 4

Hey shansen43,
Those are great comments for the GoPiGo firmware. It does look like the firmware has got bloated over time and a lot of bugs are in there.

For line 119: I think the flag was there was some purpose earlier but we stopped using it. Will remove it in the next firmware update.

line 292: Will update the documentation for cmd[0]

line 386: payload[1] has the lower byte for the data from the distance sensor since it can be greater than 255. Am I missing anything and is there any other reason why it should be 0.

line 412: It should have been 1 in there. I’ll get that updated. Thanks for looking into it

line 414: Yeah, com[0]=0 has to be there too. Will add it

line 417: the PID logic has been faltering in the past and that might be one of the reasons. I am going to rework on the logic there and that this condition into consideration too.

line 723,755: I’ll make the changes so that encoder targeting does not disrupt other functions

line 859,873: There will be a buffer overrun but that was a compromise that we had to take. If we had too much code in the I2C ISR, then the performance got degraded. So we just kept 4 as the default message size and make sure that nothing is sent to the GoPiGo that is not in the proper message format. We’ll add a note on this in the firmware so that people don;t make mistakes.

Thanks again for all the comments. It rarely happens that someone goes through the code in such great detail and I really appreciate your effort and comments. I really feel that more feedback would help us make the product better. Feel free to post any more feedback about the code and anything else about our products.


Hi Karan
for line 368: If I read it right, we are sending back two bytes for digital_read - payload[0] is the current value read from the given pin and payload[1] is unspecified

Sorry, I thought it was line 386. Yeah the digitalRead whould have payload[1]=0.