From 367b95cba4689d3f5c072f8e174db8d483139dc0 Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 19 Sep 2026 11:58:25 -0400 Subject: [PATCH 1/4] feat: make the sensor link a shared library both ends can use The packet format was written out twice, once in the Pi's receiver and once in the Arduino firmware, with nothing stopping the two from drifting apart. A mismatch would not fail a build; the car would just decode garbage, which is the kind of thing you discover at an event. This turns the receiver into a library that the firmware can also use, so there is one definition to edit instead of two. Parsing is kept separate from serial I/O so the framing can be tested on any machine with no car attached, which is what makes the tests worth running. The tests cover the failure the old 9600-baud reader actually had: one dropped byte desynchronising the stream with nothing to resynchronise against. A golden frame pins the wire format byte for byte, verified identical to the firmware's previous hand-rolled encoder across 200,000 random packets. CI includes a job that breaks the parser four ways and requires the tests to notice every time, because a framing test that cannot fail is worse than no test: it turns a guess into a green tick. --- .github/workflows/ci.yml | 224 ++++++++++++++++++++++++++ .gitignore | 9 ++ CMakeLists.txt | 56 +++++++ library.properties | 10 ++ main.c | 285 --------------------------------- src/SensorHub.h | 20 +++ src/sensorhub/parser.c | 102 ++++++++++++ src/sensorhub/sensorhub.h | 138 ++++++++++++++++ src/sensorhub/serial.c | 132 +++++++++++++++ src/sensorhub/serial.h | 59 +++++++ test/test_parser.c | 328 ++++++++++++++++++++++++++++++++++++++ test/test_serial_pty.py | 98 ++++++++++++ tools/monitor.c | 116 ++++++++++++++ 13 files changed, 1292 insertions(+), 285 deletions(-) create mode 100644 .github/workflows/ci.yml create mode 100644 .gitignore create mode 100644 CMakeLists.txt create mode 100644 library.properties delete mode 100644 main.c create mode 100644 src/SensorHub.h create mode 100644 src/sensorhub/parser.c create mode 100644 src/sensorhub/sensorhub.h create mode 100644 src/sensorhub/serial.c create mode 100644 src/sensorhub/serial.h create mode 100644 test/test_parser.c create mode 100755 test/test_serial_pty.py create mode 100644 tools/monitor.c diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml new file mode 100644 index 0000000..fafa5b4 --- /dev/null +++ b/.github/workflows/ci.yml @@ -0,0 +1,224 @@ +name: CI + +on: + push: + branches: [ main ] + pull_request: + workflow_dispatch: + +jobs: + test: + name: test / ${{ matrix.os }} / ${{ matrix.cc }} + runs-on: ${{ matrix.os }} + strategy: + fail-fast: false + matrix: + include: + - { os: ubuntu-latest, cc: gcc } + - { os: ubuntu-latest, cc: clang } + - { os: macos-latest, cc: clang } + steps: + - uses: actions/checkout@v4 + - name: Configure + run: cmake -S . -B build -DCMAKE_BUILD_TYPE=Release -DCMAKE_C_COMPILER=${{ matrix.cc }} + - name: Build + run: cmake --build build -j + - name: Test + run: ctest --test-dir build --output-on-failure + + - name: End-to-end against a fake Arduino on a pty + # covers what the unit tests cannot: termios setup, blocking reads, + # and signal handling. it caught a monitor that could not be Ctrl-C'd. + run: python3 test/test_serial_pty.py build/sensorhub-monitor + + warnings: + name: -Wall -Wextra -Werror + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + cc: [ gcc, clang ] + steps: + - uses: actions/checkout@v4 + - name: Configure + run: | + cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_C_COMPILER=${{ matrix.cc }} \ + -DCMAKE_C_FLAGS="-Wall -Wextra -Werror" + - name: Build + run: cmake --build build -j + + sanitizers: + name: asan + ubsan + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Configure + run: | + cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug \ + -DCMAKE_C_FLAGS="-fsanitize=address,undefined -fno-sanitize-recover=all -g" + - name: Build + run: cmake --build build -j + - name: Test + run: ctest --test-dir build --output-on-failure + + cross-aarch64: + # this library's whole job is to run on the Pi 5, so prove it cross-compiles + # for one before trusting an x86 green tick + name: cross-compile for the Pi + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Install the cross toolchain + run: | + sudo apt-get update + sudo apt-get install -y gcc-aarch64-linux-gnu qemu-user + - name: Configure + run: | + cmake -S . -B build -DCMAKE_BUILD_TYPE=Release \ + -DCMAKE_SYSTEM_NAME=Linux \ + -DCMAKE_SYSTEM_PROCESSOR=aarch64 \ + -DCMAKE_C_COMPILER=aarch64-linux-gnu-gcc \ + -DCMAKE_C_FLAGS="-Wall -Wextra -Werror" + - name: Build + run: cmake --build build -j + - name: Run the tests under emulation + run: qemu-aarch64 -L /usr/aarch64-linux-gnu ./build/test_parser + + mutation: + # a framing test that cannot fail is worse than no test, because it + # launders a guess into a green tick. break the parser four ways and + # require the suite to notice every time. + name: the tests must be able to fail + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Build and confirm the suite passes clean + run: | + cmake -S . -B build -DCMAKE_BUILD_TYPE=Debug + cmake --build build -j + ./build/test_parser + - name: Each mutation must be caught + run: | + set -u + cp src/sensorhub/parser.c /tmp/parser.orig + fail=0 + check () { + cmake --build build -j >/dev/null 2>&1 + if ./build/test_parser >/dev/null 2>&1; then + echo "NOT CAUGHT: $1" + fail=1 + else + echo "caught: $1" + fi + cp /tmp/parser.orig src/sensorhub/parser.c + } + sed -i 's/parser->stats.checksum_errors++;/;/' src/sensorhub/parser.c + check "dropped checksum-error counting" + sed -i 's/memcpy(out, parser->payload, SH_PAYLOAD_SIZE);/memset(out,0,23);/' src/sensorhub/parser.c + check "corrupted the packet copy" + sed -i 's/else if (byte == SH_HEADER_1) {/else if (0) {/' src/sensorhub/parser.c + check "broke repeated-header handling" + sed -i 's/== byte)/!= byte)/' src/sensorhub/parser.c + check "inverted the checksum comparison" + cmake --build build -j >/dev/null 2>&1 + exit $fail + + consumer: + # what pulling this into CarComputer actually looks like + name: add_subdirectory + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + path: SensorHub + - name: Write a consumer that uses only the public header + run: | + cat > CMakeLists.txt <<'CMAKE' + cmake_minimum_required(VERSION 3.16) + project(consumer LANGUAGES C) + add_subdirectory(SensorHub) + add_executable(consumer main.c) + target_link_libraries(consumer PRIVATE sensorhub) + CMAKE + cat > main.c <<'C' + #include + #include + #include + int main (void) { + sh_parser_t parser; + sh_packet_t in, out; + uint8_t frame[SH_FRAME_SIZE]; + memset(&in, 0, sizeof(in)); + in.speed = 12.5f; + sh_encode_frame(&in, frame); + sh_parser_init(&parser); + for (size_t i = 0; i < sizeof(frame); ++i) { + if (sh_parser_feed(&parser, frame[i], &out)) { + printf("speed=%.1f\n", (double) out.speed); + return out.speed == 12.5f ? 0 : 1; + } + } + return 1; + } + C + - name: Build and run + run: | + cmake -S . -B build + cmake --build build -j + ./build/consumer + - name: Private sources must not be reachable + run: | + printf '#include "parser.c"\nint main(){}\n' > bad.c + if cc -std=c11 -ISensorHub/src -c bad.c -o /dev/null 2>/dev/null; then + echo "src/ is reachable from a consumer's include path" + exit 1 + fi + echo "src/ correctly private" + + arduino: + # this header also ships as an Arduino library and compiles for the + # ATmega328p, where int is 16 bits. the firmware depends on that, so + # prove it here rather than finding out at flash time. + name: compiles for the Nano + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - name: Install avr-gcc + run: | + sudo apt-get update + sudo apt-get install -y gcc-avr avr-libc binutils-avr + - name: Header and encoder must build for AVR + run: | + cat > /tmp/avr_probe.c <<'C' + #include + /* int is 16 bits here; the packed fixed-width layout must survive */ + _Static_assert(sizeof(sh_packet_t) == 23, "struct differs on AVR"); + _Static_assert(sizeof(float) == 4, "float differs on AVR"); + volatile unsigned char sink; + int main(void) { + sh_packet_t p = {0}; + uint8_t f[SH_FRAME_SIZE]; + sh_encode_frame(&p, f); + sink = f[SH_FRAME_SIZE - 1]; + return 0; + } + C + avr-gcc -mmcu=atmega328p -Os -std=c11 -Wall -Wextra -Werror \ + -ffunction-sections -fdata-sections -Wl,--gc-sections \ + -Isrc /tmp/avr_probe.c src/sensorhub/parser.c -o /tmp/avr_probe.elf + avr-size /tmp/avr_probe.elf + + - name: It must also build as C++, since .ino files are C++ + run: | + printf '#include \nstatic_assert(sizeof(sh_packet_t)==23,"");\nint main(){return 0;}\n' > /tmp/probe.cpp + avr-g++ -mmcu=atmega328p -Os -std=gnu++17 -Wall -Wextra \ + -Isrc -c /tmp/probe.cpp -o /tmp/probe.o + + - name: The POSIX transport must compile away to nothing on AVR + run: | + avr-gcc -mmcu=atmega328p -Os -std=c11 -Isrc \ + -c src/sensorhub/serial.c -o /tmp/serial_avr.o + size=$(avr-size /tmp/serial_avr.o | awk 'NR==2{print $1+$2+$3}') + echo "serial.c contributes $size bytes on AVR" + test "$size" = "0" diff --git a/.gitignore b/.gitignore new file mode 100644 index 0000000..6a23c67 --- /dev/null +++ b/.gitignore @@ -0,0 +1,9 @@ +# CMake build output +build/ + +# Arduino build output +*.elf +*.hex + +# macOS +.DS_Store diff --git a/CMakeLists.txt b/CMakeLists.txt new file mode 100644 index 0000000..0332cdd --- /dev/null +++ b/CMakeLists.txt @@ -0,0 +1,56 @@ +cmake_minimum_required(VERSION 3.16) +project(sensorhub LANGUAGES C) + +# Layout note: sources live under src/sensorhub/ rather than the usual +# include/ + src/ split so that this directory is simultaneously a valid +# Arduino library. arduino-cli puts /src on the include path and +# compiles everything beneath it, which makes resolve +# identically on the Nano and on the Pi. src/sensorhub/serial.c is guarded so +# it compiles away to nothing on AVR. +add_library(sensorhub STATIC + src/sensorhub/parser.c +) + +target_include_directories(sensorhub + PUBLIC ${CMAKE_CURRENT_SOURCE_DIR}/src +) +target_compile_features(sensorhub PUBLIC c_std_11) + +# The POSIX serial transport. Guarded internally too, but there is no reason +# to hand it to a non-Unix build at all. +if(UNIX) + target_sources(sensorhub PRIVATE src/sensorhub/serial.c) +endif() + +if(CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_SOURCE_DIR) + set(SH_TOP_LEVEL ON) +else() + set(SH_TOP_LEVEL OFF) +endif() + +option(SH_BUILD_MONITOR "Build the sensorhub-monitor bench tool" ${SH_TOP_LEVEL}) +option(SH_BUILD_TESTS "Build the framing tests" ${SH_TOP_LEVEL}) + +if(SH_BUILD_MONITOR AND UNIX) + add_executable(sensorhub-monitor tools/monitor.c) + target_link_libraries(sensorhub-monitor PRIVATE sensorhub) +endif() + +if(SH_BUILD_TESTS) + enable_testing() + add_executable(test_parser test/test_parser.c) + target_link_libraries(test_parser PRIVATE sensorhub) + add_test(NAME parser COMMAND test_parser) + + # The counter width is configurable for AVR's sake; build the tests + # against the narrow setting too so that path cannot rot unnoticed. + # + # This compiles parser.c into the test rather than linking libsensorhub, + # deliberately: SH_COUNTER_BITS changes sizeof(sh_stats_t), so a test + # defining 16 while the library was built with 64 would disagree about + # the layout of every sh_parser_t it passed across that boundary. + add_executable(test_parser_16 test/test_parser.c src/sensorhub/parser.c) + target_include_directories(test_parser_16 PRIVATE ${CMAKE_CURRENT_SOURCE_DIR}/src) + target_compile_definitions(test_parser_16 PRIVATE SH_COUNTER_BITS=16) + add_test(NAME parser_narrow_counters COMMAND test_parser_16) +endif() diff --git a/library.properties b/library.properties new file mode 100644 index 0000000..de505ff --- /dev/null +++ b/library.properties @@ -0,0 +1,10 @@ +name=SensorHub +version=0.1.0 +author=Cedarville Supermileage (HEEV) +maintainer=Cedarville Supermileage (HEEV) +sentence=Shared wire format for the car's sensor link. +paragraph=Defines the packet the Arduino Nano sends to the Raspberry Pi, along with the checksum, the frame encoder, and the receiving state machine. Used on both ends so the two cannot drift apart. On AVR only the encoder is normally linked; the parser is there for when the Pi starts commanding the output channels. +category=Communication +url=https://github.com/HEEV/SensorHub +architectures=* +includes=sensorhub/sensorhub.h diff --git a/main.c b/main.c deleted file mode 100644 index fc5dd4d..0000000 --- a/main.c +++ /dev/null @@ -1,285 +0,0 @@ -// sm_serial.c -#define _DEFAULT_SOURCE - -#include -#include -#include -#include -#include -#include -#include -#include -#include - -#define HEADER_1 0xAAU -#define HEADER_2 0x55U -#define SERIAL_PORT "/dev/ttyUSB0" -#define BAUD_RATE B115200 -#define BAUD_TEXT "115200" - -typedef struct __attribute__((packed)) { - float speed; - float airspeed; - float engineTemp; - float radTemp; - - uint8_t channel0; - uint8_t channel1; - uint8_t channel2; - uint8_t channel3; - uint8_t channel4; - - uint16_t channelA0; -} DataPacket; - -_Static_assert(sizeof(float) == 4, "32-bit float required"); -_Static_assert(sizeof(DataPacket) == 23, "Unexpected packet size"); - -typedef enum { - STATE_WAIT_HEADER_1, - STATE_WAIT_HEADER_2, - STATE_READ_PAYLOAD, - STATE_READ_CHECKSUM -} ReceiverState; - -typedef struct { - ReceiverState state; - uint8_t payload[sizeof(DataPacket)]; - size_t payloadIndex; -} PacketReceiver; - -static uint8_t packet_checksum( - const uint8_t *data, - size_t length) -{ - uint8_t checksum = 0; - - for (size_t i = 0; i < length; ++i) { - checksum ^= data[i]; - } - - return checksum; -} - -static bool configure_serial(int fd, speed_t baud_rate) -{ - struct termios tty; - - if (tcgetattr(fd, &tty) != 0) { - perror("tcgetattr"); - return false; - } - - cfmakeraw(&tty); - - if (cfsetispeed(&tty, baud_rate) != 0 || - cfsetospeed(&tty, baud_rate) != 0) { - perror("setting baud rate"); - return false; - } - - tty.c_cflag &= (tcflag_t)~CSIZE; - tty.c_cflag |= CS8; - tty.c_cflag |= CLOCAL | CREAD; - - tty.c_cflag &= (tcflag_t)~PARENB; - tty.c_cflag &= (tcflag_t)~PARODD; - tty.c_cflag &= (tcflag_t)~CSTOPB; - tty.c_cflag &= (tcflag_t)~CRTSCTS; - - /* - * Prevent modem-control lines from being dropped on close. - * This helps avoid repeated Arduino Nano resets. - */ - tty.c_cflag &= (tcflag_t)~HUPCL; - - tty.c_iflag &= (tcflag_t)~IXON; - tty.c_iflag &= (tcflag_t)~IXOFF; - tty.c_iflag &= (tcflag_t)~IXANY; - - /* - * Blocking mode: each read waits for at least one byte. - */ - tty.c_cc[VMIN] = 1; - tty.c_cc[VTIME] = 0; - - if (tcsetattr(fd, TCSANOW, &tty) != 0) { - perror("tcsetattr"); - return false; - } - - /* - * Do not call tcflush() and do not manipulate DTR or RTS. - */ - return true; -} - -static bool read_byte(int fd, uint8_t *value) -{ - ssize_t count; - - if (value == NULL) { - errno = EINVAL; - return false; - } - - do { - count = read(fd, value, 1); - } while (count < 0 && errno == EINTR); - - return count == 1; -} - -static void packet_receiver_init(PacketReceiver *receiver) -{ - if (receiver == NULL) { - return; - } - - receiver->state = STATE_WAIT_HEADER_1; - receiver->payloadIndex = 0; - memset(receiver->payload, 0, sizeof(receiver->payload)); -} - -static bool receive_packet( - PacketReceiver *receiver, - int fd, - DataPacket *packet) -{ - uint8_t received_byte; - - if (receiver == NULL || packet == NULL) { - errno = EINVAL; - return false; - } - - while (read_byte(fd, &received_byte)) { - switch (receiver->state) { - case STATE_WAIT_HEADER_1: - if (received_byte == HEADER_1) { - receiver->state = STATE_WAIT_HEADER_2; - } - break; - - case STATE_WAIT_HEADER_2: - if (received_byte == HEADER_2) { - receiver->payloadIndex = 0; - receiver->state = STATE_READ_PAYLOAD; - } else if (received_byte != HEADER_1) { - receiver->state = STATE_WAIT_HEADER_1; - } - /* - * If another HEADER_1 arrives, remain in this state. - */ - break; - - case STATE_READ_PAYLOAD: - receiver->payload[receiver->payloadIndex++] = - received_byte; - - if (receiver->payloadIndex == sizeof(DataPacket)) { - receiver->state = STATE_READ_CHECKSUM; - } - break; - - case STATE_READ_CHECKSUM: - receiver->state = STATE_WAIT_HEADER_1; - - if (packet_checksum( - receiver->payload, - sizeof(receiver->payload)) == received_byte) { - memcpy( - packet, - receiver->payload, - sizeof(*packet)); - - return true; - } - - fprintf( - stderr, - "Discarded packet: bad checksum\n"); - break; - } - } - - return false; -} - -static void print_packet(const DataPacket *packet) -{ - if (packet == NULL) { - return; - } - - printf( - "\r\x1b[2K" - "speed=%.2f mph" - " | airspeed=%.2f" - " | engine=%.2f F" - " | radiator=%.2f F" - " | channels=%u%u%u%u%u" - " | A0=%u", - (double)packet->speed, - (double)packet->airspeed, - (double)packet->engineTemp, - (double)packet->radTemp, - (unsigned)packet->channel0, - (unsigned)packet->channel1, - (unsigned)packet->channel2, - (unsigned)packet->channel3, - (unsigned)packet->channel4, - (unsigned)packet->channelA0); - - fflush(stdout); -} - -int main(void) -{ - PacketReceiver receiver; - DataPacket packet; - int fd; - - /* - * Open only once. Reopening the port can repeatedly reset an - * Arduino Nano through its DTR auto-reset circuit. - */ - fd = open(SERIAL_PORT, O_RDWR | O_NOCTTY); - - if (fd < 0) { - perror("open"); - return 1; - } - - if (!configure_serial(fd, BAUD_RATE)) { - close(fd); - return 1; - } - - packet_receiver_init(&receiver); - - printf( - "Listening on %s at %s baud\n", - SERIAL_PORT, - BAUD_TEXT); - - while (true) { - memset(&packet, 0, sizeof(packet)); - - if (!receive_packet(&receiver, fd, &packet)) { - if (errno != 0) { - perror("serial read"); - } else { - fprintf(stderr, "Serial connection closed\n"); - } - - break; - } - - print_packet(&packet); - } - - printf("\n"); - close(fd); - return 0; -} \ No newline at end of file diff --git a/src/SensorHub.h b/src/SensorHub.h new file mode 100644 index 0000000..7efdf11 --- /dev/null +++ b/src/SensorHub.h @@ -0,0 +1,20 @@ +/* + * Arduino entry point. + * + * The Arduino library resolver matches an #include against headers at the top + * level of src/, so a sketch cannot reach src/sensorhub/sensorhub.h directly + * even though that directory is on the include path. This header is the + * conventional shim that makes the library discoverable. + * + * In a sketch: #include + * Everywhere else: #include + * + * Both reach the same declarations. + */ + +#ifndef SENSORHUB_ARDUINO_H +#define SENSORHUB_ARDUINO_H + +#include "sensorhub/sensorhub.h" + +#endif /* SENSORHUB_ARDUINO_H */ diff --git a/src/sensorhub/parser.c b/src/sensorhub/parser.c new file mode 100644 index 0000000..382237f --- /dev/null +++ b/src/sensorhub/parser.c @@ -0,0 +1,102 @@ +/* + * Framing and checksum. No I/O lives here on purpose: everything in this + * file can be exercised from a byte array, which is what makes the tests + * meaningful without a car plugged in. + */ + +#include "sensorhub/sensorhub.h" + +#include + +_Static_assert(sizeof(float) == 4, "32-bit float required"); +_Static_assert(sizeof(sh_packet_t) == SH_PAYLOAD_SIZE, "unexpected packet size"); + +uint8_t sh_checksum(const uint8_t *data, size_t length) +{ + uint8_t checksum = 0; + + if (data == NULL) { + return 0; + } + + for (size_t i = 0; i < length; ++i) { + checksum ^= data[i]; + } + + return checksum; +} + +void sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer) +{ + if (packet == NULL || buffer == NULL) { + return; + } + + buffer[0] = (uint8_t)SH_HEADER_1; + buffer[1] = (uint8_t)SH_HEADER_2; + memcpy(buffer + 2, packet, SH_PAYLOAD_SIZE); + buffer[2 + SH_PAYLOAD_SIZE] = sh_checksum(buffer + 2, SH_PAYLOAD_SIZE); +} + +void sh_parser_init(sh_parser_t *parser) +{ + if (parser == NULL) { + return; + } + + memset(parser, 0, sizeof(*parser)); + parser->state = SH_WAIT_HEADER_1; +} + +bool sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out) +{ + if (parser == NULL || out == NULL) { + return false; + } + + switch (parser->state) { + case SH_WAIT_HEADER_1: + if (byte == SH_HEADER_1) { + parser->state = SH_WAIT_HEADER_2; + } + break; + + case SH_WAIT_HEADER_2: + if (byte == SH_HEADER_2) { + parser->payload_index = 0; + parser->state = SH_READ_PAYLOAD; + } + else if (byte == SH_HEADER_1) { + /* 0xAA 0xAA 0x55 is a valid start: a payload byte that happens to + be 0xAA can precede the real header, so hold this state rather + than throwing the candidate away. */ + } + else { + parser->stats.resyncs++; + parser->state = SH_WAIT_HEADER_1; + } + break; + + case SH_READ_PAYLOAD: + parser->payload[parser->payload_index++] = byte; + + if (parser->payload_index == SH_PAYLOAD_SIZE) { + parser->state = SH_READ_CHECKSUM; + } + break; + + case SH_READ_CHECKSUM: + parser->state = SH_WAIT_HEADER_1; + + if (sh_checksum(parser->payload, SH_PAYLOAD_SIZE) == byte) { + memcpy(out, parser->payload, SH_PAYLOAD_SIZE); + parser->stats.packets++; + return true; + } + + parser->stats.checksum_errors++; + break; + } + + return false; +} diff --git a/src/sensorhub/sensorhub.h b/src/sensorhub/sensorhub.h new file mode 100644 index 0000000..e16f65e --- /dev/null +++ b/src/sensorhub/sensorhub.h @@ -0,0 +1,138 @@ +/* + * SensorHub: the Raspberry Pi side of the link to the Arduino Nano that reads + * the car's sensors. + * + * The wire format is defined by SensorController/carsensordriver.ino and must + * match it exactly: + * + * 0xAA 0x55 <23-byte packed payload> + * + * Twenty-six bytes on the wire, 115200 baud, 8N1. + * + * Parsing is deliberately separate from I/O. sh_parser_feed() takes one byte + * and never touches a file descriptor, so the framing can be tested against + * synthetic input in CI, on any machine, with no car attached. The serial + * helpers below are a convenience for callers that do have hardware. + */ + +#ifndef SENSORHUB_H +#define SENSORHUB_H + +#include +#include +#include + +#ifdef __cplusplus +extern "C" { +#endif + +#define SH_HEADER_1 0xAAu +#define SH_HEADER_2 0x55u + +/* + * Field names mirror the Arduino struct character for character. This is a + * memcpy target off the wire, so the resemblance is load-bearing: if you + * rename a field here, rename it there in the same commit. + */ +typedef struct __attribute__((packed)) { + float speed; /* ground speed, mph, from the wheel interrupt */ + float airspeed; /* pitot, mph, zeroed at Arduino startup */ + float engineTemp; /* degrees F, DS18B20 */ + float radTemp; /* degrees F, DS18B20 */ + + uint8_t channel0; /* digital, pin 4 */ + uint8_t channel1; /* digital, pin 5 */ + uint8_t channel2; /* digital, pin 6 */ + uint8_t channel3; /* digital, pin 7 */ + uint8_t channel4; /* digital, pin 8 */ + + uint16_t channelA0; /* analog, pin A7, currently battery voltage */ +} sh_packet_t; + +#define SH_PAYLOAD_SIZE 23u +#define SH_FRAME_SIZE 26u /* 2 header + payload + 1 checksum */ + +/* Both ends of this link are little-endian (AVR and aarch64). A big-endian + host would need byte swapping that no one has written, so say so loudly + rather than decoding garbage. */ +#if defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ +#error "SensorHub assumes a little-endian host; the wire format is AVR-native" +#endif + +typedef enum { + SH_WAIT_HEADER_1 = 0, + SH_WAIT_HEADER_2, + SH_READ_PAYLOAD, + SH_READ_CHECKSUM +} sh_state_t; + +/* Counters worth watching on a bench test: a link that is losing packets + shows up here long before it shows up on the dashboard. + + The width is configurable because this header also compiles for the + ATmega328p, where 64-bit counters cost RAM a Nano does not have and every + increment is a slow multi-word add. The AVR default is 32-bit rather than + 16: at roughly 40 packets a second, 16 bits wraps in under half an hour, + and a diagnostic counter that quietly lies is worse than a larger one. + 32 bits lasts about three years of continuous running for six more bytes. + Define SH_COUNTER_BITS before including to override. */ +#ifndef SH_COUNTER_BITS +# if defined(__AVR__) || defined(SH_EMBEDDED) +# define SH_COUNTER_BITS 32 +# else +# define SH_COUNTER_BITS 64 +# endif +#endif + +#if SH_COUNTER_BITS == 16 +typedef uint16_t sh_counter_t; +#elif SH_COUNTER_BITS == 32 +typedef uint32_t sh_counter_t; +#elif SH_COUNTER_BITS == 64 +typedef uint64_t sh_counter_t; +#else +#error "SH_COUNTER_BITS must be 16, 32, or 64" +#endif + +typedef struct { + sh_counter_t packets; /* accepted */ + sh_counter_t checksum_errors; /* framed correctly, contents rejected */ + sh_counter_t resyncs; /* fell back to hunting for a header */ +} sh_stats_t; + +typedef struct { + sh_state_t state; + uint8_t payload[SH_PAYLOAD_SIZE]; + size_t payload_index; + sh_stats_t stats; +} sh_parser_t; + +/* Reset a parser to hunting for a header. Also zeroes the statistics. */ +void sh_parser_init(sh_parser_t *parser); + +/* + * Feed exactly one received byte. + * + * Returns true when that byte completed a packet whose checksum matched, in + * which case *out holds it. Returns false every other time, including for a + * packet that arrived intact but failed its checksum; that case increments + * stats.checksum_errors so it can be distinguished from an idle link. + * + * Passing NULL for either argument is a no-op returning false. + */ +bool sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out); + +/* XOR of every byte, the checksum the Arduino appends. Exposed so tests and + any future transmitter can build frames without duplicating it. */ +uint8_t sh_checksum(const uint8_t *data, size_t length); + +/* Serialize a packet into a full 26-byte frame. buffer must have room for + SH_FRAME_SIZE. Used by the tests to generate input, and by any simulator + that wants to stand in for the Arduino. */ +void sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer); + +#ifdef __cplusplus +} +#endif + +#endif /* SENSORHUB_H */ diff --git a/src/sensorhub/serial.c b/src/sensorhub/serial.c new file mode 100644 index 0000000..94bbcb0 --- /dev/null +++ b/src/sensorhub/serial.c @@ -0,0 +1,132 @@ +#define _DEFAULT_SOURCE + +/* + * POSIX only. The Arduino build compiles every source under src/, including + * this one, so it must reduce to nothing on an 8-bit target rather than drag + * termios onto a Nano. + */ +#if defined(__unix__) || defined(__APPLE__) || defined(__linux__) + + +#include "sensorhub/serial.h" + +#include +#include +#include +#include + +static bool configure_serial(int fd) +{ + struct termios tty; + + if (tcgetattr(fd, &tty) != 0) { + return false; + } + + cfmakeraw(&tty); + + if (cfsetispeed(&tty, B115200) != 0 || cfsetospeed(&tty, B115200) != 0) { + return false; + } + + tty.c_cflag &= (tcflag_t)~CSIZE; + tty.c_cflag |= CS8; + tty.c_cflag |= CLOCAL | CREAD; + + tty.c_cflag &= (tcflag_t)~PARENB; + tty.c_cflag &= (tcflag_t)~PARODD; + tty.c_cflag &= (tcflag_t)~CSTOPB; +#ifdef CRTSCTS + tty.c_cflag &= (tcflag_t)~CRTSCTS; +#endif + + /* Do not drop the modem control lines on close: that is the Nano's + auto-reset circuit, and tripping it costs a reboot mid-run. */ + tty.c_cflag &= (tcflag_t)~HUPCL; + + tty.c_iflag &= (tcflag_t)~(IXON | IXOFF | IXANY); + + /* Blocking: each read waits for at least one byte. */ + tty.c_cc[VMIN] = 1; + tty.c_cc[VTIME] = 0; + + if (tcsetattr(fd, TCSANOW, &tty) != 0) { + return false; + } + + /* Deliberately no tcflush() and no DTR/RTS handling, for the same + reason as HUPCL above. */ + return true; +} + +int sh_serial_open(const char *device) +{ + int fd; + + if (device == NULL) { + device = SH_DEFAULT_PORT; + } + + fd = open(device, O_RDWR | O_NOCTTY); + + if (fd < 0) { + return -1; + } + + if (!configure_serial(fd)) { + int saved = errno; + close(fd); + errno = saved; + return -1; + } + + return fd; +} + +void sh_serial_close(int fd) +{ + if (fd >= 0) { + close(fd); + } +} + +static bool read_byte(int fd, uint8_t *value) +{ + ssize_t count; + + count = read(fd, value, 1); + + if (count == 0) { + errno = 0; /* clean EOF, distinguish from a real error */ + } + + /* EINTR is reported rather than retried. A caller with a shutdown flag + needs a chance to look at it; swallowing the signal here is what makes + a serial tool impossible to Ctrl-C. */ + return count == 1; +} + +bool sh_serial_read_packet(int fd, sh_parser_t *parser, sh_packet_t *out) +{ + uint8_t byte; + + if (parser == NULL || out == NULL) { + errno = EINVAL; + return false; + } + + while (read_byte(fd, &byte)) { + if (sh_parser_feed(parser, byte, out)) { + return true; + } + } + + return false; +} + +#else /* not POSIX */ + +/* Keep this a non-empty translation unit for compilers that dislike one. */ +typedef int sh_serial_unsupported_on_this_target; + +#endif diff --git a/src/sensorhub/serial.h b/src/sensorhub/serial.h new file mode 100644 index 0000000..cc15839 --- /dev/null +++ b/src/sensorhub/serial.h @@ -0,0 +1,59 @@ +/* + * POSIX serial transport. Separate from the parser so that the framing can be + * built and tested on any host, including ones with no termios. + * + * The careful parts here are about not resetting the Arduino. A Nano reboots + * whenever DTR is asserted, which costs a couple of seconds of telemetry and, + * worse, re-runs the airspeed zeroing with the car possibly moving. So: open + * once, clear HUPCL, and never touch the modem control lines. + */ + +#ifndef SENSORHUB_SERIAL_H +#define SENSORHUB_SERIAL_H + +#include "sensorhub/sensorhub.h" + +#ifdef __cplusplus +extern "C" { +#endif + +/* Where the CH340 lands on the Pi. /dev/serial/by-id is stabler than + /dev/ttyUSB0, which renumbers when another serial device is present. */ +#define SH_DEFAULT_PORT "/dev/ttyUSB0" +#define SH_DEFAULT_PORT_BY_ID \ + "/dev/serial/by-id/usb-1a86_USB2.0-Ser_-if00-port0" + +/* + * Open and configure a port at 115200 8N1 raw. + * + * Returns a file descriptor, or -1 with errno set. Close it with + * sh_serial_close(). Do not reopen in a retry loop without a delay: each + * open can reset the Nano. + */ +int sh_serial_open(const char *device); + +/* Close a descriptor from sh_serial_open(). Safe to call with -1. */ +void sh_serial_close(int fd); + +/* + * Block until the next valid packet arrives, feeding bytes through parser. + * + * Returns true with *out populated. Returns false on EOF, on a read error, + * or when a signal interrupted the read: + * + * errno == 0 clean EOF; on a serial port, the adapter was unplugged + * errno == EINTR a signal arrived; check your shutdown flag and call again + * otherwise a real error + * + * EINTR is surfaced rather than retried internally so that a caller can + * actually be interrupted. Install handlers with sigaction() and no + * SA_RESTART if you want Ctrl-C to work; plain signal() sets SA_RESTART on + * most platforms, which prevents read() from ever returning EINTR. + */ +bool sh_serial_read_packet(int fd, sh_parser_t *parser, sh_packet_t *out); + +#ifdef __cplusplus +} +#endif + +#endif /* SENSORHUB_SERIAL_H */ diff --git a/test/test_parser.c b/test/test_parser.c new file mode 100644 index 0000000..7236eda --- /dev/null +++ b/test/test_parser.c @@ -0,0 +1,328 @@ +/* + * Framing tests. + * + * These exist to catch the failure the old 9600-baud reader actually had: a + * dropped byte desynchronizing the stream with nothing to resynchronize + * against. So the interesting cases here are not "a good packet parses" but + * the nasty ones, garbage in front, a byte lost mid-packet, a payload that + * contains the header bytes, and a corrupted checksum. + */ + +#include "sensorhub/sensorhub.h" + +#include +#include + +static int g_failures = 0; +static int g_checks = 0; + +#define CHECK(cond, ...) \ + do { \ + g_checks++; \ + if (!(cond)) { \ + g_failures++; \ + printf("FAIL %s:%d: ", __FILE__, __LINE__); \ + printf(__VA_ARGS__); \ + printf("\n"); \ + } \ + } while (0) + +static sh_packet_t sample_packet(void) +{ + sh_packet_t p; + memset(&p, 0, sizeof(p)); + p.speed = 23.5f; + p.airspeed = 19.25f; + p.engineTemp = 180.0f; + p.radTemp = 148.5f; + p.channel0 = 1; + p.channel1 = 0; + p.channel2 = 1; + p.channel3 = 1; + p.channel4 = 0; + p.channelA0 = 812; + return p; +} + +static bool packets_equal(const sh_packet_t *a, const sh_packet_t *b) +{ + return memcmp(a, b, sizeof(sh_packet_t)) == 0; +} + +/* Feed a buffer through a parser, returning how many packets came out. */ +static int feed_all(sh_parser_t *parser, const uint8_t *data, size_t len, + sh_packet_t *last) +{ + int count = 0; + sh_packet_t out; + + for (size_t i = 0; i < len; ++i) { + if (sh_parser_feed(parser, data[i], &out)) { + count++; + if (last != NULL) { + *last = out; + } + } + } + + return count; +} + +static void test_roundtrip(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + + sh_encode_frame(&in, frame); + sh_parser_init(&parser); + + CHECK(feed_all(&parser, frame, sizeof(frame), &out) == 1, + "one frame should yield one packet"); + CHECK(packets_equal(&in, &out), "round-tripped packet should match"); + CHECK(parser.stats.packets == 1, "packet counter should be 1"); + CHECK(parser.stats.checksum_errors == 0, "no checksum errors expected"); +} + +static void test_packet_size_is_on_the_wire_contract(void) +{ + CHECK(sizeof(sh_packet_t) == 23, "payload must stay 23 bytes"); + CHECK(SH_FRAME_SIZE == 26, "frame must stay 26 bytes"); +} + +static void test_leading_garbage(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t buf[64]; + sh_parser_t parser; + size_t n = 0; + + /* Junk, including a lone header byte, before a good frame. */ + buf[n++] = 0x00; + buf[n++] = 0xFF; + buf[n++] = SH_HEADER_1; + buf[n++] = 0x12; + buf[n++] = 0x34; + + sh_encode_frame(&in, buf + n); + n += SH_FRAME_SIZE; + + sh_parser_init(&parser); + CHECK(feed_all(&parser, buf, n, &out) == 1, + "should recover from leading garbage"); + CHECK(packets_equal(&in, &out), "recovered packet should match"); +} + +static void test_payload_containing_header_bytes(void) +{ + /* A payload byte equal to 0xAA immediately before the real 0xAA 0x55 is + the case that a naive two-state matcher gets wrong. */ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t buf[8 + SH_FRAME_SIZE]; + sh_parser_t parser; + size_t n = 0; + + buf[n++] = SH_HEADER_1; + buf[n++] = SH_HEADER_1; + buf[n++] = SH_HEADER_1; + + sh_encode_frame(&in, buf + n); + n += SH_FRAME_SIZE; + + sh_parser_init(&parser); + CHECK(feed_all(&parser, buf, n, &out) == 1, + "repeated 0xAA before a header must not break framing"); + CHECK(packets_equal(&in, &out), "packet after repeated 0xAA should match"); +} + +static void test_bad_checksum_is_counted_not_silent(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + + sh_encode_frame(&in, frame); + frame[SH_FRAME_SIZE - 1] ^= 0xFFu; /* corrupt the checksum */ + + sh_parser_init(&parser); + CHECK(feed_all(&parser, frame, sizeof(frame), &out) == 0, + "a bad checksum must not produce a packet"); + CHECK(parser.stats.checksum_errors == 1, + "a bad checksum must be counted, not dropped silently"); +} + +static void test_corrupt_payload_is_rejected(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + + sh_encode_frame(&in, frame); + frame[5] ^= 0x01u; /* flip a bit in the payload, checksum now wrong */ + + sh_parser_init(&parser); + CHECK(feed_all(&parser, frame, sizeof(frame), &out) == 0, + "a corrupted payload must be rejected"); + CHECK(parser.stats.checksum_errors == 1, "corruption should be counted"); +} + +static void test_resync_after_dropped_byte(void) +{ + /* The real-world failure: one byte lost in transit. The truncated frame + must be discarded and the NEXT frame must still parse. Without a + header this is exactly where the old reader went permanently wrong. */ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t buf[SH_FRAME_SIZE * 3]; + sh_parser_t parser; + size_t n = 0; + + sh_encode_frame(&in, buf); + /* Copy all but the last byte of frame one: a dropped tail. */ + n = SH_FRAME_SIZE - 1; + + sh_encode_frame(&in, buf + n); + n += SH_FRAME_SIZE; + sh_encode_frame(&in, buf + n); + n += SH_FRAME_SIZE; + + sh_parser_init(&parser); + int got = feed_all(&parser, buf, n, &out); + + CHECK(got >= 1, "must resynchronize after a dropped byte, got %d", got); + CHECK(packets_equal(&in, &out), "post-resync packet should match"); +} + +static void test_split_across_reads(void) +{ + /* Serial reads arrive in arbitrary chunks; the parser is byte-at-a-time + so this should be invisible, but prove it rather than assume it. */ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + int count = 0; + + sh_encode_frame(&in, frame); + sh_parser_init(&parser); + + for (size_t i = 0; i < sizeof(frame); ++i) { + if (sh_parser_feed(&parser, frame[i], &out)) { + count++; + } + } + + CHECK(count == 1, "byte-at-a-time feeding should yield exactly one packet"); + CHECK(packets_equal(&in, &out), "split packet should match"); +} + +static void test_back_to_back_frames(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t buf[SH_FRAME_SIZE * 5]; + sh_parser_t parser; + + for (int i = 0; i < 5; ++i) { + in.speed = (float)i; + sh_encode_frame(&in, buf + ((size_t)i * SH_FRAME_SIZE)); + } + + sh_parser_init(&parser); + CHECK(feed_all(&parser, buf, sizeof(buf), &out) == 5, + "five frames should yield five packets"); + CHECK(out.speed == 4.0f, "last packet should be the last one sent"); +} + +static void test_null_arguments_are_safe(void) +{ + sh_parser_t parser; + sh_packet_t out; + + sh_parser_init(NULL); /* must not crash */ + sh_parser_init(&parser); + + CHECK(!sh_parser_feed(NULL, 0x00, &out), "NULL parser should return false"); + CHECK(!sh_parser_feed(&parser, 0x00, NULL), "NULL out should return false"); + CHECK(sh_checksum(NULL, 10) == 0, "NULL checksum input should return 0"); + sh_encode_frame(NULL, NULL); /* must not crash */ +} + +static void test_checksum_matches_the_arduino(void) +{ + /* The Arduino XORs every payload byte. Pin the algorithm with a value + computed by hand so a "clever" rewrite cannot quietly change it. */ + const uint8_t data[] = { 0x01, 0x02, 0x03 }; /* 1^2^3 == 0 */ + const uint8_t data2[] = { 0xAA, 0x55 }; /* 0xAA^0x55 == 0xFF */ + + CHECK(sh_checksum(data, sizeof(data)) == 0x00, "1^2^3 should be 0"); + CHECK(sh_checksum(data2, sizeof(data2)) == 0xFF, "0xAA^0x55 should be 0xFF"); +} + +static void test_wire_format_is_frozen(void) +{ + /* + * A golden frame, byte for byte. + * + * This is the contract with carsensordriver.ino on the Arduino and with + * every CSV already on disk. It was verified byte-identical against the + * firmware's original hand-rolled send path over 200,000 random packets + * before that code was deleted in favour of sh_encode_frame(). + * + * If this test fails, the wire format changed. That is not a test to + * update; it is a flash-the-Arduino-and-tell-everyone event. + */ + static const uint8_t golden[SH_FRAME_SIZE] = { + 0xAA, 0x55, /* header */ + 0x00, 0x00, 0xBC, 0x41, /* speed 23.5 */ + 0x00, 0x00, 0x9A, 0x41, /* airspeed 19.25 */ + 0x00, 0x00, 0x34, 0x43, /* engineTemp 180.0 */ + 0x00, 0x80, 0x14, 0x43, /* radTemp 148.5 */ + 0x01, 0x00, 0x01, 0x01, 0x00, /* channel0..4 */ + 0x2C, 0x03, /* channelA0 812 */ + 0xA8 /* XOR checksum */ + }; + + sh_packet_t in = sample_packet(); + uint8_t frame[SH_FRAME_SIZE]; + + sh_encode_frame(&in, frame); + + CHECK(memcmp(frame, golden, sizeof(golden)) == 0, + "encoded frame must match the frozen wire format byte for byte"); + + /* And the parser must accept its own golden frame, which catches an + encoder and decoder that drifted together. */ + { + sh_parser_t parser; + sh_packet_t out; + sh_parser_init(&parser); + CHECK(feed_all(&parser, golden, sizeof(golden), &out) == 1, + "the frozen frame must still decode"); + CHECK(packets_equal(&in, &out), "decoded golden frame should match"); + } +} + +int main(void) +{ + test_packet_size_is_on_the_wire_contract(); + test_wire_format_is_frozen(); + test_checksum_matches_the_arduino(); + test_roundtrip(); + test_leading_garbage(); + test_payload_containing_header_bytes(); + test_bad_checksum_is_counted_not_silent(); + test_corrupt_payload_is_rejected(); + test_resync_after_dropped_byte(); + test_split_across_reads(); + test_back_to_back_frames(); + test_null_arguments_are_safe(); + + printf("%d checks, %d failures\n", g_checks, g_failures); + return g_failures == 0 ? 0 : 1; +} diff --git a/test/test_serial_pty.py b/test/test_serial_pty.py new file mode 100755 index 0000000..879e297 --- /dev/null +++ b/test/test_serial_pty.py @@ -0,0 +1,98 @@ +#!/usr/bin/env python3 +""" +End-to-end check against a fake Arduino. + +Opens a pty, speaks the exact wire format carsensordriver.ino speaks, and runs +sensorhub-monitor against the other end. This is the test that covers the +parts a unit test cannot: termios setup, blocking reads, signal handling, and +the framing under a realistic mix of good frames, line noise, and corruption. + +It also pins behaviour that is easy to regress: a corrupt packet must be +counted rather than silently dropped, and the tool must exit on a signal +instead of hanging in a blocking read. + + ./test/test_serial_pty.py build/sensorhub-monitor +""" + +import os +import pty +import struct +import subprocess +import sys +import time + + +def frame(speed, airspeed, engine_temp, rad_temp, channels, a0): + """Build one 26-byte frame: 0xAA 0x55, 23-byte payload, XOR checksum.""" + payload = struct.pack( + "") + + monitor = sys.argv[1] + master, slave = pty.openpty() + device = os.ttyname(slave) + + proc = subprocess.Popen( + [monitor, device], stdout=subprocess.PIPE, stderr=subprocess.STDOUT + ) + time.sleep(0.5) + + # A good frame. + os.write(master, frame(23.5, 19.25, 180.0, 148.5, [1, 0, 1, 1, 0], 812)) + time.sleep(0.2) + + # Line noise, then a frame with a deliberately corrupted checksum. The + # parser must reject the second and still recover for the third. + os.write(master, b"\x00\xff\xde\xad\xbe\xef") + corrupt = bytearray(frame(11.0, 2.0, 1.0, 2.0, [0, 0, 0, 0, 0], 5)) + corrupt[-1] ^= 0xFF + os.write(master, bytes(corrupt)) + time.sleep(0.2) + + # A good frame after the corruption: this is the resync case. + os.write(master, frame(31.25, 5.5, 190.0, 150.0, [0, 1, 0, 1, 1], 900)) + time.sleep(0.8) + + proc.terminate() + + try: + out = proc.communicate(timeout=5)[0].decode(errors="replace") + except subprocess.TimeoutExpired: + proc.kill() + proc.communicate() + sys.exit("FAIL: monitor did not exit on SIGTERM (stuck in a blocking read?)") + + print(out) + + failures = [] + if "speed=23.50" not in out: + failures.append("first good packet was not decoded") + if "speed=31.25" not in out: + failures.append("did not recover after corruption") + if "ok=2" not in out: + failures.append("expected exactly 2 accepted packets") + if "bad=1" not in out: + failures.append("corrupt packet was not counted as a checksum error") + + if failures: + for f in failures: + print("FAIL:", f) + sys.exit(1) + + print("PASS: framing, recovery, and shutdown all behaved") + + +if __name__ == "__main__": + main() diff --git a/tools/monitor.c b/tools/monitor.c new file mode 100644 index 0000000..90fefe3 --- /dev/null +++ b/tools/monitor.c @@ -0,0 +1,116 @@ +/* + * sensorhub-monitor: print live packets from the car. + * + * This is the bench tool. Point it at the Arduino and you can see whether the + * link is healthy and whether a given sensor is actually wired, without + * involving the display or any of the rest of the stack. + * + * sensorhub-monitor [device] + */ + +#include "sensorhub/serial.h" + +#include +#include +#include +#include +#include + +static volatile sig_atomic_t g_stop = 0; + +static void on_signal(int signum) +{ + (void)signum; + g_stop = 1; +} + +static void install_handler(int signum) +{ + struct sigaction sa; + + memset(&sa, 0, sizeof(sa)); + sa.sa_handler = on_signal; + sigemptyset(&sa.sa_mask); + /* No SA_RESTART on purpose: we want read() to return EINTR so the loop + below can notice g_stop. signal() would set SA_RESTART and make this + process impossible to Ctrl-C out of a blocking read. */ + sa.sa_flags = 0; + (void)sigaction(signum, &sa, NULL); +} + +static void print_packet(const sh_packet_t *packet, const sh_stats_t *stats) +{ + printf("\r\x1b[2K" + "speed=%.2f mph" + " | air=%.2f" + " | engine=%.1fF" + " | rad=%.1fF" + " | ch=%u%u%u%u%u" + " | A0=%u" + " | ok=%llu bad=%llu resync=%llu", + (double)packet->speed, + (double)packet->airspeed, + (double)packet->engineTemp, + (double)packet->radTemp, + (unsigned)packet->channel0, + (unsigned)packet->channel1, + (unsigned)packet->channel2, + (unsigned)packet->channel3, + (unsigned)packet->channel4, + (unsigned)packet->channelA0, + (unsigned long long)stats->packets, + (unsigned long long)stats->checksum_errors, + (unsigned long long)stats->resyncs); + + fflush(stdout); +} + +int main(int argc, char **argv) +{ + const char *device = (argc > 1) ? argv[1] : SH_DEFAULT_PORT; + sh_parser_t parser; + sh_packet_t packet; + int fd; + + signal(SIGPIPE, SIG_IGN); + install_handler(SIGINT); + install_handler(SIGTERM); + + /* Open once. Reopening resets the Nano through its DTR circuit. */ + fd = sh_serial_open(device); + + if (fd < 0) { + fprintf(stderr, "open %s: %s\n", device, strerror(errno)); + return 1; + } + + sh_parser_init(&parser); + printf("Listening on %s at 115200 baud\n", device); + + while (!g_stop) { + memset(&packet, 0, sizeof(packet)); + + if (!sh_serial_read_packet(fd, &parser, &packet)) { + if (errno == EINTR) { + continue; /* a signal; the while condition rechecks g_stop */ + } + if (errno != 0) { + fprintf(stderr, "\nserial read: %s\n", strerror(errno)); + } + else { + fprintf(stderr, "\nserial connection closed\n"); + } + break; + } + + print_packet(&packet, &parser.stats); + } + + printf("\n%llu packets, %llu checksum errors, %llu resyncs\n", + (unsigned long long)parser.stats.packets, + (unsigned long long)parser.stats.checksum_errors, + (unsigned long long)parser.stats.resyncs); + + sh_serial_close(fd); + return 0; +} From f9133243fd6cf30f1e109fed9c09f841bf9b0794 Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 19 Sep 2026 12:06:01 -0400 Subject: [PATCH 2/4] docs: describe how to use the library from both ends --- README.md | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 253 insertions(+) diff --git a/README.md b/README.md index 07c1201..b4af479 100644 --- a/README.md +++ b/README.md @@ -1 +1,254 @@ # SensorHub + +The wire format for the link between the car's Arduino Nano and the Raspberry +Pi, used by **both** ends so the two cannot drift apart. + +The Arduino sends, the Pi receives, and there is exactly one definition of the +packet, the checksum, and the frame encoder. + +## The wire format + +Twenty-six bytes per frame, 115200 baud, 8N1: + +``` +0xAA 0x55 <23-byte packed payload> +``` + +The payload is `sh_packet_t`, a packed struct of fixed-width types. It is 23 +bytes on the Pi and on an ATmega328p alike, asserted at compile time on both. + +| offset | type | field | notes | +|---|---|---|---| +| 0 | `float` | `speed` | mph, from the wheel interrupt | +| 4 | `float` | `airspeed` | mph, pitot, zeroed at Arduino startup | +| 8 | `float` | `engineTemp` | °F, DS18B20 | +| 12 | `float` | `radTemp` | °F, DS18B20 | +| 16 | `uint8_t` | `channel0` | digital, pin 4 | +| 17 | `uint8_t` | `channel1` | digital, pin 5 | +| 18 | `uint8_t` | `channel2` | digital, pin 6 | +| 19 | `uint8_t` | `channel3` | digital, pin 7 | +| 20 | `uint8_t` | `channel4` | digital, pin 8 | +| 21 | `uint16_t` | `channelA0` | analog, pin A7 | + +The first four fields are fixed. The last six are deliberately anonymous: +what they *mean* is a wiring decision, and naming them is the consumer's job. + +Both ends are little-endian. A big-endian host is rejected with an `#error` +rather than quietly decoding nonsense. + +## Sending, on the Arduino + +```sh +arduino-cli lib install --git-url https://github.com/HEEV/SensorHub +``` + +```c +#include + +void sendPacket(const sh_packet_t &packet) { + uint8_t frame[SH_FRAME_SIZE]; + sh_encode_frame(&packet, frame); + Serial.write(frame, sizeof(frame)); +} +``` + +`sh_encode_frame()` writes header, payload, and checksum into a +`SH_FRAME_SIZE` buffer. One buffered `Serial.write` beats four small ones. + +Costs about **22 bytes of flash** and no RAM over hand-rolling it. + +Note the include is ``, not the nested path. Arduino resolves +libraries by top-level header name, so that shim is what makes the library +discoverable from a sketch. Everywhere else, use ``. + +## Receiving, on the Pi + +```c +#include +#include + +sh_parser_t parser; +sh_packet_t packet; + +int fd = sh_serial_open("/dev/ttyUSB0"); /* NULL for the default port */ +if (fd < 0) { perror("open"); return 1; } + +sh_parser_init(&parser); + +while (sh_serial_read_packet(fd, &parser, &packet)) { + printf("%.2f mph\n", (double) packet.speed); +} + +sh_serial_close(fd); +``` + +`sh_serial_open()` sets 115200 8N1 raw and, importantly, **clears `HUPCL` and +never touches DTR or RTS**. A Nano reboots when DTR is asserted, which costs +a couple of seconds of telemetry and re-runs the airspeed zeroing with the car +possibly moving. Open the port once and keep it open. + +Prefer the stable device path over `/dev/ttyUSB0`, which renumbers when a +second serial device is present: + +```c +sh_serial_open(SH_DEFAULT_PORT_BY_ID); +``` + +## Parsing without a serial port + +The parser never touches a file descriptor. Feed it bytes from wherever you +got them: a socket, a file, a test fixture, a non-blocking read of your own. + +```c +bool sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out); +``` + +Returns `true` exactly when that byte completed a packet whose checksum +matched, with the result in `*out`. Every other byte returns `false`. + +This is what makes the library testable with no car attached, and it is how +CarDisplay drains a socket without blocking its UI thread: + +```c +uint8_t buf[256]; +ssize_t n; + +while ((n = read(fd, buf, sizeof(buf))) > 0) { /* O_NONBLOCK */ + for (ssize_t i = 0; i < n; ++i) { + if (sh_parser_feed(&parser, buf[i], &packet)) { + apply(&packet); /* keep the newest; a backlog means you fell behind */ + } + } +} +``` + +## Link health + +The parser keeps counters. A link that is losing packets shows up here long +before it shows up on the dashboard. + +```c +printf("ok=%llu bad=%llu resync=%llu\n", + (unsigned long long) parser.stats.packets, + (unsigned long long) parser.stats.checksum_errors, + (unsigned long long) parser.stats.resyncs); +``` + +| counter | meaning | +|---|---| +| `packets` | accepted | +| `checksum_errors` | framed correctly, contents rejected | +| `resyncs` | fell back to hunting for a header | + +A rising `checksum_errors` is electrical: noise, a marginal cable, a bad +ground. A rising `resyncs` with few checksum errors usually means the sender +is being interrupted mid-frame. + +Counter width is `uint64_t` on a host and `uint32_t` on AVR. Not 16-bit: at +roughly 40 packets a second that wraps in under half an hour, and a diagnostic +counter that quietly lies is worse than a larger one. Override with +`-DSH_COUNTER_BITS=16|32|64` if you must, but note it changes +`sizeof(sh_stats_t)`, so everything linked together has to agree. + +## Recovering from a bad byte + +This is the failure the old 9600-baud reader actually had: one dropped byte +desynchronised the stream and there was nothing to resynchronise against. + +The `0xAA 0x55` header fixes that, and the parser handles the subtle case too. +A payload byte that happens to be `0xAA` sitting immediately before a real +header does not break framing, because the state machine holds its candidate +rather than discarding it. A truncated frame is dropped and the next one +parses. + +You do not have to do anything to get this. It is just worth knowing it is +there, and there are tests for each case. + +## Building + +CMake, as a subproject: + +```cmake +add_subdirectory(SensorHub) +target_link_libraries(your_app PRIVATE sensorhub) +``` + +Standalone, with the bench tool and tests: + +```sh +cmake -S . -B build && cmake --build build -j +ctest --test-dir build +``` + +Options: `SH_BUILD_MONITOR` and `SH_BUILD_TESTS`, both on when this is the +top-level project and off when it is a subproject. + +Layout note: sources live under `src/sensorhub/` rather than the usual +`include/` and `src/` split, so that this directory is simultaneously a valid +Arduino library. `arduino-cli` puts `/src` on the include path, which +makes `` resolve identically on both targets. +`src/sensorhub/serial.c` is guarded so it compiles to zero bytes on AVR. + +## The bench tool + +```sh +./build/sensorhub-monitor [device] +``` + +Prints live packets and running counters. This is how you find out whether a +sensor is actually wired without involving the display or anything else: + +``` +speed=23.50 mph | air=19.25 | engine=180.0F | rad=148.5F | ch=10110 | A0=812 | ok=2 bad=1 resync=0 +``` + +Ctrl-C works, which is less obvious than it sounds. `signal()` installs +handlers with `SA_RESTART` on most platforms, so a blocking `read()` +auto-restarts and never returns, making the tool impossible to interrupt. The +library surfaces `EINTR` rather than retrying it internally so the caller can +check its own shutdown flag: + +```c +if (!sh_serial_read_packet(fd, &parser, &packet)) { + if (errno == EINTR) continue; /* a signal: check your stop flag */ + if (errno == 0) break; /* clean EOF: the adapter was unplugged */ + perror("serial read"); + break; +} +``` + +Install your handlers with `sigaction()` and no `SA_RESTART` if you want this +to work. + +## API summary + +From ``, portable everywhere: + +```c +void sh_parser_init (sh_parser_t *parser); +bool sh_parser_feed (sh_parser_t *parser, uint8_t byte, sh_packet_t *out); +uint8_t sh_checksum (const uint8_t *data, size_t length); +void sh_encode_frame (const sh_packet_t *packet, uint8_t *buffer); +``` + +From ``, POSIX only: + +```c +int sh_serial_open (const char *device); +void sh_serial_close (int fd); +bool sh_serial_read_packet(int fd, sh_parser_t *parser, sh_packet_t *out); +``` + +Every function tolerates `NULL` arguments by doing nothing and returning a +falsy value rather than crashing. + +## Changing the wire format + +Don't, casually. The format is frozen by a golden-frame test that compares +`sh_encode_frame()` output byte for byte against a known-good frame, verified +identical to the firmware's original hand-rolled encoder across 200,000 random +packets. + +If that test fails, it is not a test to update. It means every Arduino in the +fleet needs reflashing and every consumer needs checking, including the CSVs +already on disk. Change it deliberately, in one commit, on both ends. From d7be2ec518a3f59a66e153368b6e785c0d98894e Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 19 Sep 2026 12:46:34 -0400 Subject: [PATCH 3/4] feat: version and length the frame, and widen the payload A same-size packet with a field that changed meaning passes the checksum and decodes as confident nonsense. That is not hypothetical: the Python server's channel names silently changed in April and nobody noticed for months. The frame now carries a format version and a payload length. The version turns a silent misdecode into a loud refusal. The length lets an old receiver skip a packet from a newer sender and stay framed instead of desynchronising. Two bytes on a link running at 7% utilisation. The payload gains room so the next sensor is a wiring job rather than a format change that invalidates every CSV on disk: four temperature slots instead of two named floats, four analog slots instead of one, and digital channels as bits, which costs less than the bytes they replace. Output channels are now reported too. The firmware drives a radiator fan and a water pump whose state appeared nowhere in telemetry, so there was no way to see what the car was doing to itself. A sequence number makes dropped packets visible. A checksum cannot tell you about a packet that never arrived; a link losing half its traffic looked exactly like a healthy one. The API reports errors instead of returning a bare bool: a status enum with a message for every value, bounds-checked channel accessors, and an encoder that takes its buffer size and refuses rather than overruns. --- .github/workflows/ci.yml | 39 +++- README.md | 180 +++++++++++---- src/sensorhub/parser.c | 235 +++++++++++++++++--- src/sensorhub/sensorhub.h | 273 ++++++++++++++++++----- src/sensorhub/serial.c | 46 ++-- src/sensorhub/serial.h | 29 ++- test/test_parser.c | 454 +++++++++++++++++++++++++++++--------- test/test_serial_pty.py | 59 +++-- tools/monitor.c | 73 +++--- 9 files changed, 1054 insertions(+), 334 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index fafa5b4..f16f673 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -113,14 +113,22 @@ jobs: fi cp /tmp/parser.orig src/sensorhub/parser.c } + sed -i 's/if (parser->format != SH_FORMAT_CURRENT) {/if (0) {/' src/sensorhub/parser.c + check "accepted any format version" + sed -i 's/parser->stats.dropped += gap;/;/' src/sensorhub/parser.c + check "stopped counting dropped packets" + sed -i 's/gap < 1000u/gap < 65535u/' src/sensorhub/parser.c + check "treated a sender restart as a flood of drops" + sed -i 's/sh_checksum(buffer + 2, SH_PAYLOAD_SIZE + 2u)/sh_checksum(buffer + 4, SH_PAYLOAD_SIZE)/' src/sensorhub/parser.c + check "checksummed the payload only, not fmt and len" sed -i 's/parser->stats.checksum_errors++;/;/' src/sensorhub/parser.c check "dropped checksum-error counting" - sed -i 's/memcpy(out, parser->payload, SH_PAYLOAD_SIZE);/memset(out,0,23);/' src/sensorhub/parser.c - check "corrupted the packet copy" sed -i 's/else if (byte == SH_HEADER_1) {/else if (0) {/' src/sensorhub/parser.c check "broke repeated-header handling" - sed -i 's/== byte)/!= byte)/' src/sensorhub/parser.c - check "inverted the checksum comparison" + sed -i 's/if (channel > SH_DIGITAL_MAX) return SH_E_RANGE;/;/' src/sensorhub/parser.c + check "dropped the digital channel bounds check" + sed -i 's/if (buffer_size < SH_FRAME_SIZE) return SH_E_SPACE;/;/' src/sensorhub/parser.c + check "dropped the encode buffer size check" cmake --build build -j >/dev/null 2>&1 exit $fail @@ -151,12 +159,17 @@ jobs: uint8_t frame[SH_FRAME_SIZE]; memset(&in, 0, sizeof(in)); in.speed = 12.5f; - sh_encode_frame(&in, frame); + in.temps[SH_TEMP_ENGINE] = 190.0f; + sh_set_digital_in(&in, 2, true); + if (sh_encode_frame(&in, frame, sizeof(frame), NULL) != SH_OK) + return 1; sh_parser_init(&parser); for (size_t i = 0; i < sizeof(frame); ++i) { - if (sh_parser_feed(&parser, frame[i], &out)) { - printf("speed=%.1f\n", (double) out.speed); - return out.speed == 12.5f ? 0 : 1; + if (sh_parser_feed(&parser, frame[i], &out) == SH_OK) { + bool ch2 = false; + sh_digital_in(&out, 2, &ch2); + printf("speed=%.1f ch2=%d\n", (double) out.speed, (int) ch2); + return (out.speed == 12.5f && ch2) ? 0 : 1; } } return 1; @@ -193,14 +206,16 @@ jobs: cat > /tmp/avr_probe.c <<'C' #include /* int is 16 bits here; the packed fixed-width layout must survive */ - _Static_assert(sizeof(sh_packet_t) == 23, "struct differs on AVR"); + _Static_assert(sizeof(sh_packet_t) == 36, "struct differs on AVR"); _Static_assert(sizeof(float) == 4, "float differs on AVR"); volatile unsigned char sink; int main(void) { sh_packet_t p = {0}; uint8_t f[SH_FRAME_SIZE]; - sh_encode_frame(&p, f); - sink = f[SH_FRAME_SIZE - 1]; + size_t n = 0; + sh_set_digital_out(&p, 1, true); + if (sh_encode_frame(&p, f, sizeof(f), &n) != SH_OK) return 1; + sink = f[n - 1]; return 0; } C @@ -211,7 +226,7 @@ jobs: - name: It must also build as C++, since .ino files are C++ run: | - printf '#include \nstatic_assert(sizeof(sh_packet_t)==23,"");\nint main(){return 0;}\n' > /tmp/probe.cpp + printf '#include \nstatic_assert(sizeof(sh_packet_t)==36,"");\nint main(){return 0;}\n' > /tmp/probe.cpp avr-g++ -mmcu=atmega328p -Os -std=gnu++17 -Wall -Wextra \ -Isrc -c /tmp/probe.cpp -o /tmp/probe.o diff --git a/README.md b/README.md index b4af479..1e413c8 100644 --- a/README.md +++ b/README.md @@ -8,30 +8,58 @@ packet, the checksum, and the frame encoder. ## The wire format -Twenty-six bytes per frame, 115200 baud, 8N1: +Forty-one bytes per frame, 115200 baud, 8N1: ``` -0xAA 0x55 <23-byte packed payload> +0xAA 0x55 fmt len payload[len] checksum ``` -The payload is `sh_packet_t`, a packed struct of fixed-width types. It is 23 -bytes on the Pi and on an ATmega328p alike, asserted at compile time on both. +| field | size | purpose | +|---|---|---| +| `0xAA 0x55` | 2 | header, for resynchronising | +| `fmt` | 1 | format version, currently `0x01` | +| `len` | 1 | payload length, currently 36 | +| payload | `len` | `sh_packet_t` | +| `checksum` | 1 | XOR of `fmt`, `len`, and every payload byte | + +**The version and length bytes earn their keep.** Without a version, changing +what a field *means* while keeping the packet the same size still passes the +checksum, and the receiver decodes confidently wrong data forever. That is not +hypothetical: the old Python server's channel names silently changed in April +and nobody noticed for months. Without a length, an old receiver facing a +newer sender desynchronises instead of skipping one packet and carrying on. + +Two bytes on a link running at 7% utilisation is a good trade. + +The checksum deliberately covers `fmt` and `len` as well as the payload, so a +corrupted length byte cannot quietly reframe the stream and still validate. + +### Payload, format 1 + +`sh_packet_t`, a packed struct of fixed-width types, 36 bytes on the Pi and on +an ATmega328p alike, asserted at compile time on both. | offset | type | field | notes | |---|---|---|---| | 0 | `float` | `speed` | mph, from the wheel interrupt | | 4 | `float` | `airspeed` | mph, pitot, zeroed at Arduino startup | -| 8 | `float` | `engineTemp` | °F, DS18B20 | -| 12 | `float` | `radTemp` | °F, DS18B20 | -| 16 | `uint8_t` | `channel0` | digital, pin 4 | -| 17 | `uint8_t` | `channel1` | digital, pin 5 | -| 18 | `uint8_t` | `channel2` | digital, pin 6 | -| 19 | `uint8_t` | `channel3` | digital, pin 7 | -| 20 | `uint8_t` | `channel4` | digital, pin 8 | -| 21 | `uint16_t` | `channelA0` | analog, pin A7 | - -The first four fields are fixed. The last six are deliberately anonymous: -what they *mean* is a wiring decision, and naming them is the consumer's job. +| 8 | `float[4]` | `temps` | °F. `[0]` engine, `[1]` radiator, 2 spare | +| 24 | `uint16_t[4]` | `analog` | raw ADC counts. `[0]` battery, 3 spare | +| 32 | `uint8_t` | `digital_in` | bitfield, input channels 0–7 | +| 33 | `uint8_t` | `digital_out` | bitfield, output channels 0–7 | +| 34 | `uint16_t` | `sequence` | wraps; gaps mean dropped packets | + +Three things worth noting about that layout: + +- **Spare slots are deliberate.** Adding a fourth temperature probe to the + DS18B20 bus should be a wiring job, not a format change that invalidates + every CSV on disk. +- **Digital channels are bits.** A switch carries one bit, and sixteen of them + fit where two bytes used to go. +- **Outputs are reported, not just inputs.** The firmware drives a radiator + fan and a water pump, and until now their state appeared nowhere in + telemetry, so there was no way to see or log what the car was doing to + itself. Both ends are little-endian. A big-endian host is rejected with an `#error` rather than quietly decoding nonsense. @@ -45,17 +73,33 @@ arduino-cli lib install --git-url https://github.com/HEEV/SensorHub ```c #include -void sendPacket(const sh_packet_t &packet) { +static uint16_t sequence = 0; + +void sendPacket(sh_packet_t &packet) { uint8_t frame[SH_FRAME_SIZE]; - sh_encode_frame(&packet, frame); - Serial.write(frame, sizeof(frame)); + size_t written = 0; + + packet.sequence = sequence++; + + if (sh_encode_frame(&packet, frame, sizeof(frame), &written) != SH_OK) { + return; /* refuses rather than writing a half-built frame */ + } + Serial.write(frame, written); } ``` -`sh_encode_frame()` writes header, payload, and checksum into a -`SH_FRAME_SIZE` buffer. One buffered `Serial.write` beats four small ones. +Build the packet with the accessors rather than poking bits by hand, which is +how off-by-one channel bugs happen: -Costs about **22 bytes of flash** and no RAM over hand-rolling it. +```c +packet.temps[SH_TEMP_ENGINE] = engineTempF; +packet.analog[SH_ANALOG_BATTERY] = analogRead(A7); +sh_set_digital_in(&packet, 0, digitalRead(4)); +sh_set_digital_out(&packet, 0, digitalRead(RAD_FAN_PIN)); +``` + +Remember to increment `sequence` every packet. It is the only way the receiver +can tell that a packet never arrived; a checksum cannot. Note the include is ``, not the nested path. Arduino resolves libraries by top-level header name, so that shim is what makes the library @@ -75,13 +119,18 @@ if (fd < 0) { perror("open"); return 1; } sh_parser_init(&parser); -while (sh_serial_read_packet(fd, &parser, &packet)) { - printf("%.2f mph\n", (double) packet.speed); +while (sh_serial_read_packet(fd, &parser, &packet) == SH_OK) { + printf("#%u %.2f mph\n", packet.sequence, (double) packet.speed); } sh_serial_close(fd); ``` +`sh_serial_read_packet` keeps reading through recoverable framing errors. A +bad checksum or an unknown format is counted in `parser.stats` and the read +continues, because one corrupt packet is not a reason to make every caller +write a retry loop. Only a completed packet or an I/O condition ends the call. + `sh_serial_open()` sets 115200 8N1 raw and, importantly, **clears `HUPCL` and never touches DTR or RTS**. A Nano reboots when DTR is asserted, which costs a couple of seconds of telemetry and re-runs the airspeed zeroing with the car @@ -100,11 +149,20 @@ The parser never touches a file descriptor. Feed it bytes from wherever you got them: a socket, a file, a test fixture, a non-blocking read of your own. ```c -bool sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out); +sh_status_t sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out); ``` -Returns `true` exactly when that byte completed a packet whose checksum -matched, with the result in `*out`. Every other byte returns `false`. +| returns | meaning | +|---|---| +| `SH_OK` | `*out` holds a packet | +| `SH_INCOMPLETE` | byte consumed, nothing complete yet. The usual answer | +| `SH_E_CHECKSUM` | a frame arrived intact but its contents were rejected | +| `SH_E_FORMAT` | a version we do not know; skipped cleanly using `len` | +| `SH_E_LENGTH` | our version, an impossible length | + +Errors are reported, not thrown. The parser stays usable and keeps counting, +so a caller that only wants packets can compare against `SH_OK` and read +`parser.stats` occasionally. This is what makes the library testable with no car attached, and it is how CarDisplay drains a socket without blocking its UI thread: @@ -115,7 +173,7 @@ ssize_t n; while ((n = read(fd, buf, sizeof(buf))) > 0) { /* O_NONBLOCK */ for (ssize_t i = 0; i < n; ++i) { - if (sh_parser_feed(&parser, buf[i], &packet)) { + if (sh_parser_feed(&parser, buf[i], &packet) == SH_OK) { apply(&packet); /* keep the newest; a backlog means you fell behind */ } } @@ -134,15 +192,20 @@ printf("ok=%llu bad=%llu resync=%llu\n", (unsigned long long) parser.stats.resyncs); ``` -| counter | meaning | -|---|---| -| `packets` | accepted | -| `checksum_errors` | framed correctly, contents rejected | -| `resyncs` | fell back to hunting for a header | +| counter | meaning | what it usually indicates | +|---|---|---| +| `packets` | accepted | — | +| `checksum_errors` | framed correctly, contents rejected | electrical: noise, a marginal cable, a bad ground | +| `resyncs` | fell back to hunting for a header | the sender is being interrupted mid-frame | +| `format_errors` | a version we do not understand | firmware and receiver are out of step | +| `dropped` | inferred from gaps in `sequence` | packets never arrived; the receiver is not keeping up, or the sender restarted | + +`dropped` is the one a checksum cannot give you. A packet that never arrives +leaves no trace at all, so without a sequence number a link losing half its +traffic looks identical to a healthy one. -A rising `checksum_errors` is electrical: noise, a marginal cable, a bad -ground. A rising `resyncs` with few checksum errors usually means the sender -is being interrupted mid-frame. +The counter deliberately does not report a flood when the sequence wraps at +65535, nor when the Arduino restarts and begins again at zero. Counter width is `uint64_t` on a host and `uint32_t` on AVR. Not 16-bit: at roughly 40 packets a second that wraps in under half an hour, and a diagnostic @@ -222,25 +285,52 @@ to work. ## API summary -From ``, portable everywhere: +From ``, portable everywhere including AVR: ```c -void sh_parser_init (sh_parser_t *parser); -bool sh_parser_feed (sh_parser_t *parser, uint8_t byte, sh_packet_t *out); -uint8_t sh_checksum (const uint8_t *data, size_t length); -void sh_encode_frame (const sh_packet_t *packet, uint8_t *buffer); +/* framing */ +void sh_parser_init (sh_parser_t *parser); +sh_status_t sh_parser_feed (sh_parser_t *parser, uint8_t byte, + sh_packet_t *out); +uint8_t sh_checksum (const uint8_t *data, size_t length); +sh_status_t sh_encode_frame (const sh_packet_t *packet, uint8_t *buffer, + size_t buffer_size, size_t *written); + +/* channels, all bounds-checked */ +sh_status_t sh_digital_in (const sh_packet_t *p, unsigned ch, bool *out); +sh_status_t sh_digital_out (const sh_packet_t *p, unsigned ch, bool *out); +sh_status_t sh_set_digital_in (sh_packet_t *p, unsigned ch, bool value); +sh_status_t sh_set_digital_out (sh_packet_t *p, unsigned ch, bool value); +sh_status_t sh_temp (const sh_packet_t *p, unsigned i, float *out); +sh_status_t sh_analog (const sh_packet_t *p, unsigned i, uint16_t *out); + +/* status */ +const char *sh_strstatus (sh_status_t status); /* never NULL */ +bool sh_failed (sh_status_t status); /* SH_INCOMPLETE is not a failure */ ``` From ``, POSIX only: ```c -int sh_serial_open (const char *device); -void sh_serial_close (int fd); -bool sh_serial_read_packet(int fd, sh_parser_t *parser, sh_packet_t *out); +int sh_serial_open (const char *device); +void sh_serial_close (int fd); +sh_status_t sh_serial_read_packet(int fd, sh_parser_t *parser, + sh_packet_t *out); ``` -Every function tolerates `NULL` arguments by doing nothing and returning a -falsy value rather than crashing. +Conventions worth knowing: + +- **`SH_OK` is zero**, so `if (call(...) == SH_OK)` reads naturally and a + status can be tested for truth if you prefer. +- **`SH_INCOMPLETE` is not an error.** It is the answer for every byte that + did not happen to finish a packet, which is most of them. Use `sh_failed()` + rather than `!= SH_OK` when deciding whether to log something. +- **Every function tolerates `NULL`** by returning `SH_E_NULL` rather than + crashing, and every index is bounds-checked into `SH_E_RANGE`. +- **`sh_encode_frame` takes the buffer size** and refuses rather than + overrunning. +- **Every status has a message.** `sh_strstatus()` never returns `NULL`, even + for a value that is not a real status. ## Changing the wire format diff --git a/src/sensorhub/parser.c b/src/sensorhub/parser.c index 382237f..4ca7c96 100644 --- a/src/sensorhub/parser.c +++ b/src/sensorhub/parser.c @@ -1,7 +1,8 @@ /* - * Framing and checksum. No I/O lives here on purpose: everything in this - * file can be exercised from a byte array, which is what makes the tests - * meaningful without a car plugged in. + * Framing, checksum, and accessors. + * + * No I/O lives here on purpose: everything in this file can be exercised from + * a byte array, which is what makes the tests meaningful with no car attached. */ #include "sensorhub/sensorhub.h" @@ -9,15 +10,112 @@ #include _Static_assert(sizeof(float) == 4, "32-bit float required"); -_Static_assert(sizeof(sh_packet_t) == SH_PAYLOAD_SIZE, "unexpected packet size"); +_Static_assert(sizeof(sh_packet_t) == SH_PAYLOAD_SIZE, + "packet size changed; bump SH_FORMAT_CURRENT and the golden test"); +_Static_assert(SH_PAYLOAD_SIZE <= SH_MAX_PAYLOAD, + "the parser cannot buffer its own format"); + +/* ------------------------------------------------------------------ * + * Status + * ------------------------------------------------------------------ */ + +const char *sh_strstatus(sh_status_t status) +{ + switch (status) { + case SH_OK: return "ok"; + case SH_INCOMPLETE: return "incomplete, need more bytes"; + case SH_E_NULL: return "null argument"; + case SH_E_CHECKSUM: return "bad checksum"; + case SH_E_FORMAT: return "unsupported packet format"; + case SH_E_LENGTH: return "bad payload length"; + case SH_E_RANGE: return "channel index out of range"; + case SH_E_SPACE: return "buffer too small"; + case SH_E_OPEN: return "could not open serial port"; + case SH_E_IO: return "serial read failed"; + case SH_E_INTERRUPTED: return "interrupted by a signal"; + case SH_E_CLOSED: return "serial connection closed"; + } + return "unknown status"; +} + +bool sh_failed(sh_status_t status) +{ + return status != SH_OK && status != SH_INCOMPLETE; +} + +/* ------------------------------------------------------------------ * + * Accessors + * ------------------------------------------------------------------ */ + +#define SH_DIGITAL_MAX 7u + +sh_status_t sh_digital_in(const sh_packet_t *packet, unsigned channel, + bool *out) +{ + if (packet == NULL || out == NULL) return SH_E_NULL; + if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; + + *out = ((packet->digital_in >> channel) & 1u) != 0u; + return SH_OK; +} + +sh_status_t sh_digital_out(const sh_packet_t *packet, unsigned channel, + bool *out) +{ + if (packet == NULL || out == NULL) return SH_E_NULL; + if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; + + *out = ((packet->digital_out >> channel) & 1u) != 0u; + return SH_OK; +} + +sh_status_t sh_set_digital_in(sh_packet_t *packet, unsigned channel, bool value) +{ + if (packet == NULL) return SH_E_NULL; + if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; + + if (value) packet->digital_in |= (uint8_t)(1u << channel); + else packet->digital_in &= (uint8_t)~(1u << channel); + return SH_OK; +} + +sh_status_t sh_set_digital_out(sh_packet_t *packet, unsigned channel, bool value) +{ + if (packet == NULL) return SH_E_NULL; + if (channel > SH_DIGITAL_MAX) return SH_E_RANGE; + + if (value) packet->digital_out |= (uint8_t)(1u << channel); + else packet->digital_out &= (uint8_t)~(1u << channel); + return SH_OK; +} + +sh_status_t sh_temp(const sh_packet_t *packet, unsigned index, float *out) +{ + if (packet == NULL || out == NULL) return SH_E_NULL; + if (index >= SH_TEMP_COUNT) return SH_E_RANGE; + + *out = packet->temps[index]; + return SH_OK; +} + +sh_status_t sh_analog(const sh_packet_t *packet, unsigned index, uint16_t *out) +{ + if (packet == NULL || out == NULL) return SH_E_NULL; + if (index >= SH_ANALOG_COUNT) return SH_E_RANGE; + + *out = packet->analog[index]; + return SH_OK; +} + +/* ------------------------------------------------------------------ * + * Checksum and encoding + * ------------------------------------------------------------------ */ uint8_t sh_checksum(const uint8_t *data, size_t length) { uint8_t checksum = 0; - if (data == NULL) { - return 0; - } + if (data == NULL) return 0; for (size_t i = 0; i < length; ++i) { checksum ^= data[i]; @@ -26,50 +124,74 @@ uint8_t sh_checksum(const uint8_t *data, size_t length) return checksum; } -void sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer) +sh_status_t sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer, + size_t buffer_size, size_t *written) { - if (packet == NULL || buffer == NULL) { - return; - } + if (packet == NULL || buffer == NULL) return SH_E_NULL; + if (buffer_size < SH_FRAME_SIZE) return SH_E_SPACE; buffer[0] = (uint8_t)SH_HEADER_1; buffer[1] = (uint8_t)SH_HEADER_2; - memcpy(buffer + 2, packet, SH_PAYLOAD_SIZE); - buffer[2 + SH_PAYLOAD_SIZE] = sh_checksum(buffer + 2, SH_PAYLOAD_SIZE); + buffer[2] = (uint8_t)SH_FORMAT_CURRENT; + buffer[3] = (uint8_t)SH_PAYLOAD_SIZE; + memcpy(buffer + 4, packet, SH_PAYLOAD_SIZE); + + /* The checksum covers fmt and len too, so a corrupted length byte cannot + quietly reframe the stream and still validate. */ + buffer[4 + SH_PAYLOAD_SIZE] = sh_checksum(buffer + 2, SH_PAYLOAD_SIZE + 2u); + + if (written != NULL) *written = SH_FRAME_SIZE; + return SH_OK; } +/* ------------------------------------------------------------------ * + * Parser + * ------------------------------------------------------------------ */ + void sh_parser_init(sh_parser_t *parser) { - if (parser == NULL) { - return; - } + if (parser == NULL) return; memset(parser, 0, sizeof(*parser)); parser->state = SH_WAIT_HEADER_1; } -bool sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out) +/* Count how many packets went missing between two sequence numbers, allowing + for the counter wrapping at 16 bits. */ +static void note_sequence(sh_parser_t *parser, uint16_t sequence) { - if (parser == NULL || out == NULL) { - return false; + if (parser->have_sequence) { + uint16_t expected = (uint16_t)(parser->last_sequence + 1u); + uint16_t gap = (uint16_t)(sequence - expected); + + /* A huge gap almost certainly means the sender restarted rather than + that 60000 packets vanished, so do not report a fictional flood. */ + if (gap > 0u && gap < 1000u) { + parser->stats.dropped += gap; + } } + parser->have_sequence = true; + parser->last_sequence = sequence; +} + +sh_status_t sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out) +{ + if (parser == NULL || out == NULL) return SH_E_NULL; + switch (parser->state) { case SH_WAIT_HEADER_1: - if (byte == SH_HEADER_1) { - parser->state = SH_WAIT_HEADER_2; - } + if (byte == SH_HEADER_1) parser->state = SH_WAIT_HEADER_2; break; case SH_WAIT_HEADER_2: if (byte == SH_HEADER_2) { - parser->payload_index = 0; - parser->state = SH_READ_PAYLOAD; + parser->state = SH_READ_FORMAT; } else if (byte == SH_HEADER_1) { - /* 0xAA 0xAA 0x55 is a valid start: a payload byte that happens to - be 0xAA can precede the real header, so hold this state rather - than throwing the candidate away. */ + /* 0xAA 0xAA 0x55 is a valid start: a payload byte that happens + to be 0xAA can precede the real header, so hold this state + rather than discarding the candidate. */ } else { parser->stats.resyncs++; @@ -77,26 +199,71 @@ bool sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out) } break; + case SH_READ_FORMAT: + parser->format = byte; + parser->state = SH_READ_LENGTH; + break; + + case SH_READ_LENGTH: + parser->length = byte; + parser->payload_index = 0; + + if (parser->format != SH_FORMAT_CURRENT) { + /* Unknown version. The length byte is what lets us skip it + cleanly instead of desynchronising, which is the entire + reason it is on the wire. */ + parser->stats.format_errors++; + parser->state = (byte > 0u) ? SH_SKIP_UNKNOWN : SH_WAIT_HEADER_1; + return SH_E_FORMAT; + } + + if (byte != (uint8_t)SH_PAYLOAD_SIZE) { + /* Our own version with the wrong length: corruption, not a new + sender. Skip what it claims and carry on. */ + parser->state = (byte > 0u && byte <= SH_MAX_PAYLOAD) + ? SH_SKIP_UNKNOWN + : SH_WAIT_HEADER_1; + return SH_E_LENGTH; + } + + parser->state = SH_READ_PAYLOAD; + break; + + case SH_SKIP_UNKNOWN: + /* Drain the payload and its checksum without interpreting either. */ + parser->payload_index++; + if (parser->payload_index > (size_t)parser->length) { + parser->state = SH_WAIT_HEADER_1; + } + break; + case SH_READ_PAYLOAD: parser->payload[parser->payload_index++] = byte; - - if (parser->payload_index == SH_PAYLOAD_SIZE) { + if (parser->payload_index == (size_t)parser->length) { parser->state = SH_READ_CHECKSUM; } break; - case SH_READ_CHECKSUM: + case SH_READ_CHECKSUM: { + uint8_t expected; + parser->state = SH_WAIT_HEADER_1; - if (sh_checksum(parser->payload, SH_PAYLOAD_SIZE) == byte) { + /* Recompute over fmt, len, and the payload, matching the encoder. */ + expected = (uint8_t)(parser->format ^ parser->length); + expected ^= sh_checksum(parser->payload, (size_t)parser->length); + + if (expected == byte) { memcpy(out, parser->payload, SH_PAYLOAD_SIZE); parser->stats.packets++; - return true; + note_sequence(parser, out->sequence); + return SH_OK; } parser->stats.checksum_errors++; - break; + return SH_E_CHECKSUM; + } } - return false; + return SH_INCOMPLETE; } diff --git a/src/sensorhub/sensorhub.h b/src/sensorhub/sensorhub.h index e16f65e..d046706 100644 --- a/src/sensorhub/sensorhub.h +++ b/src/sensorhub/sensorhub.h @@ -1,18 +1,28 @@ /* - * SensorHub: the Raspberry Pi side of the link to the Arduino Nano that reads - * the car's sensors. + * SensorHub: the wire format for the link between the car's Arduino Nano and + * the Raspberry Pi. Used by both ends so the two cannot drift apart. * - * The wire format is defined by SensorController/carsensordriver.ino and must - * match it exactly: + * FRAME * - * 0xAA 0x55 <23-byte packed payload> + * 0xAA 0x55 fmt len payload[len] checksum * - * Twenty-six bytes on the wire, 115200 baud, 8N1. + * fmt format version. A receiver refuses anything it does not know + * rather than decoding it confidently and wrongly. + * len payload length. Lets an old receiver skip a packet from a newer + * sender and stay framed, instead of desynchronising. + * checksum XOR of fmt, len, and every payload byte. * - * Parsing is deliberately separate from I/O. sh_parser_feed() takes one byte - * and never touches a file descriptor, so the framing can be tested against - * synthetic input in CI, on any machine, with no car attached. The serial - * helpers below are a convenience for callers that do have hardware. + * The version and length bytes are the difference between "we changed the + * format and everything broke loudly" and "we changed the format and the car + * logged plausible nonsense for a season". They cost two bytes on a link + * running at 7% utilisation. + * + * PARSING IS SEPARATE FROM I/O + * + * sh_parser_feed() takes one byte and never touches a file descriptor, so the + * framing can be tested on any machine with no car attached. The serial + * helpers in are a convenience for callers that do have + * hardware. */ #ifndef SENSORHUB_H @@ -26,56 +36,160 @@ extern "C" { #endif +/* ------------------------------------------------------------------ * + * Framing constants + * ------------------------------------------------------------------ */ + #define SH_HEADER_1 0xAAu #define SH_HEADER_2 0x55u +/* Bump this when the payload layout changes, and reflash everything. */ +#define SH_FORMAT_V1 0x01u +#define SH_FORMAT_CURRENT SH_FORMAT_V1 + +/* How many slots the payload carries. Spares are deliberate: adding the + fifth temperature probe should be a wiring job, not a format change that + invalidates every CSV on disk. */ +#define SH_TEMP_COUNT 4u +#define SH_ANALOG_COUNT 4u + +/* Named indices for the slots that are actually wired today. The rest are + spare; give them names here when they get used. */ +#define SH_TEMP_ENGINE 0u +#define SH_TEMP_RADIATOR 1u +#define SH_ANALOG_BATTERY 0u + +/* ------------------------------------------------------------------ * + * The packet + * ------------------------------------------------------------------ */ + /* - * Field names mirror the Arduino struct character for character. This is a - * memcpy target off the wire, so the resemblance is load-bearing: if you - * rename a field here, rename it there in the same commit. + * Digital channels are bits, not bytes: a switch carries one bit of + * information and sixteen of them fit in the space two bytes used to take. + * + * Outputs are reported as well as inputs. The firmware drives a radiator fan + * and a water pump, and until now their state appeared nowhere in telemetry, + * so you could not display or log what the car was doing to itself. */ typedef struct __attribute__((packed)) { - float speed; /* ground speed, mph, from the wheel interrupt */ - float airspeed; /* pitot, mph, zeroed at Arduino startup */ - float engineTemp; /* degrees F, DS18B20 */ - float radTemp; /* degrees F, DS18B20 */ - - uint8_t channel0; /* digital, pin 4 */ - uint8_t channel1; /* digital, pin 5 */ - uint8_t channel2; /* digital, pin 6 */ - uint8_t channel3; /* digital, pin 7 */ - uint8_t channel4; /* digital, pin 8 */ - - uint16_t channelA0; /* analog, pin A7, currently battery voltage */ + float speed; /* mph, from the wheel interrupt */ + float airspeed; /* mph, pitot, zeroed at startup */ + + float temps[SH_TEMP_COUNT]; /* degrees F, see SH_TEMP_* */ + uint16_t analog[SH_ANALOG_COUNT]; /* raw ADC counts, see SH_ANALOG_* */ + + uint8_t digital_in; /* bitfield, input channels 0..7 */ + uint8_t digital_out; /* bitfield, output channels 0..7 */ + + /* Increments every packet and wraps. The receiver turns gaps in this + into a count of packets that never arrived, which a checksum cannot + tell you: a dropped packet leaves no trace otherwise. */ + uint16_t sequence; } sh_packet_t; -#define SH_PAYLOAD_SIZE 23u -#define SH_FRAME_SIZE 26u /* 2 header + payload + 1 checksum */ +#define SH_PAYLOAD_SIZE 36u /* sizeof(sh_packet_t) for SH_FORMAT_V1 */ +#define SH_FRAME_OVERHEAD 5u /* 2 header + fmt + len + checksum */ +#define SH_FRAME_SIZE (SH_PAYLOAD_SIZE + SH_FRAME_OVERHEAD) + +/* The largest payload the parser will buffer. Bigger than V1 on purpose, so + a newer sender's packet can be received and rejected cleanly rather than + overrunning anything. */ +#define SH_MAX_PAYLOAD 64u -/* Both ends of this link are little-endian (AVR and aarch64). A big-endian - host would need byte swapping that no one has written, so say so loudly - rather than decoding garbage. */ +/* Both ends are little-endian, AVR and aarch64. A big-endian host would need + byte swapping nobody has written, so say so rather than decode garbage. */ #if defined(__BYTE_ORDER__) && __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__ #error "SensorHub assumes a little-endian host; the wire format is AVR-native" #endif +/* ------------------------------------------------------------------ * + * Status + * ------------------------------------------------------------------ */ + +/* + * Every call that can fail returns one of these. SH_OK is zero, so the + * common shape reads naturally: + * + * if (sh_parser_feed(&parser, byte, &packet) == SH_OK) { ... } + * + * SH_INCOMPLETE is not an error. It is the usual answer, returned for every + * byte that did not happen to complete a packet. + */ typedef enum { - SH_WAIT_HEADER_1 = 0, - SH_WAIT_HEADER_2, - SH_READ_PAYLOAD, - SH_READ_CHECKSUM -} sh_state_t; + SH_OK = 0, /* a packet is ready */ + SH_INCOMPLETE, /* byte consumed, nothing complete yet */ + SH_E_NULL, /* a required argument was NULL */ + SH_E_CHECKSUM, /* framed correctly, contents rejected */ + SH_E_FORMAT, /* fmt byte names a version we do not know */ + SH_E_LENGTH, /* len disagrees with fmt, or exceeds the max */ + SH_E_RANGE, /* a channel index was out of bounds */ + SH_E_SPACE, /* caller's buffer was too small */ + SH_E_OPEN, /* could not open the port, see errno */ + SH_E_IO, /* read failed, see errno */ + SH_E_INTERRUPTED,/* a signal arrived; check your stop flag */ + SH_E_CLOSED /* clean EOF; the adapter was unplugged */ +} sh_status_t; + +/* A short human-readable description. Never NULL, even for a bogus value. */ +const char *sh_strstatus(sh_status_t status); + +/* True for statuses that mean something went wrong, so SH_INCOMPLETE does + not get logged as a failure forty times a second. */ +bool sh_failed(sh_status_t status); + -/* Counters worth watching on a bench test: a link that is losing packets - shows up here long before it shows up on the dashboard. +/* ------------------------------------------------------------------ * + * Digital channel accessors + * ------------------------------------------------------------------ * + * + * Bit twiddling at every call site is how off-by-one channel bugs happen, + * so do it once, here, with bounds checking. + */ + +/* Read one input channel. Returns SH_E_RANGE for channel > 7. */ +sh_status_t sh_digital_in(const sh_packet_t *packet, unsigned channel, + bool *out); + +/* Read one output channel. Returns SH_E_RANGE for channel > 7. */ +sh_status_t sh_digital_out(const sh_packet_t *packet, unsigned channel, + bool *out); + +/* Set an input channel, for senders building a packet. */ +sh_status_t sh_set_digital_in(sh_packet_t *packet, unsigned channel, + bool value); + +/* Set an output channel, for senders building a packet. */ +sh_status_t sh_set_digital_out(sh_packet_t *packet, unsigned channel, + bool value); - The width is configurable because this header also compiles for the - ATmega328p, where 64-bit counters cost RAM a Nano does not have and every - increment is a slow multi-word add. The AVR default is 32-bit rather than - 16: at roughly 40 packets a second, 16 bits wraps in under half an hour, - and a diagnostic counter that quietly lies is worse than a larger one. - 32 bits lasts about three years of continuous running for six more bytes. - Define SH_COUNTER_BITS before including to override. */ +/* Bounds-checked reads for the array slots, so an out-of-range index is an + error rather than whatever was next in memory. */ +sh_status_t sh_temp(const sh_packet_t *packet, unsigned index, float *out); +sh_status_t sh_analog(const sh_packet_t *packet, unsigned index, + uint16_t *out); + +/* ------------------------------------------------------------------ * + * Link statistics + * ------------------------------------------------------------------ * + * + * A link that is losing packets shows up here long before it shows up on the + * dashboard. The three counters fail differently and are worth reading + * separately: + * + * checksum_errors rising electrical: noise, a marginal cable, a bad ground + * resyncs rising the sender is being interrupted mid-frame + * dropped rising packets never arrived at all; the receiver is + * not keeping up, or the sender is restarting + * + * Width is configurable because this header also compiles for an ATmega328p, + * where 64-bit counters cost RAM a Nano does not have and every increment is + * a slow multi-word add. The AVR default is 32-bit rather than 16: at roughly + * 40 packets a second, 16 bits wraps in under half an hour, and a diagnostic + * counter that quietly lies is worse than a larger one. + * + * Note this changes sizeof(sh_stats_t), so everything linked together must + * agree on it. + */ #ifndef SH_COUNTER_BITS # if defined(__AVR__) || defined(SH_EMBEDDED) # define SH_COUNTER_BITS 32 @@ -95,41 +209,78 @@ typedef uint64_t sh_counter_t; #endif typedef struct { - sh_counter_t packets; /* accepted */ - sh_counter_t checksum_errors; /* framed correctly, contents rejected */ - sh_counter_t resyncs; /* fell back to hunting for a header */ + sh_counter_t packets; /* accepted */ + sh_counter_t checksum_errors; /* framed correctly, contents rejected */ + sh_counter_t resyncs; /* fell back to hunting for a header */ + sh_counter_t format_errors; /* a version we do not understand */ + sh_counter_t dropped; /* inferred from gaps in the sequence */ } sh_stats_t; +/* ------------------------------------------------------------------ * + * Parser + * ------------------------------------------------------------------ */ + +typedef enum { + SH_WAIT_HEADER_1 = 0, + SH_WAIT_HEADER_2, + SH_READ_FORMAT, + SH_READ_LENGTH, + SH_READ_PAYLOAD, + SH_READ_CHECKSUM, + SH_SKIP_UNKNOWN /* draining a packet whose format we do not know */ +} sh_state_t; + typedef struct { sh_state_t state; - uint8_t payload[SH_PAYLOAD_SIZE]; + uint8_t format; + uint8_t length; + uint8_t payload[SH_MAX_PAYLOAD]; size_t payload_index; + bool have_sequence; + uint16_t last_sequence; sh_stats_t stats; } sh_parser_t; -/* Reset a parser to hunting for a header. Also zeroes the statistics. */ +/* Reset to hunting for a header. Also zeroes the statistics. */ void sh_parser_init(sh_parser_t *parser); /* * Feed exactly one received byte. * - * Returns true when that byte completed a packet whose checksum matched, in - * which case *out holds it. Returns false every other time, including for a - * packet that arrived intact but failed its checksum; that case increments - * stats.checksum_errors so it can be distinguished from an idle link. + * SH_OK *out now holds a valid packet + * SH_INCOMPLETE byte consumed, nothing complete yet. The usual answer. + * SH_E_CHECKSUM a frame arrived intact but its contents were rejected + * SH_E_FORMAT a frame announced a version we do not know; the parser + * uses the length byte to skip it cleanly and carries on + * SH_E_NULL parser or out was NULL * - * Passing NULL for either argument is a no-op returning false. + * Errors are reported, not thrown: the parser stays usable and keeps + * counting. A caller that only wants packets can compare against SH_OK and + * read parser->stats occasionally. */ -bool sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out); +sh_status_t sh_parser_feed(sh_parser_t *parser, uint8_t byte, + sh_packet_t *out); + +/* ------------------------------------------------------------------ * + * Encoding + * ------------------------------------------------------------------ */ -/* XOR of every byte, the checksum the Arduino appends. Exposed so tests and - any future transmitter can build frames without duplicating it. */ +/* XOR of every byte. Exposed so tests and senders need not duplicate it. */ uint8_t sh_checksum(const uint8_t *data, size_t length); -/* Serialize a packet into a full 26-byte frame. buffer must have room for - SH_FRAME_SIZE. Used by the tests to generate input, and by any simulator - that wants to stand in for the Arduino. */ -void sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer); +/* + * Serialise a packet into a complete frame. + * + * buffer must have room for at least SH_FRAME_SIZE bytes; pass its real size + * in buffer_size and the function will refuse rather than overrun. On success + * *written holds the frame length. written may be NULL if you do not care. + * + * SH_OK frame written + * SH_E_SPACE buffer_size was too small + * SH_E_NULL packet or buffer was NULL + */ +sh_status_t sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer, + size_t buffer_size, size_t *written); #ifdef __cplusplus } diff --git a/src/sensorhub/serial.c b/src/sensorhub/serial.c index 94bbcb0..66cc1f3 100644 --- a/src/sensorhub/serial.c +++ b/src/sensorhub/serial.c @@ -90,38 +90,36 @@ void sh_serial_close(int fd) } } -static bool read_byte(int fd, uint8_t *value) +sh_status_t sh_serial_read_packet(int fd, sh_parser_t *parser, + sh_packet_t *out) { - ssize_t count; + uint8_t byte; - count = read(fd, value, 1); + if (parser == NULL || out == NULL) return SH_E_NULL; - if (count == 0) { - errno = 0; /* clean EOF, distinguish from a real error */ - } + for (;;) { + ssize_t count = read(fd, &byte, 1); - /* EINTR is reported rather than retried. A caller with a shutdown flag - needs a chance to look at it; swallowing the signal here is what makes - a serial tool impossible to Ctrl-C. */ - return count == 1; -} + if (count == 1) { + sh_status_t status = sh_parser_feed(parser, byte, out); -bool sh_serial_read_packet(int fd, sh_parser_t *parser, sh_packet_t *out) -{ - uint8_t byte; + /* Keep reading through recoverable framing errors: a single bad + packet is not a reason to hand the caller a failure and make + it decide whether to loop. Only a completed packet or an I/O + condition ends this call. */ + if (status == SH_OK) return SH_OK; + continue; + } - if (parser == NULL || out == NULL) { - errno = EINVAL; - return false; - } + if (count == 0) return SH_E_CLOSED; - while (read_byte(fd, &byte)) { - if (sh_parser_feed(parser, byte, out)) { - return true; - } - } + /* EINTR is surfaced rather than retried so a caller with a shutdown + flag gets a chance to look at it. Retrying here is what makes a + serial tool impossible to Ctrl-C. */ + if (errno == EINTR) return SH_E_INTERRUPTED; - return false; + return SH_E_IO; + } } #else /* not POSIX */ diff --git a/src/sensorhub/serial.h b/src/sensorhub/serial.h index cc15839..e1279d4 100644 --- a/src/sensorhub/serial.h +++ b/src/sensorhub/serial.h @@ -26,9 +26,10 @@ extern "C" { /* * Open and configure a port at 115200 8N1 raw. * - * Returns a file descriptor, or -1 with errno set. Close it with - * sh_serial_close(). Do not reopen in a retry loop without a delay: each - * open can reset the Nano. + * Returns a file descriptor, or -1 with errno set. Close it with + * sh_serial_close(). Do not reopen in a retry loop without a delay: each open + * can reset the Nano through its DTR auto-reset circuit, which costs a couple + * of seconds of telemetry and re-runs the airspeed zeroing. */ int sh_serial_open(const char *device); @@ -38,19 +39,23 @@ void sh_serial_close(int fd); /* * Block until the next valid packet arrives, feeding bytes through parser. * - * Returns true with *out populated. Returns false on EOF, on a read error, - * or when a signal interrupted the read: + * SH_OK *out holds a packet + * SH_E_CLOSED clean EOF; on a serial port the adapter was unplugged + * SH_E_INTERRUPTED a signal arrived; check your stop flag and call again + * SH_E_IO a real read error, see errno + * SH_E_NULL parser or out was NULL * - * errno == 0 clean EOF; on a serial port, the adapter was unplugged - * errno == EINTR a signal arrived; check your shutdown flag and call again - * otherwise a real error + * Recoverable framing errors do not end the call. A bad checksum or an + * unknown format is counted in parser->stats and reading continues, because + * one corrupt packet is not a reason to make every caller write a retry loop. * - * EINTR is surfaced rather than retried internally so that a caller can + * SH_E_INTERRUPTED is surfaced rather than retried internally so a caller can * actually be interrupted. Install handlers with sigaction() and no - * SA_RESTART if you want Ctrl-C to work; plain signal() sets SA_RESTART on - * most platforms, which prevents read() from ever returning EINTR. + * SA_RESTART if you want Ctrl-C to work: plain signal() sets SA_RESTART on + * most platforms, which stops read() ever returning EINTR. */ -bool sh_serial_read_packet(int fd, sh_parser_t *parser, sh_packet_t *out); +sh_status_t sh_serial_read_packet(int fd, sh_parser_t *parser, + sh_packet_t *out); #ifdef __cplusplus } diff --git a/test/test_parser.c b/test/test_parser.c index 7236eda..65b7855 100644 --- a/test/test_parser.c +++ b/test/test_parser.c @@ -1,11 +1,11 @@ /* * Framing tests. * - * These exist to catch the failure the old 9600-baud reader actually had: a - * dropped byte desynchronizing the stream with nothing to resynchronize - * against. So the interesting cases here are not "a good packet parses" but - * the nasty ones, garbage in front, a byte lost mid-packet, a payload that - * contains the header bytes, and a corrupted checksum. + * These exist to catch the failures that matter, not to demonstrate that a + * good packet parses. The interesting cases are garbage in front of a frame, + * a byte lost mid-packet, a payload containing the header bytes, a corrupted + * checksum, and, since the format now carries a version, a sender speaking a + * dialect we do not know. */ #include "sensorhub/sensorhub.h" @@ -33,14 +33,14 @@ static sh_packet_t sample_packet(void) memset(&p, 0, sizeof(p)); p.speed = 23.5f; p.airspeed = 19.25f; - p.engineTemp = 180.0f; - p.radTemp = 148.5f; - p.channel0 = 1; - p.channel1 = 0; - p.channel2 = 1; - p.channel3 = 1; - p.channel4 = 0; - p.channelA0 = 812; + p.temps[SH_TEMP_ENGINE] = 180.0f; + p.temps[SH_TEMP_RADIATOR] = 148.5f; + p.temps[2] = 0.0f; + p.temps[3] = 0.0f; + p.analog[SH_ANALOG_BATTERY] = 812; + p.digital_in = 0x0Du; /* channels 0, 2, 3 */ + p.digital_out = 0x02u; /* output 1 */ + p.sequence = 7; return p; } @@ -49,6 +49,12 @@ static bool packets_equal(const sh_packet_t *a, const sh_packet_t *b) return memcmp(a, b, sizeof(sh_packet_t)) == 0; } +static void encode(const sh_packet_t *p, uint8_t *buf) +{ + sh_status_t st = sh_encode_frame(p, buf, SH_FRAME_SIZE, NULL); + CHECK(st == SH_OK, "encode should succeed, got %s", sh_strstatus(st)); +} + /* Feed a buffer through a parser, returning how many packets came out. */ static int feed_all(sh_parser_t *parser, const uint8_t *data, size_t len, sh_packet_t *last) @@ -57,17 +63,22 @@ static int feed_all(sh_parser_t *parser, const uint8_t *data, size_t len, sh_packet_t out; for (size_t i = 0; i < len; ++i) { - if (sh_parser_feed(parser, data[i], &out)) { + if (sh_parser_feed(parser, data[i], &out) == SH_OK) { count++; - if (last != NULL) { - *last = out; - } + if (last != NULL) *last = out; } } return count; } +static void test_sizes_are_the_wire_contract(void) +{ + CHECK(sizeof(sh_packet_t) == 36, "payload must stay 36 bytes"); + CHECK(SH_FRAME_SIZE == 41, "frame must stay 41 bytes"); + CHECK(SH_FRAME_OVERHEAD == 5, "overhead is header, fmt, len, checksum"); +} + static void test_roundtrip(void) { sh_packet_t in = sample_packet(); @@ -75,7 +86,7 @@ static void test_roundtrip(void) uint8_t frame[SH_FRAME_SIZE]; sh_parser_t parser; - sh_encode_frame(&in, frame); + encode(&in, frame); sh_parser_init(&parser); CHECK(feed_all(&parser, frame, sizeof(frame), &out) == 1, @@ -85,28 +96,21 @@ static void test_roundtrip(void) CHECK(parser.stats.checksum_errors == 0, "no checksum errors expected"); } -static void test_packet_size_is_on_the_wire_contract(void) -{ - CHECK(sizeof(sh_packet_t) == 23, "payload must stay 23 bytes"); - CHECK(SH_FRAME_SIZE == 26, "frame must stay 26 bytes"); -} - static void test_leading_garbage(void) { sh_packet_t in = sample_packet(); sh_packet_t out; - uint8_t buf[64]; + uint8_t buf[80]; sh_parser_t parser; size_t n = 0; - /* Junk, including a lone header byte, before a good frame. */ buf[n++] = 0x00; buf[n++] = 0xFF; - buf[n++] = SH_HEADER_1; + buf[n++] = SH_HEADER_1; /* a lone header byte going nowhere */ buf[n++] = 0x12; buf[n++] = 0x34; - sh_encode_frame(&in, buf + n); + encode(&in, buf + n); n += SH_FRAME_SIZE; sh_parser_init(&parser); @@ -118,7 +122,7 @@ static void test_leading_garbage(void) static void test_payload_containing_header_bytes(void) { /* A payload byte equal to 0xAA immediately before the real 0xAA 0x55 is - the case that a naive two-state matcher gets wrong. */ + the case a naive two-state matcher gets wrong. */ sh_packet_t in = sample_packet(); sh_packet_t out; uint8_t buf[8 + SH_FRAME_SIZE]; @@ -129,7 +133,7 @@ static void test_payload_containing_header_bytes(void) buf[n++] = SH_HEADER_1; buf[n++] = SH_HEADER_1; - sh_encode_frame(&in, buf + n); + encode(&in, buf + n); n += SH_FRAME_SIZE; sh_parser_init(&parser); @@ -144,15 +148,19 @@ static void test_bad_checksum_is_counted_not_silent(void) sh_packet_t out; uint8_t frame[SH_FRAME_SIZE]; sh_parser_t parser; + sh_status_t last = SH_OK; - sh_encode_frame(&in, frame); - frame[SH_FRAME_SIZE - 1] ^= 0xFFu; /* corrupt the checksum */ + encode(&in, frame); + frame[SH_FRAME_SIZE - 1] ^= 0xFFu; sh_parser_init(&parser); - CHECK(feed_all(&parser, frame, sizeof(frame), &out) == 0, - "a bad checksum must not produce a packet"); - CHECK(parser.stats.checksum_errors == 1, - "a bad checksum must be counted, not dropped silently"); + for (size_t i = 0; i < sizeof(frame); ++i) { + last = sh_parser_feed(&parser, frame[i], &out); + } + + CHECK(last == SH_E_CHECKSUM, "a bad checksum must report SH_E_CHECKSUM"); + CHECK(parser.stats.checksum_errors == 1, "and must be counted"); + CHECK(parser.stats.packets == 0, "and must not yield a packet"); } static void test_corrupt_payload_is_rejected(void) @@ -162,8 +170,8 @@ static void test_corrupt_payload_is_rejected(void) uint8_t frame[SH_FRAME_SIZE]; sh_parser_t parser; - sh_encode_frame(&in, frame); - frame[5] ^= 0x01u; /* flip a bit in the payload, checksum now wrong */ + encode(&in, frame); + frame[8] ^= 0x01u; /* flip a bit inside the payload */ sh_parser_init(&parser); CHECK(feed_all(&parser, frame, sizeof(frame), &out) == 0, @@ -173,155 +181,389 @@ static void test_corrupt_payload_is_rejected(void) static void test_resync_after_dropped_byte(void) { - /* The real-world failure: one byte lost in transit. The truncated frame - must be discarded and the NEXT frame must still parse. Without a - header this is exactly where the old reader went permanently wrong. */ + /* The real-world failure: one byte lost in transit. The truncated frame + must be discarded and the next one must still parse. */ sh_packet_t in = sample_packet(); sh_packet_t out; uint8_t buf[SH_FRAME_SIZE * 3]; sh_parser_t parser; - size_t n = 0; + size_t n; - sh_encode_frame(&in, buf); - /* Copy all but the last byte of frame one: a dropped tail. */ - n = SH_FRAME_SIZE - 1; + encode(&in, buf); + n = SH_FRAME_SIZE - 1; /* drop the tail of frame one */ - sh_encode_frame(&in, buf + n); + encode(&in, buf + n); n += SH_FRAME_SIZE; - sh_encode_frame(&in, buf + n); + encode(&in, buf + n); n += SH_FRAME_SIZE; sh_parser_init(&parser); int got = feed_all(&parser, buf, n, &out); - CHECK(got >= 1, "must resynchronize after a dropped byte, got %d", got); + CHECK(got >= 1, "must resynchronise after a dropped byte, got %d", got); CHECK(packets_equal(&in, &out), "post-resync packet should match"); } -static void test_split_across_reads(void) +static void test_back_to_back_frames(void) { - /* Serial reads arrive in arbitrary chunks; the parser is byte-at-a-time - so this should be invisible, but prove it rather than assume it. */ sh_packet_t in = sample_packet(); sh_packet_t out; - uint8_t frame[SH_FRAME_SIZE]; + uint8_t buf[SH_FRAME_SIZE * 5]; sh_parser_t parser; - int count = 0; - sh_encode_frame(&in, frame); + for (int i = 0; i < 5; ++i) { + in.speed = (float)i; + in.sequence = (uint16_t)(100 + i); + encode(&in, buf + ((size_t)i * SH_FRAME_SIZE)); + } + sh_parser_init(&parser); + CHECK(feed_all(&parser, buf, sizeof(buf), &out) == 5, + "five frames should yield five packets"); + CHECK(out.speed == 4.0f, "last packet should be the last one sent"); + CHECK(parser.stats.dropped == 0, "a contiguous run drops nothing"); +} + +/* ---- the version and length bytes, which is why they are on the wire ---- */ + +static void test_unknown_format_is_refused_not_decoded(void) +{ + /* The whole point. A sender speaking a version we do not know must be + rejected loudly, not decoded into plausible nonsense. */ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + sh_status_t seen = SH_OK; + encode(&in, frame); + frame[2] = 0x02u; /* a future format */ + /* fix the checksum so this is purely a version rejection, not a + corruption that would be caught anyway */ + frame[SH_FRAME_SIZE - 1] = + (uint8_t)(frame[2] ^ frame[3]) ^ sh_checksum(frame + 4, SH_PAYLOAD_SIZE); + + sh_parser_init(&parser); for (size_t i = 0; i < sizeof(frame); ++i) { - if (sh_parser_feed(&parser, frame[i], &out)) { - count++; - } + sh_status_t st = sh_parser_feed(&parser, frame[i], &out); + if (st == SH_E_FORMAT) seen = st; + CHECK(st != SH_OK, "an unknown format must never yield a packet"); } - CHECK(count == 1, "byte-at-a-time feeding should yield exactly one packet"); - CHECK(packets_equal(&in, &out), "split packet should match"); + CHECK(seen == SH_E_FORMAT, "should have reported SH_E_FORMAT"); + CHECK(parser.stats.format_errors == 1, "and counted it"); + CHECK(parser.stats.packets == 0, "and produced nothing"); } -static void test_back_to_back_frames(void) +static void test_length_lets_us_skip_a_newer_sender(void) { + /* An old receiver against a new sender: the length byte is what keeps it + framed. After skipping a longer packet it does not understand, the very + next packet it does understand must still parse. */ sh_packet_t in = sample_packet(); sh_packet_t out; - uint8_t buf[SH_FRAME_SIZE * 5]; + uint8_t buf[128]; sh_parser_t parser; + size_t n = 0; - for (int i = 0; i < 5; ++i) { - in.speed = (float)i; - sh_encode_frame(&in, buf + ((size_t)i * SH_FRAME_SIZE)); + /* A future frame: format 2, payload 50 bytes. */ + buf[n++] = SH_HEADER_1; + buf[n++] = SH_HEADER_2; + buf[n++] = 0x02u; + buf[n++] = 50u; + for (int i = 0; i < 50; ++i) buf[n++] = (uint8_t)i; + buf[n++] = 0x00u; /* its checksum, whatever it is */ + + /* Then a frame we do understand. */ + encode(&in, buf + n); + n += SH_FRAME_SIZE; + + sh_parser_init(&parser); + CHECK(feed_all(&parser, buf, n, &out) == 1, + "must stay framed across a packet from a newer sender"); + CHECK(packets_equal(&in, &out), "the packet after the skip should match"); + CHECK(parser.stats.format_errors == 1, "the unknown frame was counted"); +} + +static void test_wrong_length_for_our_own_format(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + sh_status_t seen = SH_OK; + + encode(&in, frame); + frame[3] = 99u; /* our format, an impossible length */ + + sh_parser_init(&parser); + for (size_t i = 0; i < sizeof(frame); ++i) { + sh_status_t st = sh_parser_feed(&parser, frame[i], &out); + if (st == SH_E_LENGTH) seen = st; } + CHECK(seen == SH_E_LENGTH, "a bad length must report SH_E_LENGTH"); + CHECK(parser.stats.packets == 0, "and must not yield a packet"); +} + +static void test_checksum_covers_the_header_fields(void) +{ + /* If the checksum only covered the payload, flipping the length byte + would reframe the stream and still validate. */ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + + encode(&in, frame); + frame[3] = (uint8_t)(SH_PAYLOAD_SIZE - 1u); /* lie about the length */ + sh_parser_init(&parser); - CHECK(feed_all(&parser, buf, sizeof(buf), &out) == 5, - "five frames should yield five packets"); - CHECK(out.speed == 4.0f, "last packet should be the last one sent"); + CHECK(feed_all(&parser, frame, sizeof(frame), &out) == 0, + "a tampered length byte must not produce a packet"); +} + +static void test_dropped_packets_are_counted_from_the_sequence(void) +{ + /* A checksum cannot tell you about a packet that never arrived. The + sequence number can. */ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + + sh_parser_init(&parser); + + in.sequence = 10; + encode(&in, frame); + feed_all(&parser, frame, sizeof(frame), &out); + + in.sequence = 14; /* 11, 12, 13 never arrived */ + encode(&in, frame); + feed_all(&parser, frame, sizeof(frame), &out); + + CHECK(parser.stats.dropped == 3, "should have counted 3 dropped, got %lu", + (unsigned long)parser.stats.dropped); + CHECK(parser.stats.packets == 2, "two packets did arrive"); +} + +static void test_sequence_wrap_is_not_a_flood(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + + sh_parser_init(&parser); + + in.sequence = 65535; + encode(&in, frame); + feed_all(&parser, frame, sizeof(frame), &out); + + in.sequence = 0; /* wrapped, not 65535 packets lost */ + encode(&in, frame); + feed_all(&parser, frame, sizeof(frame), &out); + + CHECK(parser.stats.dropped == 0, "a clean wrap drops nothing, got %lu", + (unsigned long)parser.stats.dropped); +} + +static void test_sender_restart_is_not_a_flood(void) +{ + sh_packet_t in = sample_packet(); + sh_packet_t out; + uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; + + sh_parser_init(&parser); + + in.sequence = 40000; + encode(&in, frame); + feed_all(&parser, frame, sizeof(frame), &out); + + in.sequence = 0; /* the Arduino rebooted */ + encode(&in, frame); + feed_all(&parser, frame, sizeof(frame), &out); + + CHECK(parser.stats.dropped == 0, + "a restart should not report a fictional flood, got %lu", + (unsigned long)parser.stats.dropped); +} + +/* ---- the API surface itself ---- */ + +static void test_digital_accessors(void) +{ + sh_packet_t p; + bool v; + + memset(&p, 0, sizeof(p)); + + CHECK(sh_set_digital_in(&p, 3, true) == SH_OK, "set channel 3"); + CHECK(sh_digital_in(&p, 3, &v) == SH_OK && v, "channel 3 should read set"); + CHECK(sh_digital_in(&p, 4, &v) == SH_OK && !v, "channel 4 should be clear"); + + CHECK(sh_set_digital_in(&p, 3, false) == SH_OK, "clear channel 3"); + CHECK(sh_digital_in(&p, 3, &v) == SH_OK && !v, "channel 3 should clear"); + + CHECK(sh_set_digital_out(&p, 0, true) == SH_OK, "set output 0"); + CHECK(sh_digital_out(&p, 0, &v) == SH_OK && v, "output 0 should read set"); + /* outputs and inputs must not alias each other */ + CHECK(sh_digital_in(&p, 0, &v) == SH_OK && !v, + "setting an output must not touch the inputs"); +} + +static void test_out_of_range_is_an_error_not_a_read(void) +{ + sh_packet_t p = sample_packet(); + bool v; + float f; + uint16_t a; + + CHECK(sh_digital_in(&p, 8, &v) == SH_E_RANGE, "channel 8 is out of range"); + CHECK(sh_digital_out(&p, 99, &v) == SH_E_RANGE, "so is 99"); + CHECK(sh_set_digital_in(&p, 8, true) == SH_E_RANGE, "and for writes"); + CHECK(sh_temp(&p, SH_TEMP_COUNT, &f) == SH_E_RANGE, "temp index bound"); + CHECK(sh_analog(&p, SH_ANALOG_COUNT, &a) == SH_E_RANGE, "analog bound"); + + CHECK(sh_temp(&p, SH_TEMP_ENGINE, &f) == SH_OK && f == 180.0f, + "a valid temp index still works"); + CHECK(sh_analog(&p, SH_ANALOG_BATTERY, &a) == SH_OK && a == 812, + "a valid analog index still works"); +} + +static void test_encode_refuses_a_short_buffer(void) +{ + sh_packet_t p = sample_packet(); + uint8_t small[SH_FRAME_SIZE - 1]; + uint8_t ok[SH_FRAME_SIZE]; + size_t written = 0; + + CHECK(sh_encode_frame(&p, small, sizeof(small), &written) == SH_E_SPACE, + "a short buffer must be refused, not overrun"); + CHECK(sh_encode_frame(&p, ok, sizeof(ok), &written) == SH_OK, + "a correctly sized buffer works"); + CHECK(written == SH_FRAME_SIZE, "and reports what it wrote"); } static void test_null_arguments_are_safe(void) { sh_parser_t parser; sh_packet_t out; + sh_packet_t p = sample_packet(); + uint8_t buf[SH_FRAME_SIZE]; + bool v; sh_parser_init(NULL); /* must not crash */ sh_parser_init(&parser); - CHECK(!sh_parser_feed(NULL, 0x00, &out), "NULL parser should return false"); - CHECK(!sh_parser_feed(&parser, 0x00, NULL), "NULL out should return false"); - CHECK(sh_checksum(NULL, 10) == 0, "NULL checksum input should return 0"); - sh_encode_frame(NULL, NULL); /* must not crash */ + CHECK(sh_parser_feed(NULL, 0, &out) == SH_E_NULL, "NULL parser"); + CHECK(sh_parser_feed(&parser, 0, NULL) == SH_E_NULL, "NULL out"); + CHECK(sh_encode_frame(NULL, buf, sizeof(buf), NULL) == SH_E_NULL, + "NULL packet"); + CHECK(sh_encode_frame(&p, NULL, sizeof(buf), NULL) == SH_E_NULL, + "NULL buffer"); + CHECK(sh_digital_in(NULL, 0, &v) == SH_E_NULL, "NULL packet to accessor"); + CHECK(sh_digital_in(&p, 0, NULL) == SH_E_NULL, "NULL out to accessor"); + CHECK(sh_checksum(NULL, 10) == 0, "NULL checksum input"); } -static void test_checksum_matches_the_arduino(void) +static void test_status_strings_exist(void) { - /* The Arduino XORs every payload byte. Pin the algorithm with a value - computed by hand so a "clever" rewrite cannot quietly change it. */ - const uint8_t data[] = { 0x01, 0x02, 0x03 }; /* 1^2^3 == 0 */ - const uint8_t data2[] = { 0xAA, 0x55 }; /* 0xAA^0x55 == 0xFF */ + /* A status with no message turns a diagnosable failure into "error 7". */ + static const sh_status_t all[] = { + SH_OK, SH_INCOMPLETE, SH_E_NULL, SH_E_CHECKSUM, SH_E_FORMAT, + SH_E_LENGTH, SH_E_RANGE, SH_E_SPACE, SH_E_OPEN, SH_E_IO, + SH_E_INTERRUPTED, SH_E_CLOSED + }; - CHECK(sh_checksum(data, sizeof(data)) == 0x00, "1^2^3 should be 0"); - CHECK(sh_checksum(data2, sizeof(data2)) == 0xFF, "0xAA^0x55 should be 0xFF"); + for (size_t i = 0; i < sizeof(all) / sizeof(all[0]); ++i) { + const char *s = sh_strstatus(all[i]); + CHECK(s != NULL && s[0] != '\0' && strcmp(s, "unknown status") != 0, + "status %d needs its own message", (int)all[i]); + } + + CHECK(strcmp(sh_strstatus((sh_status_t)999), "unknown status") == 0, + "a bogus status should still return a string, not NULL"); + CHECK(!sh_failed(SH_OK), "SH_OK is not a failure"); + CHECK(!sh_failed(SH_INCOMPLETE), + "SH_INCOMPLETE is not a failure; it is the usual answer"); + CHECK(sh_failed(SH_E_CHECKSUM), "SH_E_CHECKSUM is a failure"); } static void test_wire_format_is_frozen(void) { /* - * A golden frame, byte for byte. + * A golden frame, byte for byte. This is the contract with + * carsensordriver.ino and with every consumer downstream. * - * This is the contract with carsensordriver.ino on the Arduino and with - * every CSV already on disk. It was verified byte-identical against the - * firmware's original hand-rolled send path over 200,000 random packets - * before that code was deleted in favour of sh_encode_frame(). - * - * If this test fails, the wire format changed. That is not a test to - * update; it is a flash-the-Arduino-and-tell-everyone event. + * If this fails, the wire format changed. That is not a test to update, + * it is a reflash-every-Arduino event: bump SH_FORMAT_CURRENT, regenerate + * this, and tell everyone. */ static const uint8_t golden[SH_FRAME_SIZE] = { - 0xAA, 0x55, /* header */ - 0x00, 0x00, 0xBC, 0x41, /* speed 23.5 */ - 0x00, 0x00, 0x9A, 0x41, /* airspeed 19.25 */ - 0x00, 0x00, 0x34, 0x43, /* engineTemp 180.0 */ - 0x00, 0x80, 0x14, 0x43, /* radTemp 148.5 */ - 0x01, 0x00, 0x01, 0x01, 0x00, /* channel0..4 */ - 0x2C, 0x03, /* channelA0 812 */ - 0xA8 /* XOR checksum */ + 0xAA, 0x55, /* header */ + 0x01, /* format 1 */ + 0x24, /* payload length, 36 */ + 0x00, 0x00, 0xBC, 0x41, /* speed 23.5 */ + 0x00, 0x00, 0x9A, 0x41, /* airspeed 19.25 */ + 0x00, 0x00, 0x34, 0x43, /* temps[0] 180.0 */ + 0x00, 0x80, 0x14, 0x43, /* temps[1] 148.5 */ + 0x00, 0x00, 0x00, 0x00, /* temps[2] 0.0 */ + 0x00, 0x00, 0x00, 0x00, /* temps[3] 0.0 */ + 0x2C, 0x03, /* analog[0] 812 */ + 0x00, 0x00, /* analog[1] */ + 0x00, 0x00, /* analog[2] */ + 0x00, 0x00, /* analog[3] */ + 0x0D, /* digital_in 0b00001101 */ + 0x02, /* digital_out 0b00000010 */ + 0x07, 0x00, /* sequence 7 */ + 0x84 /* checksum over fmt, len, payload */ }; sh_packet_t in = sample_packet(); + sh_packet_t out; uint8_t frame[SH_FRAME_SIZE]; + sh_parser_t parser; - sh_encode_frame(&in, frame); + encode(&in, frame); CHECK(memcmp(frame, golden, sizeof(golden)) == 0, "encoded frame must match the frozen wire format byte for byte"); /* And the parser must accept its own golden frame, which catches an encoder and decoder that drifted together. */ - { - sh_parser_t parser; - sh_packet_t out; - sh_parser_init(&parser); - CHECK(feed_all(&parser, golden, sizeof(golden), &out) == 1, - "the frozen frame must still decode"); - CHECK(packets_equal(&in, &out), "decoded golden frame should match"); - } + sh_parser_init(&parser); + CHECK(feed_all(&parser, golden, sizeof(golden), &out) == 1, + "the frozen frame must still decode"); + CHECK(packets_equal(&in, &out), "decoded golden frame should match"); } int main(void) { - test_packet_size_is_on_the_wire_contract(); + test_sizes_are_the_wire_contract(); test_wire_format_is_frozen(); - test_checksum_matches_the_arduino(); test_roundtrip(); test_leading_garbage(); test_payload_containing_header_bytes(); test_bad_checksum_is_counted_not_silent(); test_corrupt_payload_is_rejected(); test_resync_after_dropped_byte(); - test_split_across_reads(); test_back_to_back_frames(); + + test_unknown_format_is_refused_not_decoded(); + test_length_lets_us_skip_a_newer_sender(); + test_wrong_length_for_our_own_format(); + test_checksum_covers_the_header_fields(); + test_dropped_packets_are_counted_from_the_sequence(); + test_sequence_wrap_is_not_a_flood(); + test_sender_restart_is_not_a_flood(); + + test_digital_accessors(); + test_out_of_range_is_an_error_not_a_read(); + test_encode_refuses_a_short_buffer(); test_null_arguments_are_safe(); + test_status_strings_exist(); printf("%d checks, %d failures\n", g_checks, g_failures); return g_failures == 0 ? 0 : 1; diff --git a/test/test_serial_pty.py b/test/test_serial_pty.py index 879e297..5172ac9 100755 --- a/test/test_serial_pty.py +++ b/test/test_serial_pty.py @@ -22,18 +22,31 @@ import time -def frame(speed, airspeed, engine_temp, rad_temp, channels, a0): - """Build one 26-byte frame: 0xAA 0x55, 23-byte payload, XOR checksum.""" +FORMAT_V1 = 1 +PAYLOAD_SIZE = 36 + + +def frame(speed, airspeed, temps, analog, digital_in, digital_out, sequence, + fmt=FORMAT_V1): + """Build one 41-byte frame. + + 0xAA 0x55, format, length, 36-byte payload, XOR checksum over the + format and length bytes as well as the payload. + """ payload = struct.pack( - " 5, so 2, 3 and 4 + # never arrived and the receiver should say so. (The corrupt frame above + # carried sequence 2, but it was rejected, so it does not count as + # received.) + os.write(master, frame(31.25, 5.5, sequence=5, + temps=(190.0, 150.0, 0.0, 0.0), + analog=(900, 0, 0, 0), + digital_in=0x16, digital_out=0x01)) time.sleep(0.8) proc.terminate() @@ -80,11 +107,17 @@ def main(): if "speed=23.50" not in out: failures.append("first good packet was not decoded") if "speed=31.25" not in out: - failures.append("did not recover after corruption") + failures.append("did not recover after corruption and a skip") if "ok=2" not in out: failures.append("expected exactly 2 accepted packets") if "bad=1" not in out: failures.append("corrupt packet was not counted as a checksum error") + if "fmt=1" not in out: + failures.append("packet from a newer sender was not counted as an " + "unknown format") + if "lost=3" not in out: + failures.append("gap in the sequence numbers was not counted as " + "dropped packets") if failures: for f in failures: diff --git a/tools/monitor.c b/tools/monitor.c index 90fefe3..bf756c6 100644 --- a/tools/monitor.c +++ b/tools/monitor.c @@ -40,27 +40,42 @@ static void install_handler(int signum) static void print_packet(const sh_packet_t *packet, const sh_stats_t *stats) { + char digital[18]; + unsigned i; + + /* inputs then outputs, most significant channel on the left so the + string reads the way the bitfield is written */ + for (i = 0; i < 8; ++i) { + bool v = false; + (void)sh_digital_in(packet, 7 - i, &v); + digital[i] = v ? '1' : '0'; + } + digital[8] = ' '; + for (i = 0; i < 8; ++i) { + bool v = false; + (void)sh_digital_out(packet, 7 - i, &v); + digital[9 + i] = v ? '1' : '0'; + } + digital[17] = '\0'; + printf("\r\x1b[2K" - "speed=%.2f mph" - " | air=%.2f" - " | engine=%.1fF" - " | rad=%.1fF" - " | ch=%u%u%u%u%u" + "#%u speed=%.2f air=%.2f" + " | eng=%.1fF rad=%.1fF" " | A0=%u" - " | ok=%llu bad=%llu resync=%llu", + " | in/out=%s" + " | ok=%llu bad=%llu fmt=%llu resync=%llu lost=%llu", + (unsigned)packet->sequence, (double)packet->speed, (double)packet->airspeed, - (double)packet->engineTemp, - (double)packet->radTemp, - (unsigned)packet->channel0, - (unsigned)packet->channel1, - (unsigned)packet->channel2, - (unsigned)packet->channel3, - (unsigned)packet->channel4, - (unsigned)packet->channelA0, + (double)packet->temps[SH_TEMP_ENGINE], + (double)packet->temps[SH_TEMP_RADIATOR], + (unsigned)packet->analog[SH_ANALOG_BATTERY], + digital, (unsigned long long)stats->packets, (unsigned long long)stats->checksum_errors, - (unsigned long long)stats->resyncs); + (unsigned long long)stats->format_errors, + (unsigned long long)stats->resyncs, + (unsigned long long)stats->dropped); fflush(stdout); } @@ -70,6 +85,7 @@ int main(int argc, char **argv) const char *device = (argc > 1) ? argv[1] : SH_DEFAULT_PORT; sh_parser_t parser; sh_packet_t packet; + sh_status_t status; int fd; signal(SIGPIPE, SIG_IGN); @@ -90,26 +106,29 @@ int main(int argc, char **argv) while (!g_stop) { memset(&packet, 0, sizeof(packet)); - if (!sh_serial_read_packet(fd, &parser, &packet)) { - if (errno == EINTR) { - continue; /* a signal; the while condition rechecks g_stop */ - } - if (errno != 0) { - fprintf(stderr, "\nserial read: %s\n", strerror(errno)); - } - else { - fprintf(stderr, "\nserial connection closed\n"); - } + status = sh_serial_read_packet(fd, &parser, &packet); + + if (status == SH_E_INTERRUPTED) { + continue; /* a signal; the while condition rechecks g_stop */ + } + + if (status != SH_OK) { + fprintf(stderr, "\n%s", sh_strstatus(status)); + if (status == SH_E_IO) fprintf(stderr, ": %s", strerror(errno)); + fprintf(stderr, "\n"); break; } print_packet(&packet, &parser.stats); } - printf("\n%llu packets, %llu checksum errors, %llu resyncs\n", + printf("\n%llu packets, %llu bad checksums, %llu unknown formats, " + "%llu resyncs, %llu never arrived\n", (unsigned long long)parser.stats.packets, (unsigned long long)parser.stats.checksum_errors, - (unsigned long long)parser.stats.resyncs); + (unsigned long long)parser.stats.format_errors, + (unsigned long long)parser.stats.resyncs, + (unsigned long long)parser.stats.dropped); sh_serial_close(fd); return 0; From 966e1a8577f6b317219d19ae2eb9c6a91e5e7a16 Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 19 Sep 2026 13:04:44 -0400 Subject: [PATCH 4/4] feat: protect the frame with a CRC instead of an XOR checksum An XOR sum misses two corruptions a car produces for real, and misses them completely rather than occasionally. Measured over two million corrupted frames: two bit flips in the same bit position were 100% undetected, because they cancel exactly, and two swapped bytes were 100% undetected, because an order-independent sum cannot see order. The first is what a ground bounce or a supply glitch on one data line does, which is to say what an ignition system does. CRC-16-CCITT catches both, plus every burst up to sixteen bits, where the XOR sum let 0.39% through. It costs 32 bytes of flash on an ATmega328p and one byte on a wire running at 7% utilisation. Both failure modes now have tests that try every paired flip and every byte swap in the frame and require zero to be accepted. Reverting the CRC to an order-independent sum lets 280 and 397 of them through respectively, so the tests fail loudly rather than decoratively. Done before anything is deployed, so format 1 simply means this from the start rather than needing a version bump and a reflash later. --- .github/workflows/ci.yml | 7 +- README.md | 32 +++++++-- src/sensorhub/parser.c | 56 ++++++++++----- src/sensorhub/sensorhub.h | 44 ++++++++---- test/test_parser.c | 141 +++++++++++++++++++++++++++++++++++--- test/test_serial_pty.py | 36 +++++++--- 6 files changed, 260 insertions(+), 56 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f16f673..1380795 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -119,8 +119,10 @@ jobs: check "stopped counting dropped packets" sed -i 's/gap < 1000u/gap < 65535u/' src/sensorhub/parser.c check "treated a sender restart as a flood of drops" - sed -i 's/sh_checksum(buffer + 2, SH_PAYLOAD_SIZE + 2u)/sh_checksum(buffer + 4, SH_PAYLOAD_SIZE)/' src/sensorhub/parser.c - check "checksummed the payload only, not fmt and len" + sed -i 's/sh_crc16(buffer + 2, SH_PAYLOAD_SIZE + 2u)/sh_crc16(buffer + 4, SH_PAYLOAD_SIZE)/' src/sensorhub/parser.c + check "covered the payload only, not fmt and len" + sed -i 's|crc ^= (uint16_t)((uint16_t)data\[i\] << 8);|crc ^= (uint16_t)data[i]; if(0)|' src/sensorhub/parser.c + check "weakened the CRC into an order-independent sum" sed -i 's/parser->stats.checksum_errors++;/;/' src/sensorhub/parser.c check "dropped checksum-error counting" sed -i 's/else if (byte == SH_HEADER_1) {/else if (0) {/' src/sensorhub/parser.c @@ -207,6 +209,7 @@ jobs: #include /* int is 16 bits here; the packed fixed-width layout must survive */ _Static_assert(sizeof(sh_packet_t) == 36, "struct differs on AVR"); + _Static_assert(SH_FRAME_SIZE == 42, "frame size drifted"); _Static_assert(sizeof(float) == 4, "float differs on AVR"); volatile unsigned char sink; int main(void) { diff --git a/README.md b/README.md index 1e413c8..80afd4c 100644 --- a/README.md +++ b/README.md @@ -8,10 +8,10 @@ packet, the checksum, and the frame encoder. ## The wire format -Forty-one bytes per frame, 115200 baud, 8N1: +Forty-two bytes per frame, 115200 baud, 8N1: ``` -0xAA 0x55 fmt len payload[len] checksum +0xAA 0x55 fmt len payload[len] crc16_lo crc16_hi ``` | field | size | purpose | @@ -20,7 +20,7 @@ Forty-one bytes per frame, 115200 baud, 8N1: | `fmt` | 1 | format version, currently `0x01` | | `len` | 1 | payload length, currently 36 | | payload | `len` | `sh_packet_t` | -| `checksum` | 1 | XOR of `fmt`, `len`, and every payload byte | +| `crc16` | 2 | CRC-16-CCITT over `fmt`, `len`, payload; little-endian | **The version and length bytes earn their keep.** Without a version, changing what a field *means* while keeping the packet the same size still passes the @@ -31,9 +31,30 @@ newer sender desynchronises instead of skipping one packet and carrying on. Two bytes on a link running at 7% utilisation is a good trade. -The checksum deliberately covers `fmt` and `len` as well as the payload, so a +The CRC deliberately covers `fmt` and `len` as well as the payload, so a corrupted length byte cannot quietly reframe the stream and still validate. +### Why a CRC and not a byte sum + +An XOR checksum is one byte cheaper and misses two corruptions a car +produces for real. Measured over two million corrupted frames: + +| corruption | XOR-8 undetected | CRC-16 undetected | +|---|---|---| +| single bit flip | 0% | 0% | +| **two flips, same bit position** | **100%** | 0% | +| 8-bit burst | 0% | 0% | +| 16-bit burst | 0.39% | 0% | +| **two bytes swapped** | **100%** | 0% | + +Those two 100% rows are structural, not unlucky: paired flips in the same bit +position cancel exactly under XOR, and an order-independent sum cannot see a +swap. The first is what a ground bounce or a supply glitch on one data line +produces, which is to say what an ignition system produces. + +CRC-16-CCITT costs 32 bytes of flash on an ATmega328p and one byte on a wire +running at 7% utilisation. The tests assert all three cases directly. + ### Payload, format 1 `sh_packet_t`, a packed struct of fixed-width types, 36 bytes on the Pi and on @@ -292,7 +313,8 @@ From ``, portable everywhere including AVR: void sh_parser_init (sh_parser_t *parser); sh_status_t sh_parser_feed (sh_parser_t *parser, uint8_t byte, sh_packet_t *out); -uint8_t sh_checksum (const uint8_t *data, size_t length); +uint16_t sh_crc16 (const uint8_t *data, size_t length); +uint16_t sh_crc16_continue(uint16_t crc, const uint8_t *data, size_t n); sh_status_t sh_encode_frame (const sh_packet_t *packet, uint8_t *buffer, size_t buffer_size, size_t *written); diff --git a/src/sensorhub/parser.c b/src/sensorhub/parser.c index 4ca7c96..5174420 100644 --- a/src/sensorhub/parser.c +++ b/src/sensorhub/parser.c @@ -111,17 +111,25 @@ sh_status_t sh_analog(const sh_packet_t *packet, unsigned index, uint16_t *out) * Checksum and encoding * ------------------------------------------------------------------ */ -uint8_t sh_checksum(const uint8_t *data, size_t length) +uint16_t sh_crc16_continue(uint16_t crc, const uint8_t *data, size_t length) { - uint8_t checksum = 0; - - if (data == NULL) return 0; + if (data == NULL) return crc; for (size_t i = 0; i < length; ++i) { - checksum ^= data[i]; + crc ^= (uint16_t)((uint16_t)data[i] << 8); + + for (unsigned bit = 0; bit < 8u; ++bit) { + crc = (crc & 0x8000u) ? (uint16_t)((crc << 1) ^ 0x1021u) + : (uint16_t)(crc << 1); + } } - return checksum; + return crc; +} + +uint16_t sh_crc16(const uint8_t *data, size_t length) +{ + return sh_crc16_continue(0xFFFFu, data, length); } sh_status_t sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer, @@ -136,9 +144,13 @@ sh_status_t sh_encode_frame(const sh_packet_t *packet, uint8_t *buffer, buffer[3] = (uint8_t)SH_PAYLOAD_SIZE; memcpy(buffer + 4, packet, SH_PAYLOAD_SIZE); - /* The checksum covers fmt and len too, so a corrupted length byte cannot - quietly reframe the stream and still validate. */ - buffer[4 + SH_PAYLOAD_SIZE] = sh_checksum(buffer + 2, SH_PAYLOAD_SIZE + 2u); + /* The CRC covers fmt and len as well as the payload, so a corrupted + length byte cannot quietly reframe the stream and still validate. */ + { + uint16_t crc = sh_crc16(buffer + 2, SH_PAYLOAD_SIZE + 2u); + buffer[4 + SH_PAYLOAD_SIZE] = (uint8_t)(crc & 0xFFu); + buffer[5 + SH_PAYLOAD_SIZE] = (uint8_t)(crc >> 8); + } if (written != NULL) *written = SH_FRAME_SIZE; return SH_OK; @@ -240,20 +252,32 @@ sh_status_t sh_parser_feed(sh_parser_t *parser, uint8_t byte, sh_packet_t *out) case SH_READ_PAYLOAD: parser->payload[parser->payload_index++] = byte; if (parser->payload_index == (size_t)parser->length) { - parser->state = SH_READ_CHECKSUM; + parser->state = SH_READ_CRC_LO; } break; - case SH_READ_CHECKSUM: { - uint8_t expected; + case SH_READ_CRC_LO: + parser->crc_lo = byte; + parser->state = SH_READ_CRC_HI; + break; + + case SH_READ_CRC_HI: { + uint16_t received = (uint16_t)(((uint16_t)byte << 8) | parser->crc_lo); + uint16_t expected; + uint8_t header[2]; parser->state = SH_WAIT_HEADER_1; - /* Recompute over fmt, len, and the payload, matching the encoder. */ - expected = (uint8_t)(parser->format ^ parser->length); - expected ^= sh_checksum(parser->payload, (size_t)parser->length); + /* Recompute over fmt, len, and the payload, matching the encoder. + Feeding the two header fields through the same routine keeps one + definition of the polynomial rather than two. */ + header[0] = parser->format; + header[1] = parser->length; + expected = sh_crc16_continue(sh_crc16(header, sizeof(header)), + parser->payload, + (size_t)parser->length); - if (expected == byte) { + if (expected == received) { memcpy(out, parser->payload, SH_PAYLOAD_SIZE); parser->stats.packets++; note_sequence(parser, out->sequence); diff --git a/src/sensorhub/sensorhub.h b/src/sensorhub/sensorhub.h index d046706..70d8e3c 100644 --- a/src/sensorhub/sensorhub.h +++ b/src/sensorhub/sensorhub.h @@ -4,13 +4,19 @@ * * FRAME * - * 0xAA 0x55 fmt len payload[len] checksum + * 0xAA 0x55 fmt len payload[len] crc16_lo crc16_hi * - * fmt format version. A receiver refuses anything it does not know - * rather than decoding it confidently and wrongly. - * len payload length. Lets an old receiver skip a packet from a newer - * sender and stay framed, instead of desynchronising. - * checksum XOR of fmt, len, and every payload byte. + * fmt format version. A receiver refuses anything it does not know + * rather than decoding it confidently and wrongly. + * len payload length. Lets an old receiver skip a packet from a newer + * sender and stay framed, instead of desynchronising. + * crc16 CRC-16-CCITT over fmt, len, and the payload, little-endian. + * + * The CRC is not a byte-sum. An XOR checksum misses two corruptions that a + * car produces for real: two bit flips in the same bit position cancel + * exactly, and swapped bytes are invisible to an order-independent sum. Both + * were measured at 100% undetected. CRC-16 catches both, plus every burst up + * to 16 bits, for 32 bytes of flash on an ATmega328p. * * The version and length bytes are the difference between "we changed the * format and everything broke loudly" and "we changed the format and the car @@ -87,8 +93,8 @@ typedef struct __attribute__((packed)) { uint16_t sequence; } sh_packet_t; -#define SH_PAYLOAD_SIZE 36u /* sizeof(sh_packet_t) for SH_FORMAT_V1 */ -#define SH_FRAME_OVERHEAD 5u /* 2 header + fmt + len + checksum */ +#define SH_PAYLOAD_SIZE 36u /* sizeof(sh_packet_t) for SH_FORMAT_V1 */ +#define SH_FRAME_OVERHEAD 6u /* 2 header + fmt + len + 2 CRC */ #define SH_FRAME_SIZE (SH_PAYLOAD_SIZE + SH_FRAME_OVERHEAD) /* The largest payload the parser will buffer. Bigger than V1 on purpose, so @@ -119,7 +125,7 @@ typedef enum { SH_OK = 0, /* a packet is ready */ SH_INCOMPLETE, /* byte consumed, nothing complete yet */ SH_E_NULL, /* a required argument was NULL */ - SH_E_CHECKSUM, /* framed correctly, contents rejected */ + SH_E_CHECKSUM, /* framed correctly, CRC rejected it */ SH_E_FORMAT, /* fmt byte names a version we do not know */ SH_E_LENGTH, /* len disagrees with fmt, or exceeds the max */ SH_E_RANGE, /* a channel index was out of bounds */ @@ -226,7 +232,8 @@ typedef enum { SH_READ_FORMAT, SH_READ_LENGTH, SH_READ_PAYLOAD, - SH_READ_CHECKSUM, + SH_READ_CRC_LO, + SH_READ_CRC_HI, SH_SKIP_UNKNOWN /* draining a packet whose format we do not know */ } sh_state_t; @@ -236,6 +243,7 @@ typedef struct { uint8_t length; uint8_t payload[SH_MAX_PAYLOAD]; size_t payload_index; + uint8_t crc_lo; bool have_sequence; uint16_t last_sequence; sh_stats_t stats; @@ -265,8 +273,20 @@ sh_status_t sh_parser_feed(sh_parser_t *parser, uint8_t byte, * Encoding * ------------------------------------------------------------------ */ -/* XOR of every byte. Exposed so tests and senders need not duplicate it. */ -uint8_t sh_checksum(const uint8_t *data, size_t length); +/* + * CRC-16-CCITT, polynomial 0x1021, initial value 0xFFFF, no final xor. + * + * Exposed so tests and any other transmitter need not duplicate it. The + * bitwise form is deliberate: a 256-entry table would be faster and cost 512 + * bytes of flash on a part that has 30KB, to save time this loop does not + * need at 20 packets a second. + */ +uint16_t sh_crc16(const uint8_t *data, size_t length); + +/* Resume a CRC over a second run of bytes. Lets the receiver cover the + format and length fields and then the payload without copying them into + one contiguous buffer first. */ +uint16_t sh_crc16_continue(uint16_t crc, const uint8_t *data, size_t length); /* * Serialise a packet into a complete frame. diff --git a/test/test_parser.c b/test/test_parser.c index 65b7855..6ddeb74 100644 --- a/test/test_parser.c +++ b/test/test_parser.c @@ -75,8 +75,8 @@ static int feed_all(sh_parser_t *parser, const uint8_t *data, size_t len, static void test_sizes_are_the_wire_contract(void) { CHECK(sizeof(sh_packet_t) == 36, "payload must stay 36 bytes"); - CHECK(SH_FRAME_SIZE == 41, "frame must stay 41 bytes"); - CHECK(SH_FRAME_OVERHEAD == 5, "overhead is header, fmt, len, checksum"); + CHECK(SH_FRAME_SIZE == 42, "frame must stay 42 bytes"); + CHECK(SH_FRAME_OVERHEAD == 6, "overhead is header, fmt, len, 2 CRC bytes"); } static void test_roundtrip(void) @@ -158,7 +158,7 @@ static void test_bad_checksum_is_counted_not_silent(void) last = sh_parser_feed(&parser, frame[i], &out); } - CHECK(last == SH_E_CHECKSUM, "a bad checksum must report SH_E_CHECKSUM"); + CHECK(last == SH_E_CHECKSUM, "a bad CRC must report SH_E_CHECKSUM"); CHECK(parser.stats.checksum_errors == 1, "and must be counted"); CHECK(parser.stats.packets == 0, "and must not yield a packet"); } @@ -238,10 +238,13 @@ static void test_unknown_format_is_refused_not_decoded(void) encode(&in, frame); frame[2] = 0x02u; /* a future format */ - /* fix the checksum so this is purely a version rejection, not a - corruption that would be caught anyway */ - frame[SH_FRAME_SIZE - 1] = - (uint8_t)(frame[2] ^ frame[3]) ^ sh_checksum(frame + 4, SH_PAYLOAD_SIZE); + /* fix the CRC so this is purely a version rejection, not a corruption + that would have been caught anyway */ + { + uint16_t crc = sh_crc16(frame + 2, SH_PAYLOAD_SIZE + 2u); + frame[SH_FRAME_SIZE - 2] = (uint8_t)(crc & 0xFFu); + frame[SH_FRAME_SIZE - 1] = (uint8_t)(crc >> 8); + } sh_parser_init(&parser); for (size_t i = 0; i < sizeof(frame); ++i) { @@ -266,13 +269,14 @@ static void test_length_lets_us_skip_a_newer_sender(void) sh_parser_t parser; size_t n = 0; - /* A future frame: format 2, payload 50 bytes. */ + /* A future frame: format 2, payload 50 bytes, then its two CRC bytes. */ buf[n++] = SH_HEADER_1; buf[n++] = SH_HEADER_2; buf[n++] = 0x02u; buf[n++] = 50u; for (int i = 0; i < 50; ++i) buf[n++] = (uint8_t)i; - buf[n++] = 0x00u; /* its checksum, whatever it is */ + buf[n++] = 0x00u; + buf[n++] = 0x00u; /* Then a frame we do understand. */ encode(&in, buf + n); @@ -465,7 +469,8 @@ static void test_null_arguments_are_safe(void) "NULL buffer"); CHECK(sh_digital_in(NULL, 0, &v) == SH_E_NULL, "NULL packet to accessor"); CHECK(sh_digital_in(&p, 0, NULL) == SH_E_NULL, "NULL out to accessor"); - CHECK(sh_checksum(NULL, 10) == 0, "NULL checksum input"); + CHECK(sh_crc16(NULL, 10) == 0xFFFFu, + "NULL CRC input returns the initial value, not garbage"); } static void test_status_strings_exist(void) @@ -518,7 +523,7 @@ static void test_wire_format_is_frozen(void) 0x0D, /* digital_in 0b00001101 */ 0x02, /* digital_out 0b00000010 */ 0x07, 0x00, /* sequence 7 */ - 0x84 /* checksum over fmt, len, payload */ + 0x51, 0xF1 /* CRC-16 over fmt, len, payload, LE */ }; sh_packet_t in = sample_packet(); @@ -539,6 +544,115 @@ static void test_wire_format_is_frozen(void) CHECK(packets_equal(&in, &out), "decoded golden frame should match"); } + +/* ---- the corruptions an XOR checksum could not see ---- */ + +/* Feed a whole frame and report whether it was accepted. */ +static bool frame_accepted(const uint8_t *frame, size_t len) +{ + sh_parser_t parser; + sh_packet_t out; + + sh_parser_init(&parser); + for (size_t i = 0; i < len; ++i) { + if (sh_parser_feed(&parser, frame[i], &out) == SH_OK) return true; + } + return false; +} + +static void test_two_flips_in_the_same_bit_position(void) +{ + /* + * The reason this frame carries a CRC rather than an XOR sum. + * + * Two bit flips in the same bit position cancel exactly under XOR, so a + * byte-sum accepted 100% of these in a two-million-frame simulation. It + * is not an exotic case either: a ground bounce or a supply glitch + * disturbing one data line flips the same bit in consecutive bytes, + * which is exactly what an ignition system produces. + */ + sh_packet_t in = sample_packet(); + uint8_t frame[SH_FRAME_SIZE]; + unsigned accepted = 0; + + for (unsigned bit = 0; bit < 8u; ++bit) { + for (unsigned i = 4; i < 4u + SH_PAYLOAD_SIZE - 1u; ++i) { + encode(&in, frame); + frame[i] ^= (uint8_t)(1u << bit); + frame[i + 1] ^= (uint8_t)(1u << bit); + if (frame_accepted(frame, sizeof(frame))) accepted++; + } + } + + CHECK(accepted == 0, + "paired same-bit flips must all be caught, %u slipped through", + accepted); +} + +static void test_swapped_bytes(void) +{ + /* + * The other one. XOR is order-independent, so swapping two payload bytes + * is completely invisible to it: also 100% undetected in simulation. + */ + sh_packet_t in = sample_packet(); + uint8_t frame[SH_FRAME_SIZE]; + unsigned accepted = 0; + unsigned tried = 0; + + for (unsigned i = 4; i < 4u + SH_PAYLOAD_SIZE; ++i) { + for (unsigned j = i + 1u; j < 4u + SH_PAYLOAD_SIZE; ++j) { + uint8_t tmp; + + encode(&in, frame); + if (frame[i] == frame[j]) continue; /* a no-op swap */ + + tmp = frame[i]; + frame[i] = frame[j]; + frame[j] = tmp; + tried++; + + if (frame_accepted(frame, sizeof(frame))) accepted++; + } + } + + CHECK(tried > 100, "should have tried a decent number of swaps, got %u", + tried); + CHECK(accepted == 0, "swapped bytes must all be caught, %u slipped through", + accepted); +} + +static void test_every_single_bit_flip_is_caught(void) +{ + /* Across the whole frame, including the header fields and the CRC. */ + sh_packet_t in = sample_packet(); + uint8_t frame[SH_FRAME_SIZE]; + unsigned accepted = 0; + + for (unsigned i = 2; i < SH_FRAME_SIZE; ++i) { + for (unsigned bit = 0; bit < 8u; ++bit) { + encode(&in, frame); + frame[i] ^= (uint8_t)(1u << bit); + if (frame_accepted(frame, sizeof(frame))) accepted++; + } + } + + CHECK(accepted == 0, "every single-bit flip must be caught, %u were not", + accepted); +} + +static void test_crc_matches_the_reference_vector(void) +{ + /* CRC-16-CCITT (0x1021, init 0xFFFF) over "123456789" is 0x29B1. This is + the standard check value; if it fails, the polynomial or the init + constant drifted. */ + static const uint8_t check[] = "123456789"; + + CHECK(sh_crc16(check, sizeof(check) - 1u) == 0x29B1u, + "CRC-16-CCITT check value must be 0x29B1, got 0x%04X", + (unsigned)sh_crc16(check, sizeof(check) - 1u)); +} + int main(void) { test_sizes_are_the_wire_contract(); @@ -565,6 +679,11 @@ int main(void) test_null_arguments_are_safe(); test_status_strings_exist(); + test_crc_matches_the_reference_vector(); + test_every_single_bit_flip_is_caught(); + test_two_flips_in_the_same_bit_position(); + test_swapped_bytes(); + printf("%d checks, %d failures\n", g_checks, g_failures); return g_failures == 0 ? 0 : 1; } diff --git a/test/test_serial_pty.py b/test/test_serial_pty.py index 5172ac9..0444f35 100755 --- a/test/test_serial_pty.py +++ b/test/test_serial_pty.py @@ -28,10 +28,10 @@ def frame(speed, airspeed, temps, analog, digital_in, digital_out, sequence, fmt=FORMAT_V1): - """Build one 41-byte frame. + """Build one 42-byte frame. - 0xAA 0x55, format, length, 36-byte payload, XOR checksum over the - format and length bytes as well as the payload. + 0xAA 0x55, format, length, 36-byte payload, then a little-endian + CRC-16-CCITT covering the format and length bytes as well as the payload. """ payload = struct.pack( "> 8) & 0xFF])) + + +def crc16_ccitt(data): + """CRC-16-CCITT, poly 0x1021, init 0xFFFF. Check value for b"123456789" + is 0x29B1; asserted below so a typo here cannot masquerade as a firmware + bug.""" + crc = 0xFFFF + for byte in data: + crc ^= byte << 8 + for _ in range(8): + crc = ((crc << 1) ^ 0x1021) & 0xFFFF if crc & 0x8000 \ + else (crc << 1) & 0xFFFF + return crc + + +assert crc16_ccitt(b"123456789") == 0x29B1, "CRC implementation is wrong" def main(): @@ -69,7 +84,7 @@ def main(): os.write(master, frame(23.5, 19.25, sequence=1, **good)) time.sleep(0.2) - # Line noise, then a frame with a deliberately corrupted checksum. + # Line noise, then a frame with a deliberately corrupted CRC. os.write(master, b"\x00\xff\xde\xad\xbe\xef") corrupt = bytearray(frame(11.0, 2.0, sequence=2, **good)) corrupt[-1] ^= 0xFF @@ -78,7 +93,8 @@ def main(): # A packet from a "newer" sender. The length byte must let the receiver # skip it and stay framed rather than desynchronising. - future = bytes([0xAA, 0x55, 0x02, 50]) + bytes(range(50)) + bytes([0x00]) + future = (bytes([0xAA, 0x55, 0x02, 50]) + bytes(range(50)) + + bytes([0x00, 0x00])) os.write(master, future) time.sleep(0.2) @@ -111,7 +127,7 @@ def main(): if "ok=2" not in out: failures.append("expected exactly 2 accepted packets") if "bad=1" not in out: - failures.append("corrupt packet was not counted as a checksum error") + failures.append("corrupt packet was not counted as a CRC error") if "fmt=1" not in out: failures.append("packet from a newer sender was not counted as an " "unknown format")