From 59ce2b3c11cec8c94e6c73720398ac579d02f1ac Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 26 Sep 2026 09:52:41 -0400 Subject: [PATCH 1/4] fix: stop the temperature read stalling the packet stream, and make the speed read atomic Three problems, one of which was producing wrong data for months. The temperature functions fell off the end without returning a value on the cache-hit path, which ran 50 times out of every 51. That is undefined behaviour: the caller got whatever was in the return register. A radiator temperature frozen at exactly 48.4 in every CSV on the car is consistent with one real early reading left sitting there and never updated again. A sensor that has never answered now reports NAN instead of an invented number, which is what the engine probe should have been doing all along: its address is still all zeroes, so it has never been on the bus. getTempF() also blocks. It issues CONVERT_T then waits out the conversion, 750 ms at the library's default 12-bit resolution, and both sensors were read inline on every pass through a loop meant to turn over in 50 ms. Dropping to 9-bit resolution and reading one sensor per 100 ms cuts that by about 16x. 0.5 C is far finer than anything useful about coolant temperature. To be accurate about the damage: this did not lose wheel interrupts. Arduino's delay() spins on micros() with interrupts enabled, so the magnet ISR kept timestamping throughout. The cost was packet cadence, not speed accuracy. Separately, getSpeed() read two 32-bit ISR variables without disabling interrupts. On an 8-bit part that is four byte loads each, so an interrupt landing mid-read yields half the old value and half the new one, and a torn deltaTime looks like a plausible speed rather than an obvious fault. Now snapshotted with interrupts off, with an explicit guard on a zero delta. Fully non-blocking temperature reads need raw OneWire: this library exposes no way to start a conversion and collect it later, since getTempC, getTempF and doConversion all wait internally and readScratchpad is private. Worth doing, but not blind. --- carsensordriver.ino | 187 +++++++++++++++++++++++++++++++++----------- 1 file changed, 142 insertions(+), 45 deletions(-) diff --git a/carsensordriver.ino b/carsensordriver.ino index d4c7e15..301a202 100644 --- a/carsensordriver.ino +++ b/carsensordriver.ino @@ -1,6 +1,7 @@ #include #include #include +#include /* NAN for a sensor that has never answered */ /* * The packet layout, the checksum, and the frame encoder live in the @@ -38,6 +39,24 @@ static uint16_t sequence = 0; #define circumference (2 * wheelRadius * PI) // in in #define pulseDist (circumference / numMagnets) +/* + * One magnet on the wheel means one sample per revolution, so the sensor + * updates at about 10 Hz flat out and much slower everywhere else: + * + * 10 Hz -> 35.7 mph one sample every 100 ms + * 2 Hz -> 7.1 mph one sample every 500 ms + * 0.5 Hz -> 1.8 mph one sample every 2000 ms + * + * Two consequences. Speed is a whole-revolution average, not a 50 ms one, so + * it cannot resolve a fast transient. And the loop sends at 20 Hz, so at best + * every other packet repeats the previous speed. + * + * That is also why nothing in loop() may block for longer than one pulse + * interval: a missed edge doubles the apparent pulse period and halves the + * reported speed. + */ +#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 +73,59 @@ 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}; +/* + * Temperature polling: one sensor per cycle, at most every 100 ms. + * + * DS18B20::getTempF() is expensive in a way the call site does not show. It + * issues CONVERT_T and then calls delayForConversion(), a blocking delay of + * 750 ms at the library's default 12-bit resolution. Reading both sensors + * inline every loop meant up to 1500 ms inside an iteration meant to turn + * over in 50 ms, which is roughly 30 packets not sent. + * + * It does NOT lose wheel interrupts: Arduino's delay() spins on micros() with + * interrupts enabled, so the magnet ISR keeps timestamping throughout. The + * damage is to packet cadence, not to speed accuracy. + * + * Two changes bring the cost down by about 16x: + * + * - 9-bit resolution. The conversion drops from 750 ms to 94 ms, and + * 0.5 C is far finer than anything useful about coolant temperature. + * - One sensor per cycle, no more than every 100 ms, which is also the + * fastest the part can usefully be read. + * + * select() is not free either. Bus reset, ROM match, nine scratchpad bytes + * and a power-mode query, with OneWire holding interrupts off around each bit + * it times. Calling it twice per loop put all of that on the hot path; now it + * happens once per 100 ms. + * + * Going fully non-blocking needs raw OneWire rather than this library, which + * exposes no way to start a conversion and collect it later: getTempC(), + * getTempF() and doConversion() all wait internally, and readScratchpad() is + * private. That is a worthwhile follow-up, not a change to make blind. + */ +#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 +}; + +/* + * Last good reading per sensor. A sensor that has never answered reports NAN + * rather than a stale or invented number. + * + * This matters: the previous code fell off the end of updateEngineTemp() and + * updateRadiatorTemp() without returning a value on the cache-hit path, which + * is undefined behaviour, and that path ran 50 times out of every 51. The + * caller got whatever happened to be in the return register. A radiator + * temperature frozen at exactly 48.4 in every CSV on the car is consistent + * with a real early reading left sitting there and never updated again. + */ +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 +169,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 +226,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() - lastMagnet >= SPEED_STALE_MS) { + return 0.0f; /* stopped, or slower than about 0.94 mph */ + } - 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 (delta == 0) { + return 0.0f; /* guard the divide; debounce should prevent this */ } - return 0.0; + /* + * 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 (ds.select((uint8_t *)tempAddr[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); } + From 529625c0013ad297a8c080fea4f070f7d9727049 Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 26 Sep 2026 10:01:54 -0400 Subject: [PATCH 2/4] ci: install SensorHub with a plain clone `lib install --git-url` is disabled unless library.enable_unsafe_install is set, so that path always failed and the fallback clone was doing the work. --- .github/workflows/ci.yml | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7a11ff5..759f64e 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" From 3f4e683e0163986eed55d67d5142b271c5ccf417 Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 26 Sep 2026 10:37:21 -0400 Subject: [PATCH 3/4] test: run the firmware in a simulator and require its output to decode Compiling the firmware and the Pi's library separately proves they both build. It proves nothing about whether they agree on the wire, which is the failure that actually costs a race weekend. This runs the real compiled firmware on a simulated ATmega328p, captures the bytes it puts on the UART, and feeds them through SensorHub's real parser. Three seconds of simulated car is about 59 packets and takes two seconds of wall clock. Every frame must verify, the format byte must be understood, and the sequence numbers must have no gaps. It reads the UART off the peripheral's output interrupt rather than simavr's console, which is a pretty-printer: it substitutes '.' for non-printable bytes, so the format byte 0x01 arrives as 0x2E and nothing parses at all. The last step corrupts every format byte and requires the check to fail, because a harness that cannot fail turns a guess into a green tick. Running it against this branch already shows the temperature fix working: both probes report NAN, where the old undefined-behaviour path returned whatever was in the register and looked like a plausible reading. --- .github/workflows/ci.yml | 73 +++++++++++++++++++++++++++ test/check_frames.c | 105 +++++++++++++++++++++++++++++++++++++++ test/run_firmware.c | 102 +++++++++++++++++++++++++++++++++++++ 3 files changed, 280 insertions(+) create mode 100644 test/check_frames.c create mode 100644 test/run_firmware.c diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 759f64e..814ca9c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -78,3 +78,76 @@ 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 three seconds of simulated car + # 3 s at 20 Hz is about 59 packets, and takes ~2 s of wall clock + run: /tmp/run_firmware /tmp/fw/*.elf /tmp/uart.bin 3000000 + + - name: Every frame must decode, with no gaps + run: /tmp/check_frames /tmp/uart.bin 50 + + - 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/test/check_frames.c b/test/check_frames.c new file mode 100644 index 0000000..eb241a2 --- /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 + +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); + + if (accepted < want) { + printf("FAIL: wanted at least %ld packets\n", want); + failures++; + } + if (parser.stats.checksum_errors != 0) { + printf("FAIL: the firmware emitted frames the parser could not verify\n"); + failures++; + } + if (parser.stats.format_errors != 0) { + printf("FAIL: format byte disagreement between firmware and library\n"); + failures++; + } + if (parser.stats.resyncs != 0) { + printf("FAIL: lost framing on a clean simulated link\n"); + failures++; + } + if (parser.stats.dropped != 0) { + printf("FAIL: gap in the sequence numbers\n"); + failures++; + } + + /* A clean run of N packets must span exactly N sequence numbers. */ + if (accepted > 0 && (uint16_t)(last - first) != (uint16_t)(accepted - 1)) { + printf("FAIL: %ld packets but the sequence spans %u\n", accepted, + (unsigned)(uint16_t)(last - first + 1)); + 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..5cd6cbb --- /dev/null +++ b/test/run_firmware.c @@ -0,0 +1,102 @@ +/* + * 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); + avr->frequency = FREQ; + avr_load_firmware(avr, &fw); + + out = fopen(argv[2], "wb"); + if (out == NULL) { + perror(argv[2]); + return 2; + } + + 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); + + fprintf(stderr, "captured %ld bytes over %.3f s simulated\n", captured, + (double)avr->cycle / (double)FREQ); + + if (captured == 0) { + fprintf(stderr, "the firmware transmitted nothing\n"); + return 1; + } + + return 0; +} From ec7f1c5066fa697119fdfe1be5457f1df445c94f Mon Sep 17 00:00:00 2001 From: Kieran Klukas Date: Sat, 26 Sep 2026 11:07:33 -0400 Subject: [PATCH 4/4] refactor: cut the comments back, and stop the simulation test lying The comments in this file ran to 40% of it, with blocks of 30, 16 and 14 lines narrating debugging sessions that the commit messages already record. Trimmed to the facts that change a decision: the timing table for the wheel sensor, why the temperature read is structured the way it is, and why NAN. The const cast on the sensor address is now a named one-line function, so the next reader can see it is the library's signature at fault and not a mutation. check_frames grew six copies of "if (bad) { printf; failures++ }". It is a table now, so the seventh check is data rather than another branch. And the simulation harness was overstating what it measures. Capture stalls after about fifty frames whatever the budget, because the AVR's transmit ring fills and simavr never drains it; the firmware keeps running, wedged in HardwareSerial::write. The frame count is therefore a sample, not a rate. That mattered: the CI threshold was 50 against an observed 51, so it had one packet of margin and would have gone red for reasons having nothing to do with this repo. It is 25 now, and the harness says plainly that the count is not a rate. Comparing counts between two builds measures where the simulator stalls, which is a trap I walked into before catching it. --- .github/workflows/ci.yml | 9 ++-- carsensordriver.ino | 92 +++++++++------------------------------- test/check_frames.c | 50 +++++++++++----------- test/run_firmware.c | 22 ++++++++-- 4 files changed, 70 insertions(+), 103 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 814ca9c..5dd2088 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -130,12 +130,15 @@ jobs: sketch/carsensordriver/test/check_frames.c \ "$LIB/src/sensorhub/parser.c" -o /tmp/check_frames - - name: Run three seconds of simulated car - # 3 s at 20 Hz is about 59 packets, and takes ~2 s of wall clock + - 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 - run: /tmp/check_frames /tmp/uart.bin 50 + # 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 diff --git a/carsensordriver.ino b/carsensordriver.ino index 301a202..f3fc9dd 100644 --- a/carsensordriver.ino +++ b/carsensordriver.ino @@ -3,20 +3,9 @@ #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. */ @@ -39,22 +28,10 @@ static uint16_t sequence = 0; #define circumference (2 * wheelRadius * PI) // in in #define pulseDist (circumference / numMagnets) -/* - * One magnet on the wheel means one sample per revolution, so the sensor - * updates at about 10 Hz flat out and much slower everywhere else: - * - * 10 Hz -> 35.7 mph one sample every 100 ms - * 2 Hz -> 7.1 mph one sample every 500 ms - * 0.5 Hz -> 1.8 mph one sample every 2000 ms - * - * Two consequences. Speed is a whole-revolution average, not a 50 ms one, so - * it cannot resolve a fast transient. And the loop sends at 20 Hz, so at best - * every other packet repeats the previous speed. - * - * That is also why nothing in loop() may block for longer than one pulse - * interval: a missed edge doubles the apparent pulse period and halves the - * reported speed. - */ +/* 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 @@ -73,36 +50,10 @@ 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 }; -/* - * Temperature polling: one sensor per cycle, at most every 100 ms. - * - * DS18B20::getTempF() is expensive in a way the call site does not show. It - * issues CONVERT_T and then calls delayForConversion(), a blocking delay of - * 750 ms at the library's default 12-bit resolution. Reading both sensors - * inline every loop meant up to 1500 ms inside an iteration meant to turn - * over in 50 ms, which is roughly 30 packets not sent. - * - * It does NOT lose wheel interrupts: Arduino's delay() spins on micros() with - * interrupts enabled, so the magnet ISR keeps timestamping throughout. The - * damage is to packet cadence, not to speed accuracy. - * - * Two changes bring the cost down by about 16x: - * - * - 9-bit resolution. The conversion drops from 750 ms to 94 ms, and - * 0.5 C is far finer than anything useful about coolant temperature. - * - One sensor per cycle, no more than every 100 ms, which is also the - * fastest the part can usefully be read. - * - * select() is not free either. Bus reset, ROM match, nine scratchpad bytes - * and a power-mode query, with OneWire holding interrupts off around each bit - * it times. Calling it twice per loop put all of that on the hot path; now it - * happens once per 100 ms. - * - * Going fully non-blocking needs raw OneWire rather than this library, which - * exposes no way to start a conversion and collect it later: getTempC(), - * getTempF() and doConversion() all wait internally, and readScratchpad() is - * private. That is a worthwhile follow-up, not a change to make blind. - */ +/* 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 @@ -111,17 +62,14 @@ static const uint8_t *const tempAddr[TEMP_SENSOR_COUNT] = { engineTempAddr, radTempAddr }; -/* - * Last good reading per sensor. A sensor that has never answered reports NAN - * rather than a stale or invented number. - * - * This matters: the previous code fell off the end of updateEngineTemp() and - * updateRadiatorTemp() without returning a value on the cache-hit path, which - * is undefined behaviour, and that path ran 50 times out of every 51. The - * caller got whatever happened to be in the return register. A radiator - * temperature frozen at exactly 48.4 in every CSV on the car is consistent - * with a real early reading left sitting there and never updated again. - */ +/* 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; @@ -288,7 +236,7 @@ void serviceTemps() { } tempTimer = now; - if (ds.select((uint8_t *)tempAddr[tempIndex])) { + if (selectSensor(tempIndex)) { ds.setResolution(TEMP_RESOLUTION); tempValue[tempIndex] = ds.getTempF(); } else { diff --git a/test/check_frames.c b/test/check_frames.c index eb241a2..41afe29 100644 --- a/test/check_frames.c +++ b/test/check_frames.c @@ -17,6 +17,7 @@ #include #include +#include int main(int argc, char **argv) { @@ -69,32 +70,31 @@ int main(int argc, char **argv) printf("resyncs %llu\n", (unsigned long long)parser.stats.resyncs); printf("dropped %llu\n\n", (unsigned long long)parser.stats.dropped); - if (accepted < want) { - printf("FAIL: wanted at least %ld packets\n", want); - failures++; - } - if (parser.stats.checksum_errors != 0) { - printf("FAIL: the firmware emitted frames the parser could not verify\n"); - failures++; - } - if (parser.stats.format_errors != 0) { - printf("FAIL: format byte disagreement between firmware and library\n"); - failures++; - } - if (parser.stats.resyncs != 0) { - printf("FAIL: lost framing on a clean simulated link\n"); - failures++; - } - if (parser.stats.dropped != 0) { - printf("FAIL: gap in the sequence numbers\n"); - failures++; - } + { + /* 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" }, + }; - /* A clean run of N packets must span exactly N sequence numbers. */ - if (accepted > 0 && (uint16_t)(last - first) != (uint16_t)(accepted - 1)) { - printf("FAIL: %ld packets but the sequence spans %u\n", accepted, - (unsigned)(uint16_t)(last - first + 1)); - failures++; + 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) { diff --git a/test/run_firmware.c b/test/run_firmware.c index 5cd6cbb..2818a26 100644 --- a/test/run_firmware.c +++ b/test/run_firmware.c @@ -62,8 +62,9 @@ int main(int argc, char **argv) } avr_init(avr); - avr->frequency = FREQ; + fw.frequency = FREQ; avr_load_firmware(avr, &fw); + avr->frequency = FREQ; out = fopen(argv[2], "wb"); if (out == NULL) { @@ -71,6 +72,15 @@ int main(int argc, char **argv) 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"); @@ -90,8 +100,14 @@ int main(int argc, char **argv) fclose(out); - fprintf(stderr, "captured %ld bytes over %.3f s simulated\n", captured, - (double)avr->cycle / (double)FREQ); + /* 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");