RobTillaart / CRC

CRC library for Arduino
MIT License
79 stars 17 forks source link

The CRC16-Modbus calculation with the bytes swapped for Dwin displays #40

Closed rtek1000 closed 6 months ago

rtek1000 commented 6 months ago

Hi, I'm trying the Dwin displays, which have CRC16, but they use the Modbus calculation with the bytes swapped. How can I get the calculation done by Arduino to be changed in the library instead of in the sketch?

https://github.com/RobTillaart/CRC/blob/master/examples/CRC16_test_modbus/CRC16_test_modbus.ino

For example, the code sent to the display should be this: 5A A5 06 83 20 00 01 E9 AA.

5A A5: header 06: length 83: Read VP command 20 00: VP addr 01: data E9 AA: CRC16 checksum (for 83-20-00-01 only)

But the online calculator and this CRC16_test_modbus.ino returns the CRC16 Modbus (hex):

AA E9

It is 'AA E9' Instead of 'E9 AA' as Dwin sends for display in the 'SP Order' tab of the DGUS software.

rtek1000 commented 6 months ago

My workaround:

//
//    FILE: CRC16_test_modbus.ino
//  AUTHOR: Rob Tillaart
// PURPOSE: demo
//    DATE: 2023-07-25
//    (c) : MIT

#include "CRC16.h"

CRC16 crc(CRC16_MODBUS_POLYNOME,
          CRC16_MODBUS_INITIAL,
          CRC16_MODBUS_XOR_OUT,
          CRC16_MODBUS_REV_IN,
          CRC16_MODBUS_REV_OUT);

void setup() {
  Serial.begin(115200);
  Serial.println(__FILE__);

  // Dwin display order: 5A A5 06 '83 20 00 01' E9 AA
  // 5A A5: header
  // 06: length
  // 83: Read VP command
  // 20 00: VP addr
  // 01: data
  // E9 AA: CRC16 checksum (for 83-20-00-01 only)

  uint8_t arr[5] = { 0x83, 0x20, 0x00, 0x01 };

  for (int i = 0; i < 4; i++) {
    crc.add(arr[i]);
  }

  uint16_t res16 = crc.calc();

  uint16_t res16_swapped = ((uint8_t)(res16 & 0xFF) << 8) | (uint8_t)((res16 >> 8) & 0xFF);

  Serial.print(F("CRC16-Modbus: "));
  Serial.println(crc.calc(), HEX);

  Serial.print(F("CRC16-Modbus swapped: "));
  Serial.println(res16_swapped, HEX);
}

void loop() {
}
// -- END OF FILE --

Result (Serial Monitor):

03:40:24.435 -> CRC16-Modbus: AAE9 03:40:24.435 -> CRC16-Modbus swapped: E9AA

RobTillaart commented 6 months ago

Thanks for your question. Byte reversing should not need the casting to 8 bits. The following should work too.

uint16_t res16_swapped = (res16  << 8) | (res16 >> 8);

Byte reversing would only make sense for CRC16 and other multiples of 8.. CRC4/8/12 would not use it as it swapping bytes is less defined. Implementing it into the classes "breaks their similarity of interface".

I could add swap() functions, so you could write:

  uint16_t res16 = swap(crc.calc());

Would that solve your problem?

RobTillaart commented 6 months ago

Note: https://github.com/RobTillaart/bitHelpers provides more swappers and reverse algorithms.

rtek1000 commented 6 months ago

Ok, thank you.

I thought there was a way to generate the result with the bytes exchanged in the library algorithm. So if it's not possible, might it be better to create a new library that extends this library and adds byte swapping, with an additional function?

RobTillaart commented 6 months ago

@rtek1000 It is very well possible to implement, however I need to oversee the implications for the whole library.

So far It can be implemented pretty harmless by adding an extra parameter boolean reverseBytes = false. That should be the last parameter of the constructor. For compatibility it should be added to CRC32 and CRC64 too however that could be done on request. That is so far the best solution I can imagine that keeps the library backwards compatible.

rtek1000 commented 6 months ago

I believe I managed to implement the creation and verification of Dwin's CRC, below is the sketch:

https://github.com/rtek1000/CRC/blob/master/examples/CRC16-test_dwin_display/CRC16-test_dwin_display.ino

Basically I used the crc.restart() function (to force update the variables in a new use of the CRC buffer) and the bytes swapping is disguised, this way it also uses the 8-bit data for the verification stage. So I believe it is not necessary to modify the CRC library. What do you think about this code?

bool calc_DWIN_CRC(uint8_t *array, bool only_check) {
  crc.restart();

  for (int i = 3; i < (array[2] + 1); i++) {
    crc.add(array[i]);
  }

  uint16_t res = crc.calc();
  uint8_t res0 = res;
  uint8_t res1 = res >> 8;

  if ((array[array[2] + 1] == res0) && (array[array[2] + 2] == res1)) {
    return true;
  }

  if (only_check == false) {
    array[array[2] + 1] = res0;
    array[array[2] + 2] = res1;
  }

  return false;
}
RobTillaart commented 6 months ago

Looks good, some minor remarks

bool calc_DWIN_CRC(uint8_t *array, bool only_check) {
  crc.restart();
  int length = array[2] + 1;   //   is used enough to spend a local variable on the length

  //  check if length is valid.
  if ((length < 4) || (length > MAXLENGTH)) return false;

  for (int i = 3; i < length; i++) {
    crc.add(array[i]);
  }

  uint16_t res = crc.calc();
  uint8_t res0 = res;
  uint8_t res1 = res >> 8;

  if ((array[length] == res0) && (array[length + 1] == res1)) {
    return true;
  }

  if (only_check == false) {
    array[length] = res0;
    array[length + 1] = res1;
  }

  return false;
}
rtek1000 commented 6 months ago

Nice, thank you.