Issues calculating CRC16 MCRF4XX for more than 1 byte

Question

I've been trying to perform CRC16 MCRF4XX in my code however I've managed to do it correctly for only 1 byte.

I've followed this guide, for the specific method: http://www.piclist.com/techref/method/error/quickcrc16.htm and i've tested the same byte in https://crccalc.com/

code is as follows:

register uint32_t i;
    uint16_t Crc = 0;


    for ( i = 0; i < Len; i++ )
        Crc = Utils_CRC16_MCRF4XX(Crc,pData[i]);

    return ( Crc );

the function "Utils_CRC16_MCRF4XX":

    uint8_t     i;
uint16_t    TempByte, CurrentCRC = 0xFFFF;
//make byte 16 bit format
TempByte = (uint16_t)Byte;

for ( i = 0; i < 8; i++ )
{
    if ( (CurrentCRC & 0x0001) == (TempByte & 0x0001) )
    {
        //right shift crc
        CurrentCRC >>= 1;
        //right shift data
        TempByte >>= 1;   
    }
    else
    {
        CurrentCRC >>= 1;
        TempByte >>= 1;
        CurrentCRC = CurrentCRC ^ 0x8408; /* 1000 0100 0000 1000 = x^16 + x^12 + x^5 + 1 */
    }
}

return ( Crc ^ CurrentCRC);

the output for byte 0x54 would be 0x1B26. I've tried XORing the output with the inserted Crc, but it doesn't add up right.

now my issue starts when I'm trying to feed the function more than 1 byte.

if let's say i would send it : 0x54 0xFF. it would give me a totally different calculation than the calculator gives.

I'm assuming my error is where i add up the bytes together, after performing the action on each byte.

appreciate the help!

The calling code is passing CRC in a parameter, but the function is never using it, and instead is setting CurrentCRC = 0xffff on every call. — rcgldr, Dec 26 '19 at 09:42
i see what you're saying, but as i mentioned, i tried XORing the return to the Crc argument, however eventually it returned wrong CRC for 2 bytes calculated. and as far as i understood, i need to do the initial FFFF for every byte, correct me if i'm wrong. — Barak, Dec 26 '19 at 09:47
You are wrong. CRCs are initialized only once before each block of bytes to calculate the CRC for. Depending on the algorithm, on calculating you even need to feed 0-bytes in place of an appended CRC to get the right CRC, and on checking you will feed the appended CRC and check the resulting CRC for 0. You might like to read http://ross.net/crc/download/crc_v3.txt for more details. — the busybee, Dec 26 '19 at 11:15
Post complete code - not just the _function body_. It will make it easier and more likely that someone will build teh code to replicate your issue. Anyhow the parameter `Crc` is the current CRC - `currentCrc` serves no purpose. You are not of course the first to do this - inventing your own is unneecessary: https://gist.github.com/aurelj/270bb8af82f65fa645c1, https://ww1.microchip.com/downloads/en/AppNotes/00752a.pdf — Clifford, Dec 26 '19 at 11:54

Clifford · Accepted Answer · 2019-12-29T13:04:19.533

Your function Utils_CRC16_MCRF4XX should update the Crc, but keeps its own CurrentCRC variable that bares no relationship to the current CRC value and is reinitialised to 0xFFFF on each call. The Crc parameter passed in is teh current CRC and that should be updated.

Adapting your function with minimal changes:

uint16_t Utils_CRC16_MCRF4XX( uint16_t Crc, uint8_t Byte )
{
    //make byte 16 bit format
    uint16_t TempByte = (uint16_t)Byte;

    for( uint8_t i = 0; i < 8; i++ )
    {
        if( (Crc & 0x0001) == (TempByte & 0x0001) )
        {
            //right shift crc
            Crc >>= 1;
            //right shift data
            TempByte >>= 1;
        }
        else
        {
            Crc >>= 1;
            TempByte >>= 1;
            Crc = Crc ^ 0x8408;
        }
    }

    return Crc ;
}

In the code that calls this, the Crc must be initialised to 0xFFFF, not zero:

uint16_t crc( uint8_t* pData, uint32_t Len )
{
    uint16_t Crc = 0xffffu ;

    for( uint32_t i = 0; i < Len; i++ )
    {
        Crc = Utils_CRC16_MCRF4XX( Crc, pData[i] );
    }
    return (Crc);
}

