Search code examples
c++qtcrccrc16

CRC16 modbus calculation returns wrong value for long data packets

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.

Solution

  • 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.