What's the correct form of this CRC algorithm

Question

We are working on communication between AVR and a Crous PTZ.

We found its protocol and now we can turn it on and so.

We have problem with its CRC. This is Crous protocol doc (Got it here), at end of it, CRC algorithm is described (see image below), but seems it's wrong because the counter parameter will never be 8 but it is checked to see if it is 8 or not.

Our coder has wrote the following code for that algorithm , but returned CRC is not the same as the CRC returned by Crous device.

#include <stdint.h>

unsigned int crc_chk(unsigned char dta, unsigned char length) 
{ 
  int16_t j;
  uint16_t a,b,c; 
  uint16_t reg_crc=0x0000; 
  for( c = 0; c < length; c++ ) 
  { 
    a = reg_crc / 256;
    a ^= test_crc[dta];
    dta++; 
    a *= 256;
    b = reg_crc&0xff;
    reg_crc = a | b;
    for( j = 0; j < 8; j++ ) 
    { 
      if ( reg_crc & 0x8000 ) 
        reg_crc = ( reg_crc * 2 ) ^ 0X8005; /* LSB(b0)=1 */ 
      else 
        reg_crc = reg_crc * 2;
    } 
  } 
  return reg_crc; // the value that sent back to the CRC register finally 
}

He found true CRC in device respond, for example , In this device respond

01 BF 15 0F 94 95 96 97 9F 9E A2 A0 98 99 72 6B 6A 88 89 64 62 63 65 66 03 2D AC

and according to page 45 of The protocol Doc, CRC of

0F 94 95 96 97 9F 9E A2 A0 98 99 72 6B 6A 88 89 64 62 63 65 66

it should be 2DAC, 03 shows end of data, and 15 is size of data that when we calculate it in decimal, will be 21 that you see we have 21 bytes too.

What is the correct form of this algorithm?

Answer 1

At a glance, it looks like the "counter = 8?" decision block should probably have a "NO" path that feeds back to, probably, "the bit 15 CRC = 1?" decision block input. And the existing output of the "counter = 8?" decision block should be labelled "YES"...

As for the implementation, I'm having some trouble making sense of it. What is test_crc and where is it defined? Shouldn't dta be a pointer in the function signature? I made some updates to the code in your post to improve readability.

I faithfully implemented a function based on the block diagram and my assumption and ran it in IDEOne and get the result I think you expected... here's my code.

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

unsigned int crc16(uint8_t * data, uint8_t length) 
{ 
  uint16_t CRC = 0;
  uint16_t A = 0, B = 0;
  uint8_t index = 0, Counter = 0; 

  for( index = 0; index < length; index++ )
  {
    uint8_t byte = data[index];

    A = CRC / 256;
    A = A ^ byte;
    A = A * 256;

    B = CRC & 0xFF;
    CRC = A | B;

    for( Counter = 0; Counter < 8; Counter++ ){
      if( CRC & 0x8000 )
      {
        CRC = CRC * 2;
        CRC = CRC ^ 0x8005;
      }
      else
      {
        CRC = CRC * 2;
      }
    }
  }
  return CRC;
}

int main(int argc, char *argv[]){
  uint8_t data[] = { 0x01, 0xBF, 0x15, 0x0F, 0x94, 0x95, 0x96, 0x97, 0x9F, 0x9E, 0xA2, 0xA0, 0x98, 0x99, 0x72, 0x6B, 0x6A, 0x88, 0x89, 0x64, 0x62, 0x63, 0x65, 0x66, 0x03 };
  uint8_t length = sizeof(data);
  uint16_t crc = 0;

  printf("Length = %d\n", length);
  crc = crc16(data, length);
  printf("CRC = %04x", crc);

  return 0;

}

It outputs:

Length = 25
CRC = ac2d

Answer 2

There is nothing wrong with the CRC computation - you are simply computing the CRC for the wrong thing.

The data you want to send is:

0F 94 95 96 97 9F 9E A2 A0 98 99 72 6B 6A 88 89 64 62 63 65 66

Once you have framed it suitably for transmission to Crous PTZ, the actual bytes you are sending is

01 BF 15 0F 94 95 96 97 9F 9E A2 A0 98 99 72 6B 6A 88 89 64 62 63 65 66 03

The CRC 2DAC is for the frame as a whole, not just the data in it. The code below is an essentially unmodified version of the your code that computes the CRC for frame, and you can see that it derives the correct value.

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

int main (void) { 
    unsigned char test_crc[] = {
        0x01, 0xBF, 0x15, 0x0F, 0x94, 0x95, 0x96, 0x97, 0x9F, 0x9E, 0xA2, 0xA0, 0x98, 0x99, 0x72, 0x6B, 0x6A, 0x88, 0x89, 0x64, 0x62, 0x63, 0x65, 0x66, 0x03
    };

    int length = sizeof test_crc / sizeof test_crc[0];
    unsigned char dta = 0;
    int16_t j;
    uint16_t a,b,c; 
    uint16_t reg_crc=0x0000; 
    for( c = 0; c < length; c++ ) 
    { 
        printf("0x%02x%c", test_crc[dta], (dta+1)%5?' ':'\n');

        a = reg_crc / 256;
        a ^= test_crc[dta];
        dta++; 
        a *= 256;
        b = reg_crc&0xff;
        reg_crc = a | b;
        for( j = 0; j < 8; j++ ) 
        { 
            if ( reg_crc & 0x8000 ) 
                reg_crc = ( reg_crc * 2 ) ^ 0X8005; /* LSB(b0)=1 */ 
            else 
            reg_crc = reg_crc * 2;
        } 
    } 
    printf("\n");
    printf("CRC: 0x%04X\n", reg_crc);
    return 0;
}

When executed:

$ make crc16-orig && ./crc16-orig
cc     crc16-orig.c   -o crc16-orig
0x01 0xbf 0x15 0x0f 0x94
0x95 0x96 0x97 0x9f 0x9e
0xa2 0xa0 0x98 0x99 0x72
0x6b 0x6a 0x88 0x89 0x64
0x62 0x63 0x65 0x66 0x03

CRC: 0xAC2D

While the code is functionally correct, it could do with some improvements. @vicatcu's version is much nicer.