The following test code, produces the result 0x6F91 which concurs with https://crccalc.com/:

int main()
{
    uint8_t test[] = "123456789" ;
    uint16_t c = crc( test, sizeof(test) - 1 ) ;
    printf( "%X", (int)c ) ;

    return 0 ;
}

The implicit conversion that occurs when applying the & operator make TempByte redundant so further simplification is possible:

uint16_t Utils_CRC16_MCRF4XX( uint16_t Crc, uint8_t Byte )
{
    for( uint8_t i = 0; i < 8; i++ )
    {
        if( (Crc & 0x0001) == (Byte & 0x0001) )
        {
            Crc >>= 1;
            Byte >>= 1;
        }
        else
        {
            Crc >>= 1;
            Byte >>= 1;
            Crc = Crc ^ 0x8408;
        }
    }

    return Crc ;
}

Adapting the solution at https://gist.github.com/aurelj/270bb8af82f65fa645c1 yields the somewhat more succinct solution:

uint16_t Utils_CRC16_MCRF4XX( uint16_t Crc, uint8_t Byte )
{
    Crc ^= Byte ;

    for( uint8_t i = 0; i < 8; i++ )
    {
        Crc = (Crc & 0x0001) != 0 ? (Crc >> 1) ^ 0x8408 : 
                                    Crc >> 1 ;
    }

    return Crc ;
}

On a two's complement machine, `crc = (crc>>1)^((0-(crc&1))&0x8408);` (no conditionals). An optimizing compiler might produce essentially the same code for the `? :` version. — rcgldr, Dec 26 '19 at 19:32
@rcgldr : sure, but the question was about fixing the code rather than optimising it. Given the nature of the MCRF4XX, I doubt further optimisation is necessary, and would favour clarity. An alternative implementation at the same link does not even use the bit- loop, but if you have to explain it, probably best not to use it if the optimisation is not required. — Clifford, Dec 26 '19 at 19:43
A table lookup for dealing with 8 bits at a time is more common when optimizing CRC. My prior comment was only about an alternative to the ? : that eliminates the conditional, and there may be optimizing compilers that do this already. — rcgldr, Dec 26 '19 at 23:38
A look-up table will optimise for speed at the expense of memory - so it depends what you want to optimise. My point was that these are all "premature optimisations" worth being aware of in general, but probably unnecessary given the low data rate (typically 70kbps) and small data lengths (32bits) from MCRF4XX devices. — Clifford, Dec 27 '19 at 00:46
@rcgldr It is the lookup table that increases the memory required, not the code. — Clifford, Dec 27 '19 at 08:51
I meant a few bytes more of code (which could be part of the required memory, depending on where code is run from), if the CRC is implemented a bit at a time. — rcgldr, Dec 27 '19 at 17:05

score 1 · Answer 2 · answered Dec 26 '19 at 12:13

Completed code, including main() driver.

#include <stdint.h>
#include <stdio.h>

uint16_t Utils_CRC16_MCRF4XX(uint16_t crc, uint16_t byte);

int main(int argc, char **argv) {

uint32_t i;
    uint16_t crc ;
    uint8_t data[200] =  { 0 };
    uint32_t len ;

    for(len = 0; len+1 < argc; len++ ) {
        sscanf(argv[len+1], "%hhx", &data[len] );
        }

    crc = 0xffff;
    for ( i = 0; i < len; i++ ) {
        crc = Utils_CRC16_MCRF4XX(crc, data[i] );
        fprintf(stderr, "[%u] %2hhx CrC=%04x\n", (unsigned) i, data[i], (unsigned) crc);
    }

    fprintf(stderr, "CrC=%04x\n", (unsigned) crc);
    return 0 ;
}

uint16_t Utils_CRC16_MCRF4XX(uint16_t crc, uint16_t byte)
{
uint8_t i;

for ( i = 0; i < 8; i++ ) {
    register int samelow;
    samelow =  (crc & 1) == (byte & 1) ?1 : 0 ;

    crc >>= 1;
    byte >>= 1;
    if (!samelow) crc ^= 0x8408; /* 1000 0100 0000 1000 = x^16 + x^12 + x^5 + 1 */
    }

  return crc;
}

Issues calculating CRC16 MCRF4XX for more than 1 byte

2 Answers2

Linked