Bug in checksum?

[ir-fuel]

In your docs it says:

Ck0 and Ck1 are the checksum values to confirm the integrity of the message: to calculate this value we add all of the previous bytes together except 0x55 and 0xAA. We then take this sum and preform an XOR operation (^) with the sum and 0xFFFF. The result of this calculation contains Ck1 and Ck0, respectively

However, when I check the code:

calc += data;
calc ^= 0xFFFF;
if(calc == (reciever[messagelength - 1] + reciever[messagelength])) recieverindex++;
else recieverindex = 10;

You just add the 2 numbers in the incoming data stream. Shouldn't they be OR'ed together with the high byte shift left 8 places as to make an uint16?

Moreover it seems you add those checksum values to the calc variable too? Shouldn't they be excluded from the calculation?

[camcamfresh]

When checking the checksum of the incoming message the order doesn't matter (it's like adding 100 & 10 vs 10 & 100, you'll always get 110 as the sum).

The calc is the calculated the messages check sum from the actual message; we compare this with what the message is telling us the check sum was when it was sent. If they're the same, we assume the message hasn't changed.

[ir-fuel]

But it's not right as it is there. You are comparing a 16 bit value to the sum of 2 8 bit values.

I implemented it myself and I had to do this:

        if(dataChk == (buffer[bufferIndex - 1] + (buffer[bufferIndex] << 8))) {

The checksum is 16 bit (dataChk) and you want to compare it to a 16 bit value (the 2 bytes at the end of the incoming message), so you have to left-shift the high byte of the incoming message 8 bits before adding it to the low byte, or the result simply isn't correct.

Imagine dataChk is 0xFF0E, so for the message to be correct the command buffer should contain 0x0E and 0xFF in the last 2 bytes.

what your code does is compare 0xFF0E to 0xFF + 0x0E which equals 0x10D

0xFF0E != 0xFF + 0x0E

what the code should do is compare 0xFF0E with the 2 byte number 0xFF0E which is:

0xFF0E = (0xFF << 8) + 0x0E

Also in the code the first byte of the checksum value itself is added to calc too, which isn't correct.

[camcamfresh]

Great catch, I totally misunderstood that! There is no break in that case, so I think the switch was overflowing to the next case causing the bug to go unnoticed. I'll post an update to the code.

[kyuhyong]

I also checked the code myself and tried to apply the update you made as well as my solutions however, they all not working and only the original code as below can receive data correctly.

case 3:
                receiver[receiverMessageIndex] = data;
                calc += data;
                receiverMessageIndex++;
                if(receiverMessageIndex == receiverMessageLength) receiverIndex++;
                break;
            case 4:
                receiver[receiverMessageIndex] = data;
                calc += data;
                calc ^= 0xFFFF;
                if(calc == (receiver[receiverMessageLength - 1] + receiver[receiverMessageLength])) receiverIndex++;
                    else receiverIndex = 10;

Have you checked your code with working scooter?

[ir-fuel]

Just take one message, do the calculations yourself and compare it to the checksum.

example:

55 aa 04 23 01 1a 34 01 88 ff 4 + 23 + 1 + 1a + 34 + 1 = 0x77

0x77 ^ 0xFFFF = 0xFF88

What are the last 2 bytes of the command? 0x88 and 0xFF or 0xFF << 8 + 0x88

[camcamfresh]

I had problems with changing it at first, but eventually got it to work. The main problem I had was when reading Serial1 I would get the messages sent not the replies. I fixed this by adding the line "if(receiver[3] == 0x20) receiverIndex= 10;" to the end of case 3.

[kyuhyong]

I tried this one which is pretty much the same as what you proposed, but didn't work. I didn't try to print each byte and hand calculate them though.

case 3:
   receiver[receiverMessageIndex] = data;
   calc += data;
   receiverMessageIndex++;
   if(receiverMessageIndex < receiver[2] + 5) {
      calc += data;     //Add data until message index reaches last-1
   } 
   if(receiverMessageIndex == receiver[2] + 6) {//Read until message length
      receiverIndex++;
   }
   if(receiver[3] == 0x20) receiverIndex = 10;
   break;
case 4:     //Validate Checksum
   receiver[receiverMessageIndex] = data;
   calc ^= 0xFFFF;
   if(calc == (receiver[receiverMessageLength - 1] + (receiver[receiverMessageLength]<<8) )) {
      receiverIndex++;        //Check sum is correct!
   } else receiverIndex = 10;
   break;

Sorry, I forgot to redo shift left I corrected before.

[ir-fuel]

your code does not do the shift left operation.

[camcamfresh]

@kyuhyong in case 5, print to your computer the message packets you receive after the checksum is completed and see if/what the device is reading on Serial1.