From 91821ebb4a527fec753cc7ca22fc3b230053876b Mon Sep 17 00:00:00 2001 From: mrsqr Date: Thu, 20 Aug 2026 22:09:47 +0100 Subject: [PATCH] feat(ui): open the gated menu on REVIEW Opening the menu lands on the first item with no swipe at all, and everything else costs at least one, so the order is a statement about when each item is wanted. Review was last but one, four swipes from a hold - the furthest thing in the menu from the gesture that opens it - despite being wanted at the one moment the driver is definitely stopped and definitely reaching for the device. Mode, which is set once and rarely touched again, was free. The order now reads Review, Mode, Track, Trigger, Setup, Diagnostics: Review first, the three things decided at the circuit next in the order they are decided, and Diagnostics last because it is least often wanted. The trade is explicit rather than hidden. Review goes from a hold, four swipes and a press to a hold and a press; Mode gains one gesture, because it is no longer what a hold lands on. Both costs are asserted in tests so a later reorder has to face them. MenuItem is never persisted or serialised - menu_item() casts the carousel index straight to it - so nothing outside the UI depends on the values. The correspondence between the enum and the carousel array is positional and was maintained by hand, with a static_assert that only checked the count. It would have caught a missing entry but not a swapped pair, which is the easier mistake to make while reordering and the harder one to notice. Each entry's label is now checked against its enumerator at compile time. Closes #160 Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + .../track_timer/ui/shell_navigation.hpp | 12 +++- firmware/main/screen_router.cpp | 26 ++++++++- tests/cpp/test_shell_navigation.cpp | 56 ++++++++++++------- 4 files changed, 72 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 47b1536..eb5b3b2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -68,6 +68,7 @@ The project follows Semantic Versioning once the first firmware release is tagge - vertical G alongside the lateral and longitudinal pair, for kerbs and compressions - gyroscope zero-rate offset measured at rest and removed, 4.4 dps on this board - device settings persisted in NVS, so Mode survives a reboot +- the gated menu opens on REVIEW, which is wanted the moment a session ends - top-level TRIGGER selection: MANUAL starts on the button, IMU on a launch, GPS at the line - pending phase on the running screen for a session armed and waiting for its trigger - overrun timer counting up in deep purple once a session reaches 00:00, with the lap diff --git a/firmware/components/ui/include/track_timer/ui/shell_navigation.hpp b/firmware/components/ui/include/track_timer/ui/shell_navigation.hpp index 0377f58..8694f7f 100644 --- a/firmware/components/ui/include/track_timer/ui/shell_navigation.hpp +++ b/firmware/components/ui/include/track_timer/ui/shell_navigation.hpp @@ -25,14 +25,20 @@ enum class ShellLevel : std::uint8_t { // Track selection sits at the top level, between Mode and Setup: at a circuit it is the // thing most often changed, and burying it under Setup made it the deepest common task. +// Order is position, and position is a statement about when each item is wanted: opening +// the menu lands on the first one with no swipe at all, and everything else costs at least +// one. +// +// Review comes first because it is wanted at the one moment the driver is definitely +// stopped and definitely reaching for the device - just after a session. Mode, Track and +// Trigger follow, in the order they are decided at the circuit before going out. +// Diagnostics is last because it is the least often wanted. enum class MenuItem : std::uint8_t { + review, mode, track, - // What starts a session. Sits with the track because both are set at the circuit, - // before going out, rather than buried under Setup with the rarely-touched options. trigger, setup, - review, diagnostics, }; inline constexpr std::size_t kMenuItemCount = 6; diff --git a/firmware/main/screen_router.cpp b/firmware/main/screen_router.cpp index 14fc6d9..1d4801b 100644 --- a/firmware/main/screen_router.cpp +++ b/firmware/main/screen_router.cpp @@ -148,16 +148,40 @@ constexpr std::uint32_t kRuby = 0xFF8FA3; } } +// Must stay in MenuItem order: the shell casts the carousel index straight to the enum. constexpr ui::CarouselEntry kMenuEntries[] = { + {LV_SYMBOL_LIST, "REVIEW", kAzure}, {LV_SYMBOL_POWER, "MODE", kRuby}, {LV_SYMBOL_GPS, "TRACK", kAzure}, {LV_SYMBOL_CHARGE, "TRIGGER", kGreen}, {LV_SYMBOL_SETTINGS, "SETUP", kAmber}, - {LV_SYMBOL_LIST, "REVIEW", kAzure}, {LV_SYMBOL_EYE_OPEN, "DIAGNOSTICS", kGreen}, }; +// The shell casts the carousel index straight to MenuItem, so the array's order is the +// menu's order. Checking the count alone would catch a missing entry but not a swapped +// pair, which is the easier mistake to make and the harder one to notice. +constexpr bool same_text(const char* left, const char* right) noexcept +{ + while (*left != '\0' && *left == *right) { + ++left; + ++right; + } + return *left == *right; +} + +constexpr bool menu_entry_is(const ui::MenuItem item, const char* const label) noexcept +{ + return same_text(kMenuEntries[static_cast(item)].label, label); +} + static_assert(std::size(kMenuEntries) == ui::kMenuItemCount, "the carousel must offer exactly the items the shell can select"); +static_assert(menu_entry_is(ui::MenuItem::review, "REVIEW")); +static_assert(menu_entry_is(ui::MenuItem::mode, "MODE")); +static_assert(menu_entry_is(ui::MenuItem::track, "TRACK")); +static_assert(menu_entry_is(ui::MenuItem::trigger, "TRIGGER")); +static_assert(menu_entry_is(ui::MenuItem::setup, "SETUP")); +static_assert(menu_entry_is(ui::MenuItem::diagnostics, "DIAGNOSTICS")); // Manual first: it is what the device did before a trigger existed, and what a driver // falls back to when a trigger cannot arm. diff --git a/tests/cpp/test_shell_navigation.cpp b/tests/cpp/test_shell_navigation.cpp index dd8bc63..6519045 100644 --- a/tests/cpp/test_shell_navigation.cpp +++ b/tests/cpp/test_shell_navigation.cpp @@ -32,7 +32,7 @@ void only_a_hold_opens_the_menu() const auto open = shell.dispatch(InputAction::long_press); assert(open.outcome == ShellOutcome::menu_opened); assert(open.state.level == ShellLevel::menu); - assert(shell.menu_item() == MenuItem::mode); + assert(shell.menu_item() == MenuItem::review); } // Hops to a menu item by name. Counting swipes meant that inserting an item silently @@ -48,14 +48,16 @@ void to_menu(ShellNavigation& shell, const MenuItem item) assert(false && "menu item not reachable"); } -// Track and Trigger sit between Mode and Setup: at a circuit both are set before going -// out, so neither should be buried under Setup with the rarely-touched options. -void the_menu_offers_mode_then_track_and_wraps() +// Opening the menu costs no swipe to reach the first item and at least one for everything +// else, so Review leads: it is wanted the moment a session ends, when the driver is +// stopped and already reaching for the device. Mode, Track and Trigger follow in the order +// they are decided at the circuit, and Diagnostics is last. +void the_menu_opens_on_review_and_wraps() { auto shell = opened(); - assert(shell.menu_item() == MenuItem::mode); - for (const auto expected : {MenuItem::track, MenuItem::trigger, MenuItem::setup, - MenuItem::review, MenuItem::diagnostics, MenuItem::mode}) { + assert(shell.menu_item() == MenuItem::review); + for (const auto expected : {MenuItem::mode, MenuItem::track, MenuItem::trigger, + MenuItem::setup, MenuItem::diagnostics, MenuItem::review}) { (void)shell.dispatch(InputAction::swipe_left); assert(shell.menu_item() == expected); } @@ -67,8 +69,7 @@ void the_menu_offers_mode_then_track_and_wraps() void the_track_list_is_sized_by_the_caller() { auto shell = opened(); - (void)shell.dispatch(InputAction::swipe_left); // TRACK - assert(shell.menu_item() == MenuItem::track); + to_menu(shell, MenuItem::track); auto result = shell.dispatch(InputAction::press); assert(result.outcome == ShellOutcome::entered); assert(result.state.level == ShellLevel::section); @@ -102,22 +103,35 @@ void a_shorter_track_list_resets_an_out_of_range_index() assert(shell.state().section_index == 0); } -// Choosing a mode is the entire interaction: enter Mode, swipe to it, press. -void a_mode_is_three_gestures_from_the_dashboard() +// Choosing a mode is still the whole interaction, and still shallow. Putting Review first +// costs Mode one gesture - it is no longer what a hold lands on - which is the trade the +// order makes: Review is wanted right after a session, Mode is set once and left. +void a_mode_is_a_handful_of_gestures_from_the_dashboard() { - auto shell = opened(); // 1: hold - auto result = shell.dispatch(InputAction::press); // 2: enter Mode + auto shell = opened(); // 1: hold + to_menu(shell, MenuItem::mode); // 2: swipe + auto result = shell.dispatch(InputAction::press); // 3: enter Mode assert(result.outcome == ShellOutcome::entered); assert(result.state.level == ShellLevel::section); - assert(!result.emits_action); // Mode has no destination screen + assert(!result.emits_action); // Mode has no destination screen - result = shell.dispatch(InputAction::swipe_left); // 3: to Race + result = shell.dispatch(InputAction::swipe_left); // 4: to Race assert(result.state.section_index == 1); - result = shell.dispatch(InputAction::press); // 4: select + result = shell.dispatch(InputAction::press); // 5: select assert(result.outcome == ShellOutcome::mode_selected); assert(result.state.section_index == 1); } +// The other half of that trade: Review is now a hold and a press, where it used to be a +// hold, four swipes and a press. +void review_is_two_gestures_from_the_dashboard() +{ + auto shell = opened(); + assert(shell.menu_item() == MenuItem::review); + const auto result = shell.dispatch(InputAction::press); + assert(result.emits_action && result.action == NavigationAction::open_review); +} + void review_and_diagnostics_hand_over_to_the_destination_model() { auto shell = opened(); @@ -128,7 +142,7 @@ void review_and_diagnostics_hand_over_to_the_destination_model() assert(result.emits_action && result.action == NavigationAction::open_review); assert(result.state.level == ShellLevel::menu); - (void)shell.dispatch(InputAction::swipe_left); + to_menu(shell, MenuItem::diagnostics); result = shell.dispatch(InputAction::press); assert(result.emits_action && result.action == NavigationAction::open_diagnostics); } @@ -201,6 +215,9 @@ void a_live_session_blocks_and_closes_the_menu() shell.synchronize_session(false); (void)shell.dispatch(InputAction::long_press); + // Review is a destination and stays at menu level, so descend into one that has + // children to prove a live session closes the menu from depth. + to_menu(shell, MenuItem::mode); (void)shell.dispatch(InputAction::press); assert(shell.state().level == ShellLevel::section); @@ -278,10 +295,11 @@ void an_out_of_range_section_falls_back_to_the_first() int main() { only_a_hold_opens_the_menu(); - the_menu_offers_mode_then_track_and_wraps(); + the_menu_opens_on_review_and_wraps(); the_track_list_is_sized_by_the_caller(); a_shorter_track_list_resets_an_out_of_range_index(); - a_mode_is_three_gestures_from_the_dashboard(); + a_mode_is_a_handful_of_gestures_from_the_dashboard(); + review_is_two_gestures_from_the_dashboard(); review_and_diagnostics_hand_over_to_the_destination_model(); a_setting_value_is_reachable_by_descending_three_levels(); every_level_climbs_back_out_one_at_a_time();