From dd3543bc9feaa17865f5a8021fd4312a4d0df30a Mon Sep 17 00:00:00 2001 From: "David J. Fiddes" Date: Fri, 29 Aug 2025 12:33:55 +0100 Subject: [PATCH] Fix LIN checksum calculation There is a typo in the LIN checksum calculation that introduces a data dependency bug. According to Section 2.3.1.5 Checksum of the LIN Protocol Specification Revision 2.1 the checksum should check if the value is greater than or equal to 256 before subtracting 255. Add a set of unit tests with representative LIN data. Only TestSyntheticLinStatusResponse1() actually exhibits the bug. Tests: - Run new unit tests - Verify VWCoolantHeater simulator built with ESP32 and https://github.com/gicking/LIN_slave_portable_Arduino communicates data reliably with ZombieVerter VCU --- include/linbus.h | 7 +++- src/linbus.cpp | 2 +- test/Makefile | 6 +-- test/stub_libopencm3.c | 91 ++++++++++++++++++++++++++++++++++++++++ test/test_linbus.cpp | 95 ++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 195 insertions(+), 6 deletions(-) create mode 100644 test/test_linbus.cpp diff --git a/include/linbus.h b/include/linbus.h index f7a4981..20b07f6 100644 --- a/include/linbus.h +++ b/include/linbus.h @@ -19,6 +19,7 @@ #ifndef LINBUS_H #define LINBUS_H +#include class LinBus { @@ -41,8 +42,6 @@ class LinBus uint16_t pin; }; - static uint8_t Checksum(uint8_t pid, uint8_t* data, int len); - static uint8_t Parity(uint8_t id); static const HwInfo hwInfo[]; static const int payloadIndex = 3; @@ -51,6 +50,10 @@ class LinBus const HwInfo* hw; uint8_t sendBuffer[11]; uint8_t recvBuffer[12]; + + public: + static uint8_t Checksum(uint8_t pid, uint8_t* data, int len); + static uint8_t Parity(uint8_t id); }; #endif // LINBUS_H diff --git a/src/linbus.cpp b/src/linbus.cpp index adcd978..dca0bd2 100644 --- a/src/linbus.cpp +++ b/src/linbus.cpp @@ -152,7 +152,7 @@ uint8_t LinBus::Checksum(uint8_t pid, uint8_t* data, int len) for (int i = 0; i < len; i++) { uint16_t tmp = (uint16_t)checksum + (uint16_t)data[i]; - if (tmp > 256) tmp -= 255; + if (tmp >= 256) tmp -= 255; checksum = tmp; } return checksum ^ 0xff; diff --git a/test/Makefile b/test/Makefile index d8e0a71..4344fea 100644 --- a/test/Makefile +++ b/test/Makefile @@ -4,12 +4,12 @@ CAN_SIGNED ?= 0 CC = gcc CPP = g++ LD = g++ -CFLAGS = -std=c99 -ggdb -DSTM32F1 -DCAN_SIGNED=$(CAN_SIGNED) -Itest-include -I../include -I../../libopencm3/include -CPPFLAGS = -ggdb -DSTM32F1 -DCAN_SIGNED=$(CAN_SIGNED) -Itest-include -I../include -I../../libopencm3/include +CFLAGS = -std=c99 -ggdb -fpermissive -DSTM32F1 -DCAN_SIGNED=$(CAN_SIGNED) -Itest-include -I../include -I../../libopencm3/include +CPPFLAGS = -ggdb -fpermissive -DSTM32F1 -DCAN_SIGNED=$(CAN_SIGNED) -Itest-include -I../include -I../../libopencm3/include LDFLAGS = -g BINARY = test_libopeninv OBJS = test_main.o fu.o test_fu.o test_fp.o my_fp.o my_string.o params.o \ - stub_canhardware.o test_canmap.o canmap.o \ + stub_canhardware.o test_canmap.o canmap.o test_linbus.o linbus.o \ stub_libopencm3.o VPATH = ../src ../libopeninv/src diff --git a/test/stub_libopencm3.c b/test/stub_libopencm3.c index 29dd208..8165bd1 100644 --- a/test/stub_libopencm3.c +++ b/test/stub_libopencm3.c @@ -56,3 +56,94 @@ uint32_t crc_calculate_block(uint32_t *datap, int size) void crc_reset(void) { } + +void gpio_set_mode(uint32_t gpioport, uint8_t mode, uint8_t cnf, uint16_t gpios) +{ +} + +void usart_set_baudrate(uint32_t usart, uint32_t baud) +{ +} + +void usart_set_databits(uint32_t usart, uint32_t bits) +{ +} + +void usart_set_stopbits(uint32_t usart, uint32_t stopbits) +{ +} + +void usart_set_mode(uint32_t usart, uint32_t mode) +{ +} + +void usart_set_parity(uint32_t usart, uint32_t parity) +{ +} + +void usart_set_flow_control(uint32_t usart, uint32_t flowcontrol) +{ +} + +void usart_enable_tx_dma(uint32_t usart) +{ +} + +void usart_enable_rx_dma(uint32_t usart) +{ +} + +void usart_enable(uint32_t usart) +{ +} + +void dma_channel_reset(uint32_t dma, uint8_t channel) +{ +} + +void dma_set_read_from_memory(uint32_t dma, uint8_t channel) +{ +} + +void dma_set_peripheral_address(uint32_t dma, uint8_t channel, uint32_t address) +{ +} + +void dma_set_memory_address(uint32_t dma, uint8_t channel, uint32_t address) +{ +} + +void dma_set_peripheral_size(uint32_t dma, uint8_t channel, + uint32_t peripheral_size) +{ +} + +void dma_set_memory_size(uint32_t dma, uint8_t channel, uint32_t mem_size) +{ +} + +void dma_enable_memory_increment_mode(uint32_t dma, uint8_t channel) +{ +} + +void dma_set_number_of_data(uint32_t dma, uint8_t channel, uint16_t number) +{ +} + +uint16_t dma_get_number_of_data(uint32_t dma, uint8_t channel) +{ + return 0x5a; +} + +void dma_clear_interrupt_flags(uint32_t dma, uint8_t channel, + uint32_t interrupts) +{ +} + +void dma_enable_channel(uint32_t dma, uint8_t channel) +{ +} + +void dma_disable_channel(uint32_t dma, uint8_t channel) +{ +} diff --git a/test/test_linbus.cpp b/test/test_linbus.cpp new file mode 100644 index 0000000..6ef2935 --- /dev/null +++ b/test/test_linbus.cpp @@ -0,0 +1,95 @@ +/* + * This file is part of the libopeninv project. + * + * Copyright (C) 2025 David J. Fiddes + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ + +#include +#include "test.h" + +class LinBusTest : public UnitTest +{ +public: + LinBusTest(const std::list *cases) : UnitTest(cases) {} +}; + +static void TestProtectedIdentifierGeneration() +{ + // Examples from 2.8.2 TABLE OF VALID FRAME IDENTIFIERS in + // Lin Protocol Specification Revision 2.1 + + ASSERT(LinBus::Parity(0x00) == 0x80); + ASSERT(LinBus::Parity(0x17) == 0x97); + ASSERT(LinBus::Parity(0x2B) == 0x2B); + ASSERT(LinBus::Parity(0x3B) == 0xFB); +} + +static void TestSpecChecksumExample() +{ + // Example from 2.8.2 EXAMPLE OF CHECKSUM CALCULATION in + // Lin Protocol Specification Revision 2.1 + + uint8_t data[] = {0x55, 0x93, 0xe5}; + ASSERT(LinBus::Checksum(0x4a, data, sizeof(data)) == 0xe6); +} + +static void TestSyntheticLinStatusResponse1() +{ + // Synthesized using https://github.com/gicking/LIN_slave_portable_Arduino + // captured and verified using Saleae Logic analyzer + // and https://linchecksumcalculator.machsystems.cz/ + + uint8_t data[] = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17}; + ASSERT(LinBus::Checksum(LinBus::Parity(0x30), data, sizeof(data)) == 0x72); +} + +static void TestSyntheticLinStatusResponse2() +{ + // Synthesized using https://linchecksumcalculator.machsystems.cz/ + + uint8_t data[] = {0x01, 0x02, 0x03, 0x04, 0xaa, 0x55, 0xaa, 0x55}; + ASSERT(LinBus::Checksum(0xf0, data, sizeof(data)) == 0x05); +} + +static void TestVAGPTCHeaterStatusResponse1() +{ + // Chosen at random from + // https://raw.githubusercontent.com/Tom-evnut/ID3-LIN-Bus/refs/heads/main/Lin%20Logs/PTC%20Cabin%20Heat%20on%20off.csv + + uint8_t data[] = {0x54, 0x69, 0x55, 0x00, 0xCA, 0x10, 0x00, 0x04}; + + ASSERT(LinBus::Checksum(LinBus::Parity(0x0B), data, sizeof(data)) == 0x82); +} + +static void TestVAGPTCHeaterStatusResponse2() +{ + // Chosen at random from + // https://raw.githubusercontent.com/Tom-evnut/ID3-LIN-Bus/refs/heads/main/Lin%20Logs/PTC%20Cabin%20Heat%20on%20off.csv + + uint8_t data[] = {0xFF, 0x0F, 0x80, 0x64, 0x8B, 0xC9, 0xDD, 0x47}; + + ASSERT(LinBus::Checksum(LinBus::Parity(0x37), data, sizeof(data)) == 0x5A); +} + +// This line registers the test +REGISTER_TEST( + LinBusTest, + TestProtectedIdentifierGeneration, + TestSpecChecksumExample, + TestSyntheticLinStatusResponse1, + TestVAGPTCHeaterStatusResponse1, + TestVAGPTCHeaterStatusResponse2, + TestSyntheticLinStatusResponse2);