diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7a11ff5..5dd2088 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -37,11 +37,11 @@ jobs: - name: Install SensorHub, which supplies the shared packet format run: | - arduino-cli lib install --git-url https://github.com/HEEV/SensorHub || { - echo "falling back to a direct checkout" - git clone --depth 1 https://github.com/HEEV/SensorHub.git \ - "$(arduino-cli config get directories.user)/libraries/SensorHub" - } + # a plain checkout rather than `lib install --git-url`: that flag is + # disabled unless library.enable_unsafe_install is turned on, so the + # git-url path could never have been the one doing the work + git clone --depth 1 https://github.com/HEEV/SensorHub.git \ + "$(arduino-cli config get directories.user)/libraries/SensorHub" # a clone can succeed while producing something Arduino cannot use, # so check for the entry point rather than trusting the exit status lib="$(arduino-cli config get directories.user)/libraries/SensorHub" @@ -78,3 +78,79 @@ jobs: echo "over 80% of flash; time to think about what to cut" exit 1 fi + + simulate: + # Run the firmware on a simulated ATmega328p and require SensorHub's real + # parser to accept what it transmits. + # + # This is the test that would catch the wire format drifting on either + # side. Compiling both halves separately proves nothing about whether they + # agree; this feeds real transmitted bytes into the real decoder. + name: the firmware's output must decode + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + path: sketch/carsensordriver + + - uses: arduino/setup-arduino-cli@v2 + + - name: Install the AVR core and libraries + run: | + arduino-cli core update-index + arduino-cli core install arduino:avr + arduino-cli lib install DS18B20 + arduino-cli lib install OneWire + + - name: Install SensorHub + run: | + git clone --depth 1 https://github.com/HEEV/SensorHub.git \ + "$(arduino-cli config get directories.user)/libraries/SensorHub" + + - name: Install simavr + run: | + sudo apt-get update + sudo apt-get install -y simavr libsimavr-dev libelf-dev + + - name: Build the firmware + run: | + set -o pipefail + arduino-cli compile \ + --fqbn arduino:avr:nano:cpu=atmega328 \ + --output-dir /tmp/fw \ + sketch/carsensordriver + + - name: Build the harnesses + run: | + LIB="$(arduino-cli config get directories.user)/libraries/SensorHub" + cc -O2 -std=c11 -Wall -Wextra -Werror \ + sketch/carsensordriver/test/run_firmware.c \ + -lsimavr -lelf -o /tmp/run_firmware + cc -O2 -std=c11 -Wall -Wextra -Werror -I"$LIB/src" \ + sketch/carsensordriver/test/check_frames.c \ + "$LIB/src/sensorhub/parser.c" -o /tmp/check_frames + + - name: Run the firmware + # capture stalls around 50 frames whatever the budget: the AVR's TX + # ring fills and simavr does not drain it. Plenty to check framing on. + run: /tmp/run_firmware /tmp/fw/*.elf /tmp/uart.bin 3000000 + + - name: Every frame must decode, with no gaps + # 25, not 50: the observed floor is ~51 and a threshold one packet + # under it would fail for reasons having nothing to do with this repo + run: /tmp/check_frames /tmp/uart.bin 25 + + - name: And the check must be able to fail + # a harness that cannot fail launders a guess into a green tick + run: | + python3 - <<'PY' + d = bytearray(open('/tmp/uart.bin','rb').read()) + for i in range(2, len(d), 41): + d[i] ^= 0xFF # corrupt every format byte + open('/tmp/uart_bad.bin','wb').write(bytes(d)) + PY + if /tmp/check_frames /tmp/uart_bad.bin 50 > /dev/null 2>&1; then + echo "the check passed corrupted input; it is not checking anything" + exit 1 + fi + echo "corrupted input correctly rejected" diff --git a/carsensordriver.ino b/carsensordriver.ino index d4c7e15..f3fc9dd 100644 --- a/carsensordriver.ino +++ b/carsensordriver.ino @@ -1,21 +1,11 @@ #include #include #include +#include /* NAN for a sensor that has never answered */ -/* - * The packet layout, the checksum, and the frame encoder live in the - * SensorHub library, which the Raspberry Pi uses to decode this. Sharing one - * definition is the whole point: the two ends cannot drift apart, because - * there is only one of them to edit. - * - * Install with: - * arduino-cli lib install --git-url https://github.com/HEEV/SensorHub - * - * Only the encoder is linked here, about 90 bytes of flash more than the - * hand-rolled version it replaced. The receiving state machine comes along in - * the same header for whenever the Pi starts commanding the output channels - * on pins 10, 11, and 12. - */ +/* Packet layout, checksum and frame encoder come from SensorHub, which the + Pi uses to decode this, so the two ends cannot drift apart. + arduino-cli lib install --git-url https://github.com/HEEV/SensorHub */ #include /* Keep the local spelling so the call sites below read unchanged. */ @@ -38,6 +28,12 @@ static uint16_t sequence = 0; #define circumference (2 * wheelRadius * PI) // in in #define pulseDist (circumference / numMagnets) +/* One magnet per revolution, so ~10 Hz flat out and slower elsewhere: + 10 Hz -> 35.7 mph 2 Hz -> 7.1 mph 0.5 Hz -> 1.8 mph + Speed is a whole-revolution average and the loop sends at 20 Hz, so at + best every other packet repeats. Never block longer than one pulse. */ +#define SPEED_STALE_MS 3800UL /* below ~0.94 mph, report a standstill */ + // other wheelspeed variables volatile unsigned long magnetTimes[2] = { 0 }; // volatile modifier due to write in interrupt volatile unsigned long deltaTime = 0; @@ -54,9 +50,30 @@ DS18B20 ds(3); const uint8_t engineTempAddr[8] = { 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00 }; const uint8_t radTempAddr[8] = { 0x28, 0xD0, 0xEB, 0x87, 0x00, 0xCA, 0x26, 0x82 }; -// Index 0 is engine temp, index 1 is rad temp -const int cacheTTL[] = {50, 50}; -int cacheLife[] = {0, 0}; +/* getTempF() blocks: CONVERT_T then a 750 ms wait at 12-bit. Both sensors + inline every 50 ms loop cost ~30 packets. 9-bit and one sensor per 100 ms + cuts that ~16x. It never lost wheel interrupts: Arduino's delay() spins + with interrupts enabled. Fully async needs raw OneWire; no API for it. */ +#define TEMP_POLL_INTERVAL_MS 100UL /* the part cannot do better than ~90 ms */ +#define TEMP_RESOLUTION 9 /* 94 ms conversion instead of 750 ms */ +#define TEMP_SENSOR_COUNT 2 + +static const uint8_t *const tempAddr[TEMP_SENSOR_COUNT] = { + engineTempAddr, radTempAddr +}; + +/* DS18B20::select() takes a non-const pointer it does not write through, so + the cast is the library's fault, not ours. Named once rather than inline. */ +static uint8_t selectSensor(uint8_t i) { + return ds.select((uint8_t *)tempAddr[i]); +} + +/* NAN until a sensor answers. The old code fell off the end of these + functions and returned register contents, which looked like real data. */ +static float tempValue[TEMP_SENSOR_COUNT] = { NAN, NAN }; +static uint8_t tempIndex = 0; +static unsigned long tempTimer = 0; + void sendPacket(const DataPacket &packet) { /* One buffered write rather than four: header, version, length, payload, @@ -100,9 +117,12 @@ void loop() { // Update speed values speed = getSpeed(); - // Update temperature cache values - float engTemp = updateEngineTemp(); - float radTemp = updateRadiatorTemp(); + // Step the temperature poller: reads at most one sensor, and only every + // 100 ms, rather than both on every pass through here. + serviceTemps(); + + float engTemp = tempValue[0]; + float radTemp = tempValue[1]; DataPacket packet; memset(&packet, 0, sizeof(packet)); @@ -154,52 +174,77 @@ void handleMagnet() { } float getSpeed() { + unsigned long lastMagnet; + unsigned long delta; + + /* + * Snapshot both ISR variables with interrupts off. + * + * These are 32-bit on an 8-bit part, so a plain read is four separate byte + * loads. If handleMagnet() fires between them the result is half the old + * value and half the new one, which produces a speed that was never real. + * At 10 Hz the window is small but it is not zero, and a torn deltaTime + * shows up as an implausible spike rather than as an obvious fault. + */ + noInterrupts(); + lastMagnet = magnetTimes[0]; + delta = deltaTime; + interrupts(); + + if (lastMagnet == 0) { + return 0.0f; /* no magnet seen since boot */ + } - if (millis() - magnetTimes[0] < 3800 && magnetTimes[0] != 0) { - // Calculating our speed based on the magnet timings - - /* - current magnet setup (X is a magnet) - *********** - * X * - * * - * O * - * * - * * - *********** - */ - - // Calculate speed in inches per second - float inps = ((circumference / numMagnets) / deltaTime) * 1000.0f; - - // convert the speed we calculated from Inches/Sec to Miles/Hr - return ((inps / 12.0f) / 5280.0f) * 3600.0f; + if (millis() - lastMagnet >= SPEED_STALE_MS) { + return 0.0f; /* stopped, or slower than about 0.94 mph */ } - return 0.0; + if (delta == 0) { + return 0.0f; /* guard the divide; debounce should prevent this */ + } + + /* + * current magnet setup (X is a magnet) + * *********** + * * X * + * * * + * * O * + * * * + * * * + * *********** + */ + + /* inches per second, then inches/sec -> miles/hour */ + float inps = (pulseDist / (float)delta) * 1000.0f; + return ((inps / 12.0f) / 5280.0f) * 3600.0f; } // Get Temperatures, but only every so often because these sensors are slow. -float updateEngineTemp() { - if (ds.select(engineTempAddr)){ - if (cacheLife[0] > cacheTTL[0]) { - cacheLife[0] = 0; - return ds.getTempF(); - } else { - cacheLife[0]++; - } - } -} +/* + * Read one temperature sensor, at most every TEMP_POLL_INTERVAL_MS. + * + * Blocks for one 9-bit conversion, about 94 ms, when it does read. That is + * the best this library allows; see the note above. A sensor that does not + * select is left as NAN rather than reported as a plausible number. + */ +void serviceTemps() { + unsigned long now = millis(); -float updateRadiatorTemp() { - if (ds.select(radTempAddr)){ - if (cacheLife[1] > cacheTTL[1]) { - cacheLife[1] = 0; - return ds.getTempF(); - } - else { - cacheLife[1]++; - } + if (now - tempTimer < TEMP_POLL_INTERVAL_MS) { + return; + } + tempTimer = now; + + if (selectSensor(tempIndex)) { + ds.setResolution(TEMP_RESOLUTION); + tempValue[tempIndex] = ds.getTempF(); + } else { + /* Not on the bus. The engine probe's address is still all zeroes, so + this is its normal path. Report NAN, not an invented value. */ + tempValue[tempIndex] = NAN; } + + tempIndex = (uint8_t)((tempIndex + 1) % TEMP_SENSOR_COUNT); } + diff --git a/test/check_frames.c b/test/check_frames.c new file mode 100644 index 0000000..41afe29 --- /dev/null +++ b/test/check_frames.c @@ -0,0 +1,105 @@ +/* + * Check that what the firmware actually transmitted is what the Raspberry Pi + * can actually decode. + * + * Input is raw UART bytes captured from the firmware running on a simulated + * ATmega328p. They are fed through SensorHub's real parser, the same code the + * car runs. Both halves of the wire format are therefore checked against each + * other with no hardware in the loop. + * + * check_frames + * + * Fails on anything less than a clean run: a CRC error, an unrecognised + * format, a resync, a gap in the sequence numbers, or too few packets. + */ + +#include + +#include +#include +#include + +int main(int argc, char **argv) +{ + FILE *f; + sh_parser_t parser; + sh_packet_t packet; + long accepted = 0; + long want; + int c; + int failures = 0; + uint16_t first = 0, last = 0; + + if (argc < 3) { + fprintf(stderr, "usage: check_frames \n"); + return 2; + } + + want = atol(argv[2]); + + f = fopen(argv[1], "rb"); + if (f == NULL) { + perror(argv[1]); + return 2; + } + + sh_parser_init(&parser); + + while ((c = fgetc(f)) != EOF) { + if (sh_parser_feed(&parser, (uint8_t)c, &packet) == SH_OK) { + if (accepted == 0) { + first = packet.sequence; + printf("first packet: seq=%u speed=%.2f air=%.2f " + "eng=%.1f rad=%.1f A0=%u in=0x%02X out=0x%02X\n", + packet.sequence, (double)packet.speed, + (double)packet.airspeed, (double)packet.temps[0], + (double)packet.temps[1], packet.analog[0], + packet.digital_in, packet.digital_out); + } + last = packet.sequence; + accepted++; + } + } + + fclose(f); + + printf("\naccepted %ld\n", accepted); + printf("sequence %u .. %u\n", first, last); + printf("crc errors %llu\n", (unsigned long long)parser.stats.checksum_errors); + printf("format errors %llu\n", (unsigned long long)parser.stats.format_errors); + printf("resyncs %llu\n", (unsigned long long)parser.stats.resyncs); + printf("dropped %llu\n\n", (unsigned long long)parser.stats.dropped); + + { + /* A table, not six copies of one branch. The seventh check is data. */ + const struct { bool bad; const char *why; } checks[] = { + { accepted < want, + "too few packets" }, + { parser.stats.checksum_errors != 0, + "frames the parser could not verify" }, + { parser.stats.format_errors != 0, + "format byte disagreement between firmware and library" }, + { parser.stats.resyncs != 0, + "lost framing on a clean simulated link" }, + { parser.stats.dropped != 0, + "gap in the sequence numbers" }, + /* a clean run of N packets spans exactly N sequence numbers */ + { accepted > 0 && + (uint16_t)(last - first) != (uint16_t)(accepted - 1), + "packet count and sequence span disagree" }, + }; + + for (size_t i = 0; i < sizeof(checks) / sizeof(checks[0]); ++i) { + if (checks[i].bad) { + printf("FAIL: %s\n", checks[i].why); + failures++; + } + } + } + + if (failures == 0) { + printf("PASS: the firmware and the library agree on the wire\n"); + } + + return failures == 0 ? 0 : 1; +} diff --git a/test/run_firmware.c b/test/run_firmware.c new file mode 100644 index 0000000..2818a26 --- /dev/null +++ b/test/run_firmware.c @@ -0,0 +1,118 @@ +/* + * Run the compiled firmware on a simulated ATmega328p and capture the raw + * bytes it puts on the UART. + * + * Why not just read simavr's console output: that is a pretty-printer. It + * substitutes '.' for every non-printable byte, so the format byte 0x01 comes + * out as 0x2E and nothing parses. Hooking UART_IRQ_OUTPUT gives the bytes the + * chip would actually have transmitted. + * + * run_firmware + * + * The budget is in simulated microseconds, not iterations: avr_run() executes + * one instruction, so counting calls measures something nobody cares about. + */ + +#include +#include +#include +#include + +#include +#include +#include + +#define MCU "atmega328p" +#define FREQ 16000000 + +static FILE *out; +static long captured; + +static void on_uart_byte(struct avr_irq_t *irq, uint32_t value, void *param) +{ + (void)irq; + (void)param; + fputc((int)(value & 0xFF), out); + captured++; +} + +int main(int argc, char **argv) +{ + elf_firmware_t fw; + avr_t *avr; + avr_irq_t *irq; + avr_cycle_count_t limit; + + memset(&fw, 0, sizeof(fw)); + + if (argc < 4) { + fprintf(stderr, "usage: run_firmware \n"); + return 2; + } + + if (elf_read_firmware(argv[1], &fw) != 0) { + fprintf(stderr, "could not read %s\n", argv[1]); + return 2; + } + + avr = avr_make_mcu_by_name(MCU); + if (avr == NULL) { + fprintf(stderr, "simavr does not know " MCU "\n"); + return 2; + } + + avr_init(avr); + fw.frequency = FREQ; + avr_load_firmware(avr, &fw); + avr->frequency = FREQ; + + out = fopen(argv[2], "wb"); + if (out == NULL) { + perror(argv[2]); + return 2; + } + + /* Console echo off: it is a pretty-printer that substitutes '.' for + non-printable bytes, and we want the real ones. */ + { + uint32_t flags = 0; + avr_ioctl(avr, AVR_IOCTL_UART_GET_FLAGS('0'), &flags); + flags &= (uint32_t)~AVR_UART_FLAG_STDIO; + avr_ioctl(avr, AVR_IOCTL_UART_SET_FLAGS('0'), &flags); + } + + irq = avr_io_getirq(avr, AVR_IOCTL_UART_GETIRQ('0'), UART_IRQ_OUTPUT); + if (irq == NULL) { + fprintf(stderr, "no UART output irq on " MCU "\n"); + return 2; + } + avr_irq_register_notify(irq, on_uart_byte, NULL); + + limit = (avr_cycle_count_t)((double)atoll(argv[3]) * (double)FREQ / 1e6); + + while (avr->cycle < limit) { + int state = avr_run(avr); + if (state == cpu_Done || state == cpu_Crashed) { + fprintf(stderr, "firmware stopped early: state %d\n", state); + break; + } + } + + fclose(out); + + /* Known limitation: capture stops after roughly 50 frames, whatever the + budget, because the AVR's TX ring fills and simavr does not drain it. + The firmware keeps running, wedged in HardwareSerial::write. So treat + the frame count as a sample, not as a rate: it says nothing about how + fast the loop runs, and comparing counts between builds measures where + the simulator stalls rather than anything about the code. */ + fprintf(stderr, "captured %ld bytes, %.3f s simulated (stops early, see note)\n", + captured, (double)avr->cycle / (double)FREQ); + + if (captured == 0) { + fprintf(stderr, "the firmware transmitted nothing\n"); + return 1; + } + + return 0; +}