Stop the temperature read stalling the packet stream, and make the speed read atomic - #7
Merged
Merged
Conversation
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.
`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.
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.
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.
PonderForge
approved these changes
Sep 27, 2026
PonderForge
left a comment
PonderForge
left a comment
Member
There was a problem hiding this comment.
Looking good, though I'm still not sure if it's best to calc speeds of the magnets during the loop or if it'd be better to do it each time the magnet is registered
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The temperatures were undefined behaviour
hence gcc warnings
getTempF()blocks for 750 mslooking at the lib
getTempF()issuesCONVERT_Tand then callsdelayForConversion(), which is a blockingdelay(750)at the default 12-bit resolution. Both sensors were read inline which would break the loop.A few changes made this a bit faster:
getSpeed()had a torn-read raceIt read two
volatile unsigned longs 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. Now snapshotted with interrupts off, plus an explicit guard on a zero delta.The magic constant for the speed conversions is also properly defined now
Below about 0.94 mph the code reports a hard zero instead of allowing an eventual divide by zero.
Possible future fancy things
A fully non-blocking temperature read needs raw OneWire. This library exposes no way to start a conversion and collect it later:
getTempC,getTempFanddoConversionall wait internally, andreadScratchpadis private. It might be possible but would be a bit tricky.