CRC16 modbus calculation returns wrong value for long data packets

Question

I'm connecting to a device through a serial port. The device appends a CRC16 tag to the end of the data packet in big-endian mode. In software side, the code to check CRC is something like this:

bool Protocol::checkCRC(const QByteArray &buf) {
    if(buf.size()<3){
        return false;
    }
    int len = buf.size()-2; // Exclude CRC token
    quint16 crc = 0xFFFF;
    quint16 claim = static_cast<uchar>(buf.at(buf.size()-1));
    claim*=0x100;
    claim+=static_cast<uchar>(buf.at(buf.size()-2));

    for (int pos = 0; pos < len; pos++)
    {
        crc ^= (quint16)buf[pos];         // XOR byte into LSB of crc
        for (int i = 8; i != 0; i--) {    // Loop over each bit
            if ((crc & 0x0001) != 0) {    // If the LSB is set
                crc >>= 1;                // Shift right and XOR 0xA001
                crc ^= 0xA001;
            }
            else                          // Else LSB is not set
                crc >>= 1;                // Just shift right
        }
    }
    return crc==claim;
}

I copied the code from this question.

It works fine for small packets. For example following data packet is passed in CRC16 check using this function:

0x04, 0x10, 0x00, 0x3d, 0xc1

Reported CRC is 0xC13D and function calculates 0xC13D as well. But for big data packets (in my case 53 bytes) the function fails to calculate correct CRC:

0x34, 0x02, 0x02, 0x08, 0x14, 0x00, 0x00, 0x00,
0x00, 0x00, 0x10, 0x0a, 0xdf, 0x07, 0x0a, 0x39, 
0x1b, 0x02, 0x02, 0x79, 0x61, 0xbf, 0x34, 0xdd, 
0x0b, 0x83, 0x0f, 0x10, 0x03, 0x1b, 0x11, 0x02, 
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 
0x00, 0x00, 0x00, 0xfc, 0x98

Reported CRC is 0x98FC but calculated value is 0xDFC4.

Mark Adler · Accepted Answer · 2016-04-28T17:22:37.703

2

I don't know what the QByteArray type is, but I'll wager that it is an array of signed characters. As a result, when the high bit of a byte is a one, that sign bit is extended when converted to an integer which all gets exclusive-or'ed into your CRC at crc ^= (quint16)buf[pos];. So when you got to the 0xdf, crc was exclusive-or'ed with 0xffdf instead of the intended 0xdf.

So the issue is not the length, but rather the likelihood of having a byte with the high bit set.

You need to provide unsigned bytes, or fix the conversion, or do an & 0xff to the resulting byte before exclusive-or'ing with the CRC.

edited Apr 28 '16 at 17:22

answered Apr 28 '16 at 17:10

Mark Adler

101,978
13
118
158

from Qt sources: `typedef unsigned short quint16` despite the fact `QByteArray` holds `char`s it should work. The problem was `quint16` conversion in `crc ^= (quint16)buf[pos]` which should be `quint8` – sorush-r Apr 28 '16 at 17:18
Ah, right, in fact, the signedness of quint16 doesn't matter, since the damage is already one when the signed character is converted to a larger type. the `(quint8)` makes it unsigned before it gets converted to a larger type. – Mark Adler Apr 28 '16 at 17:23

CRC16 modbus calculation returns wrong value for long data packets

1 Answers1