[4.2.0] general quality of life fixes#42
Draft
TheAngryRaven wants to merge 4 commits into
Draft
Conversation
…n window, input validation, buffer wraparound Addresses the C1/C2/C3/H1/H2 findings from the professional code review: - C1: zero-initialize all crossing-line members, gate checkStartFinish() on a new startFinishLineConfigured flag (exposed via isStartFinishLineConfigured()), and reject degenerate/non-finite lines in all three line setters. loop() before setStartFinishLine() was reading indeterminate doubles (UB, phantom crossings). - C2: normalize every duration across the UTC midnight wrap via a shared timeSinceMidnightDelta() helper (DovesLapTimer + WaypointLapTimer). The crossing interpolator now computes its fix-pair delta in double, wrap-normalized and clamped to CROSSING_MAX_FIX_GAP_MS (10s), before converting back to unsigned long - the old path underflowed unsigned subtraction and pushed an out-of-range double through double->unsigned conversion (UB). Crossing validity is now a bool return instead of the time != 0 sentinel, so a legitimate crossing at exactly 00:00:00.000 is no longer discarded. - C3: CourseDetector re-anchors the lap-distance window after every completed proximity pass, match or no match. Previously a no-match lap left distanceSinceWaypoint accumulating, falsely matching a 2x-length layout on the next pass (same root cause as the v4.1.0 rejection- cooldown fix, CLAUDE.md issue 13). - H1: validate GPS input in DovesLapTimer::loop(), WaypointLapTimer::loop() and CourseDetector::update(): reject NaN/Inf/out-of-range/(0,0) fixes, sanitize non-finite alt/speed, drop single-fix teleports >500m, and re-accept sustained relocations after 3 consecutive far fixes without crediting the gap to the odometer. A single NaN or (0,0) fix previously poisoned odometers for the whole session and hung course detection. - H2: unwind the crossing ring buffer chronologically before scanning for the straddling pair. Once the ring wrapped (kart parked in the zone - standing start, red flag), the seam pair (newest entry adjacent to oldest) masqueraded as the crossing, fabricating or invalidating laps. Adds 23 new host unit tests in three new suites (test_midnight_rollover, test_input_validation, test_buffer_wraparound) plus two CourseDetector re-anchor regression tests; the C3 and H2 tests were verified to fail against the pre-fix code. All 89 tests green, NMEA replay goldens unchanged. https://claude.ai/code/session_01XNmVH6ySJ6Dz2QKDjB69mE
…4j17 Fix code-review findings C1–C3, H1, H2: line init, midnight rollover, detection window, input validation, buffer wraparound
GCC Code Coverage Report📂 Overall coverage
📄 File coverage
|
…ection fallbacks, dead memory, doc lies, CI hardening
H3 - #warning on 4-byte-double targets (classic AVR) + README/CLAUDE.md
caveats: lap counting works, odometer/interpolation accuracy degraded.
H4 - Crossing validation no longer conflates zone size with GPS sample
density: the straddle-pair sum bound scales with the pair's own
spacing (CROSSING_PAIR_SPACING_FACTOR), the zone-exiting fix is
buffered so low-rate passes have a straddle pair at all, and an
on-segment geometric check backstops the looser bound. Rejected
crossings surface via getRejectedCrossingCount() instead of dying
silently on debug serial. At 1 Hz the lap counter now works.
H5 - Lap Anything is reachable from every failure mode: CourseDetector
counts completed no-match passes (getNoMatchCount()), CourseManager
falls back after COURSE_DETECT_MAX_NO_MATCH_PASSES of them, and an
odometer failsafe (4 x longest course, 2000m floor) catches drivers
who never re-enter waypoint proximity.
H6 - Memory docs corrected: pruning saves CPU, frees zero bytes; the
~29 KB 8-slot timer array is static regardless of course count;
CourseManager does not fit on AVR Mega. (Real deallocation via a
placement-new pool deferred - see PR.)
H7 - _activateLapAnything() deactivates all course timers; new
isCourseTimerActive(index) getter.
H8 - Deleted WaypointLapTimer's write-only 50-entry proximity buffer
(1.6 KB/instance), ProximityBufferEntry, WAYPOINT_LAP_BUFFER_SIZE.
Closest-approach scalars carry the lap split; instance now ~150 B.
Also replaced the closestTime==0 validity sentinel (midnight bug
class) with the INFINITY distance sentinel.
H9 - README direction-detection section rewritten to match the actual
both-sectors temporal-order algorithm; getPaceDifference documented
as ms-per-meter everywhere; pointLineSegmentDistance documented as
meters; doc-claim spot check added to the release checklist.
H10 - test/Makefile lists every header (src, mocks, replay runner, NMEA
fixtures) as prerequisites - 'touch header && make' rebuilds now.
H11 - New suites: test_course_manager.cpp (accept/reject/fallbacks/prune/
deactivation), test_waypoint_lap_timer.cpp (state machine + lap
accounting), test_low_rate_gps.cpp (1Hz/5Hz + rejection counter).
Line coverage 51% -> 84.5%.
H12 - All workflow actions pinned to verified commit SHAs (resolved via
git ls-remote), peaceiris/actions-gh-pages bumped v3 -> v4,
dependabot.yml added (github-actions, weekly, grouped), gh-pages/
badge deploys gated on github.ref == refs/heads/master.
H13 - COVERAGE_GATE raised 1% -> 80% (measured 84.5%).
H14 - Shared constexpr unit-conversion constants in GeoMath.h
(GEOMATH_KNOTS_TO_KMH / KMH_TO_MPH / MPH_TO_KMH / METERS_TO_FEET,
derived from exact mile definitions); all five scattered literals
replaced; METERS_TO_FEET aliases the shared constant.
H4 and H5 regression tests verified failing against the pre-fix code.
131 host tests green; NMEA replay goldens unchanged.
https://claude.ai/code/session_01XNmVH6ySJ6Dz2QKDjB69mE
…-h14 Fix code-review findings H3–H14: low-rate GPS, detection fallbacks, dead memory, doc honesty, CI hardening
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.
Rolls up the full remediation of the professional code review (scored 4.5/10) — all 17 findings addressed across two PRs: #41 (critical: C1–C3, H1, H2) and #43 (high: H3–H14). Host test suite grew 65 → 131 tests, line coverage ~51% → 84.5% (CI gate raised 1% → 80%), and the pinned NMEA replay goldens are unchanged throughout — no timing drift on real-track data.
Correctness fixes (#41)
loop()skips start/finish detection until a valid line is set (isStartFinishLineConfigured()); setters reject degenerate (A==B, e.g. the example's0.00placeholders) or non-finite lines. Callingloop()before setup was UB with phantom-crossing potential.timeSinceMidnightDelta()(wrap-normalized); the crossing interpolator computes its fix-pair delta indoublewith a 10 s coherence clamp (CROSSING_MAX_FIX_GAP_MS) before converting back — the old path underflowed unsigned subtraction (~4.29-billion-ms laps stuck as best lap) and hit double→unsigned conversion UB. Thetime != 0validity sentinel is gone; a crossing at exactly 00:00:00.000 counts.DovesLapTimer,WaypointLapTimer, andCourseDetector; non-finite alt/speed sanitized; single-fix teleports >500 m dropped with a 3-fix re-accept that never credits the gap to the odometer.Robustness & memory (#43)
getRejectedCrossingCount().COURSE_DETECT_MAX_NO_MATCH_PASSES(3) completed no-match laps (getNoMatchCount()), and an odometer failsafe (4 × longest course, 2000 m floor) for drivers who never re-enter waypoint proximity.isCourseTimerActive(index).WaypointLapTimer's 1.6 KB write-only proximity buffer (ProximityBufferEntry,WAYPOINT_LAP_BUFFER_SIZEremoved); instance ~1.8 KB → ~150 B.constexprconstants inGeoMath.h(knots/kmh/mph/feet), replacing three mutually inconsistent literals across five files.Honesty / documentation
doubleis 32-bit: compile-time#warningon 4-byte-double targets; README/CLAUDE.md state plainly that Mega/Uno count laps but run with degraded odometer/interpolation accuracy.CourseManageris ~29 KB statically allocated regardless of course count, pruning saves CPU and frees zero bytes, and it does not fit on AVR Mega. The "~24 KB drops to ~5 KB" claim is gone.getPaceDifference()documented as ms per meter;pointLineSegmentDistance()documented as meters; doc-claim spot check added to the release checklist.Tests & CI
test_course_manager.cpp,test_waypoint_lap_timer.cpp,test_low_rate_gps.cpp— plus Fix code-review findings C1–C3, H1, H2: line init, midnight rollover, detection window, input validation, buffer wraparound #41'stest_midnight_rollover.cpp,test_input_validation.cpp,test_buffer_wraparound.cppand CourseDetector re-anchor regressions. Key regression tests verified failing against pre-fix code.touch header && makerebuilds — no more false greens on stale binaries).peaceiris/actions-gh-pagesv3 → v4, Dependabot (github-actions, weekly) added, gh-pages/badge deploys gated torefs/heads/master.New public API (backward compatible)
isStartFinishLineConfigured(),getRejectedCrossingCount()(DovesLapTimer) ·getNoMatchCount()(CourseDetector) ·isCourseTimerActive(index)(CourseManager). Removed:ProximityBufferEntry,WAYPOINT_LAP_BUFFER_SIZE(write-only internals).Known deferred items
CourseManagerallocates onlycourseCounttimers (currently honestly documented as 8-slot static).Expected in CI on this PR: the Mega/Uno compile cells now print the intentional H3
#warning; first master merge exercises the v4 gh-pages deploy for docs + coverage badge.https://claude.ai/code/session_01XNmVH6ySJ6Dz2QKDjB69mE