diff --git a/.gitignore b/.gitignore index c868732..08fd79b 100644 --- a/.gitignore +++ b/.gitignore @@ -25,3 +25,8 @@ Thumbs.db # Core dumps, which a failing host test binary drops in the repository root. core.* + +# Blog drafts. Written here because the device is the subject and the facts have to come +# from the repository, but they are Mark's words for caterham7diaries.com and are not part +# of this project's history. +blog/ diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ac78fa..d88149e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -264,6 +264,19 @@ The project follows Semantic Versioning once the first firmware release is tagge ### Fixed +- recorded G peaks were bounded by what the radar can draw rather than by what the car did. + The display clamps a reading to 2 g so the dot stays on the dial, and that clamp was + applied before the peaks were taken - so a session summary recorded the limit as though + it were a measurement. A hand-shaken session filed `2577 milli-g`, which is two axes + pinned at the clamp rather than anything that happened. + It bites unevenly, which is why it survived: a car on road tyres reaches about 1.5 g + sideways and never approaches the limit, but a kerb strike passes 2 g vertically without + trying, and was being filed as exactly 2.00 with nothing to say it had been truncated. + The measurement now feeds the peaks and the display clamps its own copy. A credibility + bound remains at 10 g, because past that is a sensor fault rather than a lap and the + summary validator refuses the record anyway - but a peak that reaches it is now flagged + as truncated, since a bound nobody is told about is the defect itself rather than the fix + - the carousel stopped accepting taps, which made every menu, mode, track and setting unselectable. `attach_gesture_input` keeps its bindings in a fixed table and refuses silently once it is full; Speed Only added a ninth screen to a table of eight, and the diff --git a/firmware/components/ui/imu_meter.cpp b/firmware/components/ui/imu_meter.cpp index ef3d264..cb257bd 100644 --- a/firmware/components/ui/imu_meter.cpp +++ b/firmware/components/ui/imu_meter.cpp @@ -12,6 +12,13 @@ float clamp_g(const float value) noexcept return std::max(-kImuDisplayLimitG, std::min(kImuDisplayLimitG, value)); } +// The same shape, a different bound, and a different reason. This one exists to keep a +// sensor fault out of the session record; the display's exists to keep a dot on screen. +float clamp_recorded_g(const float value) noexcept +{ + return std::max(-kImuRecordedLimitG, std::min(kImuRecordedLimitG, value)); +} + PlanarAcceleration rotate_sample(const ImuMeterInput& input) noexcept { const auto x = input.sample.acceleration_x_mps2 / kStandardGravityMps2; @@ -33,8 +40,12 @@ PlanarAcceleration rotate_sample(const ImuMeterInput& input) noexcept break; } - point.lateral_g = point.lateral_valid ? clamp_g(point.lateral_g) : 0.0F; - point.longitudinal_g = point.longitudinal_valid ? clamp_g(point.longitudinal_g) : 0.0F; + // Deliberately unclamped. This is the measurement, and it feeds both the radar and the + // recorded peaks - clamping here is what made a session's peak bounded by what the + // dial could draw rather than by what the car did (#162). The display clamps its own + // copy below, where the limit belongs. + point.lateral_g = point.lateral_valid ? point.lateral_g : 0.0F; + point.longitudinal_g = point.longitudinal_valid ? point.longitudinal_g : 0.0F; return point; } @@ -73,17 +84,24 @@ const ImuMeterSnapshot& ImuMeterController::update(const ImuMeterInput& input, return snapshot_; } - const auto point = rotate_sample(input); - snapshot_.current = point; - append(point); + const auto measured = rotate_sample(input); // Vertical does not rotate with the display: up is up however the unit is mounted. snapshot_.vertical_valid = input.sample_available && input.z_axis_valid; - snapshot_.vertical_g = - snapshot_.vertical_valid - ? clamp_g(input.sample.acceleration_z_mps2 / kStandardGravityMps2) - : 0.0F; + const auto measured_vertical_g = + snapshot_.vertical_valid ? input.sample.acceleration_z_mps2 / kStandardGravityMps2 + : 0.0F; - update_peaks(point); + // Peaks first, from the measurement, before anything is held back for the dial. + update_peaks(measured, measured_vertical_g); + + // Now the drawn copy. The radar and the trail take the clamped values, because a dot + // beyond the outer ring is not a reading the display can place. + auto point = measured; + point.lateral_g = point.lateral_valid ? clamp_g(point.lateral_g) : 0.0F; + point.longitudinal_g = point.longitudinal_valid ? clamp_g(point.longitudinal_g) : 0.0F; + snapshot_.current = point; + append(point); + snapshot_.vertical_g = snapshot_.vertical_valid ? clamp_g(measured_vertical_g) : 0.0F; increment_saturated(snapshot_.accepted_samples); if (!point.lateral_valid || !point.longitudinal_valid) { @@ -154,28 +172,45 @@ void ImuMeterController::append(const PlanarAcceleration point) noexcept } } -void ImuMeterController::update_peaks(const PlanarAcceleration& point) noexcept +void ImuMeterController::update_peaks(const PlanarAcceleration& point, + const float vertical_g) noexcept { + // Anything past the recorded bound is a sensor fault, not a lap. Recording it would + // put the fault in the session summary looking like a measurement, and the summary + // validator refuses the record outright beyond 10 g - so it is bounded here and the + // fact that it was bounded is kept alongside. + const auto record = [this](const float value) { + if (std::fabs(value) > kImuRecordedLimitG) { + snapshot_.peaks.truncated = true; + } + return clamp_recorded_g(value); + }; + if (snapshot_.vertical_valid) { - snapshot_.peaks.up_g = std::max(snapshot_.peaks.up_g, snapshot_.vertical_g); - snapshot_.peaks.down_g = std::max(snapshot_.peaks.down_g, -snapshot_.vertical_g); + const auto vertical = record(vertical_g); + snapshot_.peaks.up_g = std::max(snapshot_.peaks.up_g, vertical); + snapshot_.peaks.down_g = std::max(snapshot_.peaks.down_g, -vertical); } if (point.longitudinal_valid) { + const auto longitudinal = record(point.longitudinal_g); snapshot_.peaks.acceleration_g = - std::max(snapshot_.peaks.acceleration_g, point.longitudinal_g); - snapshot_.peaks.braking_g = - std::max(snapshot_.peaks.braking_g, -point.longitudinal_g); + std::max(snapshot_.peaks.acceleration_g, longitudinal); + snapshot_.peaks.braking_g = std::max(snapshot_.peaks.braking_g, -longitudinal); } if (point.lateral_valid) { - snapshot_.peaks.left_g = std::max(snapshot_.peaks.left_g, -point.lateral_g); - snapshot_.peaks.right_g = std::max(snapshot_.peaks.right_g, point.lateral_g); + const auto lateral = record(point.lateral_g); + snapshot_.peaks.left_g = std::max(snapshot_.peaks.left_g, -lateral); + snapshot_.peaks.right_g = std::max(snapshot_.peaks.right_g, lateral); } if (point.lateral_valid && point.longitudinal_valid) { - const auto total = std::sqrt(point.lateral_g * point.lateral_g + - point.longitudinal_g * point.longitudinal_g); + const auto lateral = clamp_recorded_g(point.lateral_g); + const auto longitudinal = clamp_recorded_g(point.longitudinal_g); + const auto total = std::sqrt(lateral * lateral + longitudinal * longitudinal); if (total > snapshot_.peaks.total_g) { snapshot_.peaks.total_g = total; - snapshot_.peaks.total_position = point; + // The position this peak happened at, recorded as measured. It is a record of + // where the car was, not a point the radar has to be able to plot. + snapshot_.peaks.total_position = {lateral, longitudinal, true, true}; } } } diff --git a/firmware/components/ui/include/track_timer/ui/imu_meter.hpp b/firmware/components/ui/include/track_timer/ui/imu_meter.hpp index 96176c4..7f48b98 100644 --- a/firmware/components/ui/include/track_timer/ui/imu_meter.hpp +++ b/firmware/components/ui/include/track_timer/ui/imu_meter.hpp @@ -15,7 +15,17 @@ namespace track_timer::ui { inline constexpr std::uint32_t kImuMeterSchemaVersion = 2; inline constexpr std::size_t kImuTrailCapacity = 24; inline constexpr float kStandardGravityMps2 = 9.80665F; +// What the radar can draw. A dot has to stay on the display, so the drawn value is held +// inside this - but a recorded peak is a different quantity with a different purpose, and +// must not inherit a limit that exists for the dial. inline constexpr float kImuDisplayLimitG = 2.0F; + +// What a recorded peak is allowed to be. Deliberately the same figure as the summary +// validator's kMaximumCrediblePeakG - beyond it is a sensor fault rather than a heroic +// lap, and a peak past what the validator accepts would discard the entire session +// record. The two are declared in different components, so screen_router static_asserts +// the agreement where they meet rather than leaving it to coincidence. +inline constexpr float kImuRecordedLimitG = 10.0F; inline constexpr std::uint64_t kImuRecoveredNoticeMs = 1'500; enum class ImuMeterState : std::uint8_t { @@ -56,6 +66,13 @@ struct ImuPeakSummary { // up and a compression loads it down, and they are different events to a driver. float up_g{0.0F}; float down_g{0.0F}; + // True when a reading was past kImuRecordedLimitG and the peak above is therefore the + // bound rather than the measurement. + // + // There is still a bound, so there is still a number that could be a clamp presented + // as a measurement - which is the whole defect. Saying so is what makes the remaining + // limit honest instead of simply moving the lie further out. + bool truncated{false}; }; struct ImuMeterSnapshot { @@ -86,7 +103,7 @@ class ImuMeterController { private: void clear_measurements() noexcept; void append(PlanarAcceleration point) noexcept; - void update_peaks(const PlanarAcceleration& point) noexcept; + void update_peaks(const PlanarAcceleration& point, float vertical_g) noexcept; ImuMeterSnapshot snapshot_{}; bool loss_seen_{false}; diff --git a/firmware/main/screen_router.cpp b/firmware/main/screen_router.cpp index e398e5f..9bd7137 100644 --- a/firmware/main/screen_router.cpp +++ b/firmware/main/screen_router.cpp @@ -2081,6 +2081,12 @@ class ScreenRouter { summary.peaks.up_g = peaks.up_g; summary.peaks.down_g = peaks.down_g; summary.peaks.total_g = peaks.total_g; + // The meter bounds a recorded peak at the same figure the summary validator calls + // credible. They are declared in different components, so the agreement is checked + // here, where the two actually meet: a meter bound above the validator's would let + // a peak through that silently discards the whole record. + static_assert(ui::kImuRecordedLimitG <= logger::kMaximumCrediblePeakG, + "a recorded peak must not exceed what the summary will accept"); // The card is not assumed to be there. A failed write costs the durability of this // record and nothing else: the session stays in memory and Review shows it either diff --git a/simulator/CMakeLists.txt b/simulator/CMakeLists.txt index 975e943..951f5ab 100644 --- a/simulator/CMakeLists.txt +++ b/simulator/CMakeLists.txt @@ -283,7 +283,7 @@ if(BUILD_TESTING) set_tests_properties( simulator-imu-meter-model PROPERTIES - PASS_REGULAR_EXPRESSION "Bounded G-meter peaks, orientation, reset guard, IMU states, and session-scoped vertical peaks passed" + PASS_REGULAR_EXPRESSION "orientation, reset guard, IMU states, and session-scoped vertical peaks passed" TIMEOUT 20 ) diff --git a/tests/cpp/test_imu_meter.cpp b/tests/cpp/test_imu_meter.cpp index 856bc64..756cb0d 100644 --- a/tests/cpp/test_imu_meter.cpp +++ b/tests/cpp/test_imu_meter.cpp @@ -166,7 +166,70 @@ int main() assert(near(scoped.snapshot().peaks.up_g, 0.5F)); assert(scoped.snapshot().reset_allowed); - std::cout << "Bounded G-meter peaks, orientation, reset guard, IMU states, and " + // #162. The radar clamps to 2 g so a dot stays on the dial, and that clamp used to be + // applied before the peaks were taken - so a session's recorded peak was bounded by + // what the display could draw rather than by what the car did. A hand-shaken session + // recorded 2577 milli-g, which is both horizontal axes pinned at the limit rather than + // a measurement. + { + ui::ImuMeterController recorder{}; + auto hard = sample(2.6F, 1.4F, 10); + auto recorded = recorder.update(hard, false); + + // Measured, not bounded by the dial. + assert(near(recorded.peaks.right_g, 2.6F)); + assert(near(recorded.peaks.acceleration_g, 1.4F)); + assert(!recorded.peaks.truncated); + + // The drawn value still is bounded, because the dot has to stay on the display. + assert(near(recorded.current.lateral_g, ui::kImuDisplayLimitG)); + assert(near(recorded.current.longitudinal_g, 1.4F)); + + // And the total comes from the measurement rather than from two clamped axes. + assert(near(recorded.peaks.total_g, std::sqrt(2.6F * 2.6F + 1.4F * 1.4F))); + assert(recorded.peaks.total_g > std::sqrt(2.0F * 2.0F + 1.4F * 1.4F)); + } + + // Vertical is the axis where this actually bites. A car on road tyres never reaches + // 2 g sideways, but a kerb strike passes it easily - and used to be filed as exactly + // 2.00 with nothing to say it had been truncated. + { + ui::ImuMeterController recorder{}; + auto kerb = vertical_sample(3.4F, 10); + auto recorded = recorder.update(kerb, false); + assert(near(recorded.peaks.up_g, 3.4F)); + assert(!recorded.peaks.truncated); + // The dial still shows what it can draw. + assert(near(recorded.vertical_g, ui::kImuDisplayLimitG)); + + auto compression = vertical_sample(-2.9F, 20); + recorded = recorder.update(compression, false); + assert(near(recorded.peaks.down_g, 2.9F)); + } + + // A bound remains, so there is still a number that could be a clamp wearing a + // measurement's clothes. Past 10 g is a sensor fault rather than a lap - the summary + // validator refuses the record beyond it - so it is bounded and says it was. + { + ui::ImuMeterController recorder{}; + auto fault = sample(14.0F, 0.2F, 10); + auto recorded = recorder.update(fault, false); + assert(near(recorded.peaks.right_g, ui::kImuRecordedLimitG)); + // The flag is the difference between a bound and a lie. + assert(recorded.peaks.truncated); + } + + // An ordinary session never touches either bound and is never marked. + { + ui::ImuMeterController recorder{}; + auto ordinary = sample(1.1F, 0.9F, 10); + const auto recorded = recorder.update(ordinary, false); + assert(near(recorded.peaks.right_g, 1.1F)); + assert(!recorded.peaks.truncated); + } + + std::cout << "G-meter peaks recorded as measured rather than bounded by the dial, " + "orientation, reset guard, IMU states, and " "session-scoped vertical peaks passed\n"; return 0; }