[grbl Issue#1249] eeprom checksum bug?

#1 – X3msnake 于 2017-06-14

|| is the Symbol for OR bolean Operation.

2017-06-14 9:17 GMT+01:00 ericQiang :

> int file eeprom.c ,line 144
>
> checksum = (checksum << 1) || (checksum >> 7);
> if “||” should be “|”?
> if not, what mean does this line?
> thanks.
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/grbl/grbl/issues/1249>, or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AKke-itgKCSicQw1O0n_QhZcvI1aYWviks5sD5b-gaJpZM4N5iTb>
> .
>

—
Com os melhores cumprimentos,
Vinicius Silva

#2 – ericQiang 于 2017-06-14

then, in the function:

int memcpyfromeepromwithchecksum(char *destination, unsigned int source, unsigned int size) {
unsigned char data, checksum = 0;
for(; size > 0; size--) {
data = eepromgetchar(source++);

checksum = (checksum << 1) || (checksum >> 7);

checksum += data;

*(destination++) = data;
}
return(checksum == eepromgetchar(source));
}
at last, the checksum either equal to the last “data” returned from function eepromgetchar or equal to the last “data” + 1 ???
Is it the author’s original intention ?

best regards.

#3 – chamnit 于 2017-06-15

This isn’t a coding forum to answer questions like this. You can easily perform a test yourself to see what this does. If you still think there is a potential bug, then post a new issue showing some proof. Thanks.

#4 – langwadt 于 2017-06-15

if the intention was to make the common checksum with rotate and add, it is a bug. The result of a
boolean OR (||) is either true or false (1/0), to make a rotation you need bitwise OR (|) or add

#5 – chamnit 于 2017-06-15

@langwadt : I still need some proof. I really don’t have the time to go digging in more rabbit holes. This is code that hasn’t been changed since Grbl 0.6, so it isn’t that critical given that it’ll generate the same checksum upon read (bad==bad). If there was a bug, which it now seems likely, it’s a consistent bug but it would explain the failure to detect corrupt data in EEPROM in the past.

In any case, a fix can’t be issued, because it would wipe all existing EEPROMs due to a bad checksum value. In general, a not a good idea. It’ll have to wait until v1.2 or the next project. In the meantime, I still welcome anyone willing to test this and provide a tested solution.

#6 – langwadt 于 2017-06-15

@chamnit yeh it can’t be changed now without a bunch of code to handle the change.
Using || means the checksum is basically a sum of the bytes it with will work but has weaknesses, using | or + makes it like this: https://en.wikipedia.org/wiki/BSD_checksum

“
llc@llc100:~$ cat checksumtest.c
#include
int main(void)
{
unsigned char checksum = 0xab;
unsigned char checksum1 = (checksum << 1) | (checksum >> 7);
unsigned char checksum2 = (checksum << 1) || (checksum >> 7);
printf("bitwise = 0x%x\n",checksum1);
printf("boolean = 0x%x\n",checksum2);
}
llc@llc100:~$ gcc checksumtest.c
llc@llc100:~$ ./a.out
bitwise = 0x57
boolean = 0x1
llc@llc100:~$
“

#7 – cri-s 于 2017-06-15

To my knowledge the expression is expanded to int, so if the left therm
produce false, the expression on the right of || cannot produce true.
Further the boolean or operator convert the result to 0 or 1 ,
Basically CRC is mostly last byte +1.
I’m passed to crc8ccitt_update from util/crc16.h because of massive
problems on update related to checksum and $# offsets.
Il 16/giu/2017 00:22 “langwadt” ha scritto:

> @chamnit <https://github.com/chamnit> yeh it can’t be changed now without
> a bunch of code to handle the change.
> Using || means the checksum is basically a sum of the bytes it with will
> work but has weaknesses, using | or + makes it like this:
> https://en.wikipedia.org/wiki/BSD_checksum
>
> llc@llc100:~$ cat checksumtest.c
> #include
> int main(void)
> {
> unsigned char checksum = 0xab;
> unsigned char checksum1 = (checksum << 1) | (checksum >> 7);
> unsigned char checksum2 = (checksum << 1) || (checksum >> 7);
> printf(“bitwise = 0x%x\n”,checksum1);
> printf(“boolean = 0x%x\n”,checksum2);
> }
> llc@llc100:~$ gcc checksumtest.c
> llc@llc100:~$ ./a.out
> bitwise = 0x57
> boolean = 0x1
> llc@llc100:~$
>
> —
> You are receiving this because you are subscribed to this thread.
> Reply to this email directly, view it on GitHub
> <https://github.com/grbl/grbl/issues/1249#issuecomment-308876925>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AJEOlvPRP1zqjG7f58l1vs7JlVrFBjDkks5sEaingaJpZM4N5iTb>
> .
>

#8 – ericQiang 于 2017-06-16

in any case, there is a risk here.
store “0xff” to eeprom will get a same checksum to “0xfe”.

#9 – chamnit 于 2017-06-16

FWIW, several months ago, I looked into better error-checking. I’ve been planning on using a Fletcher 16 checksum for the nonvolatile data in the ARM version, but it’s really up to the port implementor. The NVM stuff is on the port-side of the HAL. So, the only place you’ll likely see this EEPROM code is in the Mega2560 port, which may get updated there.

Don’t expect this to be updated or changed on the 328p version for the reasons in my last post.

#10 – ericQiang 于 2017-06-16

Thanks a lot.^_^

#11 – bryc 于 2019-12-26

What a joke @chamnit. Someone sees something possibly wrong in your code and you got the balls to say “this isn’t a coding forum to answer questions like this”.

Looking at the code, it’s pretty obvious this code isn’t doing what you think it is, and that whoever wrote it had no clue either:

“c
checksum = (checksum << 1) || (checksum >> 7);
checksum += data;
`

What you're trying to do here is an 8-bit rotate left by 1. But you're not. You are only shifting left 1.

So your code only needs to be:

`c
checksum = (checksum << 1); checksum += data; `

Or simplified:

`c
checksum <<= 1; checksum += data; `

Why? This is a logical comparison using an 8-bit integer, which means only a value of 0 (or 256 for overflow) can produce the FALSE condition. and in that case, 0 >> 7 is still 0, it's basically a NOP. It does not do anything. This is beginner level stuff you got wrong on a big project like grbl...

This checksum algorithm of yours is equivalent to this:

1. Start checksum at value 0 with the 8-bit type unsigned char`.
2. Multiply checksum by 2 (Shift left once).
3. Add next byte to checksum.

#12 - chamnit 于 2019-12-27

@bryc: Happy holidays to you too! As I stated before, fixing this issue would do more harm than good, as it would automatically wipe the EEPROM of anyone who uses the firmware that has this fixed. Regardless, this bug is relatively benign and been there since before 2011, when I started being involved in this project. Its best to address it on a major release than an incremental one.