plan 0003: true RPM from spark mode and cylinder count - #133
Merged
Conversation
The pickup counts ignition sparks and the tach treated one spark as one revolution. That is only true for a single cylinder firing every rev — a 2-stroke, or a 4-stroke with wasted spark — which is the common kart case and why it has been fine. Anything else is out by a fixed factor: a twin firing every rev reads DOUBLE the real speed. Two settings fix it. pulses_per_rev = cylinder_count x (wasted ? 1.0 : 0.5), and the reciprocal is applied at the period->RPM conversion in TACH_LOOP — BEFORE the Kalman filter, because the filter's tuning is in true-RPM units (Q = 800 RPM^2 models crank inertia), so correcting afterwards would filter each engine type differently. The correction point already existed and was already in the right place; it was just hardcoded to 1.0. Defaults (1 cylinder, wasted) give exactly 1.0, so a device that has never been configured reads identically to before. Anything other than an explicit "single" degrades to wasted, so a blank, garbled or future value reads as today rather than doubling every RPM. THE DEBOUNCE HAD TO FOLLOW. A fixed 3 ms gap caps ~20,000 pulses/min, which on a twin firing every rev is only ~10,000 real RPM — the debounce would have become the ceiling. minPulseGapUs() derives it as 3 ms / pulses-per-rev. The floor is 750 us, not the 1.5 ms the plan sketched: 1.5 ms still left a triple at ~13,300 true RPM, under the old ceiling. 750 us holds the full ~20,000 through four cylinders. ISR headroom was never the constraint (<1 us body, ~1300 int/s worst case) — ringing was, and the margin holds from both ends: the input is RC-filtered ~100 us and the documented pickup circuits emit pulses MILLISECONDS wide, TACHOMETER/README.md recording circuit 1's 5 ms pulse as itself the ~9800 RPM limit on that hardware. The audit plan 0003 asked for came back clean: nothing else derives RPM from pulse periods. The only other 60e6 in the tree is the simulator's pulse generator, which is the inverse and matches the default. Knock-on: the RPM thresholds the device acts on — auto-race entry, camera wake/record/stop — now mean what they say on every engine. Verified end to end in the simulator, not just at the unit level: 6000 pulses/min reports 6000 RPM at the defaults and 3000 with revsPerPulse forced to a twin. Golden fixtures and the lap oracle are unchanged, which is the evidence existing devices are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ESkRRtF4vRrANPL6huSgmD
Coverage — host-testable units📂 Overall coverage
📄 File coverage
|
clang-tidy caught bugprone-incorrect-roundings on (uint32_t)(gap + 0.5f) — a real finding, not a false positive: that idiom rounds incorrectly for negatives and is a known bug class. lroundf would have silenced it, but the float was never needed. Pulses-per-rev is either `cylinders` (wasted spark) or `cylinders / 2` (single-fire), so the base gap divides exactly in both cases once the single-fire case is written as a doubled numerator. Integer throughout keeps float rounding out of a value the ISR compares against on every pulse, and drops a libm call from the firmware. Same numbers as before, so the tests are unchanged and still pass: 3000 / 1500 / 1000 / 750 for one through four cylinders wasted, 6000 for a single-fire single, floored at 750. Verified with the exact CI invocation locally — clang-tidy is clean across all eleven analyzed units, not just this one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01ESkRRtF4vRrANPL6huSgmD
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.
Summary
The pickup counts ignition sparks; the tach reported them as revolutions. That's only true for a single cylinder firing every rev — a 2-stroke, or a 4-stroke with wasted spark — which is the common kart case and why it's been fine. Anything else is out by a fixed factor: a twin firing every rev reads double the real speed.
pulses_per_rev = cylinder_count × (wasted ? 1.0 : 0.5), and the reciprocal is applied at the period→RPM conversion inTACH_LOOP()— before the Kalman filter, because the filter's tuning is in true-RPM units (Q = 800 RPM²models crank inertia), so correcting afterwards would filter each engine type differently. The correction point already existed and was already in the right place; it was just hardcoded to1.0.Defaults reproduce today's behaviour exactly (1 cylinder, wasted → 1.0), so a device you never configure reads identically. Anything other than an explicit
"single"degrades to wasted, so a blank, garbled or future value reads as today rather than doubling every RPM.The debounce had to follow — and the plan's floor was wrong
A fixed 3 ms gap caps ~20,000 pulses/min, which on a twin firing every rev is only ~10,000 real RPM — the debounce would have become the ceiling.
minPulseGapUs()derives it as 3 ms ÷ pulses-per-rev.Plan 0003 sketched a 1.5 ms floor. I used 750 µs, because 1.5 ms still left a triple at ~13,300 true RPM — under the old ceiling, which defeats the point. 750 µs holds the full ~20,000 through four cylinders; past that the floor binds (10,000 at eight), well clear of anything this logger targets.
The plan said "verify ISR headroom before lowering" — headroom was never the constraint (<1 µs body, ~1300 int/s worst case). Ringing was, and the margin holds from both ends: the input is RC-filtered (~100 µs), and your own
TACHOMETER/README.mdrecords circuit 1 emitting 5 ms-wide pulses, which is itself the ~9800 RPM limit on that hardware — i.e. the pulse width, not the debounce, is what binds there.The audit came back clean
Plan 0003 asked me to check nothing else derives RPM from pulse periods. Nothing does — the only other
60e6in the tree is the simulator's pulse generator, which is the inverse and matches the default.Knock-on benefit: auto-race entry (>500) and the camera wake/record/stop thresholds (500/1500/300) now mean what they say on every engine instead of firing at half the real RPM on a twin.
Type of Change
Checklist
CLAUDE.md(settings table, key constants, tachometer subsystem, pure-unit map),README.md,CHANGELOG.md, plan 0003 marked SHIPPEDtach_filterpure unit, per the repo conventionString, noanalogRead(), TIMER3 untouchedNotes for Reviewers
Verified end to end in the simulator, not just at the unit level. I built a probe against the sim core that feeds 6000 pulses/min through the real ISR and
TACH_LOOP(): it reports 6000 RPM at the defaults and 3000 withrevsPerPulseforced to a twin. So the wiring is proven, not only the math. Golden fixtures (16 pages) and the lap oracle are unchanged — that's the evidence existing devices are unaffected.An earlier probe attempt was accidentally informative too: hardcoding
cylinders = 2before thegetSettingcall changed nothing, becausecylinder_count=1fromSETTINGS.jsoncorrectly overrode it. The settings path works.Two things worth your eye, since you know the hardware:
cylinder_countis "cylinders the pickup SEES", which I've documented in three places because getting it wrong halves or doubles every reading. A clamp on one plug wire of a twin sees one. Worth confirming that's how you'd phrase it to users.Pairs with DovesDataViewer #389, which adds the
enumcontrol sospark_modeis a real dropdown rather than a free-text box. Neither PR depends on the other — the app builds its rows from what the device reports, so the new settings simply appear once this ships.Generated by Claude Code