This CRC6 snippet gives wrong results

Question

This CRC6 snippet gives wrong results

A code snippet to generate CRC6 is not giving the right value. What could be the problem in the code snippet?

SPI_CRC6 = X6 + X4 + X3 + X + 1

The initial seed value is 0x3F
Input data: 24 bit.

A few tested sample values:(not from code snippet)

24b input: 0xAE0000, CRC6: 0x11
24b input: 0x950055, CRC6: 0x22

/* CRC6 calculation  */
Uint16 crc2(Uint32 datin)
{
    Uint16 byte_idx, bit_idx, crc = (0x3F << 2);//CRC_INITSEED = 0x3f

    /* byte by byte starting from most significant (3-2-1) */
    for (byte_idx = 3; byte_idx >= 1; byte_idx--)
    {
        /* XOR-in new byte from left to right */
        crc ^= ((datin >> (byte_idx << 3)) & 0x000000FF);

        /* bit by bit for each byte */
        for (bit_idx = 0; bit_idx < 8; bit_idx++)
        {
            crc = crc << 1 ^ (crc & 0x80 ? (0x5B << 2) : 0);//CRC Polynom: 0x5B
        }
    }

    return (crc >> 2 & 0x3F); /*restore two bit offset */
}

c

crc

asked on Stack Overflow Mar 8, 2019 by

AAH • edited Mar 8, 2019 by

Zeta

2 Answers

The code shifts the input by 24,16,8 bits when it should be shifting by 16,8,0 bits. One way to fix that (and simplify the code at the same time) is to use the shift count as the loop parameter.

The code is also processing the input 8 bits at a time. This results in mysterious shifts by 2 throughout the code. It's much more natural to process 6 bits at a time. In that case the code needs to shift the input by 18,12,6,0 bits.

So I would write the code like this:

/* CRC6 calculation  */
Uint16 crc2(Uint32 datin)
{
    int input_shift;
    Uint16 bit_idx, crc = 0x3F;  //CRC seed = 0x3F

    /* 6 bits at a time starting from most significant */
    for (input_shift = 18; input_shift >= 0; input_shift -= 6)
    {
        /* XOR-in new data from left to right */
        crc ^= (datin >> input_shift) & 0x3F;

        /* bit by bit for each chunk */
        for (bit_idx = 0; bit_idx < 6; bit_idx++)
        {
            crc <<= 1;
            crc ^= (crc & 0x40) ? 0x5B : 0; //CRC polynomial: 0x5B
        }
    }

    return crc & 0x3F; //return the 6-bit CRC
}

answered on Stack Overflow Mar 8, 2019 by

user3386109

user3386109's answer shows a corrected version of your code, but in this case, there's no need to split up datain into 6 bit fields.

typedef unsigned short uint16_t;
typedef unsigned int   uint32_t;

uint16_t crc1(uint32_t datin)
{
int i;
uint32_t crc = datin ^ (0x3f << 18);
    for (i = 0; i < 24; i++)
        crc = (crc << 1) ^ ((crc & 0x800000) ? (0x5B << 18) : 0);
    return crc >> 18;
}

The following example assumes two's complement math, using (-0) = 0x00000000 or (-1) = 0xffffffff as a mask to avoid using conditional code (tenary ? : ). Note that an optimizing compiler may use math to avoid conditional code for the above example as well (Visual Studio does this).

typedef unsigned short uint16_t;
typedef unsigned int   uint32_t;

uint16_t crc1(uint32_t datin)
{
int i;
uint32_t crc = datin ^ (0x3f << 18);
    for (i = 0; i < 24; i++)
        crc = (crc << 1) ^ ((-(crc >> 23)) & (0x5B << 18));
    return crc >> 18;
}

The masking trick is often used by compilers, the general sequence is:

        ...                     ;eax is zero or non-zero
        neg     eax             ;sets borrow bit if eax != 0
        sbb     eax,eax         ;eax = 0x00000000 or 0xffffffff
        and     eax,...         ;use eax as mask

answered on Stack Overflow Mar 9, 2019 by

rcgldr • edited Mar 9, 2019 by

rcgldr

User contributions licensed under CC BY-SA 3.0