Drag mode review fixes (plans 0015/0016) - #166
Open
TheAngryRaven wants to merge 8 commits into
Open
TheAngryRaven wants to merge 8 commits into
TheAngryRaven wants to merge 8 commits into
Conversation
Review finding D1. Every drag_timer abort (fix gap, mid-run standstill, prove-out) edges on a fix passed to onFix(), and the glue only feeds it while gpsData.fix. A fix that dropped mid-pass and never came back left the run LAUNCHED forever; in manual mode the tree sat in kRunning (where the Select-hold exit is deliberately suppressed) on a pinned screen that ate every button — a wedge with no way out but the reboot combo. The root cause is that the physics has no notion of wall time, so the fix lives there: DragTimer::checkFixLoss(nowMs, lastFixMs). The glue records millis() whenever it feeds a fix and calls the watchdog every loop; a staged or launched timer with no fix for kFixLossAbortMs (= the in-stream kFixGapAbortMs) drops to ARMED and forgets its previous fix so a returning fix starts a fresh stream rather than resuming a stale run. The tree's existing "runActive fell without a run" rule then shows RUN ABORTED, which accepts the exit hold. Keying on the last fix FED also covers a receiver that stops streaming with its fix flag latched. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review finding D2. dragStagingLoop() fed the tree in.fix = gpsData.fix, and GPS_LOOP fed dragTimer->onFix() on a bare fix too, while the LED search pip and the staging screen's WAITING FOR GPS line both required fix && timeValid. So the tree could run its countdown while the screen said it was waiting — and, worse, before the lock the receiver reports a placeholder date, while drag timing is Unix epoch ms: the staged anchor, the green stamp and the ET start all sat on a clock that jumps when the lock lands (a backwards step silently aborts the run, a forwards one is a garbage reaction time). gpsFixAndTimeLocked() is now the single predicate for the physics feed (automatic mode included), the tree input, the staging screen and the LED search pip, so they cannot disagree again. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review finding D3. The drag stage latch, the drag run and the sprint run re-armed the auto-idle grace by rewriting raceSessionStartedAt, but checkAutoIdle() returned early during the grace without touching an idle timer that had already started. Creep below 5 mph long enough to start the timer, then re-stage: the stale timer kept its old start, and the session ended the moment the new grace expired rather than a full hold later — exactly the staging-queue case the re-arm exists for. The grace start and the idle timer now live together in idle_policy::Clock. rearmGrace() is the only way to restart the grace and always clears the timer; advance() holds the grace/reset/hold sequence the sketch used to inline, so the whole thing is host-tested (including the creep -> re-stage regression, camera yield and millis wrap). The sketch keeps only millis() and the end-of-session effects. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review finding D4. dragStagingLoop() stamped the green edge with getGpsUnixTimestampMillis(), which is the time of the LAST PVT — the green lands anywhere up to a nav period (40 ms at 25 Hz) after it. The run start it is subtracted from is interpolated to fix-time accuracy, so every reaction time read 0-40 ms high with sampling-phase jitter. onPVTReceived() now records millis() at arrival (gpsPvtArrivalMillis) and the green is stamped gps_time::epochNowMs(lastEpoch, arrival, millis()); the subtraction moved to drag_tree::reactionTimeMs(). Both are pure and host-tested, including millis wrap and the old bias. The receiver's own output latency remains (not observable without PPS) and is documented as a small constant residual. The sim golden drag_staging_results changes: the same scripted pass now reads RT 0.66 instead of 0.68. Regenerated from --print output (only that one line differs). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review finding D5. The manual tree's exit effect called endRaceSession() directly. endRaceSession() deliberately never touches the camera (a tach session's recording must outlive a stationary grid idle), so every ender that owns the camera notifies it first — and the LOGGING STOP confirm did, while the drag exit did not: a paired Insta360 kept recording after the user explicitly ended the session. Both user-initiated enders now go through endRaceSessionByUser() (CAMERA_NOTIFY_SESSION_END, then endRaceSession), so the pair cannot drift apart again. Pure glue with no decision to extract; covered by the sim build/walk (camera surface stubbed there). Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review finding D6. NEOPIXEL_LOOP()'s paceValid became true once one run existed and the next was live, and drag's pace accessor is hard-wired 0.0 (there is no reference to pace against). From run 2 on, an automatic pass therefore got the pace pip parked in its deadband — one dim centre pixel — instead of the RPM/speed scale run 1 had. Plan 0016 had patched this for manual drag only (!dragManualActive()). The gate is now led_modes::paceValid(PaceGate), pure and host-tested, with hasPaceReference = !dragModeIsActive() — covering both drag modes by construction rather than by a per-mode exception. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review finding D7. In automatic mode a car stopped in the shutdown area stages, and driving the return road back at 20-30 mph then clears the rollout, the 15 mph prove-out and the target distance — a slow bogus run in lapHistory and the DOVEX laps line. A completed run is now discarded (rejectedRuns() counts it) only when BOTH hold: its launch->finish chord points more than 90 deg from the last recorded run's (a strip runs one way, the return road the other), AND its trap speed is below kCruiseTrapRatio (1.2) x its average speed. The ratio is the physics of a pass — from a standstill at full effort trap/average is 2.0 at constant acceleration, 1.5 at constant power, and above ~1.3 even for a car that tops out early — while a drive that settles into a cruise sits near 1.0-1.15. Requiring both keeps it conservative: the first run of a session, every same-direction run and every full-effort run in either direction are kept. A trap floor or ET ceiling was rejected because karts and brisk return-road drives overlap on both. Rationale recorded in plan 0015. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Review finding D8. Staged, a car creeping at 1-2 mph (above the 1 mph staging threshold, below the 2 mph launch speed) could cover the whole 11.25 in rollout without launching; when it then passed 2 mph the launch edge found the rollout already behind it (dPrev >= kRolloutFt), the interpolation clamped f to 0, and the ET started at the previous fix with the crept ground credited to the run's distance — a short ET. A STAGED car moving above the staging threshold that passes the rollout without launch speed now drops to ARMED. If it stops, it re-stages at the new spot a second later and the next launch times exactly (the new test also shows the old code mistimed that case through the anchor mean's lag); if it rolls straight into a launch there was no standing start and nothing is recorded. The rule is gated on Doppler speed, so standstill position jitter past the rollout radius still cannot un-stage a parked car. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Coverage — host-testable units📂 Overall coverage
📄 File coverage
|
14 tasks
This branch has not been deployed
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.
Requested by Dove · project thread
Summary
Fixes every drag-mode finding from the #160 release review. There is one commit per finding, so each can be cherry-picked or reverted on its own. Each commit carries its own test, CHANGELOG line and plan notes (see the new "Review fixes (2026-09)" sections in plans 0015 and 0016).
Before: a GPS dropout mid-pass could trap manual mode on a live run with no way out. The tree could count down before the GPS time lock. A creeping queue could end the session. Reaction time read 20–40 ms high. The Select-hold exit left a camera recording. Automatic runs after the first showed a dead LED centre pixel. A return-road drive could be recorded as a run, and creeping through the rollout started the clock late.
After: every one of those is fixed.
DragTimer::checkFixLoss()aborts a staged or running timer after 2 s with no fix fed, in both modes. The tree then shows RUN ABORTED, which accepts the exit holdgpsFixAndTimeLocked()is now the single gate for the physics feed, the tree input, the WAITING FOR GPS screen and the LED search pipidle_policy::Clock:rearmGrace()is the only way to restart the grace and always clears the idle timer. Also fixes the same latent bug in sprintgps_time::epochNowMs= last PVT epoch + millis since it arrived; RT usesdrag_tree::reactionTimeMsendRaceSessionByUser()led_modes::paceValid(PaceGate)rejectedRuns()The one sim golden change is
drag_staging_results: the same scripted pass now shows RT 0.66 instead of 0.68 because of D4. That was the only line that differed in the dumped frame.Type of change
How it was verified
ctest --test-dir tests/build): 657 cases. The new D1, D3, D4, D6, D7 and D8 tests were checked to fail against the old code where that was possibleclang-tidyclean (CI)neopixel.inoisn't in the sim, so it was only re-read by eye)The native sim passes all 6 ctests plus the 60 s soak.
Checklist
CHANGELOG.mdupdated under[Unreleased](if user-visible)ARCHITECTURE.md/CLAUDE.mdupdated (if a module or interface changed)tests/Related issues
Findings D1–D8 from the #160 release review.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SdhRyfZ4eYuooXi9uy28aw
Generated by Claude Code