From 0430c0c947446c70b20cceec261e478180e03fac Mon Sep 17 00:00:00 2001 From: PurpleSentinel <199351535+PurpleSentinel@users.noreply.github.com> Date: Sat, 17 Oct 2026 14:48:49 +0100 Subject: [PATCH] fix(session): wire the GPS trigger, which armed and could never fire Choosing TRIGGER = GPS armed the device, showed WAITING FOR THE LINE, and nothing could start the session. Only two call sites reached begin_session: a manual START and IMU launch detection. The mechanism was already there. session/gate_automation.cpp starts on pit exit and stops on pit entry, gated on settings that session_trigger.cpp already derives from the trigger, and it is host-tested. The crossings were already arriving - drain_timing_output read them off the queue and threw them away in an empty while loop. The two halves had never been introduced to each other. Offered to the automation unconditionally rather than only while armed on GPS, because the two settings it consults are the authority on whether it should act: pit-exit auto-start is derived from the trigger, and pit-entry auto-stop is its own setting a driver can want with any trigger. Gating in the router as well would put the decision in two places. Two things had to change beyond the missing call. SessionController::start takes its duration from the configuration already saved, and that was only ever written inside begin_session. A crossing-started session would therefore have run for SessionConfiguration's own default of twenty minutes rather than the duration the driver set - plausible, and wrong for anyone who set something else, which is the kind of wrong that does not look wrong. Configuration now follows the settings instead of the act of starting: written at boot and again whenever a duration is edited, but not mid-session, since a running session keeps the duration it began with. The summary's completion reason was hardcoded to driver_stop. Harmless while a driver stop was the only way a session could end; a pit entry can end one now, and a record saying the driver stopped when they did not is worse than no record of how it ended. It is taken from the controller, which already sets CompletionReason::pit_entry. stop_from_pit_entry goes straight to review having refreshed the session times, so the summary is captured after it rather than before as the driver-stop path does, and confirm_stop is not part of this route. Every automation outcome is logged except the start and finish gates, which come through the same queue several times a lap. A pit exit that did not start a session is otherwise indistinguishable from no pit exit, and the reasons differ: the setting is off, a session was already running, or the controller refused. Tests cover an unconfigured controller taking the struct default, a configured one honouring 35 minutes rather than it, a second pit exit not restarting a running session, and a pit entry ending one with its own completion reason. The first assertion was written expecting zero and corrected when it failed. Closes #214 Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 17 +++ firmware/main/screen_router.cpp | 115 +++++++++++++++++++-- tests/cpp/test_gate_session_automation.cpp | 52 ++++++++++ 3 files changed, 173 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2e15d87..102931a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -288,6 +288,23 @@ The project follows Semantic Versioning once the first firmware release is tagge ### Fixed +- the **GPS session trigger now starts a session**. Choosing it armed the device and showed + `WAITING FOR THE LINE`, and nothing could ever start the session: only a manual START and + IMU launch detection reached `begin_session`. The mechanism existed - `gate_automation` + starts on pit exit and stops on pit entry, host-tested, with `session_trigger.cpp` already + setting `pit_exit_auto_start_enabled` from the trigger - and the crossings were already + being read off the timing queue and discarded in an empty loop. The two halves had never + been introduced +- a session started by a gate crossing now runs for **the configured duration**. + `SessionController::start` takes its duration from the configuration already saved, and + that was only ever written inside the manual start path, so a crossing-started session + would have used `SessionConfiguration`'s own twenty-minute default - plausible, and wrong + for anyone who set something else. The configuration now follows the settings: written at + boot and whenever a duration is edited +- a session ended by a pit entry is **recorded as a pit entry**. The summary's completion + reason was hardcoded to `driver_stop`, which was harmless while a driver stop was the only + way a session could end + - 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 diff --git a/firmware/main/screen_router.cpp b/firmware/main/screen_router.cpp index 9bd7137..90fe0f2 100644 --- a/firmware/main/screen_router.cpp +++ b/firmware/main/screen_router.cpp @@ -12,6 +12,7 @@ #include "track_timer/diagnostics/snapshot.hpp" #include "track_timer/storage/sd_card.hpp" #include "track_timer/storage/sd_track_store.hpp" +#include "track_timer/session/gate_automation.hpp" #include "track_timer/timing/embedded_runtime.hpp" #include "track_timer/timing/engine.hpp" #include "track_timer/timing/settings_adapter.hpp" @@ -513,6 +514,14 @@ class ScreenRouter { // Unconditional: if the save failed, settings_ still holds what the device will // use for the next session, and that is what the driver should be shown. refresh_ready(); + // The same reasoning as refresh_ready, for the controller rather than the screen: + // a duration edited in the menu has to be the one a pit-exit crossing starts a + // session with. Only while nothing is running, because save_configuration is a + // transition the controller refuses mid-session anyway and the running session + // keeps the duration it began with. + if (!session_was_active_) { + configure_session(static_cast(esp_timer_get_time() / 1000)); + } } // Advances the session clock and refreshes whichever running-session screen is up. @@ -1130,6 +1139,10 @@ class ScreenRouter { // Restore the track that was selected before the last reboot. restore_selected_track(); + // Configured at boot, not only when a session is started by hand. A pit-exit + // crossing starts the session through the controller directly, and the controller + // takes the duration from whatever configuration it already holds. + configure_session(static_cast(esp_timer_get_time() / 1000)); refresh_ready(); // Whatever is already on the card, so Review opens on the last session driven // rather than on nothing after a power cycle. @@ -1281,20 +1294,33 @@ class ScreenRouter { refresh_pending(); } - void begin_session() noexcept + // Puts the current durations into the controller and leaves it ready to start. + // + // Split out of begin_session because a gate crossing can now start a session without + // going through it: SessionController::start takes its duration from the configuration + // already saved, so a controller that has never been configured starts a session of + // zero length and lands straight in overtime. Configuration therefore follows the + // settings, not the act of starting. + void configure_session(const std::int64_t now_ms) noexcept { - armed_ = false; - const auto now_ms = static_cast(esp_timer_get_time() / 1000); session::SessionConfiguration configuration{}; configuration.session_duration_ms = static_cast(settings_.session_duration_seconds) * 1'000; configuration.rest_duration_ms = static_cast(settings_.rest_duration_seconds) * 1'000; - if (session_.save_configuration(configuration, now_ms) != + if (session_.save_configuration(configuration, now_ms) == session::TransitionResult::accepted) { - (void)session_.enter_configuration(now_ms); - (void)session_.save_configuration(configuration, now_ms); + return; } + (void)session_.enter_configuration(now_ms); + (void)session_.save_configuration(configuration, now_ms); + } + + void begin_session() noexcept + { + armed_ = false; + const auto now_ms = static_cast(esp_timer_get_time() / 1000); + configure_session(now_ms); const auto started = session_.start(now_ms); ESP_LOGI("track_timer", "session start: %d over %u s", static_cast(started), static_cast(settings_.session_duration_seconds)); @@ -1435,6 +1461,45 @@ class ScreenRouter { // queue that fills and then reports drops, and a drop there would be this code's // fault rather than the engine's or the receiver's - which is exactly the distinction // the loss counters exist to preserve. + // Hands one gate crossing to the session automation and makes the rest of the shell + // agree with whatever it decided. + // + // Every outcome is logged, including the refusals. A pit exit that did not start a + // session looks exactly like no pit exit at all from outside, and the reasons are + // different enough to matter: the setting is off, the session was already running, or + // the controller refused the transition. + void apply_gate_automation(const timing::GateCrossingDecision& decision) noexcept + { + const auto now_ms = static_cast(esp_timer_get_time() / 1000); + const auto outcome = + session::apply_gate_session_automation(decision, now_ms, settings_, session_); + if (outcome.result == session::GateAutomationResult::ignored_non_event || + outcome.result == session::GateAutomationResult::ignored_gate) { + // Start and finish crossings come through this queue too, several per lap. + // Logging them would bury the two that mean something. + return; + } + ESP_LOGI("track_timer", "gate automation: %s -> %s", + timing::timing_gate_name(decision.gate), + session::gate_automation_result_name(outcome.result)); + if (!outcome.transitioned) { + return; + } + // The session was started or stopped from underneath the shell, so the two things + // begin_session and advance_session_phase would have done have to happen here. + // service_session picks up the state change itself and loads the right screen. + armed_ = false; + if (outcome.result == session::GateAutomationResult::stopped) { + // stop_from_pit_entry goes straight to review, having already refreshed the + // session times, so the summary is captured after it rather than before as + // the driver-stop path does - and confirm_stop is not part of this route. + capture_summary(); + (void)session_.complete_review(now_ms); + } + describe_mode(); + refresh_pending(); + } + void drain_timing_output() noexcept { if (!timing_started_) { @@ -1449,12 +1514,22 @@ class ScreenRouter { static_cast(duration_ms / 1'000 % 60), static_cast(duration_ms % 1'000)); } - // Drained and discarded for now. The records are the raw material for the offline - // replay in #43 and belong in the log rather than on the screen, but the queue has - // to be emptied either way: one left to fill would report drops that mean only - // that nobody was reading it. The runtime counts what passes through. + // Each crossing is offered to the session automation, which starts on pit exit + // and stops on pit entry when the settings ask for it. This is what the GPS + // trigger has always meant and never did: the automation and the queue both + // existed, and the records were read off the queue and thrown away. + // + // Offered unconditionally rather than only while armed on GPS, because the two + // settings the automation consults are the authority on whether it should act - + // pit_exit_auto_start_enabled is derived from the trigger, and pit-entry + // auto-stop is its own setting that a driver can want with any trigger. Gating + // here as well would mean two places deciding the same thing. + // + // The records are also the raw material for the offline replay in #43, which is + // why they keep being drained to the floor afterwards rather than consumed. timing::GateCrossingDecision decision{}; while (timing::try_receive_gate_crossing_record(decision)) { + apply_gate_automation(decision); } log_timing_progress(); } @@ -2056,6 +2131,20 @@ class ScreenRouter { // What the car actually did, taken at the moment the session ends and before anything // resets. Peaks come from the meter, which has been scoped to the session since it // started, so they describe this session rather than everything since boot. + [[nodiscard]] static logger::SessionCompletionReason summary_completion_reason( + const session::CompletionReason reason) noexcept + { + switch (reason) { + case session::CompletionReason::none: + return logger::SessionCompletionReason::none; + case session::CompletionReason::driver_stop: + return logger::SessionCompletionReason::driver_stop; + case session::CompletionReason::pit_entry: + return logger::SessionCompletionReason::pit_entry; + } + return logger::SessionCompletionReason::none; + } + void capture_summary() noexcept { const auto snapshot = session_.snapshot(); @@ -2069,7 +2158,11 @@ class ScreenRouter { // end, which is why one is never greater than the other. summary.session_duration_ms = std::max(0, snapshot.session_elapsed_ms); summary.session_overrun_ms = std::max(0, snapshot.session_overrun_ms); - summary.completion_reason = logger::SessionCompletionReason::driver_stop; + // Taken from the controller rather than assumed. Hardcoding driver_stop was + // harmless while a driver stop was the only way a session could end; a pit entry + // can now end one, and a record that says the driver stopped when they did not is + // worse than no record of how it ended. + summary.completion_reason = summary_completion_reason(snapshot.completion_reason); summary.integrity = logger::SummaryIntegrity::complete; // Honest rather than flattering: there is no receiver, so every session so far is // recorded without the lap data a driver would expect. diff --git a/tests/cpp/test_gate_session_automation.cpp b/tests/cpp/test_gate_session_automation.cpp index d31022e..5116289 100644 --- a/tests/cpp/test_gate_session_automation.cpp +++ b/tests/cpp/test_gate_session_automation.cpp @@ -125,6 +125,58 @@ int main() assert(std::strlen(session::gate_automation_result_name(result)) > 0); } + // A pit exit starts the session through the controller directly, so it runs for + // whatever configuration the controller already holds - not for whatever the driver + // set. A controller nobody has configured starts a session at SessionConfiguration's + // own default, which is a plausible twenty minutes and therefore the kind of wrong + // that does not look wrong. The configuration has to be written from the settings + // before a crossing can arrive, which is why it is no longer done only inside the + // manual start path. + settings.pit_exit_auto_start_enabled = true; + session::SessionController unconfigured{}; + decision = session::apply_gate_session_automation( + event(timing::TimingGate::pit_exit), 0, settings, unconfigured); + assert(decision.result == session::GateAutomationResult::started); + assert(unconfigured.snapshot().session_remaining_ms == + session::SessionConfiguration{}.session_duration_ms); + + // Configured to something other than the default, so a session that honoured the + // default rather than the setting is distinguishable from one that honoured neither. + session::SessionController configured{}; + session::SessionConfiguration configuration{}; + configuration.session_duration_ms = 35 * 60 * 1'000; + configuration.rest_duration_ms = 15 * 60 * 1'000; + assert(configuration.session_duration_ms != + session::SessionConfiguration{}.session_duration_ms); + // save_configuration is only accepted from the configuring state, which is why + // configure_session in the router falls back to entering it first. + assert(configured.enter_configuration(0) == session::TransitionResult::accepted); + assert(configured.save_configuration(configuration, 0) == + session::TransitionResult::accepted); + decision = session::apply_gate_session_automation( + event(timing::TimingGate::pit_exit), 10, settings, configured); + assert(decision.result == session::GateAutomationResult::started); + assert(configured.snapshot().session_remaining_ms == configuration.session_duration_ms); + assert(configured.snapshot().state == session::SessionState::running); + + // A second crossing while that session runs must not restart it, however many times + // the car passes the pit exit. + const auto started_remaining = configured.snapshot().session_remaining_ms; + decision = session::apply_gate_session_automation( + event(timing::TimingGate::pit_exit), 20, settings, configured); + assert(decision.result == session::GateAutomationResult::ignored_state); + assert(!decision.transitioned); + assert(configured.snapshot().session_remaining_ms == started_remaining); + + // And a pit entry ends it with its own reason rather than looking like a driver stop, + // which is what the summary records. + settings.pit_entry_auto_stop_enabled = true; + decision = session::apply_gate_session_automation( + event(timing::TimingGate::pit_entry), 5'000, settings, configured); + assert(decision.result == session::GateAutomationResult::stopped); + assert(configured.snapshot().state == session::SessionState::review); + assert(configured.snapshot().completion_reason == session::CompletionReason::pit_entry); + std::cout << "Lap-gate settings and independent pit session automation passed\n"; return 0; }