Fw_ver_13.ino: some potential issues


#1

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


#2

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.

-Karan


#3

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


#4

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