casaroli commented on code in PR #21445: URL: https://github.com/apache/nuttx/pull/21445#discussion_r4473586867
########## drivers/rpmsg/rpmsg_port_uart.c: ########## @@ -84,8 +84,8 @@ #define RPMSG_PORT_UART_EVT_TX (1 << 3) /* TX is ready or not */ #ifdef CONFIG_RPMSG_PORT_UART_CRC -# define rpmsg_port_uart_crc16(hdr) crc16ibm((FAR uint8_t *)&(hdr)->cmd, \ - (hdr)->len - sizeof((hdr)->crc)) +# define rpmsg_port_uart_crc16(hdr) crc16xor((FAR uint8_t *)&(hdr)->cmd, \ Review Comment: The frame check gets weaker, and the PR gives no number for the gain. Of 92565 two-bit errors in a 512-byte frame, crc16xor misses 5659 and crc16ibm misses 0. Two swapped 16-bit words also pass. Fix: put a measurement in the PR body, and keep crc16ibm selectable with a Kconfig choice for links that need the stronger check. ########## include/nuttx/crc16.h: ########## @@ -248,6 +248,18 @@ uint16_t crc16h8005_part(FAR const uint8_t *src, uint16_t crc16h8005(FAR const uint8_t *src, size_t len); +/**************************************************************************** + * Name: crc16xor + * + * Description: + * Return a 16-bit XOR checksum of the contents of the 'src' buffer, + * length 'len'. Uses automatic alignment handling and loop unrolling + * for maximum performance on small MCUs. + * + ****************************************************************************/ + +uint16_t crc16xor(FAR const uint8_t *src, size_t len); Review Comment: `crc16xor()` is not a CRC, but its name and header put it with the CRC functions. Fix: name it for what it is, for example `xorsum16()`, and declare it outside `crc16.h`. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
