From d893ce79739b9d638efcd84bc44750d3ce11a4cc Mon Sep 17 00:00:00 2001 From: timohueser Date: Mon, 24 Aug 2026 11:07:27 +0200 Subject: [PATCH 1/3] refactor(app): give Navigator, Settings, DFU and StorageInfo their own pending state MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The four domains whose legacy answer is already terminal now own the rider's request end to end: an intent, an operation token, a bounded effect, and one `admit_intent` / `next_effect` / `apply_outcome` seam beside the state they already hold. A plan, an update phase, a settings write and a free-space refresh each exist in exactly one place. Navigator is the load-bearing half. It owns the planning lifecycle per family (`Idle → Requested → Planning → PreviewReady → Committing → Active | Failed`), the #499 post-time annihilation, the #1146 per-family freeze release, and late-answer refusal by token — and it is the single writer of `RerouteFreeze`, which moved in from `App`. `SettingsMachine` absorbs `HostPending`'s persist half (the dirty revision, the subtree debounce, the retry backoff, the #810 stale-ack rule); `HostPending`'s remaining store-commit counter folds into `App`. Pass stages 8, 9 and 11 stop being empty and advance real machines; stage 1 consumes their outcomes. The eight `peek_host_command` / `drain_host_command` arms become translation only, through the `compat.rs` rows those effects already had. `DRAIN_ORDER`, `class()`, `HostCommand` and `HostEvent` are untouched. There is deliberately no `UiRuntime` → Navigator/DFU/StorageInfo connection: a screen names its request straight to the owner through `Ctx`, which reaches the domain before stage 1 rather than at stage 4, and is the only shape that also serves the hosts still driving `drain_host_commands` — no host runs the pass until S6. A slot beside it would be the second copy this slice exists to remove. Deleted with their replacements: seven `Activity` one-shot slots and their `take_*` / `has_*` / `pending_*` families, `App::card_free_bytes`, the whole `HostPending` type, and the four `note_plan_started` / `note_plan_ended` call sites inside the drain. Refs #1485, epic #1397. Co-Authored-By: Claude Fable 5 --- firmware/obc-app/src/activity.rs | 219 +------ firmware/obc-app/src/app.rs | 488 ++++++++++------ .../obc-app/src/device_core/connections.rs | 8 + firmware/obc-app/src/device_core/pass.rs | 105 +++- .../obc-app/src/device_core/storage_info.rs | 141 +++++ firmware/obc-app/src/dfu.rs | 130 +++++ firmware/obc-app/src/host.rs | 196 +------ firmware/obc-app/src/navigator.rs | 550 +++++++++++++++++- firmware/obc-app/src/ride_engine.rs | 15 +- firmware/obc-app/src/screen/detour.rs | 156 +++-- firmware/obc-app/src/screen/dfu.rs | 19 +- firmware/obc-app/src/screen/mod.rs | 21 +- firmware/obc-app/src/screen/nav_route.rs | 11 +- firmware/obc-app/src/screen/settings/about.rs | 16 +- .../obc-app/src/screen/settings/firmware.rs | 26 +- .../obc-app/src/screen/settings/system.rs | 19 +- firmware/obc-app/src/settings.rs | 306 +++++++++- firmware/obc-app/tests/nav.rs | 4 +- firmware/obc-fw-nrf54l/src/ride.rs | 4 +- firmware/tools/resource_baseline.json | 6 +- host/obc-host-core/tests/board_parity.rs | 6 +- .../obc-host-core/tests/device_core_compat.rs | 29 +- .../tests/device_core_conformance.rs | 307 ++++++++-- 23 files changed, 2029 insertions(+), 753 deletions(-) diff --git a/firmware/obc-app/src/activity.rs b/firmware/obc-app/src/activity.rs index eb7fa78cb..540a89263 100644 --- a/firmware/obc-app/src/activity.rs +++ b/firmware/obc-app/src/activity.rs @@ -90,13 +90,15 @@ pub(crate) struct SeamRequest { pub anchor_m: u32, } -/// A one-shot **detour-plan request** (#882): the Detour chooser's Press asks the host to plan -/// an A* detour from the rider's fix to the rejoin point at `target_m`, blacklisting the -/// corridor around the skipped span `[progress_m, target_m]`. The host resolves the rejoin -/// *coordinate* itself (`position_at(target_m)`) — it owns the active `RouteReader`; the screen -/// deliberately carries only distances, keeping the request tiny and `Copy`. Drained as -/// [`HostCommand::PlanDetour`](crate::host::HostCommand::PlanDetour); answered through -/// [`App::apply_event`](crate::App::apply_event) as `DetourPlanned`. +/// A **detour-plan request** (#882): the Detour chooser's Press asks the planner for an A* detour +/// from the rider's fix to the rejoin point at `target_m`, blacklisting the corridor around the +/// skipped span `[progress_m, target_m]`. The executor resolves the rejoin *coordinate* itself +/// (`position_at(target_m)`) — it owns the active `RouteReader`; the screen deliberately carries +/// only distances, keeping the request tiny and `Copy`. +/// +/// Lives with [`NavigatorMachine`](crate::navigator::NavigatorMachine), which is where the rider's +/// request waits until an executor takes it. The type stays here because [`Activity`] is the ride +/// model the chooser measures it against. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct DetourRequest { /// The active catalog slot the request is keyed to (durable-remapped across rescans). @@ -110,8 +112,8 @@ pub struct DetourRequest { pub target_m: u32, } -/// Which phase of the SD-sideload firmware update (epic #615 S5, #620) a [`DfuAction`] one-shot -/// asks the board to run. The two phases are separate so the UI can **confirm before arming**: +/// Which phase of the SD-sideload firmware update (epic #615 S5, #620) +/// [`DfuState`](crate::dfu::DfuState) asks the board to run. The two phases are separate so the UI can **confirm before arming**: /// [`Scan`](DfuAction::Scan) is read-only (validate `UPDATE.BIN`, cost nothing on failure) and /// answers a [`DfuScanReport`](crate::dfu::DfuScanReport); [`Install`](DfuAction::Install) is the /// irreversible arm-and-reboot (snapshot the rollback, write the boot-state page, reset into the @@ -126,13 +128,13 @@ pub enum DfuAction { Install, } -/// A one-shot **route-planning request** (epic #116, R4): the POI create-route confirm asks the -/// host to run the on-device router from the rider's fix to the POI. Coordinates are `(lon, lat)` -/// microdegrees (the OBCM/renderer convention); the name is the POI's stored name (or its subtype -/// fallback label, matching the list row), carried as a fixed inline buffer so the request — like -/// every other one-shot on [`Activity`] — stays `Copy`. Drained by -/// [`App::drain_host_commands`](crate::App::drain_host_commands); the host answers with -/// [`App::apply_event`](crate::App::apply_event). +/// A **route-planning request** (epic #116, R4): the POI create-route confirm asks the on-device +/// router for a route from the rider's fix to the POI. Coordinates are `(lon, lat)` microdegrees +/// (the OBCM/renderer convention); the name is the POI's stored name (or its subtype fallback +/// label, matching the list row), carried as a fixed inline buffer so the request stays `Copy` and +/// bounded. +/// +/// Lives with [`NavigatorMachine`](crate::navigator::NavigatorMachine) until an executor takes it. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct NavRequest { /// The rider's fix, `(lon, lat)` µdeg — the route's start. @@ -239,44 +241,10 @@ pub struct Activity { /// vanished in a racing rescan simply drains to a no-op at the host. The host deletes the /// `TP{id}.OBT` **and** every member route file, then rescans + re-feeds trips + routes. delete_trip: Option, - /// A one-shot **route-planning request** (epic #116, R4), set by the POI create-route confirm - /// and drained by the host via [`App::drain_host_commands`](crate::App::drain_host_commands), which - /// steps the resumable router, writes the reserved nav route, rescans, and answers through - /// [`App::apply_event`](crate::App::apply_event). - nav_request: Option, /// A seam re-anchor queued by the detour commit handler (#882); /// [`RideEngine`](crate::ride_engine::RideEngine) consumes it on the next tick that has the /// matching active geometry, installing matcher progress + floor at the splice seam. seam_request: Option, - /// A one-shot detour-plan request (#882), set by the Detour chooser's Press and drained via - /// [`App::drain_host_commands`](crate::App::drain_host_commands) as `PlanDetour`. - detour_request: Option, - /// A one-shot detour **commit** (#882): the preview screen's Press asks the host to splice - /// the planned detour into the active route (Phase B) and re-adopt the result. - detour_commit: bool, - /// A one-shot detour **cancel** (#882): Back on the planning or preview screen drops the - /// in-flight plan / the held detour bytes host-side. Same annihilation semantics as - /// [`nav_cancel`](Activity::nav_cancel). - detour_cancel: bool, - /// A one-shot **DFU request** (epic #615 S5, #620): the SD-sideload firmware-update flow. The - /// System settings screen posts [`DfuAction::Scan`] (validate `UPDATE.BIN`, answer through - /// [`App::apply_event`](crate::App::apply_event)); the confirm screen - /// posts [`DfuAction::Install`] (snapshot the rollback, arm the boot-state page, reboot into - /// the bootloader). The board's ride loop drains it as the `Dfu` command via - /// [`App::drain_host_commands`](crate::App::drain_host_commands) — the same slot the `dfu-install` - /// debug command drives by staging a `Dfu(Install)` directly after the drain. - dfu_request: Option, - /// A one-shot **plan-cancel request** (#499): Back on the planning screen pops it *and* - /// records this; the host drains it via [`App::drain_host_commands`](crate::App::drain_host_commands) - /// and aborts the in-flight plan (discarding the partial file, answering nothing — the rider - /// is already back on the POI detail). - nav_cancel: bool, - /// A one-shot **card-free scan request** (T8 item 6): posted when the System settings screen is - /// opened, so the host runs a FAT free-cluster scan *once on entry* (never per frame — the scan is - /// expensive) and answers through [`App::apply_event`](crate::App::apply_event). Drained via - /// [`App::drain_host_commands`](crate::App::drain_host_commands); until it answers the - /// System screen shows `--`. - card_scan_request: bool, /// The **sensor scan mode** level (BLE sensors epic #707, SE7): raised by the Sensors screen while /// a scan-list sub-screen is open (entering a HR/power/cadence row) and lowered on exit/Back. /// Unlike the delete/scan *one-shots* this is a **level**, not a drained edge — the enter/leave @@ -616,70 +584,6 @@ impl Activity { .and_then(|req| remap(req.route).map(|route| SeamRequest { route, anchor_m: req.anchor_m })); } - /// Record a one-shot detour-plan request (#882) — set by the Detour chooser's Press, drained - /// by [`App::drain_host_commands`](crate::App::drain_host_commands) as `PlanDetour`. - pub(crate) fn request_detour(&mut self, req: DetourRequest) { - self.detour_request = Some(req); - } - - /// Take (and clear) the pending detour-plan request, if any. - pub(crate) fn take_detour_request(&mut self) -> Option { - self.detour_request.take() - } - - /// Non-consuming peek at whether a detour-plan request is pending. - pub(crate) fn has_detour_request(&self) -> bool { - self.detour_request.is_some() - } - - /// Non-consuming peek at the pending detour-plan request itself — the durable-identity tests - /// pin the remap through it without draining the one-shot. - #[cfg(test)] - pub(crate) fn pending_detour_request(&self) -> Option { - self.detour_request - } - - /// Follow a queued detour-plan request through a route-catalog rescan by durable identity; - /// a vanished route drops the request (the host-side gate would refuse it anyway). - pub(crate) fn remap_detour_route(&mut self, remap: &dyn Fn(usize) -> Option) { - self.detour_request = - self.detour_request.and_then(|req| remap(req.route).map(|route| DetourRequest { route, ..req })); - } - - /// Record a one-shot detour commit (#882) — the preview screen's Press. - pub(crate) fn request_detour_commit(&mut self) { - self.detour_commit = true; - } - - /// Take (and clear) the pending detour commit, if any. - pub(crate) fn take_detour_commit(&mut self) -> bool { - core::mem::take(&mut self.detour_commit) - } - - /// Non-consuming peek at whether a detour commit is pending. - pub(crate) fn detour_commit_pending(&self) -> bool { - self.detour_commit - } - - /// Record a one-shot detour cancel (#882) — Back on the detour planning or preview screen. - /// **Post-time annihilation** (the `request_nav_cancel` rule): an undrained plan request or - /// commit is cleared here, so the host never receives work the rider already dismissed. - pub(crate) fn request_detour_cancel(&mut self) { - self.detour_request = None; - self.detour_commit = false; - self.detour_cancel = true; - } - - /// Take (and clear) the pending detour cancel, if any. - pub(crate) fn take_detour_cancel(&mut self) -> bool { - core::mem::take(&mut self.detour_cancel) - } - - /// Non-consuming peek at whether a detour cancel is pending. - pub(crate) fn detour_cancel_pending(&self) -> bool { - self.detour_cancel - } - /// Record a one-shot disposition for the open ride log, drained by the host. pub fn request_track(&mut self, action: TrackAction) { self.track_action = Some(action); @@ -760,46 +664,6 @@ impl Activity { self.delete_trip.is_some() } - /// Record a one-shot route-planning request (epic #116, R4) — set by the POI create-route - /// confirm, drained by [`App::drain_host_commands`](crate::App::drain_host_commands). - pub(crate) fn request_nav(&mut self, req: NavRequest) { - self.nav_request = Some(req); - } - - /// Take (and clear) the pending route-planning request, if any. - pub(crate) fn take_nav_request(&mut self) -> Option { - self.nav_request.take() - } - - /// Non-consuming peek at whether a route-planning request is pending. - pub(crate) fn has_nav_request(&self) -> bool { - self.nav_request.is_some() - } - - /// Record a one-shot [`DfuAction`] (epic #615 S5, #620) — set by the System settings screen - /// (`Scan`) and the update-confirm screen (`Install`), drained by the board via - /// [`App::drain_host_commands`](crate::App::drain_host_commands). A later post overwrites an - /// undrained earlier one (there is never more than one DFU phase in flight). - pub(crate) fn request_dfu(&mut self, action: DfuAction) { - self.dfu_request = Some(action); - } - - /// Take (and clear) the pending [`DfuAction`], if any. - pub(crate) fn take_dfu_request(&mut self) -> Option { - self.dfu_request.take() - } - - /// Record the one-shot **card-free scan request** (T8 item 6) — posted when the System settings - /// screen opens, drained by the host via [`App::drain_host_commands`](crate::App::drain_host_commands). - pub(crate) fn request_card_scan(&mut self) { - self.card_scan_request = true; - } - - /// Take (and clear) the pending card-free scan request. - pub(crate) fn take_card_scan_request(&mut self) -> bool { - core::mem::take(&mut self.card_scan_request) - } - /// Set the **sensor scan mode** level (BLE sensors epic #707, SE7): `true` when the scan-list /// screen opens on a HR/power/cadence row, `false` on exit/Back. A level, not a one-shot — the host /// polls it via [`App::sensor_scan_active`](crate::App::sensor_scan_active) each pass. @@ -813,43 +677,6 @@ impl Activity { self.sensor_scan } - /// Non-consuming peek at whether a [`DfuAction`] is posted but undrained — the remote-check - /// deferral gate (S6, #621): a BLE-initiated check must not overwrite a phase already in flight - /// (a later [`request_dfu`](Self::request_dfu) overwrites, by design, for the *rider's* posts). - pub(crate) fn has_dfu_request(&self) -> bool { - self.dfu_request.is_some() - } - - /// Record a one-shot plan-cancel (#499) — set by the planning screen's Back, drained by - /// [`App::drain_host_commands`](crate::App::drain_host_commands). - /// - /// **Annihilates** a still-undrained [`request_nav`](Self::request_nav): Back always comes - /// from the planning screen of the *latest* request, so a request still latched at - /// cancel-post time was confirmed and cancelled inside one input batch — the net intent is - /// "no plan", and executing it anyway would commit a ghost route whose answer nobody is - /// showing. The cancel itself still - /// latches: with nothing in flight it is a harmless host-side no-op, and a plan the host - /// already drained is still aborted. - pub(crate) fn request_nav_cancel(&mut self) { - self.nav_request = None; - self.nav_cancel = true; - } - - /// Take (and clear) the pending plan-cancel request. - pub(crate) fn take_nav_cancel(&mut self) -> bool { - core::mem::take(&mut self.nav_cancel) - } - - /// Non-consuming peek at the pending plan-cancel one-shot (the typed drain's pendency check). - pub(crate) fn nav_cancel_pending(&self) -> bool { - self.nav_cancel - } - - /// Non-consuming peek at the pending card-free scan one-shot (the typed drain's pendency check). - pub(crate) fn card_scan_pending(&self) -> bool { - self.card_scan_request - } - /// The elevation (m) to stamp on a logged [`TrackPoint`](obc_ports::TrackPoint): the /// latest barometric altitude, or 0 before any sample. /// @@ -863,16 +690,6 @@ impl Activity { /// Called when a session starts, so tracking accumulators begin fresh. pub(crate) fn reset_ride(&mut self) { self.seam_request = None; - // Dropping the detour pair here cannot strand the Recalculating freeze's `Detour` level, - // even though no `note_plan_ended` runs with it (#1150 review). Both halves are covered - // without one: an **undrained request** never engaged the freeze (the drain is the engaging - // edge), and a dropped **cancel** only forfeits one of two release edges — the host is - // still running the plan that cancel would have aborted, and it answers every plan it was - // given, so `on_detour_planned`'s unconditional release lands anyway (idempotent, and it - // fires before its own late-answer early-return for exactly this kind of reason). - self.detour_request = None; - self.detour_commit = false; - self.detour_cancel = false; self.progress_m = 0; self.off_route = false; self.dist_to_route_m = 0; diff --git a/firmware/obc-app/src/app.rs b/firmware/obc-app/src/app.rs index 0a7500f02..dfca4b392 100644 --- a/firmware/obc-app/src/app.rs +++ b/firmware/obc-app/src/app.rs @@ -10,11 +10,13 @@ use obc_route::{Profile, RouteReader}; use crate::activity::{Activity, Mode}; use crate::card_scheduler::{BootUpdate, DfuLanding, PendingUpload, UploadEvent}; use crate::catalog_state::CatalogState; +use crate::device_core::compat::{dfu_row, navigator_row, settings_row, storage_info_row}; +use crate::device_core::storage_info::StorageInfo; +use crate::dfu::DfuState; use crate::dirty::Dirty; -use crate::host::{ - DrainStatus, HostCommand, HostCommandClass, HostEvent, HostMailbox, HostPending, HOST_COMMAND_CLASSES, -}; +use crate::host::{DrainStatus, HostCommand, HostCommandClass, HostEvent, HostMailbox, HOST_COMMAND_CLASSES}; use crate::input::Gesture; +use crate::navigator::{NavigatorIntent, NavigatorMachine, PlanPhase}; use crate::placement::define_placement_constructors; use crate::reroute_freeze::PlanFamily; use crate::ride::RideSummary; @@ -556,20 +558,26 @@ pub struct App { /// decision. The bundle itself stays in the platform's store — this owns what the rider is /// *told*, never the frames. pub(crate) weather: crate::weather::WeatherDomain, - /// The app-side pending protocol state that isn't a one-shot slot on [`Activity`]: the - /// counted store-changed cue (#450) and the #810 settings-persistence state machine - /// (revision, handshake state, bounded retry pacing). Drained and answered only through the - /// typed protocol below. - pub(crate) host: HostPending, + /// Count of [`HostEvent::StoreChanged`] facts not yet acted on (#450). The host drains it once + /// per pass as the counted [`HostCommand::RescanStore`]. A counter, not a bool, so a burst of + /// commits between drains is never coalesced into a single missed rescan. + store_changed: u32, + /// The **Navigator** domain (#1397 S2): the rider's undelivered plan requests, the per-family + /// phase, the operation token every planner answer must carry back, and the Recalculating + /// freeze those transitions drive. The only writer of any of them. + pub(crate) navigator: NavigatorMachine, + /// The **settings-persistence** machine (#810, #1397 S2): the dirty revision, the subtree + /// debounce, the retry backoff and the stale-answer rule. + pub(crate) settings_ops: crate::settings::SettingsMachine, + /// The **DFU** domain (#1397 S2): the single most-recent-wins update phase and its token. + pub(crate) dfu: DfuState, + /// The **StorageInfo** domain (#1397 S2): the free-space refresh, its token, and the figure the + /// System screen prints. + pub(crate) storage: StorageInfo, /// The DeviceCore coordinator's own state (#1438): every cross-domain connection, the levels a /// stage detects an edge against, the current [`Capabilities`](crate::device_core::Capabilities) /// and the re-entrancy guard. Not domain state — nothing here decides a product rule. pub(crate) pass: crate::device_core::pass::PassState, - /// The **Recalculating freeze** (issue #1146, P2): whether a host planner run is live, which - /// (over a map base) stops map redraws, pauses the matcher, and raises the overlay banner — - /// the product rule that makes the render and nav arms of the board's scratch arena disjoint. - /// See [`crate::reroute_freeze`]. - freeze: crate::reroute_freeze::RerouteFreeze, /// The running firmware version string (T8 item 6) — the same value the DFU confirm shows as /// "Installed", fed by the host at boot via [`set_fw_version`](App::set_fw_version). The System /// settings screen's `Firmware` ledger row renders it (empty ⇒ `--`). Resident because that frame @@ -581,11 +589,6 @@ pub struct App { map_name: heapless::String<24>, /// The loaded map's OBCM format version, the right half of the `Map` row. `0` until a map loads. map_obcm_version: u8, - /// Free space on the SD card in bytes (T8 item 6), answered by the host's FAT free-cluster scan - /// after the System screen posts its one-shot on entry - /// ([`take_card_scan_request`](App::drain_host_commands) → [`set_card_free`](App::apply_event)). - /// `None` until the host answers — the screen shows `--`. - card_free_bytes: Option, /// Whether a recovered-ride offer has already reached the screen this boot. A recorder may /// report the same resumable object on every host pass; the rider sees one decision card. recovered_ride_offered: bool, @@ -639,13 +642,15 @@ impl App { clock_trust: ClockTrust::Untrusted, retention: crate::retention::RetentionMachine::new(), weather: crate::weather::WeatherDomain::new(), - host: HostPending::new(), + store_changed: 0, + navigator: NavigatorMachine::new(), + settings_ops: crate::settings::SettingsMachine::new(), + dfu: DfuState::new(), + storage: StorageInfo::new(), pass: crate::device_core::pass::PassState::new(), - freeze: crate::reroute_freeze::RerouteFreeze::new(), fw_version: heapless::String::new(), map_name: heapless::String::new(), map_obcm_version: 0, - card_free_bytes: None, recovered_ride_offered: false, } ); @@ -683,13 +688,15 @@ impl App { clock_trust, retention, weather, - host, + store_changed, + navigator, + settings_ops, + dfu, + storage, pass, - freeze, fw_version, map_name, map_obcm_version, - card_free_bytes, recovered_ride_offered, } = self; assert_eq!(*camera, state, "the camera state is preserved verbatim"); @@ -713,15 +720,22 @@ impl App { weather.installed().is_none() && !weather.refreshing() && weather.last_refresh().is_none(), "no weather installed, none requested, nothing completed this boot" ); - assert!(host.is_empty(), "no host work pending, settings Clean at revision 0"); + assert_eq!(*store_changed, 0, "no store commit pending"); + assert!(settings_ops.is_empty(), "settings Clean at revision 0"); + navigator.assert_boot_state(); + dfu.assert_boot_state(); + storage.assert_boot_state(); assert_eq!(*pass, crate::device_core::pass::PassState::new(), "no connection wired, no pass in flight"); - assert!(!freeze.plan_live() && !freeze.active(true), "no planner running, no freeze banner"); assert!(fw_version.is_empty() && map_name.is_empty(), "the host has identified nothing yet"); assert_eq!(*map_obcm_version, 0, "no map format known yet"); - assert!(card_free_bytes.is_none(), "the card scan has not answered"); assert!(!*recovered_ride_offered, "no recovered ride offered this boot"); } + /// Record one [`HostEvent::StoreChanged`] fact (saturating — a burst rides as a count). + pub(crate) fn note_store_changed(&mut self) { + self.store_changed = self.store_changed.saturating_add(1); + } + /// Advance one tick from the sensors. /// /// Polls the GPS [`LocationSource`] (recenters the camera in Follow mode) and, with a route @@ -766,7 +780,7 @@ impl App { // breadcrumb), the route-length mirror, and the climbs/waypoints cache builds — is the // ride engine's; a change there (route line appeared/vanished, breadcrumb cleared) // repaints the map even on a frame with no fresh fix. - if self.ride.sync_route_state(&mut self.activity, route) { + if self.ride.sync_route_state(&mut self.activity, &mut self.navigator, route) { self.ui.map_dirty = true; } // A detour commit queues a seam re-anchor because the commit handler owns no host @@ -1180,7 +1194,7 @@ impl App { /// [`nav_arena_precondition`](App::nav_arena_precondition) hands to /// [`ArenaGate::claim_nav`](crate::arena_gate::ArenaGate::claim_nav). pub fn reroute_freeze_active(&self) -> bool { - self.freeze.active(self.ui.base_draws_map()) + self.navigator.freeze_active(self.ui.base_draws_map()) } /// Whether a host planner run is live at all — including a **menu** plan, where no freeze is @@ -1189,7 +1203,7 @@ impl App { /// gate's search arm, [`TransferGate::begin_search`](crate::TransferGate::begin_search)) read /// this rather than the freeze. pub fn plan_in_flight(&self) -> bool { - self.freeze.plan_live() + self.navigator.plan_live() } /// The proof that the map plane is quiesced, minted from this app's own state — `None` when a @@ -1337,13 +1351,16 @@ impl App { /// `old_ids` → that id's new index (or `None` if the route vanished). See /// [`set_routes_with_ids`](App::set_routes_with_ids). fn remap_route_indices(&mut self, old_ids: &[crate::CatalogObjectId]) { - let App { catalogs, ride, activity, ui, .. } = self; + let App { catalogs, ride, activity, ui, navigator, .. } = self; let remap = |i: usize| -> Option { catalogs.remap_route(old_ids, i) }; // The navigated route + every ride-engine cache keyed on it follow the identity together // (survives → nothing resets; vanished → navigation unloads and the stale per-route state // drops with it) — the ride engine owns that walk. ride.remap_route_keys(activity, &remap); + // The undelivered detour request follows the same durable identity as the route it plans + // around, or is dropped with it — Navigator's half of the same walk. + navigator.remap_detour_route(&remap); // Every screen on the stack that holds a catalog index. The Route menu also takes the // re-resolved trips (`replace_routes` re-filed them before returning) + the new route count @@ -1551,12 +1568,12 @@ impl App { if self.passkey_card_up() || self.ui.hold_charging() || dfu_screen_up - || self.activity.has_dfu_request() + || self.dfu.request_pending() || self.activity.is_tracking() { return false; } - self.activity.request_dfu(crate::activity::DfuAction::Scan); + self.dfu.admit_intent(crate::dfu::DfuIntent::ScanRequested); let r = self.ui.stack.push(Screen::DfuCheck(crate::screen::DfuCheckScreen::new())); debug_assert!(r.is_ok(), "screen stack overflow — raise MAX_DEPTH"); self.ui.map_dirty = true; @@ -1577,7 +1594,7 @@ impl App { if self.ui.stack.iter().any(|s| matches!(s, Screen::NavPlanning(_))) { return false; } - self.activity.request_nav(crate::activity::NavRequest::new(from, to, name)); + self.admit_navigator_intent(NavigatorIntent::PlanRoute(crate::activity::NavRequest::new(from, to, name))); let _ = self.ui.stack.push(Screen::NavPlanning(crate::screen::NavPlanningScreen::new(name))); self.ui.map_dirty = true; true @@ -1595,10 +1612,8 @@ impl App { /// No production path reaches it. Stands in for a [`Route`](PlanFamily::Route) run, so a stray /// detour edge cannot release it (see [`PlanFamily`]). pub fn debug_set_plan_live(&mut self, live: bool) { - if live { - self.note_plan_started(PlanFamily::Route); - } else { - self.note_plan_ended(PlanFamily::Route); + if self.navigator.debug_set_plan_live(live) { + self.ui.map_dirty = true; } } @@ -1749,7 +1764,7 @@ impl App { self.weather.mark_fired(&c, &mut self.settings.weather_alert_marks); // The mark must survive the next boot: arm the #810 persistence handshake // exactly like a rider edit (alert-fire rate, so the write cost is negligible). - self.host.note_settings_edited(); + self.settings_ops.note_edited(); } } AlertAction::Update(c) => { @@ -1775,32 +1790,126 @@ impl App { self.ui.next_ahead.invalidate(); } - /// Engage the Recalculating freeze (issue #1146, P2) — the host begins a planner run this pass. - /// Nothing is dirtied here, and both halves of that are deliberate. The *map* isn't, because the - /// host is about to stop redrawing it and a demand raised now would be drained and lost. The - /// *overlay* isn't either, because a plan start is not the banner's edge: this plan may well have - /// begun under the opaque planning spinner, where there is no map to freeze and no banner to - /// draw. [`take_dirty`](App::take_dirty) derives that edge from the engaged level instead. - fn note_plan_started(&mut self, family: PlanFamily) { - self.freeze.plan_started(family); + /// Hand one rider request to Navigator, and repaint. + /// + /// The map is dirtied on any navigation intent because every screen that produces one is + /// changing what the rider is looking at. A plan **start** deliberately dirties nothing extra: + /// the executor is about to stop redrawing the map, and the banner's edge is the engaged + /// *level*, which [`take_dirty`](App::take_dirty) derives (a plan begun under the opaque + /// planning spinner freezes nothing at all). + pub(crate) fn admit_navigator_intent(&mut self, intent: NavigatorIntent) { + let planned = self.navigator.detour_planned(); + self.navigator.admit_intent(intent); + self.sync_detour_preview(planned); + self.ui.map_dirty = true; + } + + /// Drop the detour preview polyline when Navigator drops the plan it previews. + /// + /// The shape is *derived* from the plan: it is drawn over the still-active route, so a preview + /// of a detour that no longer exists is a line to nowhere. `was_planned` is the level from + /// before the intent, so this fires on the falling edge and never on a boot with nothing cached. + fn sync_detour_preview(&mut self, was_planned: bool) { + if was_planned && !self.navigator.detour_planned() { + self.catalogs.clear_detour_preview(); + } + } + + /// Hand `family`'s undelivered cancellation to the executor, and repaint the map the freeze + /// held still. `false` when there was nothing to deliver. + fn deliver_plan_cancel(&mut self, family: PlanFamily) -> bool { + if !self.navigator.take_cancel(family) { + return false; + } + if self.navigator.note_cancel_delivered(family) { + self.ui.map_dirty = true; + } + true + } + + /// Consume a typed [`NavigatorOutcome`](crate::navigator::NavigatorOutcome). The token is the + /// whole admission test: a cancelled or superseded operation refuses its own late answer, and + /// nothing downstream runs. + /// + /// What each accepted answer *means* to the rider is the same code the legacy events reach — + /// there is one `land_*` per product event, not one per protocol. + pub(crate) fn apply_navigator_outcome(&mut self, outcome: crate::navigator::NavigatorOutcome) { + use crate::navigator::{NavigatorError, NavigatorOutcome}; + if !self.navigator.accepts(&outcome) { + return; + } + match outcome { + NavigatorOutcome::PlanFinished { route, .. } => self.land_route_plan(Ok(route)), + NavigatorOutcome::DetourFinished { preview, .. } => self.land_detour_plan(Ok(preview)), + NavigatorOutcome::DetourCommitted { route, .. } => self.land_detour_commit(Ok(route)), + NavigatorOutcome::Failed { error, .. } => { + // The planner's own verdict is the one the rider is shown; the two resource + // failures have no tier of their own and land on the generic card, which is what + // the legacy protocol has always done with them. + let error = match error { + NavigatorError::Plan(error) => error, + NavigatorError::Workspace | NavigatorError::Store => obc_route::nav::NavError::NoPath, + }; + match self.navigator.live_family() { + Some(PlanFamily::Detour) if self.navigator.detour_committing() => { + self.land_detour_commit(Err(error)) + } + Some(PlanFamily::Detour) => self.land_detour_plan(Err(error)), + _ => self.land_route_plan(Err(error)), + } + } + // The workspace came back, or the operation was abandoned: the run is over and the + // freeze must not outlive it, but there is nothing new to put in front of the rider. + NavigatorOutcome::Released { .. } | NavigatorOutcome::Cancelled { .. } => { + let family = self.navigator.live_family().unwrap_or(PlanFamily::Route); + self.end_plan(family, PlanPhase::Idle); + } + // Pacing is `LegacyOwned::PlannerPacing` — one legacy `PlanRoute` acquires, steps and + // commits inside the executor, so no protocol in this slice produces these. They are + // accepted and change nothing until #1397 S6 gives Navigator the stepping loop. + NavigatorOutcome::Acquired { .. } | NavigatorOutcome::Stepped { .. } => {} + } + } + + /// Consume a typed [`DfuOutcome`](crate::dfu::DfuOutcome) — the same terminal cards the legacy + /// events post, behind the token that says this answer is still the phase being waited for. + pub(crate) fn apply_dfu_outcome(&mut self, outcome: crate::dfu::DfuOutcome) { + use crate::dfu::DfuOutcome; + if !self.dfu.accepts(&outcome) { + return; + } + self.dfu.note_answer(); + match outcome { + DfuOutcome::ScanFinished { report, .. } => self.post_dfu_landing(DfuLanding::Scanned(Ok(report))), + DfuOutcome::ScanFailed { error, .. } => self.post_dfu_landing(DfuLanding::Scanned(Err(error))), + DfuOutcome::InstallBegan { .. } => self.post_dfu_landing(DfuLanding::InstallBegan), + DfuOutcome::InstallFailed { error, .. } => self.post_dfu_landing(DfuLanding::InstallFailed(error)), + // An abandoned phase leaves the rider where they were: the wait screen is still up and + // the menu still works, which is more honest than a failure card for work never done. + DfuOutcome::Cancelled { .. } => {} + } } - /// Release the freeze — the run answered, failed, or was cancelled. Dirties the **map**: it held - /// still for the whole search and has a fix, a route, or a whole new geometry to catch up on. The - /// banner comes off with the same [`take_dirty`](App::take_dirty) level edge that put it up. - /// Idempotent: several release edges can land for one run. - fn note_plan_ended(&mut self, family: PlanFamily) { - if self.freeze.plan_ended(family) { + /// Note a terminal planner answer for `family` and repaint the map the freeze held still. + /// + /// Dirties the **map**: it held still for the whole search and has a fix, a route, or a whole + /// new geometry to catch up on. The banner comes off with the same + /// [`take_dirty`](App::take_dirty) level edge that put it up. Idempotent: several release edges + /// can land for one run. + fn end_plan(&mut self, family: PlanFamily, phase: PlanPhase) { + if self.navigator.note_answer(family, phase) { self.ui.map_dirty = true; } } - /// [`HostEvent::NavPlanned`]: land the plan answer in the planning screen, or drop it. - fn on_nav_planned(&mut self, result: Result) { + /// The UI's reaction to Navigator finishing a **route** plan: land it in the planning screen, + /// or drop it. Not a protocol handler — both protocols reach it through Navigator, which has + /// already decided that this answer is the one being waited for. + fn land_route_plan(&mut self, result: Result) { use obc_route::nav::NavError; // The run is over whatever happens below — including for a *late* answer whose planning // screen the rider already cancelled away, which returns early two lines down. - self.note_plan_ended(PlanFamily::Route); + self.end_plan(PlanFamily::Route, if result.is_ok() { PlanPhase::Active } else { PlanPhase::Failed }); let Some(i) = self.ui.stack.iter().position(|s| matches!(s, Screen::NavPlanning(_))) else { return; }; @@ -1838,9 +1947,10 @@ impl App { /// [`set_detour_preview`](App::set_detour_preview)), failure with the fail card carrying the /// "try a farther rejoin" hint. A late answer whose planning screen is gone (the rider /// cancelled) is dropped, and the stale preview slot cleared. - fn on_detour_planned(&mut self, result: Result) { + fn land_detour_plan(&mut self, result: Result) { use obc_route::nav::NavError; - self.note_plan_ended(PlanFamily::Detour); // the run is over — see `on_nav_planned` for the late-answer case + // The run is over — see `land_route_plan` for the late-answer case. + self.end_plan(PlanFamily::Detour, if result.is_ok() { PlanPhase::PreviewReady } else { PlanPhase::Failed }); let Some(i) = self .ui .stack @@ -1875,7 +1985,8 @@ impl App { /// RouteSwap precedent), queue the seam re-anchor (the tick that owns the `RouteReader` /// installs matcher progress + floor at the splice seam), then truncate the detour flow off /// the stack so the rider lands on the exact riding view they left. - fn on_detour_committed(&mut self, result: Result) { + fn land_detour_commit(&mut self, result: Result) { + self.navigator.note_commit(result.is_ok()); let resolved = result.and_then(|id| self.catalogs.route_index_of(id).ok_or(obc_route::nav::NavError::NoPath)); match resolved { Ok(idx) => { @@ -2087,7 +2198,7 @@ impl App { /// resets it — there is no other way to *observe* the accumulated burst without consuming it, so /// the `ble`/screen seam tests read it here. Kept deliberately; not part of the host protocol. pub fn store_changed_pending(&self) -> u32 { - self.host.store_changed_pending() + self.store_changed } /// [`HostEvent::RouteUploaded`]: forced adoption on an active replace + the advisory prompt. @@ -2291,7 +2402,7 @@ impl App { // revision handshake to Clean. Any pending edit is discarded — seeding is a boot/reload // operation, not a rider edit (the BLE-merge path uses `merge_ble_settings`, which preserves // a pending device-edit save). - self.host.reset_settings_clean(); + self.settings_ops.note_seeded(); } /// Merge the BLE-owned fields (units + device name) of a phone Config write into the live @@ -2373,7 +2484,7 @@ impl App { let epoch = self.ui.now_ms.wrapping_sub(second as u32 * 1000); self.wall_clock.set(self.settings.local_clock(), epoch); if first_trusted_this_boot || self.settings.utc_offset_min != offset_before { - self.host.note_settings_edited(); + self.settings_ops.note_edited(); } self.clock_trust = source; } @@ -2497,14 +2608,14 @@ impl App { /// the rider has left the settings subtree, and we are neither already Awaiting an ack nor inside /// a failed-write backoff window. The shared predicate behind the `PersistSettings` peek/drain. fn settings_persist_ready(&self) -> bool { - self.host.settings_persist_ready(self.ui.top_is_settings(), self.ui.now_ms) + self.settings_ops.wants_write(self.ui.top_is_settings(), self.ui.now_ms) } /// Test hook: arm a pending settings save without driving a real edit (bumps the revision and /// marks Dirty), standing in for a settings-screen edit the drain/gating tests don't replay. #[cfg(test)] fn arm_settings_save(&mut self) { - self.host.arm_settings_save(); + self.settings_ops.arm_save(); } /// Whether the top screen would draw a live **hold fill** for its current selection/state — @@ -2595,11 +2706,17 @@ impl App { // Snapshot the settings so a settings-screen edit is detected by one `==` (Settings is // `Copy + Eq`). A change flags a save for the host to pick up via `take_settings_dirty`. let settings_before = self.settings; - let App { state, activity, settings, catalogs, nav_profiles, ride, ui, .. } = self; + // Navigator's detour level before the screen speaks, so a cancellation it admits takes the + // preview polyline with it (see `sync_detour_preview`). + let detour_planned_before = self.navigator.detour_planned(); + let App { state, activity, settings, catalogs, nav_profiles, ride, ui, navigator, dfu, storage, .. } = self; let mut cx = Ctx { state, activity, settings, + navigator, + dfu, + storage, routes: catalogs.routes(), rides: catalogs.rides(), trips: catalogs.trips(), @@ -2621,6 +2738,7 @@ impl App { screen::Transition::Pop | screen::Transition::Home => depth_before > 1, screen::Transition::Push(_) | screen::Transition::Replace(_) | screen::Transition::Root(_) => true, }; + self.sync_detour_preview(detour_planned_before); screen::apply(&mut self.ui.stack, t); // Opening a POI list drops any previous snapshot so its first draw re-queries at the current // fix — the "re-enter to refresh" contract (issue #425). Gated on this being a fresh open @@ -2653,8 +2771,8 @@ impl App { } if self.settings != settings_before { // A rider edit: bump the revision and (re-)arm the save — superseding any in-flight or - // backing-off older revision (#810); see `HostPending::note_settings_edited`. - self.host.note_settings_edited(); + // backing-off older revision (#810); see `SettingsMachine::note_edited`. + self.settings_ops.note_edited(); // A change to the *local* set-point re-stamps the wall clock so Home shows the new local // time: the only settings-screen edit that shifts it now is a UTC-offset step (manual // date/time editing was removed in #641). It does **not** touch `clock_trust` — nudging @@ -3005,7 +3123,7 @@ impl App { fw_version, map_name, map_obcm_version, - card_free_bytes, + storage, .. } = self; // The shape previews draw only for the subject they were decimated for — a stale key @@ -3063,7 +3181,7 @@ impl App { fw_version: fw_version.as_str(), map_name: map_name.as_str(), map_obcm_version: *map_obcm_version, - card_free_bytes: *card_free_bytes, + card_free_bytes: storage.free_bytes(), weather: weather.snapshot, weather_refreshing: weather.refreshing, travel_deg: ride.travel_deg, @@ -3141,7 +3259,7 @@ impl App { /// the banner appear on the pass a map base lands back under a search that is still running — /// see [`RerouteFreeze::take_engaged_edge`](crate::reroute_freeze::RerouteFreeze::take_engaged_edge). pub fn take_dirty(&mut self) -> Dirty { - if self.freeze.take_engaged_edge(self.ui.base_draws_map()) { + if self.navigator.take_freeze_edge(self.ui.base_draws_map()) { self.ui.overlay_edge = true; } self.ui.take_dirty() @@ -3243,41 +3361,52 @@ impl App { return; } match event { - HostEvent::StoreChanged => self.host.note_store_changed(), + HostEvent::StoreChanged => self.note_store_changed(), HostEvent::RouteUploaded { id, replaced, elevation } => self.on_route_uploaded(id, replaced, elevation), HostEvent::TripUploaded { id, replaced } => self.on_trip_uploaded(id, replaced), HostEvent::Warning(flags) => self.on_warning(flags), - HostEvent::NavPlanned(result) => self.on_nav_planned(result), - HostEvent::DetourPlanned(result) => self.on_detour_planned(result), - HostEvent::DetourCommitted(result) => self.on_detour_committed(result), + HostEvent::NavPlanned(result) => self.land_route_plan(result), + HostEvent::DetourPlanned(result) => self.land_detour_plan(result), + HostEvent::DetourCommitted(result) => self.land_detour_commit(result), HostEvent::CardScanned { free_bytes } => { - self.card_free_bytes = free_bytes; - self.ui.map_dirty = true; + // The legacy `None` folded "no medium" and "the scan failed" into one value; the + // domain separates them, and an unqualified `None` is the failed scan. + self.land_card_scan(free_bytes); + } + HostEvent::DfuScanned(result) => { + self.dfu.note_answer(); + self.post_dfu_landing(DfuLanding::Scanned(result)); + } + HostEvent::DfuInstallFailed(reason) => { + self.dfu.note_answer(); + self.post_dfu_landing(DfuLanding::InstallFailed(reason)); + } + HostEvent::DfuInstallBegan => { + self.dfu.note_answer(); + self.post_dfu_landing(DfuLanding::InstallBegan); } - HostEvent::DfuScanned(result) => self.post_dfu_landing(DfuLanding::Scanned(result)), - HostEvent::DfuInstallFailed(reason) => self.post_dfu_landing(DfuLanding::InstallFailed(reason)), - HostEvent::DfuInstallBegan => self.post_dfu_landing(DfuLanding::InstallBegan), HostEvent::UpdateConfirmed(version) => self.post_boot_update(BootUpdate::Confirmed(version)), HostEvent::UpdateFailed { why, staged } => self.post_boot_update(BootUpdate::Failed(why, staged)), - HostEvent::SettingsPersisted { revision } => self.on_settings_persisted(revision), - HostEvent::SettingsPersistFailed { revision, error } => self.on_settings_persist_failed(revision, error), + HostEvent::SettingsPersisted { revision } => self.settings_ops.note_persisted(revision), + HostEvent::SettingsPersistFailed { revision, error: _ } => self.land_settings_failure(revision), } } - /// [`HostEvent::SettingsPersisted`]: the host durably wrote `revision`. Clear to Clean **only** - /// when it is still the latest — a stale ack (a newer edit already moved us back to Dirty) is - /// ignored, so the newer content stays pending. Revision equality is the supersede guard (#810). - fn on_settings_persisted(&mut self, revision: u16) { - self.host.on_settings_persisted(revision); + /// The product half of a settings write's failure, whichever protocol carried it: + /// [`SettingsMachine`](crate::settings::SettingsMachine) decides what the answer means, and + /// what is left here is the one thing it cannot do — telling the rider a save failed, on the + /// shared advisory warning card (#810). + fn land_settings_failure(&mut self, revision: u16) { + if self.settings_ops.note_persist_failed(revision, self.ui.now_ms) { + self.on_warning(WarningFlags::SETTINGS_ERROR); + } } - /// [`HostEvent::SettingsPersistFailed`]: the write for `revision` failed. Keep the revision dirty - /// and re-arm a bounded backoff (retried on a later frame that runs anyway — no idle wake), but - /// only when it is still the in-flight latest; a stale failure is ignored. Surface the failure on - /// the shared advisory warning card so it is more than a log line (#810). - fn on_settings_persist_failed(&mut self, revision: u16, _error: obc_ports::SettingsSaveError) { - self.host.on_settings_persist_failed(revision, self.ui.now_ms); - self.on_warning(WarningFlags::SETTINGS_ERROR); + /// The product half of a free-space measurement's answer. The figure is StorageInfo's; the + /// repaint is the System screen's. + fn land_card_scan(&mut self, free_bytes: Option) { + self.storage.note_measured(free_bytes); + self.ui.map_dirty = true; } /// Non-consuming per-class pendency for the drain's backpressure check. For delete classes this @@ -3285,9 +3414,9 @@ impl App { /// peeks `true` and then drains to nothing. fn peek_host_command(&self, class: HostCommandClass) -> bool { match class { - HostCommandClass::RescanStore => self.host.store_changed_pending() > 0, - HostCommandClass::CancelRoutePlan => self.activity.nav_cancel_pending(), - HostCommandClass::CancelDetour => self.activity.detour_cancel_pending(), + HostCommandClass::RescanStore => self.store_changed > 0, + HostCommandClass::CancelRoutePlan => self.navigator.cancel_pending(PlanFamily::Route), + HostCommandClass::CancelDetour => self.navigator.cancel_pending(PlanFamily::Detour), // The delete classes are pended by either the UI hold-to-delete (an Activity slot) or the // auto-expiry sweep (a queued action) — the host handles both identically (#638 S3). HostCommandClass::DeleteRoute => { @@ -3300,13 +3429,13 @@ impl App { HostCommandClass::StampRouteUsed => self.retention.has(crate::retention::SweepKind::StampRoute), HostCommandClass::StampRideSynced => self.retention.has(crate::retention::SweepKind::StampRide), HostCommandClass::FinishTrack => self.activity.has_track_action(), - HostCommandClass::PlanRoute => self.activity.has_nav_request(), - HostCommandClass::PlanDetour => self.activity.has_detour_request(), - HostCommandClass::CommitDetour => self.activity.detour_commit_pending(), - HostCommandClass::Dfu => self.activity.has_dfu_request(), + HostCommandClass::PlanRoute => self.navigator.request_pending(PlanFamily::Route), + HostCommandClass::PlanDetour => self.navigator.request_pending(PlanFamily::Detour), + HostCommandClass::CommitDetour => self.navigator.commit_pending(), + HostCommandClass::Dfu => self.dfu.request_pending(), HostCommandClass::ForgetBond => self.state.ble_forget_pending, HostCommandClass::PersistSettings => self.settings_persist_ready(), - HostCommandClass::ScanCardFree => self.activity.card_scan_pending(), + HostCommandClass::ScanCardFree => self.storage.refresh_pending(), HostCommandClass::LoadRideTrack => self.ride_track_request().is_some(), HostCommandClass::RefreshNavPreview => self.nav_preview_missing(), } @@ -3320,21 +3449,20 @@ impl App { fn drain_host_command(&mut self, class: HostCommandClass) -> Option { match class { HostCommandClass::RescanStore => { - let commits = self.host.take_store_changed(); + let commits = core::mem::take(&mut self.store_changed); (commits > 0).then_some(HostCommand::RescanStore { commits }) } - HostCommandClass::CancelRoutePlan => self.activity.take_nav_cancel().then(|| { - // The host is being told to drop the planner: the run is over, so the freeze - // releases here rather than on an answer that will never come (#1146 P2). - self.note_plan_ended(PlanFamily::Route); - HostCommand::CancelRoutePlan - }), - HostCommandClass::CancelDetour => self.activity.take_detour_cancel().then(|| { - // The preview polyline dies with the plan it previewed. - self.catalogs.clear_detour_preview(); - self.note_plan_ended(PlanFamily::Detour); - HostCommand::CancelDetour - }), + // Navigator decided the cancellation when the rider asked for it — the request + // annihilated, the token invalidated, the preview dropped. All that is left here is the + // command the legacy protocol spells it with, which `LegacyOwned::PlannerRelease` + // refuses to derive from the domain's `Release` effect (that one is also issued on + // success). + HostCommandClass::CancelRoutePlan => { + self.deliver_plan_cancel(PlanFamily::Route).then_some(HostCommand::CancelRoutePlan) + } + HostCommandClass::CancelDetour => { + self.deliver_plan_cancel(PlanFamily::Detour).then_some(HostCommand::CancelDetour) + } HostCommandClass::DeleteRoute => { // The UI hold-to-delete (index-resolved to a durable id) takes priority; a retention // delete drains after it, re-validated against live state and one in flight at a time @@ -3356,33 +3484,27 @@ impl App { HostCommandClass::StampRouteUsed => self.retention_stamp_command(crate::retention::SweepKind::StampRoute), HostCommandClass::StampRideSynced => self.retention_stamp_command(crate::retention::SweepKind::StampRide), HostCommandClass::FinishTrack => self.activity.take_track_action().map(HostCommand::FinishTrack), - // The two plan commands are the freeze's engaging edge: draining one is the moment the - // host actually begins a planner run (a request the rider cancelled first is - // annihilated in `Activity` and never drains, so it never freezes anything). - HostCommandClass::PlanRoute => self.activity.take_nav_request().map(|req| { - self.note_plan_started(PlanFamily::Route); - HostCommand::PlanRoute(req) - }), - HostCommandClass::PlanDetour => self.activity.take_detour_request().map(|req| { - self.note_plan_started(PlanFamily::Detour); - HostCommand::PlanDetour(req) - }), - HostCommandClass::CommitDetour => self.activity.take_detour_commit().then_some(HostCommand::CommitDetour), - HostCommandClass::Dfu => self.activity.take_dfu_request().map(HostCommand::Dfu), + // The three planner commands are Navigator's own effects, spelled in the old + // vocabulary: the domain decides that work is owed and engages the freeze as it hands + // the operation over, and `navigator_row` names the command each effect becomes. + HostCommandClass::PlanRoute => { + self.navigator.next_plan_effect(PlanFamily::Route).and_then(|e| navigator_row(e).command()) + } + HostCommandClass::PlanDetour => { + self.navigator.next_plan_effect(PlanFamily::Detour).and_then(|e| navigator_row(e).command()) + } + HostCommandClass::CommitDetour => { + self.navigator.next_commit_effect().and_then(|e| navigator_row(e).command()) + } + HostCommandClass::Dfu => self.dfu.next_effect().and_then(|e| dfu_row(e).command()), HostCommandClass::ForgetBond => { core::mem::take(&mut self.state.ble_forget_pending).then_some(HostCommand::ForgetBond) } HostCommandClass::PersistSettings => { - // Emit the current revision and await its ack — the pending state is *not* cleared - // (the #810 fix): a failed write must keep the revision retryable, so Clean is - // reached only by a matching `SettingsPersisted` ack in `apply_event`. - self.host - .drain_settings_persist(self.ui.top_is_settings(), self.ui.now_ms) - .map(|revision| HostCommand::PersistSettings { revision }) - } - HostCommandClass::ScanCardFree => { - self.activity.take_card_scan_request().then_some(HostCommand::ScanCardFree) + let (in_subtree, now_ms) = (self.ui.top_is_settings(), self.ui.now_ms); + self.settings_ops.next_effect(in_subtree, now_ms).and_then(|e| settings_row(e).command()) } + HostCommandClass::ScanCardFree => self.storage.next_effect().and_then(|e| storage_info_row(e).command()), HostCommandClass::LoadRideTrack => self.ride_track_request().map(|id| HostCommand::LoadRideTrack { id }), HostCommandClass::RefreshNavPreview => self.nav_preview_missing().then_some(HostCommand::RefreshNavPreview), } @@ -3505,7 +3627,7 @@ impl App { mod tests { use super::*; use crate::device_core::derived::{DerivedInput, DerivedInputs, DerivedTargets}; - use crate::host::SETTINGS_RETRY_BACKOFF_MS; + use crate::settings::SETTINGS_RETRY_BACKOFF_MS; use obc_ports::{CompassSource, LocationSource}; /// A location source that yields one fix then runs dry (so a single `tick` integrates it). @@ -5025,12 +5147,12 @@ mod tests { assert_eq!(app.activity.active_route, Some(2), "active navigation followed Beta to index 2"); app.apply_gesture(Gesture::Press); - assert_eq!(app.activity.pending_detour_request().unwrap().route, 2, "the open chooser followed Beta too"); + assert_eq!(app.navigator.pending_detour_request().unwrap().route, 2, "the open chooser followed Beta too"); // Before the host drains the request, another rescan moves Beta again. app.set_routes_with_ids(&[summary("Beta"), summary("Gamma"), summary("Alpha")], &[20, 30, 10]); assert_eq!(app.activity.active_route, Some(0)); - assert_eq!(app.activity.pending_detour_request().unwrap().route, 0, "the queued plan request follows Beta"); + assert_eq!(app.navigator.pending_detour_request().unwrap().route, 0, "the queued plan request follows Beta"); } #[test] @@ -5040,13 +5162,13 @@ mod tests { assert_eq!(open.activity.active_route, None, "vanished Beta unloads navigation"); open.apply_gesture(Gesture::Press); assert!(matches!(open.top_screen(), Screen::Detour(_)), "an unavailable chooser stays safely cancellable"); - assert!(open.activity.pending_detour_request().is_none(), "it never retargets the route now at old index 1"); + assert!(open.navigator.pending_detour_request().is_none(), "it never retargets the route now at old index 1"); let mut queued = app_with_detour_chooser_on_beta(); queued.apply_gesture(Gesture::Press); - assert!(queued.activity.pending_detour_request().is_some()); + assert!(queued.navigator.pending_detour_request().is_some()); queued.set_routes_with_ids(&[summary("Alpha"), summary("Gamma")], &[10, 30]); - assert!(queued.activity.pending_detour_request().is_none(), "a queued plan for vanished Beta is cancelled"); + assert!(queued.navigator.pending_detour_request().is_none(), "a queued plan for vanished Beta is cancelled"); } /// Drive the active-climb state directly through `App::update_active_climb` with a controlled @@ -5162,6 +5284,44 @@ mod tests { ); } + /// The preview polyline is *derived* from the detour plan, so Back on the preview takes it with + /// the plan it previewed. It is drawn over the still-active route: a shape that outlived its + /// detour is a line to nowhere, and the rider would be looking at a turn nobody is going to + /// make. + #[test] + fn cancelling_a_detour_drops_its_preview_polyline() { + use crate::screen::{DetourPreviewScreen, DetourScreen}; + let mut app = App::new(AppState::new(0, 0, 1.0)); + app.set_routes_with_ids(&[summary("Road")], &[7]); + app.state.has_nav_graph = true; + app.state.user_fix = Some(Fix { lon: 7_800_000, lat: 48_000_000, course: None, speed_mps: None }); + app.activity.active_route = Some(0); + app.activity.progress_m = 1_000; + app.activity.route_total_m = 20_000; + app.activity.start_session(); + + // Plan a detour and land its preview, exactly as the flow does. + let chooser = DetourScreen::new(&app.activity); + let preview = + crate::host::DetourPreview { cost_delta_m: 420, total_distance_m: 1_220, rejoin_m: 2_000, ascent_m: None }; + app.admit_navigator_intent(NavigatorIntent::PlanDetour(crate::activity::DetourRequest { + route: 0, + from: (7_800_000, 48_000_000), + progress_m: 1_000, + target_m: 1_800, + })); + let _ = app.ui.stack.push(Screen::Detour(chooser)); + let _ = app.ui.stack.push(Screen::DetourPreview(DetourPreviewScreen::new(&chooser, preview))); + app.set_detour_preview(&[(7_812_000, 48_001_000), (7_816_000, 48_001_000)]); + assert!(!app.catalogs.detour_preview_for(Some(0)).is_empty(), "the host's shape is cached"); + + app.apply_gesture(Gesture::Back); // the rider drops the detour + assert!( + app.catalogs.detour_preview_for(Some(0)).is_empty(), + "and the shape goes with the plan, not one frame later" + ); + } + /// The Detour chooser is map-backed and live, but it is an interaction in progress rather /// than an auto-switch sibling. A climb entry must preserve both the chooser and its /// selected distance. @@ -5184,7 +5344,7 @@ mod tests { assert!(matches!(app.top_screen(), Screen::Detour(_)), "climb entry preserves the open chooser"); app.apply_gesture(Gesture::Press); - let req = app.activity.pending_detour_request().expect("the preserved chooser still plans"); + let req = app.navigator.pending_detour_request().expect("the preserved chooser still plans"); assert_eq!((req.route, req.target_m), (0, 5_800), "the 800 m selection survives the climb edge"); } @@ -5531,7 +5691,7 @@ mod tests { use crate::activity::DfuAction; let mut app = App::new_idle(AppState::new(0, 0, 1.0)); assert_eq!(drain_dfu(&mut app), None, "nothing pending at boot"); - app.activity.request_dfu(DfuAction::Install); + app.dfu.admit_intent(crate::dfu::DfuIntent::InstallRequested); assert_eq!(drain_dfu(&mut app), Some(DfuAction::Install), "the posted request drains"); assert_eq!(drain_dfu(&mut app), None, "…exactly once"); } @@ -5687,21 +5847,21 @@ mod tests { // Every class, posted through the same doors the UI / hosts use (arrival order shuffled // on purpose — the drain order is the class order, not arrival). app.state.ble_forget_pending = true; - app.activity.request_card_scan(); + app.storage.admit_intent(crate::device_core::storage_info::StorageInfoIntent::RefreshRequested); app.activity.request_track(TrackAction::Save); app.apply_event(crate::HostEvent::StoreChanged); app.apply_event(crate::HostEvent::StoreChanged); - app.activity.request_nav_cancel(); // posted before the plan — a later cancel annihilates it - app.activity.request_nav(NavRequest::new((0, 0), (500, 500), "To the col")); - app.activity.request_detour_cancel(); // same annihilation rule as the nav pair (#882) - app.activity.request_detour(crate::activity::DetourRequest { + app.admit_navigator_intent(NavigatorIntent::CancelPlan); // posted before the plan — a later cancel annihilates it + app.admit_navigator_intent(NavigatorIntent::PlanRoute(NavRequest::new((0, 0), (500, 500), "To the col"))); + app.admit_navigator_intent(NavigatorIntent::CancelDetour); // same annihilation rule as the nav pair (#882) + app.admit_navigator_intent(NavigatorIntent::PlanDetour(crate::activity::DetourRequest { route: 0, from: (0, 0), progress_m: 100, target_m: 800, - }); - app.activity.request_detour_commit(); - app.activity.request_dfu(DfuAction::Scan); + })); + app.admit_navigator_intent(NavigatorIntent::CommitDetour); + app.dfu.admit_intent(crate::dfu::DfuIntent::ScanRequested); app.activity.request_route_delete(1); app.activity.request_trip_delete(42); app.activity.request_ride_delete(0); @@ -5778,10 +5938,10 @@ mod tests { let mut mailbox: HostMailbox = HostMailbox::new(); // Fill the mailbox artificially by draining a command per pass without popping… - app.activity.request_nav_cancel(); + app.admit_navigator_intent(NavigatorIntent::CancelPlan); assert_eq!(app.drain_host_commands(&mut mailbox), DrainStatus::Complete); while !mailbox.is_full() { - app.activity.request_card_scan(); + app.storage.admit_intent(crate::device_core::storage_info::StorageInfoIntent::RefreshRequested); let _ = app.drain_host_commands(&mut mailbox); } // …then post a destructive command with no room left. @@ -5825,8 +5985,8 @@ mod tests { fn dfu_slot_is_most_recent_wins() { use crate::activity::DfuAction; let mut app = App::new_idle(AppState::new(0, 0, 1.0)); - app.activity.request_dfu(DfuAction::Scan); - app.activity.request_dfu(DfuAction::Install); + app.dfu.admit_intent(crate::dfu::DfuIntent::ScanRequested); + app.dfu.admit_intent(crate::dfu::DfuIntent::InstallRequested); assert_eq!(drain_dfu(&mut app), Some(DfuAction::Install), "the later phase superseded"); assert_eq!(drain_dfu(&mut app), None); } @@ -5993,8 +6153,8 @@ mod tests { // Confirm + Back in one batch → drain yields exactly [CancelRoutePlan]. let mut app = App::new_idle(AppState::new(0, 0, 1.0)); - app.activity.request_nav(NavRequest::new((0, 0), (1, 1), "A")); - app.activity.request_nav_cancel(); + app.admit_navigator_intent(NavigatorIntent::PlanRoute(NavRequest::new((0, 0), (1, 1), "A"))); + app.admit_navigator_intent(NavigatorIntent::CancelPlan); let mut mailbox: HostMailbox = HostMailbox::new(); let _ = app.drain_host_commands(&mut mailbox); assert_eq!(mailbox.pop(), Some(HostCommand::CancelRoutePlan)); @@ -6002,18 +6162,18 @@ mod tests { // Per-class drains observe the same pending state. let mut app = App::new_idle(AppState::new(0, 0, 1.0)); - app.activity.request_nav(NavRequest::new((0, 0), (1, 1), "A")); - app.activity.request_nav_cancel(); + app.admit_navigator_intent(NavigatorIntent::PlanRoute(NavRequest::new((0, 0), (1, 1), "A"))); + app.admit_navigator_intent(NavigatorIntent::CancelPlan); assert_eq!(drain_nav(&mut app), None, "annihilated before any host saw it"); assert!(drain_cancel(&mut app), "the cancel still latches (a stale cancel is a host no-op)"); // Three gestures in one batch: Back on in-flight A's spinner, confirm B, Back on B's. let mut app = App::new_idle(AppState::new(0, 0, 1.0)); - app.activity.request_nav(NavRequest::new((0, 0), (1, 1), "A")); + app.admit_navigator_intent(NavigatorIntent::PlanRoute(NavRequest::new((0, 0), (1, 1), "A"))); assert!(drain_nav(&mut app).is_some(), "the host already holds plan A"); - app.activity.request_nav_cancel(); // Back on A's spinner — nothing undrained to annihilate - app.activity.request_nav(NavRequest::new((0, 0), (2, 2), "B")); // confirm B - app.activity.request_nav_cancel(); // Back on B's spinner — annihilates the undrained B + app.admit_navigator_intent(NavigatorIntent::CancelPlan); // Back on A's spinner — nothing undrained to annihilate + app.admit_navigator_intent(NavigatorIntent::PlanRoute(NavRequest::new((0, 0), (2, 2), "B"))); // confirm B + app.admit_navigator_intent(NavigatorIntent::CancelPlan); // Back on B's spinner — annihilates the undrained B let mut mailbox: HostMailbox = HostMailbox::new(); let _ = app.drain_host_commands(&mut mailbox); assert_eq!(mailbox.pop(), Some(HostCommand::CancelRoutePlan), "one cancel: aborts the in-flight A"); diff --git a/firmware/obc-app/src/device_core/connections.rs b/firmware/obc-app/src/device_core/connections.rs index 65f7d1bb4..f83b4332b 100644 --- a/firmware/obc-app/src/device_core/connections.rs +++ b/firmware/obc-app/src/device_core/connections.rs @@ -19,6 +19,14 @@ //! the legacy drain that still performs it — provisioning for a lifecycle nobody owns, at the cost of //! destroying a rider request. #1397 S6 brings the connection back with the domain that needs it. //! +//! There is deliberately **no** `UiRuntime` → `Navigator`, `DfuState` or `StorageInfo` row either, +//! and for the opposite reason: those domains exist, so a screen names its request straight to the +//! owner as the gesture happens (`Ctx::navigator`, `Ctx::dfu`, `Ctx::storage`). That is stronger +//! than a same-pass slot — the request is with its owner before stage 1 rather than at stage 4 — +//! and it is the only shape that also works for the hosts still driving `drain_host_commands`, +//! which run no pass at all until #1397 S6. A slot beside it would be a second place a rider's plan +//! lives, which is the defect #1397 S2 exists to remove. +//! //! ## Which direction decides the timing //! //! The pass order (see [`pass`](super::pass)) is fixed, so a connection's timing is not a policy diff --git a/firmware/obc-app/src/device_core/pass.rs b/firmware/obc-app/src/device_core/pass.rs index c5421341b..da52a0247 100644 --- a/firmware/obc-app/src/device_core/pass.rs +++ b/firmware/obc-app/src/device_core/pass.rs @@ -51,13 +51,22 @@ //! executor answers through ([`App::apply_event`], [`App::apply_derived`]) refuse while a pass is in //! flight. The next pass consumes what an executor completed; nothing mutates mid-pass. //! -//! ## What this slice does not yet do +//! ## What this pass does not yet own //! -//! Three domains own an operation token today — retention, weather and the catalog — and those are -//! exactly the three whose outcomes a pass may consume: **a domain that cannot validate a token -//! cannot be the owner of an outcome** (epic §4.3). An outcome for a domain whose state machine has -//! not landed is therefore *left in its slot*, not dropped and not guessed at. The stage where that -//! machine will advance already exists and already runs, because the order is what this slice pins. +//! Seven domains own an operation token — the catalog, retention and weather, and the four #1397 S2 +//! added (Navigator, `SettingsMachine`, `DfuState`, `StorageInfo`) — and those are exactly the +//! seven whose outcomes a pass may consume: **a domain that cannot validate a token cannot be the +//! owner of an outcome** (epic §4.3). Recorder and Bond cannot: the legacy protocol answers a ride +//! close with a catalog re-feed and a bond removal with a link-status fact, so an outcome for +//! either is *left in its slot*, not dropped and not guessed at. The stages where their machines +//! will advance already exist and already run, because the order is what #1438 pinned. +//! +//! **The rider's own requests do not wait for a stage.** A screen names what it wants to the domain +//! that owns it as the gesture happens (`Ctx::navigator`, `Ctx::dfu`, `Ctx::storage`), so a plan, an +//! update phase or a free-space refresh is already with its owner before stage 1 — earlier than a +//! same-pass slot could deliver it, in exactly one place, and by the one path that also serves the +//! hosts still driving `drain_host_commands` (no host runs this pass until #1397 S6). A connection +//! into those domains would be a second copy of the pending state this slice exists to remove. use obc_ports::{InputClock, RideClock, Sensors}; use obc_route::RouteReader; @@ -306,10 +315,10 @@ impl App { self.stage_retention(&mut effects); self.stage_catalog(&mut effects); self.stage_recorder(); - self.stage_navigator(); - self.stage_settings(); + self.stage_navigator(&mut effects); + self.stage_settings(&mut effects); self.stage_weather(&mut effects); - self.stage_platform(); + self.stage_platform(&mut effects); self.stage_admission(support); self.stage_faults(); let plan = self.stage_plan(now, effects); @@ -321,9 +330,9 @@ impl App { /// Stage 1 — validate and consume each domain's outcome slot. /// /// A domain accepts an outcome only while its own [`OperationToken`](super::OperationToken) is - /// current, which is why only a domain that *owns a token source* may consume one. The rest - /// stay in their slots: an outcome nobody can validate is not something to guess at, and the - /// slot's capacity of one is the executor's backpressure until the owner lands. + /// current, which is why only a domain that *owns a token source* may consume one. Recorder's + /// and Bond's stay in their slots: an outcome nobody can validate is not something to guess at, + /// and the slot's capacity of one is the executor's backpressure until the owner lands. fn stage_outcomes(&mut self, outcomes: &mut OutcomeSlots) { self.pass.record(PassStage::Outcomes); if let Some(outcome) = outcomes.catalog.take() { @@ -335,6 +344,23 @@ impl App { if let Some(outcome) = outcomes.weather.take() { self.weather.apply_outcome(outcome); } + if let Some(outcome) = outcomes.navigator.take() { + self.apply_navigator_outcome(outcome); + } + if let Some(outcome) = outcomes.settings.take() { + let now_ms = self.ui.now_ms; + if self.settings_ops.apply_outcome(outcome, now_ms) { + self.on_warning(crate::screen::WarningFlags::SETTINGS_ERROR); + } + } + if let Some(outcome) = outcomes.dfu.take() { + self.apply_dfu_outcome(outcome); + } + if let Some(outcome) = outcomes.storage_info.take() { + if self.storage.apply_outcome(outcome) { + self.ui.map_dirty = true; + } + } } /// Stage 2 — consume external facts and the derived inputs that answer a need. @@ -349,7 +375,7 @@ impl App { if let Some(store) = facts.store_revision() { if self.pass.store != Some(store) { self.pass.store = Some(store); - self.host.note_store_changed(); + self.note_store_changed(); } } if let Some(state) = facts.transfer() { @@ -414,6 +440,10 @@ impl App { /// The rider's ride *close* is deliberately not taken here. Recorder has no machine yet, so /// there is no domain to name it to; taking the one-shot anyway would only remove it from the /// legacy drain that still performs it, and the rider's save would be destroyed on the way. + /// + /// Navigation, update and free-space requests are not taken here either, for the opposite + /// reason: their domains **do** exist, so the screens name them directly and there is nothing + /// left in `Activity` to collect (see the module docs). fn stage_ui(&mut self, now: PassClock) { self.pass.record(PassStage::Ui); self.advance_animations(now.ui); @@ -564,8 +594,13 @@ impl App { /// /// Consumes the catalog's [`ActiveRouteRemoved`] in the same pass it was sent, and reports an /// activation to retention in the next one — an active route must not expire underneath the ride - /// it is guiding. - fn stage_navigator(&mut self) { + /// it is guiding. Then hands the executor at most one planning operation. + /// + /// There is no `UiRuntime` → `Navigator` connection to drain: a planning screen names its + /// request to Navigator as it happens (`Ctx::navigator`), so the rider's plan is already with + /// its owner before stage 1 of this pass — earlier than a slot could deliver it, and in the one + /// place that also works for the hosts still driving the legacy drain. + fn stage_navigator(&mut self, effects: &mut EffectSlots) { self.pass.record(PassStage::Navigator); if let Some(removed) = self.pass.connections.active_route_removed.take() { if self.activity.active_route.and_then(|idx| self.catalogs.route_id_at(idx)) == Some(removed.route) { @@ -581,16 +616,27 @@ impl App { self.pass.connections.route_activated.defer(RouteActivated { route }); } } + if effects.navigator.is_empty() { + if let Some(effect) = self.navigator.next_effect() { + let _ = effects.navigator.try_put(effect); + } + } } - /// Stage 9 — advance `SettingsMachine`. + /// Stage 9 — advance [`SettingsMachine`](crate::settings::SettingsMachine). /// - /// The dirty revision, the debounce, the retry backoff and the stale-ack rule live in - /// [`HostPending`](crate::host::HostPending) and advance on the frame clock stage 3 set. The - /// write itself is still owed through the legacy protocol: an effect needs an operation token, - /// and the token needs the owner that #1397 lands. - fn stage_settings(&mut self) { + /// The dirty revision, the subtree debounce, the retry backoff and the stale-answer rule are the + /// domain's; what the stage supplies is the two *levels* the decision is made against — where + /// the rider is standing, and the frame clock stage 3 set. A write owed while the rider is + /// still inside the settings subtree simply is not offered. + fn stage_settings(&mut self, effects: &mut EffectSlots) { self.pass.record(PassStage::Settings); + if effects.settings.is_empty() { + let (in_subtree, now_ms) = (self.ui.top_is_settings(), self.ui.now_ms); + if let Some(effect) = self.settings_ops.next_effect(in_subtree, now_ms) { + let _ = effects.settings.try_put(effect); + } + } } /// Stage 10 — advance `WeatherDomain`: one refresh at a time, and only while the device can @@ -606,9 +652,22 @@ impl App { /// Stage 11 — advance `DfuState`, `BondState` and `StorageInfo`. /// /// The three domains whose product state is a scan result, a bond and a number of free bytes. - /// Their machines arrive with their cutovers (#1397); the stage is where they advance. - fn stage_platform(&mut self) { + /// Two of them have their machine; `BondState`'s arrives with #1397 S6, because the legacy bond + /// removal is confirmed by a link-status fact rather than by a reply — a domain with nothing to + /// validate cannot own an outcome (epic §4.3), so it keeps the legacy path and this stage does + /// not reach for it. + fn stage_platform(&mut self, effects: &mut EffectSlots) { self.pass.record(PassStage::Platform); + if effects.dfu.is_empty() { + if let Some(effect) = self.dfu.next_effect() { + let _ = effects.dfu.try_put(effect); + } + } + if effects.storage_info.is_empty() { + if let Some(effect) = self.storage.next_effect() { + let _ = effects.storage_info.try_put(effect); + } + } } /// Stage 12 — `CoreMode`: recalculate what this device can do at all. diff --git a/firmware/obc-app/src/device_core/storage_info.rs b/firmware/obc-app/src/device_core/storage_info.rs index c393722d5..65e0fbbc5 100644 --- a/firmware/obc-app/src/device_core/storage_info.rs +++ b/firmware/obc-app/src/device_core/storage_info.rs @@ -75,3 +75,144 @@ const _: () = assert!(core::mem::size_of::() == 0, "one field const _: () = assert!(core::mem::size_of::() <= 4, "a bare token"); const _: () = assert!(core::mem::size_of::() <= 16, "a token and a byte count"); const _: () = assert!(core::mem::size_of::() <= 1, "a verdict, not a report"); + +// ==================== the StorageInfo state machine (#1397 S2) ==================== + +use crate::device_core::TokenSource; + +/// The storage-information domain: the refresh request, the operation token, and the figure the +/// System screen prints. +/// +/// The free-space *number* lives here rather than on `App` because it is this domain's only +/// product state — a measurement it owns end to end, from the request the System screen makes on +/// entry to the value that replaces the `--`. +#[derive(Debug, Default)] +pub struct StorageInfo { + /// A refresh the executor has not taken yet. Idempotent by construction: a repeat while one is + /// waiting is the same request, and a repeat while one is *running* re-arms it, because free + /// space genuinely may have moved since the scan started. + requested: bool, + /// The operation token for the measurement an executor is running. + ops: TokenSource, + /// Bytes still available on the mounted medium, or `None` until a measurement succeeds — which + /// is what the System screen shows as `--`. + free_bytes: Option, +} + +impl StorageInfo { + /// The boot state: nothing requested, nothing measured. + pub(crate) const fn new() -> Self { + StorageInfo { requested: false, ops: TokenSource::new(), free_bytes: None } + } + + /// Admit a refresh. + pub(crate) fn admit_intent(&mut self, intent: StorageInfoIntent) { + match intent { + StorageInfoIntent::RefreshRequested => self.requested = true, + } + } + + /// The next bounded measurement, or `None` when none is owed. + pub(crate) fn next_effect(&mut self) -> Option { + core::mem::take(&mut self.requested).then(|| StorageInfoEffect::MeasureFreeSpace { token: self.ops.issue() }) + } + + /// Consume the answer to a measurement. A superseded or repeated answer changes nothing. + /// + /// A failure is **reported, not retried**: the screen keeps whatever it last knew (or its `--`) + /// and the rider can ask again by re-entering. A domain that re-armed itself here would turn a + /// dead card into a free-cluster walk every pass. + pub(crate) fn apply_outcome(&mut self, outcome: StorageInfoOutcome) -> bool { + if !self.ops.is_current(outcome.token()) { + return false; + } + self.ops.invalidate(); // terminal: a duplicate of this answer is no longer current + self.note_measured(match outcome { + StorageInfoOutcome::Measured { free_bytes, .. } => Some(free_bytes), + StorageInfoOutcome::Failed { .. } | StorageInfoOutcome::Cancelled { .. } => None, + }); + true + } + + /// A measurement answered with `free_bytes`, or `None` when it did not produce a figure — the + /// token-free half, for the legacy protocol that carries no token. + /// + /// A failure leaves the last known figure alone rather than blanking it: the rider is looking + /// at a number that was true, and replacing it with `--` because one scan failed says less. + pub(crate) fn note_measured(&mut self, free_bytes: Option) { + if let Some(free_bytes) = free_bytes { + self.free_bytes = Some(free_bytes); + } + } + + /// Whether a refresh is posted but undelivered — the `ScanCardFree` peek. + pub(crate) fn refresh_pending(&self) -> bool { + self.requested + } + + /// Free space on the mounted medium, or `None` until a measurement has answered. + pub(crate) fn free_bytes(&self) -> Option { + self.free_bytes + } + + /// Assert the boot state, field by field. + #[cfg(test)] + pub(crate) fn assert_boot_state(&self) { + let StorageInfo { requested, ops, free_bytes } = self; + assert!(!*requested, "no free-space refresh posted"); + assert_eq!(format!("{ops:?}"), "TokenSource(0)", "no measurement has been issued"); + assert!(free_bytes.is_none(), "the card scan has not answered"); + } +} + +// Layout tripwire: a byte count, a token and a flag. +const _: () = assert!(core::mem::size_of::() <= 24, "one byte count and a generation"); + +#[cfg(test)] +mod storage_info_tests { + use super::*; + + /// The request is an idempotent **refresh**: repeats before the measurement leaves coalesce into + /// the one that is already waiting, and the System screen re-entered while a scan is running + /// arms the next one rather than a second concurrent walk of the allocation table. + #[test] + fn the_refresh_is_idempotent() { + let mut storage = StorageInfo::new(); + storage.admit_intent(StorageInfoIntent::RefreshRequested); + storage.admit_intent(StorageInfoIntent::RefreshRequested); + assert!(storage.next_effect().is_some(), "one measurement"); + assert!(storage.next_effect().is_none(), "…not two"); + } + + /// A failed scan is a **reported** failure, not a retry loop: the last figure the rider was + /// shown stands, nothing re-arms itself, and asking again is the rider's move. + #[test] + fn a_failed_measurement_is_reported_and_not_retried() { + let mut storage = StorageInfo::new(); + storage.admit_intent(StorageInfoIntent::RefreshRequested); + let effect = storage.next_effect().expect("the measurement goes out"); + let token = effect.token(); + assert!(storage.apply_outcome(StorageInfoOutcome::Measured { token, free_bytes: 8_000 })); + assert_eq!(storage.free_bytes(), Some(8_000)); + + storage.admit_intent(StorageInfoIntent::RefreshRequested); + let effect = storage.next_effect().expect("the refresh goes out"); + let token = effect.token(); + assert!(storage.apply_outcome(StorageInfoOutcome::Failed { token, error: StorageInfoError::ScanFailed })); + assert_eq!(storage.free_bytes(), Some(8_000), "the figure that was true is not blanked by one bad scan"); + assert!(!storage.refresh_pending(), "and nothing re-armed itself"); + assert!(storage.next_effect().is_none()); + } + + /// A superseded or repeated answer changes nothing — the terminal answer invalidates the token + /// the way every domain owner must. + #[test] + fn a_repeated_answer_is_no_longer_current() { + let mut storage = StorageInfo::new(); + storage.admit_intent(StorageInfoIntent::RefreshRequested); + let token = storage.next_effect().expect("the measurement goes out").token(); + let answer = StorageInfoOutcome::Measured { token, free_bytes: 1_000 }; + assert!(storage.apply_outcome(answer)); + assert!(!storage.apply_outcome(answer), "a duplicate of a terminal answer is not current"); + } +} diff --git a/firmware/obc-app/src/dfu.rs b/firmware/obc-app/src/dfu.rs index 1a82b51b3..849dfb5ed 100644 --- a/firmware/obc-app/src/dfu.rs +++ b/firmware/obc-app/src/dfu.rs @@ -312,3 +312,133 @@ impl DfuOutcome { const _: () = assert!(core::mem::size_of::() <= 1, "two fieldless requests"); const _: () = assert!(core::mem::size_of::() <= 8, "a bare token"); const _: () = assert!(core::mem::size_of::() <= 96, "the two fixed version strings dominate"); + +// ==================== the DFU state machine (#1397 S2) ==================== + +use crate::activity::DfuAction; + +/// The update domain's own state: the single phase slot, the operation token, and whether an +/// executor is running one. +/// +/// **One phase at a time, most-recent-wins.** There is never more than one update phase in flight, +/// so a rider's later post replaces an undelivered earlier one rather than queueing behind it — a +/// scan the rider walked away from must not run after the install they asked for instead. The +/// remote BLE door is the one caller that must *not* replace: `App::open_remote_dfu_check` reads +/// [`request_pending`](DfuState::request_pending) and defers, because a phone must never displace +/// what the rider is doing on the device. +#[derive(Debug, Default)] +pub struct DfuState { + /// The rider's (or the remote door's) request, until an executor takes it. + request: Option, + /// The operation token for the phase an executor is running. + ops: crate::device_core::TokenSource, +} + +impl DfuState { + /// The boot state: nothing staged, nothing running. + pub(crate) const fn new() -> Self { + DfuState { request: None, ops: crate::device_core::TokenSource::new() } + } + + /// Admit one update request. Most-recent-wins, and superseding invalidates the older token so a + /// scan answer cannot land on the install that replaced it. + pub(crate) fn admit_intent(&mut self, intent: DfuIntent) { + self.ops.invalidate(); + self.request = Some(match intent { + DfuIntent::ScanRequested => DfuAction::Scan, + DfuIntent::InstallRequested => DfuAction::Install, + }); + } + + /// The next bounded update operation, or `None` when nothing is owed. + pub(crate) fn next_effect(&mut self) -> Option { + Some(match self.request.take()? { + DfuAction::Scan => DfuEffect::Scan { token: self.ops.issue() }, + DfuAction::Install => DfuEffect::ArmInstall { token: self.ops.issue() }, + }) + } + + /// Whether `outcome` still answers the phase the domain is waiting for. + pub(crate) fn accepts(&self, outcome: &DfuOutcome) -> bool { + self.ops.is_current(outcome.token()) + } + + /// A terminal answer landed: the phase is over, so a repeat of it is no longer current. + pub(crate) fn note_answer(&mut self) { + self.ops.invalidate(); + } + + /// Whether a request is posted but undelivered — the `Dfu` peek, and the remote door's + /// deferral gate. + pub(crate) fn request_pending(&self) -> bool { + self.request.is_some() + } + + /// Assert the boot state, field by field. + #[cfg(test)] + pub(crate) fn assert_boot_state(&self) { + let DfuState { request, ops } = self; + assert!(request.is_none(), "no update phase posted"); + assert_eq!(format!("{ops:?}"), "TokenSource(0)", "no update operation has been issued"); + } +} + +// Layout tripwire: one phase and a token. +const _: () = assert!(core::mem::size_of::() <= 8, "one phase slot and a generation"); + +#[cfg(test)] +mod dfu_state_tests { + use super::*; + + fn phase(state: &mut DfuState) -> Option { + state.next_effect().map(|effect| match effect { + DfuEffect::Scan { .. } => DfuAction::Scan, + DfuEffect::ArmInstall { .. } => DfuAction::Install, + }) + } + + /// One phase at a time, most-recent-wins: a later post replaces an undelivered earlier one, and + /// superseding invalidates the older token so a scan's answer cannot land on the install that + /// replaced it. + #[test] + fn the_phase_slot_is_most_recent_wins() { + let mut state = DfuState::new(); + state.admit_intent(DfuIntent::ScanRequested); + state.admit_intent(DfuIntent::InstallRequested); + assert_eq!(phase(&mut state), Some(DfuAction::Install), "the rider's latest is what runs"); + assert_eq!(phase(&mut state), None, "…exactly once"); + } + + /// A scan answer that belongs to a superseded phase changes nothing — the rider is not shown a + /// scan report for an update they already asked to install. + #[test] + fn an_answer_to_a_superseded_phase_is_refused() { + let mut state = DfuState::new(); + state.admit_intent(DfuIntent::ScanRequested); + let scan = state.next_effect().expect("the scan goes out"); + state.admit_intent(DfuIntent::InstallRequested); + + let report = DfuScanReport::new("v1", "v2", false); + assert!(!state.accepts(&DfuOutcome::ScanFinished { token: scan.token(), report })); + } + + /// Both terminal failures answer the phase they belong to, so the panel's card is the one the + /// rider was waiting on. + #[test] + fn a_scan_failure_and_an_install_failure_both_answer_their_own_phase() { + for (intent, failure) in [(DfuIntent::ScanRequested, 0), (DfuIntent::InstallRequested, 1)] { + let mut state = DfuState::new(); + state.admit_intent(intent); + let effect = state.next_effect().expect("the phase goes out"); + let token = effect.token(); + let outcome = if failure == 0 { + DfuOutcome::ScanFailed { token, error: DfuScanError::NotFound } + } else { + DfuOutcome::InstallFailed { token, error: DfuInstallError::NoCard } + }; + assert!(state.accepts(&outcome), "the failure answers the phase that was asked for"); + state.note_answer(); + assert!(!state.accepts(&outcome), "and a repeat of it is no longer current"); + } + } +} diff --git a/firmware/obc-app/src/host.rs b/firmware/obc-app/src/host.rs index 662c7194d..b78cec076 100644 --- a/firmware/obc-app/src/host.rs +++ b/firmware/obc-app/src/host.rs @@ -492,194 +492,8 @@ const _: () = assert!(core::mem::size_of::() <= 48, "HostCommand gr const _: () = assert!(core::mem::size_of::() <= 88, "HostEvent grew — re-check the payload budget"); // ==================== The app-side pending protocol state (FAR-09, #802) ==================== - -/// Bounded backoff before a failed settings persist may re-emit its [`HostCommand::PersistSettings`] -/// (map-plane millis, #810). Fixed and coarse: a persist failure is rare (an RRAM/file write error), -/// the value stays live in RAM meanwhile, and the retry only re-emits on a frame that runs for -/// another reason — so this paces retries without ever scheduling an idle wake. -pub(crate) const SETTINGS_RETRY_BACKOFF_MS: u32 = 2_000; - -/// Wrap-safe "deadline reached" in the persist-backoff's **u16** millisecond space (the low 16 bits -/// of map-plane millis — see [`HostPending::retry_at_ms`]): true while `now` sits in the half-window -/// at or past `deadline`. The u16 domain wraps every 65.5 s, so a frame gap longer than ~32.7 s can -/// park a due retry in the "not yet" half and slide it by up to one wrap — bounded, harmless for a -/// rare failure path, and the price of keeping the deadline to two resident bytes (#792 rule 2). -fn retry_deadline_reached(now: u16, deadline: u16) -> bool { - now.wrapping_sub(deadline) < 0x8000 -} - -/// The settings-persistence handshake (#810, FAR-17). Editing is live in RAM the instant it happens; -/// *persisting* it is an acknowledged, retryable cross-boundary conversation keyed by the monotonic -/// [`HostPending::settings_rev`]. This replaces the old fire-and-forget `settings_dirty` bool, -/// whose drain cleared the flag **before** the host wrote — so a failed RRAM/file write silently lost -/// the retry signal. -/// -/// States and transitions (all gated on leaving the settings subtree before anything is emitted): -/// - **Clean** — the live settings are persisted. An edit → **Dirty** (and bumps the revision). -/// - **Dirty** — a save is owed. Once outside the subtree, the drain emits -/// `PersistSettings { revision }` and moves to **Awaiting**. -/// - **Awaiting** — emitted and waiting for the ack; **not re-emitted** (no RRAM spam under a slow -/// host). A matching [`HostEvent::SettingsPersisted`] → **Clean**; a matching -/// [`HostEvent::SettingsPersistFailed`] → **Backoff**. An edit here → **Dirty** (supersede: the new -/// revision will re-emit; the old in-flight ack, when it lands, no longer matches and is ignored). -/// A host that drains but never acks (the web demo has no persistent store) parks here terminally -/// — by design: harmless (edits stay live in RAM and keep superseding), honest, no re-emission. -/// - **Backoff** — the last write failed; re-emits (→ Awaiting) only once `now_ms` reaches the -/// retry deadline in [`HostPending::retry_at_ms`]. An edit → **Dirty** (a fresh revision skips -/// the wait). -/// -/// The revision is the guard for superseding: an ack is honoured only when it equals the current -/// [`settings_rev`](HostPending::settings_rev). `u16` monotonic (wrapping): a false match would need -/// exactly 65,536 edits between an emit and its ack — there is only ever one revision Awaiting at a -/// time and both shipped hosts ack within a pass, so equality is exact in practice; the narrow width -/// is the epic's resident-RAM offset (#792 rule 2). -/// -/// Deliberately **fieldless** (one byte): the Backoff deadline lives in the sibling -/// [`retry_at_ms`](HostPending::retry_at_ms) field (meaningful only in Backoff), so this byte packs -/// into an existing padding hole instead of an 8-byte payload-carrying enum. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -enum PersistState { - /// The live settings are persisted at [`settings_rev`](HostPending::settings_rev). - Clean, - /// An edit changed the live settings; a save is owed once the rider leaves the settings subtree. - Dirty, - /// A `PersistSettings { revision }` was emitted for the current revision and awaits its ack. - Awaiting, - /// The last persist failed; no retry re-emits before the deadline in - /// [`retry_at_ms`](HostPending::retry_at_ms) (map-plane millis). - Backoff, -} - -/// The app-side **pending protocol state** that isn't a one-shot slot on -/// [`Activity`](crate::Activity): the counted store-changed cue and the #810 settings-persistence -/// state machine. `App`'s typed drain ([`drain_host_commands`](crate::App::drain_host_commands)) and -/// event door ([`apply_event`](crate::App::apply_event)) are the only writers, so the whole -/// pending-state discipline (counted bursts, exactly-once emits, ack-guarded clears, bounded retry -/// pacing) lives in one place. -pub(crate) struct HostPending { - /// Count of [`HostEvent::StoreChanged`] facts not yet acted on. The host drains it once per - /// pass as the counted [`HostCommand::RescanStore`]. A counter, not a bool, so a burst of - /// commits between drains is never coalesced into a single missed rescan. - store_changed: u32, - /// The revision of the live settings — bumped by every gesture whose before/after compare - /// finds a change ([`note_settings_edited`](HostPending::note_settings_edited)). Monotonic - /// (wrapping `u16` — see [`PersistState`] for why the narrow width is sound); the persistence - /// handshake keys its acks by it. Starts `0`, re-zeroed when the boot value is seeded. - settings_rev: u16, - /// The [`Backoff`](PersistState::Backoff) retry deadline — the **low 16 bits** of map-plane - /// millis; no failed persist re-emits before `now_ms` reaches it (compared wrap-safe in u16 - /// space by [`retry_deadline_reached`], which documents the bounded ≤65.5 s slide a long frame - /// gap can add). Meaningful **only** while [`persist`](HostPending::persist) is Backoff (stale - /// otherwise); split out of the enum and narrowed so the whole handshake adds five resident - /// bytes. - retry_at_ms: u16, - /// The settings-persistence handshake state (#810) — replaces the old fire-and-forget - /// `settings_dirty` bool so a failed write stays retryable and a stale ack can't clear a newer - /// edit. Starts [`Clean`](PersistState::Clean): the boot value came from the store or the - /// default. - persist: PersistState, -} - -impl HostPending { - /// The boot state: no store changes pending, settings Clean at revision 0. - pub(crate) const fn new() -> Self { - HostPending { store_changed: 0, settings_rev: 0, retry_at_ms: 0, persist: PersistState::Clean } - } - - /// Record one [`HostEvent::StoreChanged`] fact (saturating — a burst rides as a count). - pub(crate) fn note_store_changed(&mut self) { - self.store_changed = self.store_changed.saturating_add(1); - } - - /// How many store-changed facts are pending (not yet drained). Non-zero once the store has - /// moved since the last drain. - pub(crate) fn store_changed_pending(&self) -> u32 { - self.store_changed - } - - /// Drain the pending store-changed count whole (the counted `RescanStore` cue). - pub(crate) fn take_store_changed(&mut self) -> u32 { - core::mem::take(&mut self.store_changed) - } - - /// A rider edit changed the live settings: bump the revision and (re-)arm the save. Setting - /// Dirty from *any* prior state supersedes an in-flight/backoff older revision — the new - /// content re-emits, and the older ack, when it lands, no longer matches - /// [`settings_rev`](HostPending::settings_rev) and is ignored (#810). - pub(crate) fn note_settings_edited(&mut self) { - self.settings_rev = self.settings_rev.wrapping_add(1); - self.persist = PersistState::Dirty; - } - - /// The boot value was just seeded from the store (or the default): it is already persisted, so - /// reset the handshake to Clean at revision 0. Any pending edit is discarded — seeding is a - /// boot/reload operation, not a rider edit. - pub(crate) fn reset_settings_clean(&mut self) { - self.settings_rev = 0; - self.persist = PersistState::Clean; - } - - /// Whether a settings persist is owed **and** may be emitted this pass: the live value is - /// dirty, the rider has left the settings subtree (`in_settings_subtree` is the top-screen - /// gate the caller reads off the UI), and we are neither already Awaiting an ack nor inside a - /// failed-write backoff window. The shared predicate behind the `PersistSettings` peek/drain. - pub(crate) fn settings_persist_ready(&self, in_settings_subtree: bool, now_ms: u32) -> bool { - if in_settings_subtree { - return false; - } - match self.persist { - PersistState::Dirty => true, - PersistState::Backoff => retry_deadline_reached(now_ms as u16, self.retry_at_ms), - PersistState::Clean | PersistState::Awaiting => false, - } - } - - /// Emit the pending persist (→ Awaiting) and return the revision to carry on the command, or - /// `None` when nothing may be emitted this pass. The flag is *not* cleared here (the #810 - /// fix): a failed write must keep the revision retryable, so Clean is reached only by a - /// matching [`on_settings_persisted`](HostPending::on_settings_persisted) ack. - pub(crate) fn drain_settings_persist(&mut self, in_settings_subtree: bool, now_ms: u32) -> Option { - if self.settings_persist_ready(in_settings_subtree, now_ms) { - self.persist = PersistState::Awaiting; - Some(self.settings_rev) - } else { - None - } - } - - /// [`HostEvent::SettingsPersisted`]: the host durably wrote `revision`. Clear to Clean **only** - /// when it is still the latest — a stale ack (a newer edit already moved us back to Dirty) is - /// ignored, so the newer content stays pending. Revision equality is the supersede guard (#810). - pub(crate) fn on_settings_persisted(&mut self, revision: u16) { - if self.persist == PersistState::Awaiting && revision == self.settings_rev { - self.persist = PersistState::Clean; - } - } - - /// [`HostEvent::SettingsPersistFailed`]: the write for `revision` failed. Keep the revision - /// dirty and re-arm a bounded backoff (retried on a later frame that runs anyway — no idle - /// wake), but only when it is still the in-flight latest; a stale failure is ignored. The - /// caller surfaces the failure on the shared advisory warning card (#810). - pub(crate) fn on_settings_persist_failed(&mut self, revision: u16, now_ms: u32) { - if self.persist == PersistState::Awaiting && revision == self.settings_rev { - self.retry_at_ms = (now_ms as u16).wrapping_add(SETTINGS_RETRY_BACKOFF_MS as u16); - self.persist = PersistState::Backoff; - } - } - - /// Test hook: arm a pending settings save without driving a real edit, standing in for a - /// settings-screen edit the drain/gating tests don't replay. - #[cfg(test)] - pub(crate) fn arm_settings_save(&mut self) { - self.note_settings_edited(); - } - - /// Whether nothing at all is pending: no store change, settings Clean at revision 0 — the - /// [`new`](HostPending::new) state. The destructure is exhaustive, so a field added here must - /// state its empty value too. - #[cfg(test)] - pub(crate) fn is_empty(&self) -> bool { - let HostPending { store_changed, settings_rev, retry_at_ms, persist } = self; - *store_changed == 0 && *settings_rev == 0 && *retry_at_ms == 0 && *persist == PersistState::Clean - } -} +// +// What is left here is the counted store-changed cue. The #810 settings-persistence handshake moved +// to [`SettingsMachine`](crate::settings::SettingsMachine) with #1397 S2 — the domain that owns the +// values owns the write that makes them durable — and the counter itself lives on `App` beside the +// other one-field protocol levels. diff --git a/firmware/obc-app/src/navigator.rs b/firmware/obc-app/src/navigator.rs index b55109c82..a5d1d1980 100644 --- a/firmware/obc-app/src/navigator.rs +++ b/firmware/obc-app/src/navigator.rs @@ -6,8 +6,9 @@ //! old to matter. The executor is left with five bounded mechanisms: take the sources and the //! workspace, run **one** planner step, commit a route, commit a detour, give the resources back. //! -//! This module currently holds only the vocabulary. The state machine, and the cutover from -//! [`HostCommand::PlanRoute`](crate::HostCommand) and friends, arrive in later slices of #1433. +//! [`NavigatorMachine`] is that owner. It holds the rider's request until an executor takes it, the +//! [`OperationToken`] the answer must come back with, the per-family phase, and — since #1397 S2 — +//! the [`RerouteFreeze`] the planner's liveness drives. Nothing else may write any of them. //! //! Bulk stays out: the emitted OBCR bytes, the corridor blacklist and the detour preview *polyline* //! never ride an effect or an outcome. What crosses is an identity, a bounded request, and the @@ -16,8 +17,9 @@ use obc_route::nav::NavError; use crate::activity::{DetourRequest, NavRequest}; -use crate::device_core::{NavigatorTag, OperationToken}; +use crate::device_core::{NavigatorTag, OperationToken, TokenSource}; use crate::host::DetourPreview; +use crate::reroute_freeze::{PlanFamily, RerouteFreeze}; use crate::CatalogObjectId; /// What the rider (through `UiRuntime`) asks navigation to do. An intent is a *product request*: @@ -156,3 +158,545 @@ const _: () = assert!(core::mem::size_of::() <= 24, "preview f const _: () = assert!(core::mem::size_of::() <= 2, "a verdict, not a report"); const _: () = assert!(core::mem::size_of::() <= 48, "the largest planner request"); const _: () = assert!(core::mem::size_of::() <= 1, "a two-state answer"); + +// ==================== the Navigator state machine (#1397 S2) ==================== + +/// Where one planning family is in the lifecycle Navigator owns. +/// +/// Two families run this independently — a route search and a detour search take the same nav arm +/// but have their own commands, their own answers and their own failure tiers, and conflating them +/// is the #1146 regression (see [`PlanFamily`]). The route family never reaches +/// [`PreviewReady`](PlanPhase::PreviewReady) or [`Committing`](PlanPhase::Committing): a planned +/// route is adopted straight from its answer, while a detour is previewed and then spliced. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +pub(crate) enum PlanPhase { + /// Nothing asked for, nothing running. + #[default] + Idle, + /// The rider asked; no executor has taken the work yet. A cancel here **annihilates** the + /// request (#499) — the net intent is "no plan", so nothing is ever started. + Requested, + /// An executor holds the operation under the machine's current token. + Planning, + /// A detour search finished and its preview is what the rider is looking at — the phase a + /// [`CommitDetour`](NavigatorIntent::CommitDetour) is pressed from, and the one a failed splice + /// returns to. + PreviewReady, + /// The previewed detour is being spliced into the active route. + Committing, + /// The last operation was adopted: the planned route, or the spliced detour. + Active, + /// The last operation failed. Distinct from [`Idle`](PlanPhase::Idle) so "no path" is not read + /// as "nothing was ever asked". + Failed, +} + +/// The domain that owns route planning, detour planning, preview and commit. +/// +/// Everything one rider request passes through lives here and nowhere else: the undelivered +/// request, the cancel that annihilates it, the phase, the operation token, and the freeze the +/// planner's liveness engages. Both compositions reach it through the same three-method seam — +/// [`admit_intent`](Self::admit_intent), [`next_effect`](Self::next_effect), +/// `App::apply_navigator_outcome` — so the legacy drain and the pass cannot disagree about what +/// the rider asked for. +#[derive(Debug, Default)] +pub struct NavigatorMachine { + /// The one operation token. `EffectSlots::navigator` holds one effect, so Navigator has at most + /// one physical operation in flight; [`live_family`](Self::live_family) says whose it is. + ops: TokenSource, + /// Which family the current token belongs to, while an executor holds an operation. At most + /// one: `EffectSlots::navigator` carries one effect and the token source is one generation. + live: Option, + /// The **Recalculating freeze** (#1146): a live planner run over a map base stops map redraws, + /// pauses the matcher and raises the banner. Navigator is its only writer — the four scattered + /// edge calls the drain used to make are the transitions below. S5 derives it from `CoreMode` + /// and deletes the module. + freeze: RerouteFreeze, + /// The route family's phase. + route: PlanPhase, + /// The detour family's phase. + detour: PlanPhase, + /// The rider's route-plan request, until an executor takes it. + route_request: Option, + /// The rider's detour-plan request, until an executor takes it. + detour_request: Option, + /// A route-plan cancellation the executor has not been told about yet. + route_cancel: bool, + /// A detour cancellation the executor has not been told about yet. + detour_cancel: bool, + /// The previewed detour's commit, until an executor takes it. + detour_commit: bool, +} + +impl NavigatorMachine { + /// The boot state: nothing planned, nothing running, nothing frozen. + pub(crate) const fn new() -> Self { + NavigatorMachine { + ops: TokenSource::new(), + live: None, + freeze: RerouteFreeze::new(), + route: PlanPhase::Idle, + detour: PlanPhase::Idle, + route_request: None, + detour_request: None, + route_cancel: false, + detour_cancel: false, + detour_commit: false, + } + } + + // ---- the operation seam ---- + + /// Admit one rider request. Navigator decides what it means; nothing here can fail, because + /// every intent either supersedes what came before or annihilates it. + /// + /// Two rules the drain used to hide live here: + /// + /// - **Post-time annihilation** (#499): a cancel clears an undelivered request of its own + /// family, so a plan confirmed and cancelled inside one input batch nets "no plan" and no + /// executor is ever asked for a route nobody is waiting for. + /// - **Late-answer refusal**: the operation's token stops being current the instant the rider + /// walks away, so the search's eventual answer commits nothing. + /// + /// The **freeze is not touched here**. A cancellation the executor has not been handed yet has + /// not stopped anything: the search still owns the nav arm, and resuming the map plane on the + /// rider's keypress is the arena race #1146 exists to prevent. It releases at + /// [`note_cancel_delivered`](Self::note_cancel_delivered). + pub(crate) fn admit_intent(&mut self, intent: NavigatorIntent) { + match intent { + NavigatorIntent::PlanRoute(request) => { + self.supersede(PlanFamily::Route); + self.route_request = Some(request); + self.route = PlanPhase::Requested; + } + NavigatorIntent::CancelPlan => { + self.route_request = None; // #499: the undelivered request nets out + self.route_cancel = true; + self.supersede(PlanFamily::Route); + self.route = PlanPhase::Idle; + } + NavigatorIntent::PlanDetour(request) => { + self.supersede(PlanFamily::Detour); + self.detour_request = Some(request); + self.detour = PlanPhase::Requested; + } + NavigatorIntent::CancelDetour => { + self.detour_request = None; + self.detour_commit = false; + self.detour_cancel = true; + self.supersede(PlanFamily::Detour); + self.detour = PlanPhase::Idle; + } + // A commit is the rider pressing on figures they can see, so the preview screen is its + // only producer — and a cancellation annihilates it exactly as it does a plan request, + // which is what stops a splice of a detour that no longer exists. + NavigatorIntent::CommitDetour => self.detour_commit = true, + } + } + + /// A cancellation or a replacement: `family`'s in-flight operation stops being the current one, + /// so its answer will be refused when it lands. + fn supersede(&mut self, family: PlanFamily) { + if self.live == Some(family) { + self.ops.invalidate(); + self.live = None; + } + } + + /// Which family the in-flight operation belongs to, if one is running. + pub(crate) fn live_family(&self) -> Option { + self.live + } + + /// The next bounded navigation operation, or `None` when nothing is owed. + /// + /// Offered in the drain's order — cancellations before new work — so the pass and the legacy + /// protocol ask the executor for the same thing in the same sequence. + pub(crate) fn next_effect(&mut self) -> Option { + self.next_release(PlanFamily::Route) + .or_else(|| self.next_release(PlanFamily::Detour)) + .or_else(|| self.next_plan_effect(PlanFamily::Route)) + .or_else(|| self.next_plan_effect(PlanFamily::Detour)) + .or_else(|| self.next_commit_effect()) + } + + /// The workspace release a cancellation implies. Consumes the same one-shot + /// [`take_cancel`](Self::take_cancel) does, so a cancellation reaches the executor exactly once + /// however the two protocols are composed. + fn next_release(&mut self, family: PlanFamily) -> Option { + if !self.take_cancel(family) { + return None; + } + self.note_cancel_delivered(family); + self.live = Some(family); + Some(NavigatorEffect::Release { token: self.ops.issue() }) + } + + /// Hand `family`'s undelivered request to an executor: the operation the search runs under, and + /// the moment the freeze engages. **The engaging edge is here, not at admission** — a request + /// the rider cancelled before anyone took it froze nothing, so nothing needs releasing. + pub(crate) fn next_plan_effect(&mut self, family: PlanFamily) -> Option { + let work = match family { + PlanFamily::Route => PlannerWork::Route(self.route_request.take()?), + PlanFamily::Detour => PlannerWork::Detour(self.detour_request.take()?), + }; + match family { + PlanFamily::Route => self.route = PlanPhase::Planning, + PlanFamily::Detour => self.detour = PlanPhase::Planning, + } + self.freeze.plan_started(family); + self.live = Some(family); + Some(NavigatorEffect::Acquire { token: self.ops.issue(), work }) + } + + /// Hand the previewed detour's splice to an executor. No freeze edge: a commit is a write, not + /// a search, and it does not take the nav arm. + pub(crate) fn next_commit_effect(&mut self) -> Option { + core::mem::take(&mut self.detour_commit).then(|| { + self.detour = PlanPhase::Committing; + self.live = Some(PlanFamily::Detour); + NavigatorEffect::CommitDetour { token: self.ops.issue() } + }) + } + + /// Whether `outcome` still answers the operation Navigator is waiting for. A cancelled or + /// superseded plan refuses its own late answer here — the executor never has to know. + pub(crate) fn accepts(&self, outcome: &NavigatorOutcome) -> bool { + self.ops.is_current(outcome.token()) + } + + /// Record a terminal planner answer for `family`: the run is over, so the token stops being + /// current and the freeze releases. Returns whether the freeze changed. + /// + /// Reached from both answer paths — a typed [`NavigatorOutcome`] at the pass's first stage, and + /// a legacy `HostEvent` at [`App::apply_event`](crate::App::apply_event) — so there is one + /// definition of "the run ended" whatever spoke. + pub(crate) fn note_answer(&mut self, family: PlanFamily, phase: PlanPhase) -> bool { + self.ops.invalidate(); + self.live = None; + match family { + PlanFamily::Route => self.route = phase, + PlanFamily::Detour => self.detour = phase, + } + self.freeze.plan_ended(family) + } + + /// Whether a detour plan exists at all — requested, running, previewed, committing or adopted. + /// The falling edge of this is what drops the preview polyline drawn over the active route. + pub(crate) fn detour_planned(&self) -> bool { + self.detour != PlanPhase::Idle + } + + /// Whether the in-flight detour operation is the splice rather than the search — the two have + /// the same family and different answers. + pub(crate) fn detour_committing(&self) -> bool { + self.detour == PlanPhase::Committing + } + + /// A detour commit answered. Success adopts the spliced route; a failure returns the rider to + /// the preview they came from, which is what makes a failed commit retryable. + pub(crate) fn note_commit(&mut self, committed: bool) { + self.ops.invalidate(); + self.live = None; + self.detour = if committed { PlanPhase::Active } else { PlanPhase::PreviewReady }; + } + + // ---- the legacy protocol's per-class doors (deleted at #1397 S6) ---- + + /// Whether `family` has an undelivered plan request — the `PlanRoute` / `PlanDetour` peek. + pub(crate) fn request_pending(&self, family: PlanFamily) -> bool { + match family { + PlanFamily::Route => self.route_request.is_some(), + PlanFamily::Detour => self.detour_request.is_some(), + } + } + + /// Whether `family` has an undelivered cancellation — the `CancelRoutePlan` / `CancelDetour` + /// peek. + pub(crate) fn cancel_pending(&self, family: PlanFamily) -> bool { + match family { + PlanFamily::Route => self.route_cancel, + PlanFamily::Detour => self.detour_cancel, + } + } + + /// Whether the previewed detour's commit is undelivered — the `CommitDetour` peek. + pub(crate) fn commit_pending(&self) -> bool { + self.detour_commit + } + + /// Take an undelivered cancellation for `family`. + /// + /// The legacy protocol expresses a cancellation as its own command, while the new one expresses + /// it as a [`Release`](NavigatorEffect::Release) that is *also* issued on success — which is + /// why [`LegacyOwned::PlannerRelease`](crate::device_core::compat::LegacyOwned::PlannerRelease) + /// refuses to translate one into the other, and why the drain asks for the cancel by name. + pub(crate) fn take_cancel(&mut self, family: PlanFamily) -> bool { + match family { + PlanFamily::Route => core::mem::take(&mut self.route_cancel), + PlanFamily::Detour => core::mem::take(&mut self.detour_cancel), + } + } + + /// The executor has been told to drop `family`'s search: **now** the run is over, so the freeze + /// releases and the map plane may resume. Returns whether that changed the freeze, so the + /// caller can repaint the frame that held still for it. + /// + /// **Per-family** (#1146): a detour's cancellation must never resume the map while a route + /// search still holds the nav arm — the very next frame would claim the render arm out from + /// under it. + pub(crate) fn note_cancel_delivered(&mut self, family: PlanFamily) -> bool { + self.freeze.plan_ended(family) + } + + // ---- the freeze, read-only to everyone else ---- + + /// Whether a planner run is live at all — the arena's "is the nav arm claimed?" fact. + pub(crate) fn plan_live(&self) -> bool { + self.freeze.plan_live() + } + + /// Whether the freeze is engaged: a live plan **and** a base screen that would draw the map. + pub(crate) fn freeze_active(&self, base_draws_map: bool) -> bool { + self.freeze.active(base_draws_map) + } + + /// The banner's repaint edge — see [`RerouteFreeze::take_engaged_edge`]. + pub(crate) fn take_freeze_edge(&mut self, base_draws_map: bool) -> bool { + self.freeze.take_engaged_edge(base_draws_map) + } + + /// Engage or release a `Route` run without a real planner — the simulator's `--freeze` flag and + /// the snapshot harness. No production path reaches it. + pub(crate) fn debug_set_plan_live(&mut self, live: bool) -> bool { + if live { + self.freeze.plan_started(PlanFamily::Route); + self.route = PlanPhase::Planning; + false + } else { + self.note_answer(PlanFamily::Route, PlanPhase::Idle) + } + } + + // ---- catalog identity ---- + + /// Follow the undelivered detour request through a route-catalog rescan by durable identity; a + /// vanished route drops it, exactly as it drops the caches keyed on that route. + pub(crate) fn remap_detour_route(&mut self, remap: &dyn Fn(usize) -> Option) { + self.detour_request = + self.detour_request.and_then(|req| remap(req.route).map(|route| DetourRequest { route, ..req })); + } + + /// A fresh tracking session starts with no detour in flight. + /// + /// Dropping the pair cannot strand the freeze's `Detour` level: an **undelivered** request never + /// engaged it (the effect is the engaging edge), and a dropped **cancel** only forfeits one of + /// two release edges — the executor is still running the plan that cancel would have aborted, + /// and it answers every plan it was given, so the answer's own release lands anyway. + pub(crate) fn reset_detour(&mut self) { + self.detour_request = None; + self.detour_commit = false; + self.detour_cancel = false; + } + + /// The detour family's phase — the preview/commit gate, and what the tests read. + #[cfg(test)] + pub(crate) fn detour_phase(&self) -> PlanPhase { + self.detour + } + + /// The undelivered detour request itself — the durable-identity tests pin the remap through it + /// without consuming the request. + #[cfg(test)] + pub(crate) fn pending_detour_request(&self) -> Option { + self.detour_request + } + + /// Assert the boot state, field by field. The destructure is exhaustive, so a field added here + /// must state its boot value too. + #[cfg(test)] + pub(crate) fn assert_boot_state(&self) { + let NavigatorMachine { + ops, + live, + freeze, + route, + detour, + route_request, + detour_request, + route_cancel, + detour_cancel, + detour_commit, + } = self; + assert_eq!(format!("{ops:?}"), "TokenSource(0)", "no navigation operation has been issued"); + assert!(live.is_none(), "no operation is in flight"); + assert!(!freeze.plan_live() && !freeze.active(true), "no planner running, no freeze banner"); + assert!(*route == PlanPhase::Idle && *detour == PlanPhase::Idle, "neither family has been asked"); + assert!(route_request.is_none() && detour_request.is_none(), "no request waiting"); + assert!(!*route_cancel && !*detour_cancel && !*detour_commit, "no one-shot latched"); + } +} + +// Layout tripwire: two bounded requests, two phases, a token and a handful of one-shots — never a +// route, a polyline or a screen. +const _: () = assert!(core::mem::size_of::() <= 96, "two planner requests and their phases"); + +#[cfg(test)] +mod machine_tests { + use super::*; + use obc_route::nav::NavError; + + fn route_request(name: &str) -> NavRequest { + NavRequest::new((0, 0), (1_000, 1_000), name) + } + + fn detour_request() -> DetourRequest { + DetourRequest { route: 0, from: (0, 0), progress_m: 1_000, target_m: 1_600 } + } + + /// The name of what an effect asks for, so a test can say what it expects without matching on + /// a token it never chose. + fn acquired(effect: Option) -> Option { + match effect { + Some(NavigatorEffect::Acquire { work, .. }) => Some(work), + _ => None, + } + } + + /// **#499, both families.** A cancel posted before its request reaches an executor nets "no + /// plan": the request is annihilated at post time, so nothing is ever started, and the cancel + /// still latches (a plan an executor already took is still aborted). + #[test] + fn a_cancel_before_delivery_nets_no_plan() { + for (plan, cancel, family) in [ + (NavigatorIntent::PlanRoute(route_request("col")), NavigatorIntent::CancelPlan, PlanFamily::Route), + (NavigatorIntent::PlanDetour(detour_request()), NavigatorIntent::CancelDetour, PlanFamily::Detour), + ] { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(plan); + nav.admit_intent(cancel); + assert!(!nav.request_pending(family), "the undelivered request nets out"); + assert!(nav.next_plan_effect(family).is_none(), "so no executor is ever asked to plan it"); + assert!(nav.cancel_pending(family), "and the cancel still reaches one"); + assert!(!nav.plan_live(), "nothing froze the map for a plan that never started"); + } + } + + /// **#1146, both directions.** A family's terminal edge releases only its own freeze, and two + /// live searches hold it until the last one ends. The regression is a map that never redraws + /// again with the nav arm still claimed underneath it. + #[test] + fn a_detours_terminal_edge_never_releases_a_route_freeze() { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(NavigatorIntent::PlanRoute(route_request("col"))); + nav.next_plan_effect(PlanFamily::Route).expect("the route search starts"); + assert!(nav.freeze_active(true), "a search over a map base is the freeze"); + + // A detour cancellation, delivered while the route search is still running. + nav.admit_intent(NavigatorIntent::CancelDetour); + assert!(nav.take_cancel(PlanFamily::Detour)); + assert!(!nav.note_cancel_delivered(PlanFamily::Detour), "not this run's edge"); + assert!(nav.freeze_active(true), "the route search still holds the nav arm"); + + // And two live searches: the first to end does not release it. + nav.admit_intent(NavigatorIntent::PlanDetour(detour_request())); + nav.next_plan_effect(PlanFamily::Detour).expect("the detour search starts too"); + assert!(!nav.note_answer(PlanFamily::Detour, PlanPhase::PreviewReady), "one down, one to go"); + assert!(nav.freeze_active(true)); + assert!(nav.note_answer(PlanFamily::Route, PlanPhase::Active), "the last one out releases it"); + assert!(!nav.freeze_active(true)); + } + + /// A plan answer that arrives after the rider cancelled changes nothing: the token stopped being + /// current the instant they walked away, so the search's eventual result commits no route. + #[test] + fn an_answer_after_a_cancellation_is_refused() { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(NavigatorIntent::PlanRoute(route_request("col"))); + let effect = nav.next_plan_effect(PlanFamily::Route).expect("the search starts"); + + nav.admit_intent(NavigatorIntent::CancelPlan); + let answer = NavigatorOutcome::PlanFinished { token: effect.token(), route: 7 }; + assert!(!nav.accepts(&answer), "the cancelled operation does not accept its own late answer"); + } + + /// The same rule for a *replacement*: the newer request supersedes the older operation, and the + /// older one's answer belongs to nothing. + #[test] + fn an_answer_after_a_replacement_is_refused() { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(NavigatorIntent::PlanRoute(route_request("first"))); + let first = nav.next_plan_effect(PlanFamily::Route).expect("the first search starts"); + + nav.admit_intent(NavigatorIntent::PlanRoute(route_request("second"))); + let second = nav.next_plan_effect(PlanFamily::Route).expect("the replacement starts"); + assert_ne!(first.token(), second.token(), "a new operation, a new token"); + assert!(!nav.accepts(&NavigatorOutcome::PlanFinished { token: first.token(), route: 7 })); + assert!(nav.accepts(&NavigatorOutcome::PlanFinished { token: second.token(), route: 8 })); + assert_eq!( + acquired(Some(second)) + .map(|work| matches!(work, PlannerWork::Route(request) if request.name() == "second")), + Some(true), + "and it is the newer request that went out" + ); + } + + /// A detour with no path is a **planning failure**, not the absence of the capability: the + /// family lands in `Failed`, distinguishable from the `Idle` a device that never planned is in. + /// A device without `NavigatorCapabilities::plan_detour` never reaches this path at all — the + /// UI's Detour station is not offered, so no intent is ever admitted. + #[test] + fn a_detour_without_a_path_is_a_failure_and_not_an_absent_capability() { + let mut nav = NavigatorMachine::new(); + assert_eq!(nav.detour_phase(), PlanPhase::Idle, "a device that never planned is idle"); + + nav.admit_intent(NavigatorIntent::PlanDetour(detour_request())); + let effect = nav.next_plan_effect(PlanFamily::Detour).expect("the search starts"); + let answer = NavigatorOutcome::Failed { token: effect.token(), error: NavigatorError::Plan(NavError::NoPath) }; + assert!(nav.accepts(&answer)); + nav.note_answer(PlanFamily::Detour, PlanPhase::Failed); + assert_eq!(nav.detour_phase(), PlanPhase::Failed, "…and one that tried and could not is not"); + } + + /// The lifecycle end to end, in the order the rider walks it: the detour is planned, previewed, + /// committed, and adopted — and a commit that fails returns to the preview it was pressed from, + /// which is what makes a failed commit retryable. + #[test] + fn the_detour_walks_plan_preview_commit_and_a_failure_returns_to_the_preview() { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(NavigatorIntent::PlanDetour(detour_request())); + assert_eq!(nav.detour_phase(), PlanPhase::Requested); + assert!(acquired(nav.next_plan_effect(PlanFamily::Detour)).is_some()); + assert_eq!(nav.detour_phase(), PlanPhase::Planning); + + nav.note_answer(PlanFamily::Detour, PlanPhase::PreviewReady); + nav.admit_intent(NavigatorIntent::CommitDetour); + assert!(nav.commit_pending()); + assert!(matches!(nav.next_commit_effect(), Some(NavigatorEffect::CommitDetour { .. }))); + assert_eq!(nav.detour_phase(), PlanPhase::Committing); + assert!(!nav.plan_live(), "a splice is a write, not a search — it takes no nav arm"); + + nav.note_commit(false); + assert_eq!(nav.detour_phase(), PlanPhase::PreviewReady, "a failed commit can be retried"); + nav.admit_intent(NavigatorIntent::CommitDetour); + nav.next_commit_effect().expect("…and the retry goes out"); + nav.note_commit(true); + assert_eq!(nav.detour_phase(), PlanPhase::Active); + } + + /// One stream, one order: the pass takes cancellations before new work, so both compositions ask + /// an executor for the same thing in the same sequence. + #[test] + fn the_pass_offers_a_cancellation_before_new_work() { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(NavigatorIntent::PlanDetour(detour_request())); + nav.next_plan_effect(PlanFamily::Detour).expect("a search is running"); + nav.admit_intent(NavigatorIntent::CancelDetour); + nav.admit_intent(NavigatorIntent::PlanRoute(route_request("col"))); + + assert!(matches!(nav.next_effect(), Some(NavigatorEffect::Release { .. })), "the cancellation first"); + assert!(!nav.freeze_active(true), "and delivering it is what releases the detour's freeze"); + assert!(matches!(acquired(nav.next_effect()), Some(PlannerWork::Route(_))), "then the new search"); + assert!(nav.next_effect().is_none(), "and nothing else is owed"); + } +} diff --git a/firmware/obc-app/src/ride_engine.rs b/firmware/obc-app/src/ride_engine.rs index 3f4a8ead4..a7d4282cd 100644 --- a/firmware/obc-app/src/ride_engine.rs +++ b/firmware/obc-app/src/ride_engine.rs @@ -288,7 +288,12 @@ impl RideEngine { /// list before the fix is matched. Only advance a build key when the geometry is actually /// streamable: a `None` route (idle, or a transient SD glitch) leaves the old state in place /// and retries next tick, rather than latching an empty result for the route. - pub(crate) fn sync_route_state(&mut self, activity: &mut Activity, route: Option<&RouteReader>) -> bool { + pub(crate) fn sync_route_state( + &mut self, + activity: &mut Activity, + navigator: &mut crate::navigator::NavigatorMachine, + route: Option<&RouteReader>, + ) -> bool { let mut dirty = false; if activity.active_route != self.matched_route { // Deliberately do NOT clear a pending seam re-anchor here: a detour commit queues it @@ -308,6 +313,7 @@ impl RideEngine { self.route_match.reset(); if !activity.take_resume_session() { activity.reset_ride(); + navigator.reset_detour(); // a fresh session starts with no detour in flight } self.breadcrumb.clear(); // A new session is a new pace too (WX12's projection window restarts with the ride). @@ -595,11 +601,10 @@ impl RideEngine { // navigation unloads and the stale per-route state is dropped with it. let old_active = activity.active_route; activity.active_route = old_active.and_then(remap); - // A queued seam re-anchor (one tick between detour commit and geometry) and a queued - // detour-plan request both follow the same durable route identity as `active_route`, or - // are cancelled if that route vanished. + // A queued seam re-anchor (one tick between detour commit and geometry) follows the same + // durable route identity as `active_route`, or is cancelled if that route vanished. So does + // Navigator's undelivered detour request, which its owner remaps beside this call. activity.remap_seam_route(remap); - activity.remap_detour_route(remap); if old_active.is_some() && activity.active_route.is_none() { self.route_match.reset(); // drop stale progress/off-route from the vanished route } diff --git a/firmware/obc-app/src/screen/detour.rs b/firmware/obc-app/src/screen/detour.rs index e69295de7..00c5c9359 100644 --- a/firmware/obc-app/src/screen/detour.rs +++ b/firmware/obc-app/src/screen/detour.rs @@ -28,6 +28,7 @@ use crate::activity::{Activity, DetourRequest}; use crate::app::{MAX_ZOOM, MIN_ZOOM, ZOOM_STEP}; use crate::host::DetourPreview; use crate::input::Gesture; +use crate::navigator::NavigatorIntent; use crate::Msg; use super::map::{draw_map_scene, DetourMapOverlay}; @@ -172,12 +173,12 @@ impl DetourScreen { // request freezes the corridor/prefix anchor at this instant; the host resolves // the rejoin coordinate itself (it owns the RouteReader). if let (Some(route), Some(target), Some(fix)) = (self.route, self.target_m(), cx.state.user_fix) { - cx.activity.request_detour(DetourRequest { + cx.navigator.admit_intent(NavigatorIntent::PlanDetour(DetourRequest { route, from: (fix.lon, fix.lat), progress_m: self.start_m, target_m: target, - }); + })); // Push (not Replace): Back from the planning spinner or the preview returns // here with steps intact. Transition::Push(Screen::NavPlanning(NavPlanningScreen::detour())) @@ -383,18 +384,18 @@ impl DetourPreviewScreen { if self.stale(cx.activity) { // Cancel out to the chooser, which re-anchors to live progress; the host drops the // held detour bytes. - cx.activity.request_detour_cancel(); + cx.navigator.admit_intent(NavigatorIntent::CancelDetour); return Transition::Pop; } match g { Gesture::Press if !self.committing => { - cx.activity.request_detour_commit(); + cx.navigator.admit_intent(NavigatorIntent::CommitDetour); self.committing = true; self.error = false; Transition::None } Gesture::Back => { - cx.activity.request_detour_cancel(); + cx.navigator.admit_intent(NavigatorIntent::CancelDetour); Transition::Pop } _ => Transition::None, @@ -561,21 +562,44 @@ fn inspect_viewport(w: i32, h: i32, candidate: (i32, i32), overview_zoom: f32, f #[cfg(test)] mod tests { use super::*; + use crate::navigator::{NavigatorEffect, NavigatorMachine, PlannerWork}; + use crate::reroute_freeze::PlanFamily; use crate::screen::test_ctx; use crate::{AppState, Settings}; use obc_ports::Fix; - fn with_ctx(activity: &mut Activity, f: impl FnOnce(&mut Ctx) -> T) -> T { - with_state_ctx(activity, AppState::new(0, 0, 1.0), f) + fn with_ctx(activity: &mut Activity, nav: &mut NavigatorMachine, f: impl FnOnce(&mut Ctx) -> T) -> T { + with_state_ctx(activity, nav, AppState::new(0, 0, 1.0), f) } - /// A `Ctx` whose `AppState` has a nav graph, a fix, and whatever the test staged. - fn with_state_ctx(activity: &mut Activity, mut state: AppState, f: impl FnOnce(&mut Ctx) -> T) -> T { + /// A `Ctx` whose `AppState` has a nav graph, a fix, and whatever the test staged — over the + /// caller's own Navigator, so the test can read back the request the screen posted to it. + fn with_state_ctx( + activity: &mut Activity, + nav: &mut NavigatorMachine, + mut state: AppState, + f: impl FnOnce(&mut Ctx) -> T, + ) -> T { let mut settings = Settings::default(); - let mut cx = test_ctx(&mut state, activity, &mut settings); + let mut cx = Ctx { navigator: nav, ..test_ctx(&mut state, activity, &mut settings) }; f(&mut cx) } + /// The detour-plan request Navigator holds, taken as an executor would. + fn drained_detour(nav: &mut NavigatorMachine) -> Option { + match nav.next_plan_effect(PlanFamily::Detour) { + Some(NavigatorEffect::Acquire { work: PlannerWork::Detour(req), .. }) => Some(req), + other => { + assert!(other.is_none(), "the detour arm only ever acquires a detour: {other:?}"); + None + } + } + } + + fn drained_commit(nav: &mut NavigatorMachine) -> bool { + nav.next_commit_effect().is_some() + } + fn nav_state() -> AppState { let mut s = AppState::new(0, 0, 1.0); s.has_nav_graph = true; @@ -612,14 +636,17 @@ mod tests { #[test] fn turn_then_press_posts_a_plan_request_and_pushes_planning() { + let mut nav_a = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let session = a.session(); let mode = a.mode; let mut s = DetourScreen::new(&a); - with_state_ctx(&mut a, nav_state(), |cx| assert!(matches!(s.handle(Gesture::Step(2), cx), Transition::None))); - let t = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Press, cx)); + with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| { + assert!(matches!(s.handle(Gesture::Step(2), cx), Transition::None)) + }); + let t = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Press, cx)); assert!(matches!(t, Transition::Push(Screen::NavPlanning(_))), "Press starts the plan flow"); - let req = a.take_detour_request().expect("plan request queued"); + let req = drained_detour(&mut nav_a).expect("plan request queued"); assert_eq!( (req.route, req.progress_m, req.target_m), (2, 1_000, 1_800), @@ -634,110 +661,130 @@ mod tests { #[test] fn press_without_nav_graph_or_fix_is_guarded() { + let mut nav_a = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let mut s = DetourScreen::new(&a); // No nav graph: unavailable outright. - assert!(matches!(with_ctx(&mut a, |cx| s.handle(Gesture::Press, cx)), Transition::None)); - assert!(a.take_detour_request().is_none()); + assert!(matches!(with_ctx(&mut a, &mut nav_a, |cx| s.handle(Gesture::Press, cx)), Transition::None)); + assert!(drained_detour(&mut nav_a).is_none()); // Graph but no fix: available() passes, the Press guard refuses to send a garbage start. let mut state = nav_state(); state.user_fix = None; - assert!(matches!(with_state_ctx(&mut a, state, |cx| s.handle(Gesture::Press, cx)), Transition::None)); - assert!(a.take_detour_request().is_none()); + assert!(matches!( + with_state_ctx(&mut a, &mut nav_a, state, |cx| s.handle(Gesture::Press, cx)), + Transition::None + )); + assert!(drained_detour(&mut nav_a).is_none()); } #[test] fn back_cancels_to_map_without_any_navigation_change() { + let mut nav_a = NavigatorMachine::new(); let mut a = tracking_activity(800, 4_000); let before = a; let mut s = DetourScreen::new(&a); - with_state_ctx(&mut a, nav_state(), |cx| { + with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| { let _ = s.handle(Gesture::Step(3), cx); }); - let t = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Back, cx)); + let t = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Back, cx)); assert!(matches!(t, Transition::Pop)); assert_eq!(a.progress_m, before.progress_m); assert_eq!(a.session(), before.session()); assert_eq!(a.mode, before.mode); - assert!(a.take_detour_request().is_none()); + assert!(drained_detour(&mut nav_a).is_none()); } #[test] fn route_less_off_route_and_near_end_press_are_guarded() { + let mut nav_end = NavigatorMachine::new(); + let mut nav_off = NavigatorMachine::new(); + let mut nav_route_less = NavigatorMachine::new(); let mut route_less = tracking_activity(0, 0); route_less.active_route = None; let mut s = DetourScreen::new(&route_less); assert!(matches!( - with_state_ctx(&mut route_less, nav_state(), |cx| s.handle(Gesture::Press, cx)), + with_state_ctx(&mut route_less, &mut nav_route_less, nav_state(), |cx| s.handle(Gesture::Press, cx)), Transition::None )); let mut off = tracking_activity(100, 2_000); off.off_route = true; let mut s = DetourScreen::new(&off); - assert!(matches!(with_state_ctx(&mut off, nav_state(), |cx| s.handle(Gesture::Press, cx)), Transition::None)); + assert!(matches!( + with_state_ctx(&mut off, &mut nav_off, nav_state(), |cx| s.handle(Gesture::Press, cx)), + Transition::None + )); let mut end = tracking_activity(1_950, 2_000); let mut s = DetourScreen::new(&end); - assert!(matches!(with_state_ctx(&mut end, nav_state(), |cx| s.handle(Gesture::Press, cx)), Transition::None)); + assert!(matches!( + with_state_ctx(&mut end, &mut nav_end, nav_state(), |cx| s.handle(Gesture::Press, cx)), + Transition::None + )); assert!( - route_less.take_detour_request().is_none() - && off.take_detour_request().is_none() - && end.take_detour_request().is_none() + drained_detour(&mut nav_route_less).is_none() + && drained_detour(&mut nav_off).is_none() + && drained_detour(&mut nav_end).is_none() ); } #[test] fn moving_while_open_advances_highlight_and_plan_anchor() { + let mut nav_a = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let mut s = DetourScreen::new(&a); // Rider advances before a Step; that input refreshes the live anchor and adds 100 m. a.progress_m = 1_200; - with_state_ctx(&mut a, nav_state(), |cx| { + with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| { let _ = s.handle(Gesture::Step(1), cx); }); assert_eq!((s.start_m, s.target_m()), (1_200, Some(1_900))); // Another 100 m before Press: the request is still a 700 m span, now from 1.3 km. a.progress_m = 1_300; - let t = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Press, cx)); + let t = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Press, cx)); assert!(matches!(t, Transition::Push(Screen::NavPlanning(_)))); - let req = a.take_detour_request().unwrap(); + let req = drained_detour(&mut nav_a).unwrap(); assert_eq!((req.progress_m, req.target_m), (1_300, 2_000)); } #[test] fn hold_toggles_candidate_inspection_and_turn_changes_only_zoom() { + let mut nav_a = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let mut s = DetourScreen::new(&a); let target = s.target_m(); - assert!(matches!(with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Hold, cx)), Transition::None)); + assert!(matches!( + with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Hold, cx)), + Transition::None + )); assert!(s.inspecting()); assert_eq!(s.inspect_level, INSPECT_ENTRY_LEVEL); let entry_zoom = s.inspect_zoom(); - let _ = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Step(-20), cx)); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Step(-20), cx)); assert!(s.inspect_zoom() < 1.0, "inspection can zoom a little wider than the fitted overview"); assert_eq!(s.target_m(), target, "inspection never changes the selected rejoin point"); - let _ = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Step(20), cx)); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Step(20), cx)); assert!(s.inspect_zoom() > entry_zoom, "a Step zooms back in while inspecting"); - let _ = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Hold, cx)); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Hold, cx)); assert!(!s.inspecting(), "a second Hold returns to the overview"); } #[test] fn press_from_inspection_plans_the_unchanged_candidate() { + let mut nav_a = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let mut s = DetourScreen::new(&a); let target = s.target_m().unwrap(); - let _ = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Hold, cx)); - let _ = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Step(3), cx)); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Hold, cx)); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Step(3), cx)); - let t = with_state_ctx(&mut a, nav_state(), |cx| s.handle(Gesture::Press, cx)); + let t = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| s.handle(Gesture::Press, cx)); assert!(matches!(t, Transition::Push(Screen::NavPlanning(_)))); - assert_eq!(a.take_detour_request().unwrap().target_m, target); + assert_eq!(drained_detour(&mut nav_a).unwrap().target_m, target); } // ---- the preview screen ---- @@ -752,51 +799,56 @@ mod tests { #[test] fn preview_press_commits_once_and_back_cancels() { + let mut nav_a = NavigatorMachine::new(); + let mut nav_b = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let mut p = preview_for(&a); assert_eq!(p.anchor_m(), 1_000); - let t = with_state_ctx(&mut a, nav_state(), |cx| p.handle(Gesture::Press, cx)); + let t = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| p.handle(Gesture::Press, cx)); assert!(matches!(t, Transition::None), "commit keeps the preview up until the host answers"); - assert!(a.take_detour_commit(), "the commit one-shot is queued"); + assert!(drained_commit(&mut nav_a), "the commit one-shot is queued"); // A second Press while committing is a no-op. - let _ = with_state_ctx(&mut a, nav_state(), |cx| p.handle(Gesture::Press, cx)); - assert!(!a.take_detour_commit(), "no double commit"); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| p.handle(Gesture::Press, cx)); + assert!(!drained_commit(&mut nav_a), "no double commit"); let mut b = tracking_activity(1_000, 5_000); let mut p2 = preview_for(&b); - let t = with_state_ctx(&mut b, nav_state(), |cx| p2.handle(Gesture::Back, cx)); + let t = with_state_ctx(&mut b, &mut nav_b, nav_state(), |cx| p2.handle(Gesture::Back, cx)); assert!(matches!(t, Transition::Pop)); - assert!(b.take_detour_cancel(), "Back rings the host to drop the held detour"); + assert!(nav_b.take_cancel(PlanFamily::Detour), "Back rings the host to drop the held detour"); } #[test] fn preview_commit_failure_reopens_press() { + let mut nav_a = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let mut p = preview_for(&a); - let _ = with_state_ctx(&mut a, nav_state(), |cx| p.handle(Gesture::Press, cx)); - assert!(a.take_detour_commit()); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| p.handle(Gesture::Press, cx)); + assert!(drained_commit(&mut nav_a)); p.set_commit_failed(); - let _ = with_state_ctx(&mut a, nav_state(), |cx| p.handle(Gesture::Press, cx)); - assert!(a.take_detour_commit(), "a failed commit can be retried"); + let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| p.handle(Gesture::Press, cx)); + assert!(drained_commit(&mut nav_a), "a failed commit can be retried"); } #[test] fn preview_goes_stale_when_the_rider_passes_the_rejoin_or_route_vanishes() { + let mut nav_a = NavigatorMachine::new(); + let mut nav_b = NavigatorMachine::new(); let mut a = tracking_activity(1_000, 5_000); let mut p = preview_for(&a); a.progress_m = p.target_m; // rode past the rejoin during the preview - let t = with_state_ctx(&mut a, nav_state(), |cx| p.handle(Gesture::Press, cx)); + let t = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| p.handle(Gesture::Press, cx)); assert!(matches!(t, Transition::Pop), "stale preview cancels out instead of committing"); - assert!(a.take_detour_cancel()); - assert!(!a.take_detour_commit()); + assert!(nav_a.take_cancel(PlanFamily::Detour)); + assert!(!drained_commit(&mut nav_a)); let mut b = tracking_activity(1_000, 5_000); let mut p = preview_for(&b); p.remap_routes(&|_| None); // the planned route vanished in a rescan - let t = with_state_ctx(&mut b, nav_state(), |cx| p.handle(Gesture::Step(1), cx)); + let t = with_state_ctx(&mut b, &mut nav_b, nav_state(), |cx| p.handle(Gesture::Step(1), cx)); assert!(matches!(t, Transition::Pop)); - assert!(b.take_detour_cancel()); + assert!(nav_b.take_cancel(PlanFamily::Detour)); } // ---- the preview's climb figure (#1091) ---- diff --git a/firmware/obc-app/src/screen/dfu.rs b/firmware/obc-app/src/screen/dfu.rs index 23be2610c..a7df4afa1 100644 --- a/firmware/obc-app/src/screen/dfu.rs +++ b/firmware/obc-app/src/screen/dfu.rs @@ -127,7 +127,7 @@ impl DfuConfirmScreen { Gesture::Press if self.selected == INSTALL => { // Arm: post the install one-shot (the board snapshots the rollback + arms + reboots) // and swap to the progress spinner. The confirm was pushed over the System menu. - cx.activity.request_dfu(crate::activity::DfuAction::Install); + cx.dfu.admit_intent(crate::dfu::DfuIntent::InstallRequested); Transition::Replace(Screen::DfuProgress(DfuProgressScreen::new())) } Gesture::Press => Transition::Pop, // Cancel @@ -476,17 +476,26 @@ mod tests { DfuScanReport::new(installed, staged, first_install) } - /// Build a throwaway `Ctx`, run a gesture, and hand back the transition + the drained DFU - /// one-shot the screen posted (if any). + /// Build a throwaway `Ctx`, run a gesture, and hand back the transition + the update phase the + /// screen posted to the DFU domain (if any), taken as an executor would. fn run(scr: &mut impl FnMut(&mut Ctx) -> Transition) -> (Transition, Option) { let mut st = AppState::new(0, 0, 1.0); let mut act = Activity::new(Mode::Idle); let mut settings = Settings::default(); + let mut dfu = crate::dfu::DfuState::new(); let t = { - let mut cx = test_ctx(&mut st, &mut act, &mut settings); + let mut cx = Ctx { dfu: &mut dfu, ..test_ctx(&mut st, &mut act, &mut settings) }; scr(&mut cx) }; - (t, act.take_dfu_request()) + (t, drained_dfu(&mut dfu)) + } + + /// The phase the DFU domain is holding, in the legacy vocabulary the tests already speak. + fn drained_dfu(dfu: &mut crate::dfu::DfuState) -> Option { + dfu.next_effect().map(|effect| match effect { + crate::dfu::DfuEffect::Scan { .. } => DfuAction::Scan, + crate::dfu::DfuEffect::ArmInstall { .. } => DfuAction::Install, + }) } /// The same-version warning fires only on a byte-for-byte match. diff --git a/firmware/obc-app/src/screen/mod.rs b/firmware/obc-app/src/screen/mod.rs index 95f5b923c..63e1ab41b 100644 --- a/firmware/obc-app/src/screen/mod.rs +++ b/firmware/obc-app/src/screen/mod.rs @@ -201,6 +201,14 @@ pub struct Ctx<'a> { /// `Gesture::Press` reads the highlighted hit's address out of it to save + connect. Empty outside /// a scan; every other screen leaves it untouched. pub sensor_scan_hits: &'a [crate::sensors::SensorScanHit], + /// The **Navigator** domain (#1397 S2) — the planning screens name what they want to it + /// (`admit_intent`) rather than latching a request of their own. A rider's plan, cancel or + /// commit therefore exists in exactly one place from the instant they press. + pub navigator: &'a mut crate::navigator::NavigatorMachine, + /// The **DFU** domain — the Firmware and update-confirm screens post their phase here. + pub dfu: &'a mut crate::dfu::DfuState, + /// The **StorageInfo** domain — the System screen asks for a free-space refresh on entry. + pub storage: &'a mut crate::device_core::storage_info::StorageInfo, pub now_ms: u32, } @@ -208,6 +216,11 @@ pub struct Ctx<'a> { /// the shape essentially every screen test wants. The handful that need one field populated say so /// with struct-update syntax, so the other eleven stay out of the way: /// `Ctx { routes, ..test_ctx(&mut st, &mut act, &mut s) }`. +/// +/// The three domain seams are **leaked**, one fresh set per call: they need `&mut` for a lifetime +/// the helper cannot own, and a test that asserts on one passes its own instead +/// (`Ctx { navigator: &mut nav, ..test_ctx(…) }`). A few dozen bytes per screen test, in a build +/// that has `std`. #[cfg(test)] pub(crate) fn test_ctx<'a>(state: &'a mut AppState, activity: &'a mut Activity, settings: &'a mut Settings) -> Ctx<'a> { // Shared empty borrows: the screens under test read these but never fill them, so one immutable @@ -227,6 +240,9 @@ pub(crate) fn test_ctx<'a>(state: &'a mut AppState, activity: &'a mut Activity, waypoints: &[], corridor: &[], sensor_scan_hits: &[], + navigator: Box::leak(Box::new(crate::navigator::NavigatorMachine::new())), + dfu: Box::leak(Box::new(crate::dfu::DfuState::new())), + storage: Box::leak(Box::new(crate::device_core::storage_info::StorageInfo::new())), now_ms: 0, } } @@ -402,8 +418,9 @@ pub struct Render<'a> { pub map_name: &'a str, /// The loaded map's OBCM format version — the right half of the `Map` row (`0` = no map yet). pub map_obcm_version: u8, - /// Free space on the SD card in bytes (T8 item 6), or `None` until the host answers the System - /// screen's on-entry scan ([`App::apply_event`](crate::App::apply_event)). + /// Free space on the medium in bytes (T8 item 6), or `None` until a measurement answers the + /// System screen's on-entry refresh — the figure + /// [`StorageInfo`](crate::device_core::storage_info::StorageInfo) owns. pub card_free_bytes: Option, /// The host-fed resident **weather snapshot** (WX11, epic #1185) — the 24 hourly records + /// sampled rain-frame table the weather screens derive every claim from diff --git a/firmware/obc-app/src/screen/nav_route.rs b/firmware/obc-app/src/screen/nav_route.rs index cb7151e6a..f04821d78 100644 --- a/firmware/obc-app/src/screen/nav_route.rs +++ b/firmware/obc-app/src/screen/nav_route.rs @@ -33,6 +33,7 @@ use obc_render::{ use crate::activity::NavRequest; use crate::input::Gesture; +use crate::navigator::NavigatorIntent; use crate::Msg; use super::vocab::chrome::{card_triangle, title_frame, wrapped, TITLE_BAR_H}; @@ -87,7 +88,11 @@ impl NavConfirmScreen { let Some(fix) = cx.state.user_fix else { return Transition::Replace(Screen::NavFail(NavFailScreen::not_found())); }; - cx.activity.request_nav(NavRequest::new((fix.lon, fix.lat), self.to, &self.name)); + cx.navigator.admit_intent(NavigatorIntent::PlanRoute(NavRequest::new( + (fix.lon, fix.lat), + self.to, + &self.name, + ))); // Swap to the planning screen (#499): the host steps the resumable router across // its passes and answers into it — the UI stays live (spinner + Back-to-cancel). Transition::Replace(Screen::NavPlanning(NavPlanningScreen::new(&self.name))) @@ -188,8 +193,8 @@ impl NavPlanningScreen { // discards the partial work. No failure card — the rider changed their mind. Gesture::Back => { match self.kind { - PlanKind::Nav => cx.activity.request_nav_cancel(), - PlanKind::Detour => cx.activity.request_detour_cancel(), + PlanKind::Nav => cx.navigator.admit_intent(NavigatorIntent::CancelPlan), + PlanKind::Detour => cx.navigator.admit_intent(NavigatorIntent::CancelDetour), } Transition::Pop } diff --git a/firmware/obc-app/src/screen/settings/about.rs b/firmware/obc-app/src/screen/settings/about.rs index b8fc79d09..aa083cc04 100644 --- a/firmware/obc-app/src/screen/settings/about.rs +++ b/firmware/obc-app/src/screen/settings/about.rs @@ -178,21 +178,7 @@ mod tests { let mut st = AppState::new(0, 0, 1.0); let mut s = crate::Settings::default(); let mut act = crate::activity::Activity::new(crate::Mode::Idle); - let scratch = crate::screen::PoiScratch::new(); - let mut cx = Ctx { - state: &mut st, - activity: &mut act, - settings: &mut s, - routes: &[], - rides: &[], - trips: &[], - nav_profiles: &crate::NavProfiles::EMPTY, - poi_scratch: &scratch, - waypoints: &[], - corridor: &[], - sensor_scan_hits: &[], - now_ms: 0, - }; + let mut cx = crate::screen::test_ctx(&mut st, &mut act, &mut s); scr.handle(g, &mut cx) } diff --git a/firmware/obc-app/src/screen/settings/firmware.rs b/firmware/obc-app/src/screen/settings/firmware.rs index d2fac7123..e352b0a49 100644 --- a/firmware/obc-app/src/screen/settings/firmware.rs +++ b/firmware/obc-app/src/screen/settings/firmware.rs @@ -46,7 +46,7 @@ impl FirmwareScreen { // board answers through `App::apply_event`, which swaps the wait for the // confirm screen or an error card. Gesture::Press if !cx.activity.is_tracking() => { - cx.activity.request_dfu(crate::activity::DfuAction::Scan); + cx.dfu.admit_intent(crate::dfu::DfuIntent::ScanRequested); Transition::Push(Screen::DfuCheck(DfuCheckScreen::new())) } Gesture::Back => Transition::Pop, // climb back to the System menu @@ -171,21 +171,30 @@ mod tests { use crate::settings::Settings; use crate::{AppState, Mode}; - fn run(scr: &mut FirmwareScreen, act: &mut Activity, g: Gesture) -> Transition { + fn run(scr: &mut FirmwareScreen, act: &mut Activity, dfu: &mut crate::dfu::DfuState, g: Gesture) -> Transition { let mut st = AppState::new(0, 0, 1.0); let mut settings = Settings::default(); - let mut cx = test_ctx(&mut st, act, &mut settings); + let mut cx = Ctx { dfu, ..test_ctx(&mut st, act, &mut settings) }; scr.handle(g, &mut cx) } + /// The phase the DFU domain is holding, in the legacy vocabulary the tests already speak. + fn drained_dfu(dfu: &mut crate::dfu::DfuState) -> Option { + dfu.next_effect().map(|effect| match effect { + crate::dfu::DfuEffect::Scan { .. } => DfuAction::Scan, + crate::dfu::DfuEffect::ArmInstall { .. } => DfuAction::Install, + }) + } + /// Not recording: pressing Install posts the scan one-shot and opens the "Checking card..." wait. #[test] fn press_posts_scan_and_opens_the_check_wait() { let mut act = Activity::new(Mode::Idle); let mut scr = FirmwareScreen::new(); - let t = run(&mut scr, &mut act, Gesture::Press); + let mut dfu = crate::dfu::DfuState::new(); + let t = run(&mut scr, &mut act, &mut dfu, Gesture::Press); assert!(matches!(t, Transition::Push(Screen::DfuCheck(_))), "opens the scan wait"); - assert_eq!(act.take_dfu_request(), Some(DfuAction::Scan), "and posts a Scan request"); + assert_eq!(drained_dfu(&mut dfu), Some(DfuAction::Scan), "and posts a Scan request"); } /// Recording: the row is disabled — a press does nothing and posts nothing (the arm reboots). @@ -194,9 +203,10 @@ mod tests { let mut act = Activity::new(Mode::Riding); act.start_session(); // is_tracking() ⇒ true let mut scr = FirmwareScreen::new(); - let t = run(&mut scr, &mut act, Gesture::Press); + let mut dfu = crate::dfu::DfuState::new(); + let t = run(&mut scr, &mut act, &mut dfu, Gesture::Press); assert!(matches!(t, Transition::None), "disabled while recording"); - assert_eq!(act.take_dfu_request(), None, "and nothing is posted"); + assert_eq!(drained_dfu(&mut dfu), None, "and nothing is posted"); } /// Back climbs to the System menu. @@ -204,6 +214,6 @@ mod tests { fn back_pops_to_system() { let mut act = Activity::new(Mode::Idle); let mut scr = FirmwareScreen::new(); - assert!(matches!(run(&mut scr, &mut act, Gesture::Back), Transition::Pop)); + assert!(matches!(run(&mut scr, &mut act, &mut crate::dfu::DfuState::new(), Gesture::Back), Transition::Pop)); } } diff --git a/firmware/obc-app/src/screen/settings/system.rs b/firmware/obc-app/src/screen/settings/system.rs index e9e9d29aa..15e6d9bf1 100644 --- a/firmware/obc-app/src/screen/settings/system.rs +++ b/firmware/obc-app/src/screen/settings/system.rs @@ -48,7 +48,7 @@ impl SystemScreen { // The Firmware page shows `Card free`, so run the one-shot FAT scan on entry (the // host answers via `App::apply_event`) — the same trigger the old top-level // System row carried. - cx.activity.request_card_scan(); + cx.storage.admit_intent(crate::device_core::storage_info::StorageInfoIntent::RefreshRequested); Transition::Push(Screen::Firmware(FirmwareScreen::new())) } ABOUT => Transition::Push(Screen::About(AboutScreen::new())), @@ -77,13 +77,20 @@ impl SystemScreen { mod tests { use super::*; use crate::activity::Activity; + use crate::device_core::storage_info::StorageInfo; use crate::screen::test_ctx; use crate::{AppState, Mode, Settings}; fn run(scr: &mut SystemScreen, act: &mut Activity, g: Gesture) -> Transition { + run_with(scr, act, &mut StorageInfo::new(), g) + } + + /// The same, over the caller's own StorageInfo — the Firmware row's free-space refresh lands + /// there, so a test that asserts on it hands its own in. + fn run_with(scr: &mut SystemScreen, act: &mut Activity, storage: &mut StorageInfo, g: Gesture) -> Transition { let mut st = AppState::new(0, 0, 1.0); let mut s = Settings::default(); - let mut cx = test_ctx(&mut st, act, &mut s); + let mut cx = Ctx { storage, ..test_ctx(&mut st, act, &mut s) }; scr.handle(g, &mut cx) } @@ -99,8 +106,12 @@ mod tests { assert!(matches!(run(&mut scr, &mut act, Gesture::Press), Transition::Push(Screen::Language(_)))); run(&mut scr, &mut act, Gesture::Step(1)); // → Firmware update assert_eq!(scr.selected, FIRMWARE); - assert!(matches!(run(&mut scr, &mut act, Gesture::Press), Transition::Push(Screen::Firmware(_)))); - assert!(act.take_card_scan_request(), "opening Firmware arms the free-cluster scan"); + let mut storage = StorageInfo::new(); + assert!(matches!( + run_with(&mut scr, &mut act, &mut storage, Gesture::Press), + Transition::Push(Screen::Firmware(_)) + )); + assert!(storage.next_effect().is_some(), "opening Firmware arms the free-space measurement"); run(&mut scr, &mut act, Gesture::Step(1)); // → About assert_eq!(scr.selected, ABOUT); assert!(matches!(run(&mut scr, &mut act, Gesture::Press), Transition::Push(Screen::About(_)))); diff --git a/firmware/obc-app/src/settings.rs b/firmware/obc-app/src/settings.rs index b611286ef..2174d1ef9 100644 --- a/firmware/obc-app/src/settings.rs +++ b/firmware/obc-app/src/settings.rs @@ -2033,7 +2033,8 @@ mod tests { // ==================== the Settings domain protocol (#1436) ==================== // // SettingsMachine owns the dirty revision, the debounce, the retry and the stale-ack rule that -// `HostPending` holds today. The platform executor writes **one** revision and says what happened; +// [`SettingsMachine`] holds since #1397 S2. The platform executor writes **one** revision and says +// what happened; // it decides nothing about when a write is owed or whether an old answer still counts. use crate::device_core::{OperationToken, SettingsTag}; @@ -2095,3 +2096,306 @@ impl SettingsOutcome { const _: () = assert!(core::mem::size_of::() <= 4, "a revision or nothing"); const _: () = assert!(core::mem::size_of::() <= 8, "a token and a revision"); const _: () = assert!(core::mem::size_of::() <= 8, "a token, a revision and a reason"); + +/// Bounded backoff before a failed settings persist may re-emit (map-plane millis, #810). Fixed and +/// coarse: a persist failure is rare (an RRAM/file write error), the value stays live in RAM +/// meanwhile, and the retry only re-emits on a frame that runs for another reason — so this paces +/// retries without ever scheduling an idle wake. +pub(crate) const SETTINGS_RETRY_BACKOFF_MS: u32 = 2_000; + +/// Wrap-safe "deadline reached" in the persist-backoff's **u16** millisecond space (the low 16 bits +/// of map-plane millis): true while `now` sits in the half-window at or past `deadline`. The u16 +/// domain wraps every 65.5 s, so a frame gap longer than ~32.7 s can park a due retry in the "not +/// yet" half and slide it by up to one wrap — bounded, harmless for a rare failure path, and the +/// price of keeping the deadline to two resident bytes (#792 rule 2). +fn retry_deadline_reached(now: u16, deadline: u16) -> bool { + now.wrapping_sub(deadline) < 0x8000 +} + +/// Where the settings-persistence handshake is (#810). +/// +/// Deliberately **fieldless** (one byte): the Backoff deadline lives in the sibling +/// [`SettingsMachine::retry_at_ms`] field (meaningful only in Backoff), so this byte packs into an +/// existing padding hole instead of an 8-byte payload-carrying enum. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] +enum PersistState { + /// The live settings are persisted at the current revision. + #[default] + Clean, + /// An edit changed the live settings; a save is owed once the rider leaves the settings subtree. + Dirty, + /// A write was emitted for the current revision and awaits its answer. + Awaiting, + /// The last write failed; no retry re-emits before the deadline. + Backoff, +} + +/// The settings domain's persistence machine: the dirty revision, the subtree debounce, the retry +/// backoff and the stale-answer rule. +/// +/// Editing is live in RAM the instant it happens; *persisting* it is an acknowledged, retryable +/// cross-boundary conversation keyed by the monotonic [`revision`](SettingsMachine::revision). +/// +/// - **Clean** — persisted. An edit → **Dirty** (and bumps the revision). +/// - **Dirty** — a save is owed. Once outside the subtree, the next effect writes it → **Awaiting**. +/// - **Awaiting** — emitted and waiting; **not re-emitted** (no RRAM spam under a slow executor). A +/// matching success → **Clean**; a matching failure → **Backoff**. An edit here → **Dirty** +/// (supersede: the new revision re-emits, and the older answer no longer matches). An executor +/// that takes the write but never answers (the web demo has no durable store) parks here +/// terminally — by design: edits stay live in RAM and keep superseding, and nothing re-emits. +/// - **Backoff** — the last write failed; re-emits only once the deadline is reached. An edit → +/// **Dirty**, so a fresh revision skips the wait. +/// +/// The revision is the supersede guard: an answer is honoured only when it equals the current one. +/// `u16` monotonic (wrapping) — a false match would need exactly 65,536 edits between an emit and +/// its answer, and only one revision is ever Awaiting. +#[derive(Debug, Default)] +pub(crate) struct SettingsMachine { + /// The operation token for the write an executor is running. + ops: crate::device_core::TokenSource, + /// The revision of the live settings, bumped by every edit whose before/after compare finds a + /// change. Starts `0`, re-zeroed when the boot value is seeded. + revision: u16, + /// The [`Backoff`](PersistState::Backoff) retry deadline — the **low 16 bits** of map-plane + /// millis. Meaningful only while [`persist`](SettingsMachine::persist) is Backoff. + retry_at_ms: u16, + /// Where the handshake is. + persist: PersistState, +} + +impl SettingsMachine { + /// The boot state: Clean at revision 0 — the boot value came from the store or the default. + pub(crate) const fn new() -> Self { + SettingsMachine { + ops: crate::device_core::TokenSource::new(), + revision: 0, + retry_at_ms: 0, + persist: PersistState::Clean, + } + } + + /// Admit one settings intent. + /// + /// [`Changed`](SettingsIntent::Changed) from *any* prior state supersedes an in-flight or + /// backing-off older revision: the new content re-emits, and the older answer, when it lands, + /// no longer matches the revision and is ignored (#810). + pub(crate) fn admit_intent(&mut self, intent: SettingsIntent) { + match intent { + SettingsIntent::Changed { revision } => { + self.revision = revision; + self.persist = PersistState::Dirty; + } + // The backoff elapsing is not a state change on its own: `next_effect` re-derives the + // deadline from the clock it is handed, so a due retry emits without anything to latch. + SettingsIntent::RetryDue => {} + } + } + + /// A rider edit: bump the revision and (re-)arm the save. + pub(crate) fn note_edited(&mut self) { + let revision = self.revision.wrapping_add(1); + self.admit_intent(SettingsIntent::Changed { revision }); + } + + /// The boot value was just seeded from the store (or the default): it is already persisted, so + /// reset to Clean at revision 0. Any pending edit is discarded — seeding is a boot/reload + /// operation, not a rider edit. + pub(crate) fn note_seeded(&mut self) { + self.revision = 0; + self.persist = PersistState::Clean; + } + + /// Whether a write is owed **and** may be emitted now: the value is dirty, the rider has left + /// the settings subtree, and we are neither already awaiting an answer nor inside a failed-write + /// backoff window. + pub(crate) fn wants_write(&self, in_settings_subtree: bool, now_ms: u32) -> bool { + if in_settings_subtree { + return false; + } + match self.persist { + PersistState::Dirty => true, + PersistState::Backoff => retry_deadline_reached(now_ms as u16, self.retry_at_ms), + PersistState::Clean | PersistState::Awaiting => false, + } + } + + /// The next bounded settings operation, or `None` when none is owed this pass. + /// + /// The dirty state is *not* cleared here (the #810 fix): a failed write must keep the revision + /// retryable, so Clean is reached only by a matching success. + pub(crate) fn next_effect(&mut self, in_settings_subtree: bool, now_ms: u32) -> Option { + if !self.wants_write(in_settings_subtree, now_ms) { + return None; + } + self.persist = PersistState::Awaiting; + Some(SettingsEffect::PersistRevision { token: self.ops.issue(), revision: self.revision }) + } + + /// Consume the answer to a write. Returns `true` when the write **failed** and the rider must + /// be told — the one part of this the domain cannot do itself. + /// + /// Both guards are checked, and they are independent: the token rejects a superseded + /// *operation*, the revision rejects a stale *value*. A stale answer leaves the newer content + /// pending either way. + pub(crate) fn apply_outcome(&mut self, outcome: SettingsOutcome, now_ms: u32) -> bool { + if !self.ops.is_current(outcome.token()) { + return false; + } + self.ops.invalidate(); // terminal: a duplicate of this answer is no longer current + match outcome { + SettingsOutcome::Persisted { revision, .. } => { + self.note_persisted(revision); + false + } + SettingsOutcome::PersistFailed { revision, .. } => self.note_persist_failed(revision, now_ms), + // A platform with no durable store says so here instead of parking the handshake + // forever. The value stays dirty and retryable; nothing is claimed to have been written. + SettingsOutcome::Cancelled { .. } => { + if self.persist == PersistState::Awaiting { + self.persist = PersistState::Dirty; + } + false + } + } + } + + /// `revision` reached durable storage. Clear to Clean **only** while it is still the latest — a + /// newer edit has already moved the machine back to Dirty, and that content stays pending. + /// + /// The revision is the whole guard here, because the legacy protocol carries no token: an + /// answer to a write nobody made cannot be distinguished from a stale one, and both leave the + /// live value exactly where it is. + pub(crate) fn note_persisted(&mut self, revision: u16) { + if self.persist == PersistState::Awaiting && revision == self.revision { + self.persist = PersistState::Clean; + } + } + + /// The write for `revision` failed. Keep it dirty and re-arm the bounded backoff, but only + /// while it is still the in-flight latest. Returns whether the rider is told. + pub(crate) fn note_persist_failed(&mut self, revision: u16, now_ms: u32) -> bool { + if self.persist == PersistState::Awaiting && revision == self.revision { + self.retry_at_ms = (now_ms as u16).wrapping_add(SETTINGS_RETRY_BACKOFF_MS as u16); + self.persist = PersistState::Backoff; + } + true + } + + /// Test hook: arm a pending save without driving a real edit, standing in for a settings-screen + /// edit the drain/gating tests do not replay. + #[cfg(test)] + pub(crate) fn arm_save(&mut self) { + self.note_edited(); + } + + /// Whether nothing at all is owed: Clean at revision 0 — the [`new`](SettingsMachine::new) + /// state. The destructure is exhaustive, so a field added here must state its empty value too. + #[cfg(test)] + pub(crate) fn is_empty(&self) -> bool { + let SettingsMachine { ops, revision, retry_at_ms, persist } = self; + format!("{ops:?}") == "TokenSource(0)" && *revision == 0 && *retry_at_ms == 0 && *persist == PersistState::Clean + } +} + +// Layout tripwire: a revision, a deadline, a phase and a generation — never a `Settings`. +const _: () = assert!(core::mem::size_of::() <= 12, "the handshake, not the values"); + +#[cfg(test)] +mod settings_machine_tests { + use super::*; + use obc_ports::SettingsSaveError; + + /// The token a write went out under, so a test can answer the operation the machine is actually + /// holding. + fn emit( + machine: &mut SettingsMachine, + now_ms: u32, + ) -> (crate::device_core::OperationToken, u16) { + match machine.next_effect(false, now_ms).expect("a write is owed") { + SettingsEffect::PersistRevision { token, revision } => (token, revision), + } + } + + /// The debounce: nothing is written while the rider is still inside the settings subtree — they + /// are mid-edit — and exactly one write goes out when they leave. + #[test] + fn no_write_leaves_while_the_rider_is_inside_the_settings_subtree() { + let mut machine = SettingsMachine::new(); + machine.note_edited(); + assert!(machine.next_effect(true, 100).is_none(), "still editing"); + assert!(machine.next_effect(true, 200).is_none(), "…and still editing"); + + let (_, revision) = emit(&mut machine, 300); + assert_eq!(revision, 1, "the edit's revision leaves once"); + assert!(machine.next_effect(false, 400).is_none(), "awaiting an answer — never re-emitted"); + } + + /// **#810.** A stale ack — one for a revision a newer edit has already superseded — must not + /// clear the newer content. The revision is the guard, and it is checked independently of the + /// token: the legacy protocol carries no token at all. + #[test] + fn a_stale_ack_does_not_clear_the_newer_state() { + let mut machine = SettingsMachine::new(); + machine.note_edited(); // revision 1 + emit(&mut machine, 100); + machine.note_edited(); // revision 2 supersedes it while the write is in flight + + machine.note_persisted(1); + assert!(machine.wants_write(false, 200), "the newer content is still owed"); + let (_, revision) = emit(&mut machine, 200); + assert_eq!(revision, 2, "and it is the newer revision that goes out"); + machine.note_persisted(2); + assert!(!machine.wants_write(false, 300), "the matching ack is what clears it"); + } + + /// A failed write keeps the revision dirty, backs off, and retries **once** the window elapses — + /// never before it, and never in a loop. + #[test] + fn a_failed_write_backs_off_and_retries_once() { + let mut machine = SettingsMachine::new(); + machine.note_edited(); + let (_, revision) = emit(&mut machine, 1_000); + + assert!(machine.note_persist_failed(revision, 1_000), "the rider is told a save failed"); + assert!(!machine.wants_write(false, 1_000 + SETTINGS_RETRY_BACKOFF_MS - 1), "not before the window"); + assert!(machine.wants_write(false, 1_000 + SETTINGS_RETRY_BACKOFF_MS), "and exactly once at it"); + + let (_, retried) = emit(&mut machine, 1_000 + SETTINGS_RETRY_BACKOFF_MS); + assert_eq!(retried, revision, "the same content, not a new one"); + assert!(machine.next_effect(false, 1_000 + 4 * SETTINGS_RETRY_BACKOFF_MS).is_none(), "one retry in flight"); + } + + /// A platform that takes the write and never answers (the web demo has no durable store) parks + /// — by design. Edits stay live in RAM and keep superseding, and nothing re-emits into a store + /// that will not answer. + #[test] + fn an_executor_that_never_answers_parks_without_re_emitting() { + let mut machine = SettingsMachine::new(); + machine.note_edited(); + emit(&mut machine, 100); + for ms in [200, 10_000, 100_000, 1_000_000] { + assert!(machine.next_effect(false, ms).is_none(), "no RRAM spam under a silent executor"); + } + + // …and a `Cancelled` answer is how such a platform says so honestly: the value stays dirty + // and retryable rather than parked forever. + machine.note_edited(); + let (token, _) = emit(&mut machine, 2_000_000); + assert!(!machine.apply_outcome(SettingsOutcome::Cancelled { token }, 2_000_000)); + assert!(machine.wants_write(false, 2_000_001), "the write is owed again"); + } + + /// The token and the revision are independent guards: an answer to a *superseded operation* is + /// refused before its revision is even looked at. + #[test] + fn a_superseded_operation_is_refused_on_its_token() { + let mut machine = SettingsMachine::new(); + machine.note_edited(); + let (first, revision) = emit(&mut machine, 100); + machine.note_edited(); + emit(&mut machine, 200); + + let stale = SettingsOutcome::PersistFailed { token: first, revision, error: SettingsSaveError::Backend }; + assert!(!machine.apply_outcome(stale, 200), "a superseded write cannot report a failure to the rider"); + } +} diff --git a/firmware/obc-app/tests/nav.rs b/firmware/obc-app/tests/nav.rs index 019b98855..13fa6f9d0 100644 --- a/firmware/obc-app/tests/nav.rs +++ b/firmware/obc-app/tests/nav.rs @@ -76,7 +76,7 @@ fn request_route(app: &mut App) -> NavRequest { plan_req(app).expect("Create route records the one-shot request") } -/// The drained `PlanRoute` request, if one is pending (the `take_nav_request` successor): drain the +/// The drained `PlanRoute` request, if one is pending (the `PlanRoute` drain): drain the /// typed protocol and pick the plan out of the mailbox. FAR-19, #812. fn plan_req(app: &mut App) -> Option { let mut mb: HostMailbox = HostMailbox::new(); @@ -87,7 +87,7 @@ fn plan_req(app: &mut App) -> Option { }) } -/// Whether a `CancelRoutePlan` is pending (the `take_nav_cancel` successor). FAR-19, #812. +/// Whether a `CancelRoutePlan` is pending (the `CancelRoutePlan` peek). FAR-19, #812. fn took_cancel(app: &mut App) -> bool { let mut mb: HostMailbox = HostMailbox::new(); let _ = app.drain_host_commands(&mut mb); diff --git a/firmware/obc-fw-nrf54l/src/ride.rs b/firmware/obc-fw-nrf54l/src/ride.rs index b69e93242..58c6a9e60 100644 --- a/firmware/obc-fw-nrf54l/src/ride.rs +++ b/firmware/obc-fw-nrf54l/src/ride.rs @@ -1157,7 +1157,7 @@ pub(crate) async fn run_app( // design, with its own on-glass acceptance — a separate issue. // // So: answer the typed failure the moment the request drains, exactly as the - // router-less image answers `PlanRoute`. `on_detour_planned` swaps the spinner for + // router-less image answers `PlanRoute`. the detour answer swaps the spinner for // the "Try a farther rejoin." card and releases the freeze in the same pass. obc_app::HostCommand::PlanDetour(_) => { defmt::warn!("nav: detour planning has no board half yet (#882) — answering the failure tier"); @@ -2193,7 +2193,7 @@ pub(crate) async fn run_app( // the reflective panel keeps the last frame on glass for free. Two things follow, and // both are load-bearing. The map redraw is **skipped, not queued** — latched into // `pending_map_redraw` so nothing is lost and the catch-up lands the pass the freeze - // lifts (`App::note_plan_ended` dirties the map for exactly that). And the *overlay* + // lifts (Navigator dirties the map for exactly that). And the *overlay* // still paints: `dirty.overlay` carries the freeze's edge, and the banner is what turns // a frozen screen from "the device wedged" into "it is recalculating". // diff --git a/firmware/tools/resource_baseline.json b/firmware/tools/resource_baseline.json index 145c75c41..e7be1591c 100644 --- a/firmware/tools/resource_baseline.json +++ b/firmware/tools/resource_baseline.json @@ -58,10 +58,11 @@ "_compile_note_dc4_1437": "**DC4 #1437 domain boundaries: `app` 50,520 -> 50,680 B (+160 B), both profiles.** Itemised on the CI `embedded` run for this branch against its base (099bd1be), all four items structural and none of them a buffer: (1) +72 B for the three derived cache keys in `CatalogState` (`ride_profile_for`, `ride_preview_for`, `nav_preview_route`) widening from `Option` (8 B) to `Option` / `Option` (32 B) — a durable object identity plus a source and a view revision, which is what removes the catalog-index remap and with it the late-answer and replaced-bytes staleness holes; (2) +24 B for the three `Revision` counters those keys read (`source_revision`, `ride_track_view`, `nav_preview_view`); (3) +40 B for the new `WeatherDomain` field on `App` (a token source, the installed `WeatherData` identity/revision, the in-flight refresh token, the request flag and the last terminal result); (4) +24 B inside `RetentionMachine` (its own token source and the in-flight sidecar-write slot, so a failed metadata write can be re-queued for the id it was actually about). Linked resident clears the 320,688 B `resident_ram_max` ceiling by ~16 KB on both profiles and `.uninit` is untouched at 132,096 B, so nothing moves on glass. Re-verified against S3 #1446's placement-constructor merge: `app` reads 50,680 B on both profiles there too, and `init_idle`'s own frame is 76 B of the 4,096 B ceiling. `measured_resident` (320,616 B) is a record, not a gate, and is left as it stands: it predates this branch.", "_compile_note_dc5_1438": "**DC5 #1438 pass coordinator: `app` 50,680 -> 50,920 B (+240 B), both profiles.** Two structural items, measured on the thumbv8m target, neither of them a buffer: (1) +224 B for the new `PassState` field on `App` \u2014 136 B of it the `Connections` set (eight named cross-domain slots: two UI intents, the expiry intent, the active-route notice, the fault bit set, and the three deferred slots whose two halves are what make a later-to-earlier value land on the *next* pass), and 88 B the levels a stage detects an edge against (the last store revision seen, the revision the catalog announced, the last link state, the transfer state, the active route identity, the 12 B `Capabilities` the admission stage recalculates, and the re-entrancy flag); (2) +16 B in `CatalogState` for the catalog domain's admitted-intent slot (`Option` is 16 B \u2014 one identity and a tag), with its 4 B token source and the in-flight flag landing in existing padding. The test-only stage recorder is `#[cfg(test)]` and is not in this figure. Linked resident and `.uninit` are unchanged (304,824 B / 132,096 B on the pinned host, against the 320,688 B ceiling), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B against its 4,096 B limit \u2014 the field is written straight into its `.bss` slot like every other.", "_compile_note_dc7_1440": "**DC7 #1440 conformance gate: `app` 50,920 -> 50,904 B (-16 B), both profiles.** A saving, from deleting the `UiRuntime` -> `Recorder` connection the gate proved was destroying a rider request: the pass took the ride-close one-shot at stage 4 and dropped it at stage 7, where Recorder has no machine to act on it, so the ride was never finalized and no executor was told. Out of `Connections` go the `Slot` and the `Deferred`; with the last `KeepFirst` connection gone, `Merge` goes with them and both remaining deferred slots lose their merge field. The close is back on the legacy drain that performs it, and returns with Recorder's machine at #1397 S6. Linked resident and `.uninit` are unchanged within the ceilings (304,808 B / 132,096 B on the pinned host, against 320,688 B), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B of its 4,096 B limit.", + "_compile_note_s2_1485": "**#1397 S2 answered-domain seams: `app` 50,904 -> 50,928 B (+24 B), both profiles.** The four domains whose legacy answer is already terminal get their own pending state, so the rider's request stops living in `Activity` and starts living with its owner. Out of `Activity` go seven one-shot slots (`nav_request`, `detour_request`, `detour_commit`, `detour_cancel`, `dfu_request`, `nav_cancel`, `card_scan_request`) for -88 B; out of `App` go `card_free_bytes` (-16 B) and `HostPending`, whose persist half moved to `SettingsMachine` and whose remaining store-commit counter is now a bare `u32` (-8 B). In their place: `NavigatorMachine` (96 B \u2014 the two bounded planner requests, the per-family phase, and the `RerouteFreeze` that moved in from `App` so the domain is its only writer), `StorageInfo` (24 B \u2014 the free-space figure `card_free_bytes` used to hold, plus its token), `SettingsMachine` (12 B) and `DfuState` (8 B). The +24 B is padding plus the four token generations the epic's ownership rule requires: a domain that cannot validate a token cannot own an outcome (#1433 \u00a74.3). Linked resident 304,832 B and residual main stack 54,592 B on the pinned host (against 320,688 B and a 38,808 B floor); flash 1,449,328 B against 1,524,676 B; the arena, the poll frame (9,792 B) and the main task future (1,100 B) are untouched.", "compile_time_allocations": { "framebuffer": 76800, "row_diff": 1284, - "app": 50904, + "app": 50928, "map_cache": 37084, "map_tables": 4416, "route_cache": 9260, @@ -199,10 +200,11 @@ "_compile_note_dc4_1437": "**DC4 #1437 domain boundaries: `app` 50,520 -> 50,680 B (+160 B), both profiles.** Itemised on the CI `embedded` run for this branch against its base (099bd1be), all four items structural and none of them a buffer: (1) +72 B for the three derived cache keys in `CatalogState` (`ride_profile_for`, `ride_preview_for`, `nav_preview_route`) widening from `Option` (8 B) to `Option` / `Option` (32 B) — a durable object identity plus a source and a view revision, which is what removes the catalog-index remap and with it the late-answer and replaced-bytes staleness holes; (2) +24 B for the three `Revision` counters those keys read (`source_revision`, `ride_track_view`, `nav_preview_view`); (3) +40 B for the new `WeatherDomain` field on `App` (a token source, the installed `WeatherData` identity/revision, the in-flight refresh token, the request flag and the last terminal result); (4) +24 B inside `RetentionMachine` (its own token source and the in-flight sidecar-write slot, so a failed metadata write can be re-queued for the id it was actually about). Linked resident clears the 320,688 B `resident_ram_max` ceiling by ~16 KB on both profiles and `.uninit` is untouched at 132,096 B, so nothing moves on glass. Re-verified against S3 #1446's placement-constructor merge: `app` reads 50,680 B on both profiles there too, and `init_idle`'s own frame is 76 B of the 4,096 B ceiling. `measured_resident` (320,616 B) is a record, not a gate, and is left as it stands: it predates this branch.", "_compile_note_dc5_1438": "**DC5 #1438 pass coordinator: `app` 50,680 -> 50,920 B (+240 B), both profiles.** Two structural items, measured on the thumbv8m target, neither of them a buffer: (1) +224 B for the new `PassState` field on `App` \u2014 136 B of it the `Connections` set (eight named cross-domain slots: two UI intents, the expiry intent, the active-route notice, the fault bit set, and the three deferred slots whose two halves are what make a later-to-earlier value land on the *next* pass), and 88 B the levels a stage detects an edge against (the last store revision seen, the revision the catalog announced, the last link state, the transfer state, the active route identity, the 12 B `Capabilities` the admission stage recalculates, and the re-entrancy flag); (2) +16 B in `CatalogState` for the catalog domain's admitted-intent slot (`Option` is 16 B \u2014 one identity and a tag), with its 4 B token source and the in-flight flag landing in existing padding. The test-only stage recorder is `#[cfg(test)]` and is not in this figure. Linked resident and `.uninit` are unchanged (304,824 B / 132,096 B on the pinned host, against the 320,688 B ceiling), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B against its 4,096 B limit \u2014 the field is written straight into its `.bss` slot like every other.", "_compile_note_dc7_1440": "**DC7 #1440 conformance gate: `app` 50,920 -> 50,904 B (-16 B), both profiles.** A saving, from deleting the `UiRuntime` -> `Recorder` connection the gate proved was destroying a rider request: the pass took the ride-close one-shot at stage 4 and dropped it at stage 7, where Recorder has no machine to act on it, so the ride was never finalized and no executor was told. Out of `Connections` go the `Slot` and the `Deferred`; with the last `KeepFirst` connection gone, `Merge` goes with them and both remaining deferred slots lose their merge field. The close is back on the legacy drain that performs it, and returns with Recorder's machine at #1397 S6. Linked resident and `.uninit` are unchanged within the ceilings (304,808 B / 132,096 B on the pinned host, against 320,688 B), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B of its 4,096 B limit.", + "_compile_note_s2_1485": "**#1397 S2 answered-domain seams: `app` 50,904 -> 50,928 B (+24 B), both profiles.** The four domains whose legacy answer is already terminal get their own pending state, so the rider's request stops living in `Activity` and starts living with its owner. Out of `Activity` go seven one-shot slots (`nav_request`, `detour_request`, `detour_commit`, `detour_cancel`, `dfu_request`, `nav_cancel`, `card_scan_request`) for -88 B; out of `App` go `card_free_bytes` (-16 B) and `HostPending`, whose persist half moved to `SettingsMachine` and whose remaining store-commit counter is now a bare `u32` (-8 B). In their place: `NavigatorMachine` (96 B \u2014 the two bounded planner requests, the per-family phase, and the `RerouteFreeze` that moved in from `App` so the domain is its only writer), `StorageInfo` (24 B \u2014 the free-space figure `card_free_bytes` used to hold, plus its token), `SettingsMachine` (12 B) and `DfuState` (8 B). The +24 B is padding plus the four token generations the epic's ownership rule requires: a domain that cannot validate a token cannot own an outcome (#1433 \u00a74.3). Linked resident 304,832 B and residual main stack 54,592 B on the pinned host (against 320,688 B and a 38,808 B floor); flash 1,449,328 B against 1,524,676 B; the arena, the poll frame (9,792 B) and the main task future (1,100 B) are untouched.", "compile_time_allocations": { "framebuffer": 76800, "row_diff": 1284, - "app": 50904, + "app": 50928, "map_cache": 37084, "map_tables": 4416, "route_cache": 9260, diff --git a/host/obc-host-core/tests/board_parity.rs b/host/obc-host-core/tests/board_parity.rs index 510edb1ee..4dc876d1b 100644 --- a/host/obc-host-core/tests/board_parity.rs +++ b/host/obc-host-core/tests/board_parity.rs @@ -9,7 +9,7 @@ //! //! - `RescanStore` re-feeds the catalog before subsequent work (the board's //! `take_store_changed → refeed`), so an upload/delete id resolves against the rescanned catalog. -//! - `PlanRoute` is consumed into the resumable planner (the board's `take_nav_request → plan step`), +//! - `PlanRoute` is consumed into the resumable planner (the board's `PlanRoute → plan step`), //! and a `CancelRoutePlan` posted in the same input batch **annihilates** it, so the dispatcher //! never starts a plan the rider already dismissed. @@ -45,7 +45,7 @@ fn rescan_refeeds_the_catalog_like_the_board() { assert!(!app.route_ids().contains(&gone), "the deleted id is gone from the app catalog too"); } -/// The board consumes `take_nav_request` into its one-step-per-pass planner; the dispatcher consumes +/// The board consumes `PlanRoute` into its one-step-per-pass planner; the dispatcher consumes /// `PlanRoute` into the resumable [`NavPlan`] the same way (`is_planning` after the pass). #[test] fn plan_route_enters_the_resumable_planner_like_the_board() { @@ -95,7 +95,7 @@ fn completion_matches_repeated_frame_steps() { /// A `debug_start_nav` immediately dismissed (the confirm→Back annihilation, #837) leaves the /// dispatcher with **no** plan: the cancel clears the undrained request at post time, exactly as it -/// does for the board's `take_nav_cancel` before `take_nav_request`. +/// does for the board's `CancelRoutePlan` before `PlanRoute`. #[test] fn cancel_before_the_pass_starts_no_plan() { let map = obc_fixtures::read("sim-grimsel", "grimsel.obcm").expect("full fixture suite requires map"); diff --git a/host/obc-host-core/tests/device_core_compat.rs b/host/obc-host-core/tests/device_core_compat.rs index 7c1c62794..974acfc3f 100644 --- a/host/obc-host-core/tests/device_core_compat.rs +++ b/host/obc-host-core/tests/device_core_compat.rs @@ -206,9 +206,18 @@ fn the_adapter_path_completes_one_operation_per_unanswerable_domain() { let mut host = expiring_host(); let (_, report) = host.pass(10); - assert_eq!(report.translated, 1, "the active route's use stamp is a legacy command"); + // Two domains speak in the boot pass: retention's use stamp for the active route, and the + // settings write the trusted-clock stamp made owed (#1397 S2 moved that handshake into + // `SettingsMachine`, so the pass — not the drain — is what offers it). + assert_eq!(report.translated, 2, "the use stamp and the settings write are both legacy commands"); assert_eq!(report.left, 0); - assert_eq!(host.sent, vec![HostCommand::StampRouteUsed { id: 11, utc: host.app.wall_unix_now() }]); + assert_eq!( + host.sent, + vec![ + HostCommand::StampRouteUsed { id: 11, utc: host.app.wall_unix_now() }, + HostCommand::PersistSettings { revision: 1 }, + ] + ); let (plan, report) = host.pass(20); assert_eq!(report.left, 1, "the expiry's removal has no legacy expression"); @@ -232,7 +241,14 @@ fn the_adapter_path_completes_one_operation_per_unanswerable_domain() { assert_eq!(report, LegacyReport::default(), "no domain offers anything further"); assert!(!plan.effects.has_pending(), "and the pass has nothing left to offer either"); } - assert_eq!(host.sent.len(), 1, "one command in the device's whole life on this path"); + assert_eq!(host.sent.len(), 2, "two commands in the device's whole life on this path"); + + // The difference an *answerable* domain makes, in the same run: the settings write has a + // terminal legacy event, so its answer reaches `SettingsMachine` and the domain speaks again. + host.deliver(HostEvent::SettingsPersisted { revision: 1 }).unwrap(); + let (_, report) = host.pass(80); + assert_eq!(report, LegacyReport::default(), "the write is done — nothing is owed"); + assert!(host.adapter.pending().is_empty(), "and its correlation slot is free again"); } /// The fact half of the protocol, end to end: a store commit, an upload and a warning all reach the @@ -245,7 +261,12 @@ fn legacy_facts_reach_the_rider_through_the_pass() { host.deliver(HostEvent::StoreChanged).unwrap(); host.deliver(HostEvent::Warning(WarningFlags::NO_GPS)).unwrap(); host.deliver(HostEvent::RouteUploaded { id: 22, replaced: false, elevation: None }).unwrap(); - assert!(host.adapter.pending().is_empty(), "a fact is nobody's answer"); + let owed: Vec<_> = LegacyReply::ALL.into_iter().filter(|class| host.adapter.pending().holds(*class)).collect(); + assert_eq!( + owed, + vec![LegacyReply::SettingsWrite], + "a fact is nobody's answer — the only slot in flight is the boot pass's settings write" + ); host.pass(20); assert!(host.app.debug_stack_len() > 1, "the facts put something in front of the rider"); diff --git a/host/obc-host-core/tests/device_core_conformance.rs b/host/obc-host-core/tests/device_core_conformance.rs index ebfb400a0..c7bad667c 100644 --- a/host/obc-host-core/tests/device_core_conformance.rs +++ b/host/obc-host-core/tests/device_core_conformance.rs @@ -37,16 +37,19 @@ //! is rediscovered and re-issued on every later pass. //! [`a_stamp_that_was_answered_is_not_enqueued_again`] pins the ride arm. //! -//! ## What Phase 1 does and does not own +//! ## What DeviceCore owns here, and what it does not //! -//! Three domains have a state machine today — the catalog, retention and weather — and those are -//! the three whose effects a pass emits and whose outcomes it consumes. Two of them can be reached -//! from outside `obc-app`: weather's refresh intent has no public door until #1401 lands the request -//! cutover, so this executor serves the catalog and retention, and asserts the rest stays empty. +//! Seven domains have a state machine: the catalog, retention and weather from #1438, and the four +//! #1397 S2 added — Navigator, `SettingsMachine`, `DfuState` and `StorageInfo`. Six of them can be +//! reached from outside `obc-app` (weather's refresh intent has no public door until #1401 lands the +//! request cutover), so this executor serves those six and asserts the rest stays empty. //! -//! The other six domains speak the legacy protocol still, so both DeviceCore runners drain the -//! legacy mailbox for them. [`store_owned`] and [`derived_level`] are the line between the two, and -//! every class still on the old protocol has a [`LegacyOwned`] row naming the slice that moves it. +//! **Two** domains still speak the legacy protocol: Recorder and Bond. Both are named in +//! [`LegacyOwned`] with the reason — a ride close is answered by a catalog re-feed rather than by a +//! ride identity, and a bond removal by a link-status fact rather than by a reply — and a domain +//! that cannot validate a token cannot own an outcome (epic §4.3). [`pass_owned`] and +//! [`derived_level`] are the line between the two halves, and every class still on the old protocol +//! has a row naming the slice that moves it. mod device_core_corpus; @@ -58,13 +61,13 @@ use obc_app::device_core::compat::{event_reply, LegacyOwned, LegacyReply}; use obc_app::device_core::derived::{ DerivedInput, DerivedInputs, DerivedNeeds, DerivedResult, DerivedTargets, NavPreviewKey, RideTrackKey, }; -use obc_app::device_core::storage_info::StorageInfoEffect; +use obc_app::device_core::storage_info::{StorageInfoEffect, StorageInfoOutcome}; use obc_app::device_core::{ - Capabilities, DeviceFacts, EffectSlots, LegacyAdapter, LegacyInputs, NavigatorTag, OutcomeSlots, PassClock, - PassInputs, PassPlan, PlatformSupport, Revision, SettingsTag, StoreIdentity, StoreRevision, TokenSource, + Capabilities, DeviceFacts, EffectSlots, LegacyAdapter, LegacyInputs, NavigatorTag, OperationToken, OutcomeSlots, + PassClock, PassInputs, PassPlan, PlatformSupport, Revision, SettingsTag, StoreIdentity, StoreRevision, TokenSource, TransferState, }; -use obc_app::dfu::DfuEffect; +use obc_app::dfu::{DfuEffect, DfuOutcome}; use obc_app::navigator::{NavigatorEffect, NavigatorError, NavigatorOutcome, PlannerWork}; use obc_app::recorder::RecorderEffect; use obc_app::retention::{Retention, RetentionEffect, RetentionError, RetentionOutcome, RouteRetentionMeta}; @@ -216,19 +219,30 @@ fn rider_visible(mut state: VisibleState) -> VisibleState { // ==================== what the pass owns today ==================== -/// The legacy classes DeviceCore's pass has taken over outright. +/// The twelve legacy classes DeviceCore's pass has taken over outright. /// -/// A rider's delete is consumed at stage 4 and a retention stamp leaves as a `RetentionEffect`, so -/// neither pends on the old protocol any more. A DeviceCore runner asserts they do not appear rather -/// than filtering them: executing one beside the effect that already carries it would delete or -/// stamp twice, and a class that quietly came back would be the migration coming undone. -fn store_owned(command: &HostCommand) -> bool { +/// Four from #1438 — a rider's delete is consumed at stage 4 and a retention stamp leaves as a +/// `RetentionEffect` — and the eight #1397 S2 moved: every class whose terminal answer the legacy +/// protocol already delivers, and which therefore has a domain able to validate a token for it. +/// +/// A DeviceCore runner asserts they never appear in the mailbox rather than filtering them: +/// executing one beside the effect that already carries it would plan, install or delete twice, and +/// a class that quietly came back would be the migration coming undone. +fn pass_owned(command: &HostCommand) -> bool { matches!( command, HostCommand::DeleteRoute { .. } | HostCommand::DeleteRide { .. } | HostCommand::StampRouteUsed { .. } | HostCommand::StampRideSynced { .. } + | HostCommand::PlanRoute(_) + | HostCommand::PlanDetour(_) + | HostCommand::CommitDetour + | HostCommand::CancelRoutePlan + | HostCommand::CancelDetour + | HostCommand::Dfu(_) + | HostCommand::PersistSettings { .. } + | HostCommand::ScanCardFree ) } @@ -280,6 +294,11 @@ enum Done { refeed: Refeed, }, Retention(RetentionOutcome), + /// A typed answer for one of the four domains #1397 S2 gave a machine. + Navigator(NavigatorOutcome), + Settings(SettingsOutcome), + Dfu(DfuOutcome), + Storage(StorageInfoOutcome), /// A legacy answer, for a domain whose machine has not landed. Event(HostEvent), /// The ride the recorder just finalized — answered, as the legacy protocol answers it, by a @@ -312,6 +331,10 @@ struct CoreHarness { served: BTreeSet<&'static str>, /// Effects the adapter could not express at all, by row. left: BTreeSet, + /// The settings write the typed executor is holding. A real asynchronous executor keeps the + /// token of the operation it is performing; the corpus scripts some settings answers at the + /// *action* rather than at the request, so they are built against whatever is actually running. + settings_token: Option>, } impl CoreHarness { @@ -327,6 +350,7 @@ impl CoreHarness { moved: BTreeSet::new(), served: BTreeSet::new(), left: BTreeSet::new(), + settings_token: None, } } @@ -369,9 +393,9 @@ impl CoreHarness { /// Serve what a host outside `obc-app` can actually cause. /// - /// The catalog and retention are the two domains whose effects a rider action or a retention - /// sweep produces. Weather has a machine too, but its refresh intent has no public door yet - /// (#1401 owns the request cutover), and the remaining six have no machine at all — so nothing + /// Six domains reach this executor: the catalog and retention from #1438, and the four #1397 S2 + /// gave a machine. Weather has a machine too, but its refresh intent has no public door yet + /// (#1401 owns the request cutover), and Recorder and Bond have no machine at all — so nothing /// else may appear. Asserted rather than assumed: an effect this executor cannot serve turning /// up would be a silent change of who decides, and the run must stop rather than skip it. fn serve_typed(&mut self, effects: &mut EffectSlots, done: &mut Vec) { @@ -383,7 +407,71 @@ impl CoreHarness { self.served.insert("retention"); done.push(Done::Retention(self.serve_retention(effect))); } - assert!(!effects.has_pending(), "only catalog and retention effects are reachable from a host in Phase 1"); + if let Some(effect) = effects.navigator.take() { + self.served.insert("navigator"); + if let Some(outcome) = self.serve_navigator(effect) { + done.push(Done::Navigator(outcome)); + } + } + let mut persisted = None; + if let Some(effect) = effects.settings.take() { + self.served.insert("settings"); + let SettingsEffect::PersistRevision { token, revision } = effect; + self.settings_token = Some(token); + persisted = Some(revision); + } + if let Some(effect) = effects.dfu.take() { + self.served.insert("dfu"); + if let Some(outcome) = self.serve_dfu(effect) { + done.push(Done::Dfu(outcome)); + } + } + if let Some(effect) = effects.storage_info.take() { + self.served.insert("storage"); + let StorageInfoEffect::MeasureFreeSpace { token } = effect; + done.push(Done::Storage(StorageInfoOutcome::Measured { token, free_bytes: 8 * 1024 * 1024 })); + } + assert!(!effects.has_pending(), "recorder, weather and bond are the domains a host cannot reach in Phase 1"); + self.serve_scripted(persisted, done); + } + + /// Serve one navigation operation from the corpus's scripted planner answers. + /// + /// `None` means "the executor is still working": the corpus scripts a **detour** search's answer + /// at the action that opens the preview rather than at the request (the commit it leads to needs + /// that preview to exist), so that one arrives through the app's own event door. + fn serve_navigator(&mut self, effect: NavigatorEffect) -> Option { + let token = effect.token(); + match effect { + NavigatorEffect::Acquire { work: PlannerWork::Route(_), .. } => { + self.state.pending_nav_plan.take().map(|result| match result { + Ok(route) => NavigatorOutcome::PlanFinished { token, route }, + Err(error) => NavigatorOutcome::Failed { token, error: NavigatorError::Plan(error) }, + }) + } + NavigatorEffect::Acquire { work: PlannerWork::Detour(_), .. } => None, + // The splice's answer is scripted at the action, like the detour search's — see + // `serve_scripted`. + NavigatorEffect::CommitDetour { .. } => None, + NavigatorEffect::Release { .. } => Some(NavigatorOutcome::Released { token }), + NavigatorEffect::Step { .. } | NavigatorEffect::CommitRoute { .. } => { + panic!("one legacy request runs the whole search (LegacyOwned::PlannerPacing) — {effect:?}") + } + } + } + + /// Serve one update operation from the corpus's scripted scan and install answers. + fn serve_dfu(&mut self, effect: DfuEffect) -> Option { + match effect { + DfuEffect::Scan { token } => self.state.pending_dfu_scan.take().map(|result| match result { + Ok(report) => DfuOutcome::ScanFinished { token, report }, + Err(error) => DfuOutcome::ScanFailed { token, error }, + }), + DfuEffect::ArmInstall { token } => self.state.pending_dfu_install.take().map(|result| match result { + Ok(()) => DfuOutcome::InstallBegan { token }, + Err(error) => DfuOutcome::InstallFailed { token, error }, + }), + } } fn serve_catalog(&mut self, effect: CatalogEffect) -> Done { @@ -465,6 +553,7 @@ impl CoreHarness { } } self.adapter.needs_to_commands(needs, &mut mail); + let mut persisted = None; while let Some(command) = mail.pop() { if let Some(level) = derived_level(&command) { // The adapter re-emits the two levels as their old cues; this runner answers them @@ -472,64 +561,93 @@ impl CoreHarness { self.moved.insert(level); continue; } + if let HostCommand::PersistSettings { revision } = command { + persisted = Some(revision); + } self.served.insert("adapter"); trace.record_command(&command); self.serve_legacy(command, done, trace); } + self.serve_scripted(persisted, done); } - // ---- the legacy half, for the six domains without a machine ---- + // ---- the legacy half, for the two domains without a machine ---- + // + // Recorder and Bond, and nothing else. Both are named in `LegacyOwned` with the reason they are + // not here: the legacy ride close is answered by a catalog re-feed rather than by a ride + // identity (`RideCloseAck`), and the legacy bond removal is confirmed by a link-status fact + // rather than by a reply (`BondAck`). A domain that cannot validate a token cannot own an + // outcome (epic §4.3), so both keep the old protocol until #1397 S6. fn serve_mailbox(&mut self, done: &mut Vec, trace: &mut TraceRecorder) { let mut mail: HostMailbox = HostMailbox::new(); let _ = self.state.app.drain_host_commands(&mut mail); - let mut persisted = None; while let Some(command) = mail.pop() { if let Some(level) = derived_level(&command) { self.moved.insert(level); continue; } assert!( - !store_owned(&command), + !pass_owned(&command), "{command:?} is DeviceCore's now — running it here would repeat the effect that carries it" ); - if let HostCommand::PersistSettings { revision } = command { - // The first, as `LegacyHarness::run_pass`'s `find_map` takes it — a second in one - // drain would be a coalescing bug, and taking the last would hide it. - persisted.get_or_insert(revision); - } trace.record_command(&command); self.serve_legacy(command, done, trace); } - // The scripted answers the legacy corpus arms at its protocol boundary, drained exactly as - // the legacy runner drains them — this half of both DeviceCore runners is unchanged by - // design, because these six domains have not migrated. - if let Some(result) = self.state.pending_nav_plan.take() { - done.push(Done::Event(HostEvent::NavPlanned(result))); - } + } + + /// The corpus's answers that are scripted at the **action** rather than at the request, + /// delivered once per pass exactly as `LegacyHarness::run_pass` delivers them — so all three + /// frames answer the same script. + /// + /// Two are not request-keyed and cannot be: the detour splice's answer is armed by the Press + /// that asks for it and arrives without a command (like `DetourPlanned`, whose preview that + /// Press happens on), and a settings answer may be a *stale* ack for a revision a newer edit + /// has already superseded — the whole point of that scenario, and something no in-flight write + /// of its own would carry. + /// + /// `persisted` is the revision a write went out for on this pass, when one did: the corpus's + /// retry case is the one answer that must not precede its own request. + fn serve_scripted(&mut self, persisted: Option, done: &mut Vec) { if std::mem::take(&mut self.state.commit_success_pending) { done.push(Done::Event(HostEvent::DetourCommitted(Ok(10)))); } + let Some((revision, failed)) = self.scripted_settings(persisted) else { return }; + // The typed executor answers with the token of the write it is holding, so the domain + // checks the operation *and* the revision — two independent guards (#810). The + // compatibility executor has no token to give: the adapter is holding it, and hands it back + // itself when the legacy event arrives. + let typed = matches!(self.executor, Executor::Typed).then_some(self.settings_token).flatten(); + done.push(match (typed, failed) { + (Some(token), true) => { + Done::Settings(SettingsOutcome::PersistFailed { token, revision, error: SettingsSaveError::Backend }) + } + (Some(token), false) => Done::Settings(SettingsOutcome::Persisted { token, revision }), + (None, true) => { + Done::Event(HostEvent::SettingsPersistFailed { revision, error: SettingsSaveError::Backend }) + } + (None, false) => Done::Event(HostEvent::SettingsPersisted { revision }), + }); + } + + /// The corpus's scripted settings answer, if one is due this pass. + /// + /// `persisted` is the revision a write went out for on this pass, when one did — the corpus's + /// retry case is the one answer that must not precede its own request. + fn scripted_settings(&mut self, persisted: Option) -> Option<(u16, bool)> { let ready = !matches!(self.state.pending_settings_result, Some(PendingSettingsResult::PersistLatest)) || persisted.is_some(); - if ready { - if let Some(result) = self.state.pending_settings_result.take() { - let revision = match result { - PendingSettingsResult::PersistRevision(revision) => revision, - PendingSettingsResult::PersistLatest | PendingSettingsResult::FailLatest => { - persisted.unwrap_or(self.state.settings_revision) - } - }; - done.push(Done::Event(match result { - PendingSettingsResult::FailLatest => { - HostEvent::SettingsPersistFailed { revision, error: SettingsSaveError::Backend } - } - PendingSettingsResult::PersistLatest | PendingSettingsResult::PersistRevision(_) => { - HostEvent::SettingsPersisted { revision } - } - })); - } + if !ready { + return None; } + let result = self.state.pending_settings_result.take()?; + let revision = match result { + PendingSettingsResult::PersistRevision(revision) => revision, + PendingSettingsResult::PersistLatest | PendingSettingsResult::FailLatest => { + persisted.unwrap_or(self.state.settings_revision) + } + }; + Some((revision, matches!(result, PendingSettingsResult::FailLatest))) } fn serve_legacy(&mut self, command: HostCommand, done: &mut Vec, trace: &mut TraceRecorder) { @@ -554,6 +672,9 @@ impl CoreHarness { self.state.feed_trips("core.cascade-trips", trace); } } + // The four domains #1397 S2 gave a machine reach this executor as *translated* effects, + // so the corpus's scripted answers are keyed to the command that carries the request — + // and go back through the adapter, which is holding the token. HostCommand::Dfu(DfuAction::Scan) => { if let Some(result) = self.state.pending_dfu_scan.take() { done.push(Done::Event(HostEvent::DfuScanned(result))); @@ -570,6 +691,11 @@ impl CoreHarness { HostCommand::ScanCardFree => { done.push(Done::Event(HostEvent::CardScanned { free_bytes: Some(8 * 1024 * 1024) })); } + HostCommand::PlanRoute(_) => { + if let Some(result) = self.state.pending_nav_plan.take() { + done.push(Done::Event(HostEvent::NavPlanned(result))); + } + } // The sidecar stamps are fire-and-forget in the old protocol: the write happens and // nothing acknowledges it (`LegacyOwned::SidecarAck`), which is exactly why // RetentionMachine stays in flight behind one under the compatibility executor. @@ -589,15 +715,15 @@ impl CoreHarness { } } HostCommand::FinishTrack(TrackAction::Discard) => {} - // Cancels, plan requests and the detour commit are consumed without being started: the - // corpus scripts their completion at the protocol boundary, exactly as the legacy runner - // does (`PlanHold`, `hold_detour_commit`). - HostCommand::PersistSettings { .. } - | HostCommand::CancelRoutePlan + // A cancellation is consumed without being started (the search it aborts was never run + // here), the detour search's answer is scripted at the action that opens its preview, + // and the legacy bond removal is confirmed by a link fact rather than by a reply + // (`LegacyOwned::BondAck`). + HostCommand::CancelRoutePlan | HostCommand::CancelDetour - | HostCommand::PlanRoute(_) | HostCommand::PlanDetour(_) | HostCommand::CommitDetour + | HostCommand::PersistSettings { .. } | HostCommand::ForgetBond => {} other => panic!("{other:?} is pass-owned and must not reach the legacy executor"), } @@ -676,6 +802,21 @@ impl TraceHarness for CoreHarness { Done::Retention(outcome) => { let _ = self.inbox.outcomes.retention.try_put(outcome); } + Done::Navigator(outcome) => { + let _ = self.inbox.outcomes.navigator.try_put(outcome); + } + Done::Settings(outcome) => { + if matches!(outcome, SettingsOutcome::PersistFailed { .. }) && self.state.settings_retry_requested { + self.clock_ms = self.clock_ms.max(SETTINGS_FAILURE_RETRY_MS); + } + let _ = self.inbox.outcomes.settings.try_put(outcome); + } + Done::Dfu(outcome) => { + let _ = self.inbox.outcomes.dfu.try_put(outcome); + } + Done::Storage(outcome) => { + let _ = self.inbox.outcomes.storage_info.try_put(outcome); + } Done::Event(event) => { trace.record_event(&event); let settings_failed = matches!(event, HostEvent::SettingsPersistFailed { .. }); @@ -972,6 +1113,24 @@ fn the_pass_owns_the_classes_it_took_over() { .expect("the scenario runs"); assert!(harness.moved.contains("LoadRideTrack"), "the ride-track cue is answered from the plan's key"); assert!(harness.moved.contains("RefreshNavPreview"), "and so is the nav preview"); + + // The eight classes #1397 S2 moved, each through the scenario that produces it: the rider's + // request leaves as its domain's typed effect, and `serve_mailbox`'s `pass_owned` assertion — + // which every one of these runs through — is what proves the legacy class did not pend beside + // it. `Cancel*` rides its own family's scenario, and `PersistSettings` the settings ones. + for (scenario, domain) in [ + ("navigation.plan-cancel-late-replacement", "navigator"), + ("navigation.detour-lifecycle", "navigator"), + ("dfu.scan-outcomes", "dfu"), + ("dfu.install-start", "dfu"), + ("platform.bond-and-card-space", "storage"), + ("settings.revision-success-and-stale-result", "settings"), + ] { + let mut harness = CoreHarness::new(Executor::Typed); + run_scenario_seeded(&definition(named(scenario)), RunnerMode::Immediate, &normalization_seed(), &mut harness) + .expect("the scenario runs"); + assert!(harness.served.contains(domain), "{scenario}: the rider's request left as a {domain} effect"); + } } fn named(name: &'static str) -> &'static Scenario { @@ -1213,8 +1372,24 @@ fn an_outcome_after_cancellation_changes_nothing() { assert!(plan.effects.catalog.is_empty(), "a repeat of a terminal answer starts no work"); assert_eq!(harness.state.route_ids.len(), 2, "and removes nothing a second time"); - // The navigator half of the same rule, through the adapter: a cancelled plan does not accept its - // own result, and it is the *domain* that refuses it — the adapter hands the token straight back. + // The navigator half of the same rule, on the real machine (#1397 S2): the rider's Back + // invalidates the operation, so the planner's answer commits no route at all. + let mut harness = typed(); + assert!(harness.app().debug_start_nav((0, 0), (1_000, 1_000), "col"), "the plan is admitted"); + let mut plan = harness.pass(); + let effect = plan.effects.navigator.take().expect("and leaves as one bounded planner operation"); + harness.app().apply_gesture(Gesture::Back); // the rider walks away from the spinner + let _ = + harness.inbox.outcomes.navigator.try_put(NavigatorOutcome::PlanFinished { token: effect.token(), route: 10 }); + harness.pass(); + assert_eq!(harness.state.app.active_route_index(), None, "the cancelled plan adopted nothing"); + assert!( + !matches!(harness.state.app.top_screen(), Screen::RouteOverview(_)), + "and the rider is not shown an overview for a route they cancelled" + ); + + // The same rule seen from the adapter's side: it hands the stored token straight back, and it is + // the *domain* that refuses it. let mut navigator: TokenSource = TokenSource::new(); let token = navigator.issue(); let mut adapter = LegacyAdapter::new(); @@ -1318,8 +1493,14 @@ fn a_transfer_during_planning_withdraws_heavy_capability() { // The pass sees the transfer and starts nothing; it also fails nothing. let mut harness = expiring(0); harness.inbox.facts.note_transfer(TransferState::Active); - let plan = harness.pass(); - assert!(!plan.effects.has_pending(), "no work is admitted while the transfer holds the store"); + let mut plan = harness.pass(); + assert!(plan.effects.navigator.is_empty(), "no plan is started while the transfer holds the store"); + assert!(plan.effects.dfu.is_empty(), "nor an install"); + assert!(plan.effects.catalog.is_empty() && plan.effects.retention.is_empty(), "nor a store operation"); + // The settings write is not heavy and is not withdrawn: the trusted-clock stamp this fixture + // makes is a rider edit like any other, and a transfer holding the *store* has nothing to say + // about a settings revision. That distinction is what `Capabilities` is for. + let _ = plan.effects.settings.take(); assert!(adapter.pending().holds(LegacyReply::RoutePlan), "and the running plan is untouched"); assert!(navigator.is_current(token), "its operation is still the current one"); From 15fcad32071f8f06e223b18bdacd318f69781b01 Mon Sep 17 00:00:00 2001 From: timohueser Date: Mon, 24 Aug 2026 11:36:21 +0200 Subject: [PATCH 2/3] fix(app): a card scan with no figure blanks the free-space row again MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #1486 found one rider-visible regression and two latent ones. `StorageInfo::note_measured(None)` silently kept the last byte count where develop assigned unconditionally, so a rider who pulled the card kept seeing "Card free: 8.0 GB" — and the board's only producer sends `None` for exactly that case. The assignment is unconditional again: a measurement that produced no figure leaves a `--`. `NotMounted` and `ScanFailed` both blank (neither produced a figure; they stay distinct because they are different facts), a `Cancelled` leaves the figure alone because nothing was attempted, and the stale contract comment on the board's producer is corrected. Navigator hands out **one operation at a time**. Its token source is a single generation, so a second concurrent operation superseded the first, its genuine answer was refused, and that family's freeze flag was stranded — verbatim the "map never redraws again" #1146 names. `next_plan_effect` and `next_commit_effect` now refuse while one is in flight (the request stays queued — backpressure, never a loss, as `CatalogState` does), `next_release` no longer mints a token while another family is live, and the `ops`/`live` docs state the constraint where the next author meets it. The #1146 test drives the real seam — `next_effect` and `accepts` — instead of `note_answer`, so the token layer is actually exercised. The two consumers of the cancel one-shot are documented at both doors, including the gap the pass-plus-adapter composition inherits (`PlannerRelease` is untranslatable by design) and why no shipped host reaches it. Also: `reset_detour` clears the detour phase it left behind; the settings fault is raised into the fault connection so it reaches the rider with every other notice at stage 13 (this moves two replay passes from sleep-until-event to timed, re-pinned with the reason; the two gated figures are unchanged); the `note_persist_failed` doc says it always returns true and a test pins why; and the conformance transfer trace asserts the settings effect rather than tolerating its absence. Refs #1485, epic #1397. Co-Authored-By: Claude Fable 5 --- firmware/obc-app/src/app.rs | 73 ++++++++- firmware/obc-app/src/device_core/pass.rs | 5 +- .../obc-app/src/device_core/storage_info.rs | 78 ++++++--- firmware/obc-app/src/navigator.rs | 152 +++++++++++++++--- firmware/obc-app/src/screen/detour.rs | 3 + firmware/obc-app/src/settings.rs | 25 ++- firmware/obc-fw-nrf54l/src/ride.rs | 4 +- firmware/tools/resource_baseline.json | 4 +- .../tests/device_core_conformance.rs | 15 +- 9 files changed, 296 insertions(+), 63 deletions(-) diff --git a/firmware/obc-app/src/app.rs b/firmware/obc-app/src/app.rs index dfca4b392..7054f7dc1 100644 --- a/firmware/obc-app/src/app.rs +++ b/firmware/obc-app/src/app.rs @@ -1817,6 +1817,15 @@ impl App { /// Hand `family`'s undelivered cancellation to the executor, and repaint the map the freeze /// held still. `false` when there was nothing to deliver. + /// + /// This and `NavigatorMachine::next_release` are the **two consumers of one flag**, and stage 8 + /// runs before `drain_host_commands` — so on a composition that runs the pass, the cancellation + /// leaves as a typed `Release` and this arm finds nothing. That is the intended handover, with + /// one known gap the pass cannot close on its own: under the pass *plus* `LegacyAdapter` the + /// `Release` is `LegacyOwned::PlannerRelease` and is dropped untranslated, so that composition + /// never tells the executor to drop the planner. No shipped host runs both — the production + /// hosts reach this arm and nothing else — and #1397 S6 closes it by giving Navigator an + /// executor that answers a `Release`. See `next_release` for the whole reasoning. fn deliver_plan_cancel(&mut self, family: PlanFamily) -> bool { if !self.navigator.take_cancel(family) { return false; @@ -3369,8 +3378,9 @@ impl App { HostEvent::DetourPlanned(result) => self.land_detour_plan(result), HostEvent::DetourCommitted(result) => self.land_detour_commit(result), HostEvent::CardScanned { free_bytes } => { - // The legacy `None` folded "no medium" and "the scan failed" into one value; the - // domain separates them, and an unqualified `None` is the failed scan. + // The legacy `None` says only "no figure" — the board's producer yields it for no + // mounted medium *and* for no free count — so it goes to the domain as it stands, + // and blanks the System screen back to `--`. self.land_card_scan(free_bytes); } HostEvent::DfuScanned(result) => { @@ -5284,6 +5294,22 @@ mod tests { ); } + /// The rider pulls the card while the System screen is up. The board answers its next scan with + /// `CardScanned { free_bytes: None }`, and the row goes back to `--` rather than keeping the + /// byte count it read off a card that is no longer in the device. + /// + /// This is the one path the legacy protocol actually produces: `ride.rs`'s producer yields + /// `None` for no mounted medium *and* for no FSInfo free count, and it has always blanked. + #[test] + fn a_card_scan_with_no_figure_blanks_the_free_space_row() { + let mut app = App::new_idle(AppState::new(0, 0, 1.0)); + app.apply_event(HostEvent::CardScanned { free_bytes: Some(8 * 1024 * 1024) }); + assert_eq!(app.storage.free_bytes(), Some(8 * 1024 * 1024), "the scan answered"); + + app.apply_event(HostEvent::CardScanned { free_bytes: None }); + assert_eq!(app.storage.free_bytes(), None, "and a scan with no figure leaves the rider a `--`"); + } + /// The preview polyline is *derived* from the detour plan, so Back on the preview takes it with /// the plan it previewed. It is drawn over the still-active route: a shape that outlived its /// detour is a line to nowhere, and the rider would be looking at a turn nobody is going to @@ -5876,7 +5902,10 @@ mod tests { let mut mailbox: HostMailbox = HostMailbox::new(); assert_eq!(app.drain_host_commands(&mut mailbox), DrainStatus::Complete); - assert_eq!(mailbox.len(), HOST_COMMAND_CLASSES, "one command per class"); + // Sixteen, not eighteen: Navigator hands out **one operation at a time**, so the detour + // plan and its commit wait behind the route plan rather than minting tokens that would + // supersede it (see `NavigatorMachine::ops`). Every other class drains together. + assert_eq!(mailbox.len(), HOST_COMMAND_CLASSES - 2, "one command per class, less Navigator's backpressure"); let mut drained: heapless::Vec = heapless::Vec::new(); while let Some(cmd) = mailbox.pop() { let _ = drained.push(cmd); @@ -5895,8 +5924,6 @@ mod tests { HostCommand::StampRideSynced { id: 7, .. }, HostCommand::FinishTrack(TrackAction::Save), HostCommand::PlanRoute(_), - HostCommand::PlanDetour(_), - HostCommand::CommitDetour, HostCommand::Dfu(DfuAction::Scan), HostCommand::ForgetBond, HostCommand::PersistSettings { .. }, @@ -5908,15 +5935,45 @@ mod tests { "canonical order, ids resolved at drain: {drained:?}" ); - // One-shots drained exactly once; only the derived cues re-emit (still unanswered). + // Backpressure, never a loss: the route plan's answer frees Navigator and the retained + // detour work goes out on the next drain, still in canonical order. + app.apply_event(crate::HostEvent::NavPlanned(Ok(10))); + assert_eq!(app.drain_host_commands(&mut mailbox), DrainStatus::Complete); + let mut retained: heapless::Vec = heapless::Vec::new(); + while let Some(cmd) = mailbox.pop() { + let _ = retained.push(cmd); + } + assert!( + matches!( + retained.as_slice(), + [HostCommand::PlanDetour(_), HostCommand::LoadRideTrack { id: 7 }, HostCommand::RefreshNavPreview] + ), + "the detour plan survived the busy drain: {retained:?}" + ); + + // One-shots drained exactly once; only the derived cues re-emit (still unanswered). The + // commit waits one more turn behind the detour plan it was queued with. + app.apply_event(crate::HostEvent::DetourPlanned(Err(obc_route::nav::NavError::NoPath))); assert_eq!(app.drain_host_commands(&mut mailbox), DrainStatus::Complete); let mut redrained: heapless::Vec = heapless::Vec::new(); while let Some(cmd) = mailbox.pop() { let _ = redrained.push(cmd); } assert!( - matches!(redrained.as_slice(), [HostCommand::LoadRideTrack { id: 7 }, HostCommand::RefreshNavPreview]), - "only the level-derived cues re-emit: {redrained:?}" + matches!( + redrained.as_slice(), + [HostCommand::CommitDetour, HostCommand::LoadRideTrack { id: 7 }, HostCommand::RefreshNavPreview] + ), + "and nothing was lost on the way: {redrained:?}" + ); + assert_eq!(app.drain_host_commands(&mut mailbox), DrainStatus::Complete); + let mut settled: heapless::Vec = heapless::Vec::new(); + while let Some(cmd) = mailbox.pop() { + let _ = settled.push(cmd); + } + assert!( + matches!(settled.as_slice(), [HostCommand::LoadRideTrack { id: 7 }, HostCommand::RefreshNavPreview]), + "only the level-derived cues re-emit: {settled:?}" ); // Their `set_*` answers clear them — the protocol goes quiet. diff --git a/firmware/obc-app/src/device_core/pass.rs b/firmware/obc-app/src/device_core/pass.rs index da52a0247..00aa184d7 100644 --- a/firmware/obc-app/src/device_core/pass.rs +++ b/firmware/obc-app/src/device_core/pass.rs @@ -350,7 +350,10 @@ impl App { if let Some(outcome) = outcomes.settings.take() { let now_ms = self.ui.now_ms; if self.settings_ops.apply_outcome(outcome, now_ms) { - self.on_warning(crate::screen::WarningFlags::SETTINGS_ERROR); + // Through the fault connection, not straight to a card: every notice raised in a + // pass reaches the rider together at stage 13, so a failed save shares the card + // with whatever else this pass found rather than displacing it. + self.pass.connections.faults.raise(crate::screen::WarningFlags::SETTINGS_ERROR); } } if let Some(outcome) = outcomes.dfu.take() { diff --git a/firmware/obc-app/src/device_core/storage_info.rs b/firmware/obc-app/src/device_core/storage_info.rs index 65e0fbbc5..07b67022f 100644 --- a/firmware/obc-app/src/device_core/storage_info.rs +++ b/firmware/obc-app/src/device_core/storage_info.rs @@ -119,30 +119,40 @@ impl StorageInfo { /// Consume the answer to a measurement. A superseded or repeated answer changes nothing. /// - /// A failure is **reported, not retried**: the screen keeps whatever it last knew (or its `--`) - /// and the rider can ask again by re-entering. A domain that re-armed itself here would turn a - /// dead card into a free-cluster walk every pass. + /// The rider asked how much room is left, and there are three honest answers. A number replaces + /// the figure. A **failure** replaces it with *no figure* — whether the medium is absent or the + /// walk died part-way, we do not know how much room is left, and a byte count from an earlier + /// scan under the label "Card free" would be a lie (a card the rider has since taken out is the + /// case that matters). [`NotMounted`](StorageInfoError::NotMounted) and + /// [`ScanFailed`](StorageInfoError::ScanFailed) stay distinct because they are different + /// *facts*, not because the figure differs: neither produced one. A **cancellation** leaves the + /// figure alone — nothing was attempted, so what the rider is looking at is exactly as true as + /// it was. + /// + /// Nothing is retried here: the rider asks again by re-entering the screen. A domain that + /// re-armed itself would turn a dead card into a free-cluster walk every pass. pub(crate) fn apply_outcome(&mut self, outcome: StorageInfoOutcome) -> bool { if !self.ops.is_current(outcome.token()) { return false; } self.ops.invalidate(); // terminal: a duplicate of this answer is no longer current - self.note_measured(match outcome { - StorageInfoOutcome::Measured { free_bytes, .. } => Some(free_bytes), - StorageInfoOutcome::Failed { .. } | StorageInfoOutcome::Cancelled { .. } => None, - }); + match outcome { + StorageInfoOutcome::Measured { free_bytes, .. } => self.note_measured(Some(free_bytes)), + StorageInfoOutcome::Failed { .. } => self.note_measured(None), + StorageInfoOutcome::Cancelled { .. } => {} + } true } - /// A measurement answered with `free_bytes`, or `None` when it did not produce a figure — the + /// A measurement answered with `free_bytes`, or `None` when it produced no figure — the /// token-free half, for the legacy protocol that carries no token. /// - /// A failure leaves the last known figure alone rather than blanking it: the rider is looking - /// at a number that was true, and replacing it with `--` because one scan failed says less. + /// The assignment is unconditional, which is what makes a `None` blank the screen back to `--`. + /// The legacy [`CardScanned`](crate::HostEvent::CardScanned) event's `Option` is exactly + /// this shape and always has been: its `None` is the board reporting no mounted medium *or* no + /// free count, and the System screen has always answered both with `--`. pub(crate) fn note_measured(&mut self, free_bytes: Option) { - if let Some(free_bytes) = free_bytes { - self.free_bytes = Some(free_bytes); - } + self.free_bytes = free_bytes; } /// Whether a refresh is posted but undelivered — the `ScanCardFree` peek. @@ -184,24 +194,42 @@ mod storage_info_tests { assert!(storage.next_effect().is_none(), "…not two"); } - /// A failed scan is a **reported** failure, not a retry loop: the last figure the rider was - /// shown stands, nothing re-arms itself, and asking again is the rider's move. + /// A failed measurement blanks the figure back to `--`, and does **not** retry. + /// + /// The card the rider took out is the case that matters: a stale "8.0 GB" under the label + /// "Card free" on a device with no card in it is a lie the screen would keep telling. Both + /// failure modes blank it, because neither produced a figure. #[test] - fn a_failed_measurement_is_reported_and_not_retried() { + fn a_failed_measurement_blanks_the_figure_and_is_not_retried() { + for error in [StorageInfoError::NotMounted, StorageInfoError::ScanFailed] { + let mut storage = StorageInfo::new(); + storage.admit_intent(StorageInfoIntent::RefreshRequested); + let token = storage.next_effect().expect("the measurement goes out").token(); + assert!(storage.apply_outcome(StorageInfoOutcome::Measured { token, free_bytes: 8_000 })); + assert_eq!(storage.free_bytes(), Some(8_000)); + + storage.admit_intent(StorageInfoIntent::RefreshRequested); + let token = storage.next_effect().expect("the refresh goes out").token(); + assert!(storage.apply_outcome(StorageInfoOutcome::Failed { token, error })); + assert_eq!(storage.free_bytes(), None, "{error:?} leaves the rider a `--`, never a stale count"); + assert!(!storage.refresh_pending(), "and nothing re-armed itself"); + assert!(storage.next_effect().is_none()); + } + } + + /// A cancellation is the executor saying it did not try — so the figure the rider is looking at + /// is exactly as true as it was, and blanking it would report a failure that never happened. + #[test] + fn an_abandoned_measurement_leaves_the_figure_alone() { let mut storage = StorageInfo::new(); storage.admit_intent(StorageInfoIntent::RefreshRequested); - let effect = storage.next_effect().expect("the measurement goes out"); - let token = effect.token(); + let token = storage.next_effect().expect("the measurement goes out").token(); assert!(storage.apply_outcome(StorageInfoOutcome::Measured { token, free_bytes: 8_000 })); - assert_eq!(storage.free_bytes(), Some(8_000)); storage.admit_intent(StorageInfoIntent::RefreshRequested); - let effect = storage.next_effect().expect("the refresh goes out"); - let token = effect.token(); - assert!(storage.apply_outcome(StorageInfoOutcome::Failed { token, error: StorageInfoError::ScanFailed })); - assert_eq!(storage.free_bytes(), Some(8_000), "the figure that was true is not blanked by one bad scan"); - assert!(!storage.refresh_pending(), "and nothing re-armed itself"); - assert!(storage.next_effect().is_none()); + let token = storage.next_effect().expect("the refresh goes out").token(); + assert!(storage.apply_outcome(StorageInfoOutcome::Cancelled { token })); + assert_eq!(storage.free_bytes(), Some(8_000)); } /// A superseded or repeated answer changes nothing — the terminal answer invalidates the token diff --git a/firmware/obc-app/src/navigator.rs b/firmware/obc-app/src/navigator.rs index a5d1d1980..29c55d5fc 100644 --- a/firmware/obc-app/src/navigator.rs +++ b/firmware/obc-app/src/navigator.rs @@ -201,11 +201,25 @@ pub(crate) enum PlanPhase { /// the rider asked for. #[derive(Debug, Default)] pub struct NavigatorMachine { - /// The one operation token. `EffectSlots::navigator` holds one effect, so Navigator has at most - /// one physical operation in flight; [`live_family`](Self::live_family) says whose it is. + /// The one operation token, and it is genuinely **one**: + /// [`TokenSource::issue`](crate::device_core::TokenSource::issue) bumps a single generation, so + /// only the newest operation is ever current *across both families*. A second concurrent + /// operation would therefore make the first one's answer stale, `accepts` would refuse it, and + /// its family's freeze flag would never be released — the "map that never redraws again" + /// [`RerouteFreeze`] names. That is why [`next_plan_effect`](Self::next_plan_effect) and + /// [`next_commit_effect`](Self::next_commit_effect) hand out **at most one operation at a + /// time**: the constraint is enforced where the token is minted, not assumed. + /// + /// `RerouteFreeze`'s two flags are not the same question. They track *edges* — which family's + /// terminal edge may release what — and they stay per-family (#1146) whatever the token layer + /// allows. ops: TokenSource, - /// Which family the current token belongs to, while an executor holds an operation. At most - /// one: `EffectSlots::navigator` carries one effect and the token source is one generation. + /// The family whose operation an executor is currently holding, if any. `Some` is what makes + /// [`next_plan_effect`](Self::next_plan_effect) refuse a second one; it is cleared by the + /// terminal answer, by a superseding intent, and by a cancellation reaching the executor. + /// + /// A [`Release`](NavigatorEffect::Release) never sets it: handing the workspace back owes no + /// product answer, so it is not an operation anyone is waiting on. live: Option, /// The **Recalculating freeze** (#1146): a live planner run over a map base stops map redraws, /// pauses the matcher and raises the banner. Navigator is its only writer — the four scattered @@ -320,22 +334,50 @@ impl NavigatorMachine { .or_else(|| self.next_commit_effect()) } - /// The workspace release a cancellation implies. Consumes the same one-shot - /// [`take_cancel`](Self::take_cancel) does, so a cancellation reaches the executor exactly once - /// however the two protocols are composed. + /// The workspace release a cancellation implies. + /// + /// Consumes the **same one-shot** [`take_cancel`](Self::take_cancel) does — the door + /// `App::deliver_plan_cancel` uses for the legacy `CancelRoutePlan` / `CancelDetour` arms — so a + /// cancellation reaches an executor exactly once however the two protocols are composed. Stage 8 + /// runs before `drain_host_commands`, so a composition that runs both consumes it *here*. + /// + /// **The known gap that leaves, stated rather than discovered:** under the pass *plus* + /// [`LegacyAdapter`](crate::device_core::LegacyAdapter), + /// [`navigator_row`](crate::device_core::compat::navigator_row) maps this `Release` to + /// [`LegacyOwned::PlannerRelease`](crate::device_core::compat::LegacyOwned::PlannerRelease) — + /// untranslatable by design, because a `Release` is issued on success too — so that composition + /// never tells the executor to drop the planner. No shipped host runs both (nothing calls + /// `App::run_pass` until #1397 S6, which is also what gives Navigator an executor that answers a + /// `Release`), and the legacy hosts take the cancellation through the drain arm exactly as + /// before. + /// + /// A release is offered only when this family's own operation was the one in flight. A cancel + /// with nothing running is a host-side no-op — and minting a token for it while *another* + /// family's search is live would supersede that search's answer, which is the failure + /// [`ops`](Self::ops) describes. fn next_release(&mut self, family: PlanFamily) -> Option { if !self.take_cancel(family) { return None; } self.note_cancel_delivered(family); - self.live = Some(family); - Some(NavigatorEffect::Release { token: self.ops.issue() }) + // `admit_intent` already invalidated this family's operation, so `live` is clear exactly + // when the cancellation had something of its own to stop. + self.live.is_none().then(|| NavigatorEffect::Release { token: self.ops.issue() }) } /// Hand `family`'s undelivered request to an executor: the operation the search runs under, and /// the moment the freeze engages. **The engaging edge is here, not at admission** — a request /// the rider cancelled before anyone took it froze nothing, so nothing needs releasing. + /// + /// Refused while an executor already holds an operation, whichever family's. The request stays + /// queued and goes out on a later pass — backpressure, never a loss, and the same shape + /// [`CatalogState`](crate::catalog_state::CatalogState) uses. Handing out a second one would + /// mint a token that supersedes the first, so the running search's genuine answer would be + /// refused and its freeze flag would be stuck forever (see [`ops`](Self::ops)). pub(crate) fn next_plan_effect(&mut self, family: PlanFamily) -> Option { + if self.live.is_some() { + return None; + } let work = match family { PlanFamily::Route => PlannerWork::Route(self.route_request.take()?), PlanFamily::Detour => PlannerWork::Detour(self.detour_request.take()?), @@ -350,8 +392,12 @@ impl NavigatorMachine { } /// Hand the previewed detour's splice to an executor. No freeze edge: a commit is a write, not - /// a search, and it does not take the nav arm. + /// a search, and it does not take the nav arm. Refused while another operation is in flight, + /// for the same reason [`next_plan_effect`](Self::next_plan_effect) is. pub(crate) fn next_commit_effect(&mut self) -> Option { + if self.live.is_some() { + return None; + } core::mem::take(&mut self.detour_commit).then(|| { self.detour = PlanPhase::Committing; self.live = Some(PlanFamily::Detour); @@ -497,6 +543,11 @@ impl NavigatorMachine { self.detour_request = None; self.detour_commit = false; self.detour_cancel = false; + // The phase describes a plan that is gone, so it goes with it — otherwise `detour_planned` + // and `detour_committing` would keep reporting a preview or a splice that no longer exists. + // `live` and the token are deliberately left: an executor may still be holding this + // family's operation, and its answer is what hands the workspace back. + self.detour = PlanPhase::Idle; } /// The detour family's phase — the preview/commit gate, and what the tests read. @@ -582,31 +633,70 @@ mod machine_tests { } } - /// **#1146, both directions.** A family's terminal edge releases only its own freeze, and two - /// live searches hold it until the last one ends. The regression is a map that never redraws - /// again with the nav arm still claimed underneath it. + /// **#1146.** A stray edge of one family must never release the freeze a *route* search is + /// holding the nav arm behind, and the running search's own answer must still be accepted when + /// it arrives. The regression is a map that never redraws again with an unfrozen matcher + /// drifting under it. + /// + /// Driven through the real seam — `next_effect` and `accepts`, not `note_answer` — because the + /// token layer is where this can go wrong: a second operation would mint a generation that + /// supersedes the first, its genuine answer would be refused, and nothing would ever release + /// its flag. The machine refuses the second operation instead, so both halves hold. #[test] fn a_detours_terminal_edge_never_releases_a_route_freeze() { let mut nav = NavigatorMachine::new(); nav.admit_intent(NavigatorIntent::PlanRoute(route_request("col"))); - nav.next_plan_effect(PlanFamily::Route).expect("the route search starts"); + let route = nav.next_plan_effect(PlanFamily::Route).expect("the route search starts"); assert!(nav.freeze_active(true), "a search over a map base is the freeze"); - // A detour cancellation, delivered while the route search is still running. + // A detour cancellation while the route search runs. It is not this run's edge, and it + // mints no token — a `Release` here would supersede the search that is still going. nav.admit_intent(NavigatorIntent::CancelDetour); - assert!(nav.take_cancel(PlanFamily::Detour)); - assert!(!nav.note_cancel_delivered(PlanFamily::Detour), "not this run's edge"); + assert!(nav.next_effect().is_none(), "a cancellation with nothing of its own to stop asks for no work"); assert!(nav.freeze_active(true), "the route search still holds the nav arm"); - // And two live searches: the first to end does not release it. + // A detour *request* while it runs: refused, and the request waits rather than being lost. nav.admit_intent(NavigatorIntent::PlanDetour(detour_request())); - nav.next_plan_effect(PlanFamily::Detour).expect("the detour search starts too"); - assert!(!nav.note_answer(PlanFamily::Detour, PlanPhase::PreviewReady), "one down, one to go"); + assert!(nav.next_effect().is_none(), "one navigation operation at a time"); + assert!(nav.request_pending(PlanFamily::Detour), "…and the rider's detour is still queued"); + + // The route search's genuine answer is still this operation's — which is the half a + // superseding second token would have destroyed. + let answer = NavigatorOutcome::PlanFinished { token: route.token(), route: 9 }; + assert!(nav.accepts(&answer), "the running search's answer is accepted"); + assert!(nav.note_answer(PlanFamily::Route, PlanPhase::Active), "and it is what releases the freeze"); + assert!(!nav.freeze_active(true)); + + // Only now does the queued detour go out, and it freezes on its own account. + assert!(matches!(acquired(nav.next_effect()), Some(PlannerWork::Detour(_))), "nothing was lost"); assert!(nav.freeze_active(true)); - assert!(nav.note_answer(PlanFamily::Route, PlanPhase::Active), "the last one out releases it"); + assert!(nav.note_answer(PlanFamily::Detour, PlanPhase::PreviewReady), "released by its own edge"); assert!(!nav.freeze_active(true)); } + /// The freeze's own two-flag rule, kept where the machine cannot reach it: two runs live at + /// once is not a state today's UI can produce — and `next_plan_effect` now refuses to create + /// one — but the arm is a single block, so if it ever becomes reachable the freeze must hold + /// until the *last* run is done, not the first. `RerouteFreeze`'s own tests pin that; + /// Navigator's contribution is that it never hands out the second operation that would strand + /// the first one's answer. + #[test] + fn a_second_operation_is_refused_rather_than_superseding_the_first() { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(NavigatorIntent::PlanDetour(detour_request())); + let detour = nav.next_plan_effect(PlanFamily::Detour).expect("the detour search starts"); + + // A route plan and a commit, both while it runs. + nav.admit_intent(NavigatorIntent::PlanRoute(route_request("col"))); + nav.admit_intent(NavigatorIntent::CommitDetour); + assert!(nav.next_plan_effect(PlanFamily::Route).is_none(), "no second search"); + assert!(nav.next_commit_effect().is_none(), "and no splice beside one"); + assert!(nav.accepts(&NavigatorOutcome::Acquired { token: detour.token() }), "the first is still current"); + + nav.note_answer(PlanFamily::Detour, PlanPhase::PreviewReady); + assert!(nav.next_plan_effect(PlanFamily::Route).is_some(), "the queued plan goes out once it is free"); + } + /// A plan answer that arrives after the rider cancelled changes nothing: the token stopped being /// current the instant they walked away, so the search's eventual result commits no route. #[test] @@ -684,6 +774,26 @@ mod machine_tests { assert_eq!(nav.detour_phase(), PlanPhase::Active); } + /// A fresh tracking session drops the detour, and nothing is left describing it: the phase goes + /// with the one-shots, so `detour_planned` and `detour_committing` cannot keep reporting a + /// preview or a splice the session reset just threw away. + #[test] + fn a_session_reset_leaves_nothing_describing_the_dropped_detour() { + let mut nav = NavigatorMachine::new(); + nav.admit_intent(NavigatorIntent::PlanDetour(detour_request())); + nav.next_plan_effect(PlanFamily::Detour).expect("the search starts"); + nav.note_answer(PlanFamily::Detour, PlanPhase::PreviewReady); + nav.admit_intent(NavigatorIntent::CommitDetour); + assert!(nav.detour_planned() && nav.commit_pending()); + + nav.reset_detour(); + assert!(!nav.detour_planned(), "no plan"); + assert!(!nav.detour_committing(), "no splice"); + assert!(!nav.commit_pending() && !nav.request_pending(PlanFamily::Detour)); + assert!(!nav.cancel_pending(PlanFamily::Detour)); + assert_eq!(nav.detour_phase(), PlanPhase::Idle); + } + /// One stream, one order: the pass takes cancellations before new work, so both compositions ask /// an executor for the same thing in the same sequence. #[test] diff --git a/firmware/obc-app/src/screen/detour.rs b/firmware/obc-app/src/screen/detour.rs index 00c5c9359..7e5d5aade 100644 --- a/firmware/obc-app/src/screen/detour.rs +++ b/firmware/obc-app/src/screen/detour.rs @@ -826,6 +826,9 @@ mod tests { let mut p = preview_for(&a); let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| p.handle(Gesture::Press, cx)); assert!(drained_commit(&mut nav_a)); + // The splice answered — a failure, which returns the preview to the rider *and* frees + // Navigator, so the retry has an operation slot to go out in. + nav_a.note_commit(false); p.set_commit_failed(); let _ = with_state_ctx(&mut a, &mut nav_a, nav_state(), |cx| p.handle(Gesture::Press, cx)); assert!(drained_commit(&mut nav_a), "a failed commit can be retried"); diff --git a/firmware/obc-app/src/settings.rs b/firmware/obc-app/src/settings.rs index 2174d1ef9..b645d4ddc 100644 --- a/firmware/obc-app/src/settings.rs +++ b/firmware/obc-app/src/settings.rs @@ -2272,7 +2272,12 @@ impl SettingsMachine { } /// The write for `revision` failed. Keep it dirty and re-arm the bounded backoff, but only - /// while it is still the in-flight latest. Returns whether the rider is told. + /// while it is still the in-flight latest. + /// + /// **Always returns `true`:** the rider is told a save failed whatever the revision guard says, + /// which is what the legacy handler did and the honest thing to show — a write *did* fail. The + /// guard decides only whether that revision stays retryable; a stale failure leaves the newer + /// content pending exactly as it was and re-arms nothing. pub(crate) fn note_persist_failed(&mut self, revision: u16, now_ms: u32) -> bool { if self.persist == PersistState::Awaiting && revision == self.revision { self.retry_at_ms = (now_ms as u16).wrapping_add(SETTINGS_RETRY_BACKOFF_MS as u16); @@ -2356,7 +2361,7 @@ mod settings_machine_tests { machine.note_edited(); let (_, revision) = emit(&mut machine, 1_000); - assert!(machine.note_persist_failed(revision, 1_000), "the rider is told a save failed"); + machine.note_persist_failed(revision, 1_000); assert!(!machine.wants_write(false, 1_000 + SETTINGS_RETRY_BACKOFF_MS - 1), "not before the window"); assert!(machine.wants_write(false, 1_000 + SETTINGS_RETRY_BACKOFF_MS), "and exactly once at it"); @@ -2385,6 +2390,22 @@ mod settings_machine_tests { assert!(machine.wants_write(false, 2_000_001), "the write is owed again"); } + /// The rider is told a save failed even when the failure is for a superseded revision — a write + /// did fail, and hiding it would be the quieter lie. What the revision guard decides is only + /// whether *that* revision stays retryable: a stale failure re-arms nothing, so the newer + /// content is still owed immediately rather than parked behind a backoff it never earned. + #[test] + fn a_stale_failure_is_still_shown_but_re_arms_nothing() { + let mut machine = SettingsMachine::new(); + machine.note_edited(); // revision 1 + emit(&mut machine, 100); + machine.note_edited(); // revision 2 supersedes it + + assert!(machine.note_persist_failed(1, 100), "the rider is told a save failed"); + assert!(machine.wants_write(false, 100), "but revision 2 is owed now, not after a backoff"); + assert_eq!(emit(&mut machine, 100).1, 2); + } + /// The token and the revision are independent guards: an answer to a *superseded operation* is /// refused before its revision is even looked at. #[test] diff --git a/firmware/obc-fw-nrf54l/src/ride.rs b/firmware/obc-fw-nrf54l/src/ride.rs index 58c6a9e60..efbec7bc8 100644 --- a/firmware/obc-fw-nrf54l/src/ride.rs +++ b/firmware/obc-fw-nrf54l/src/ride.rs @@ -1480,7 +1480,9 @@ pub(crate) async fn run_app( // The System settings screen's card-free scan (T8 item 6): a drained on-entry request runs // one bounded FAT free-cluster read off the card and answers through the `CardScanned` - // event (or a `None` → the screen keeps `--` when there's no card / no FSInfo free count). + // event. A `None` — no mounted card, or no FSInfo free count — is a measurement that + // produced no figure, and `StorageInfo` blanks the row back to `--` rather than leaving + // a byte count from a card that may no longer be in the device. if host_pass.card_scan { app.apply_event(obc_app::HostEvent::CardScanned { free_bytes: storage.as_ref().and_then(|s| s.card_free_bytes()), diff --git a/firmware/tools/resource_baseline.json b/firmware/tools/resource_baseline.json index e7be1591c..30e6d39cd 100644 --- a/firmware/tools/resource_baseline.json +++ b/firmware/tools/resource_baseline.json @@ -58,7 +58,7 @@ "_compile_note_dc4_1437": "**DC4 #1437 domain boundaries: `app` 50,520 -> 50,680 B (+160 B), both profiles.** Itemised on the CI `embedded` run for this branch against its base (099bd1be), all four items structural and none of them a buffer: (1) +72 B for the three derived cache keys in `CatalogState` (`ride_profile_for`, `ride_preview_for`, `nav_preview_route`) widening from `Option` (8 B) to `Option` / `Option` (32 B) — a durable object identity plus a source and a view revision, which is what removes the catalog-index remap and with it the late-answer and replaced-bytes staleness holes; (2) +24 B for the three `Revision` counters those keys read (`source_revision`, `ride_track_view`, `nav_preview_view`); (3) +40 B for the new `WeatherDomain` field on `App` (a token source, the installed `WeatherData` identity/revision, the in-flight refresh token, the request flag and the last terminal result); (4) +24 B inside `RetentionMachine` (its own token source and the in-flight sidecar-write slot, so a failed metadata write can be re-queued for the id it was actually about). Linked resident clears the 320,688 B `resident_ram_max` ceiling by ~16 KB on both profiles and `.uninit` is untouched at 132,096 B, so nothing moves on glass. Re-verified against S3 #1446's placement-constructor merge: `app` reads 50,680 B on both profiles there too, and `init_idle`'s own frame is 76 B of the 4,096 B ceiling. `measured_resident` (320,616 B) is a record, not a gate, and is left as it stands: it predates this branch.", "_compile_note_dc5_1438": "**DC5 #1438 pass coordinator: `app` 50,680 -> 50,920 B (+240 B), both profiles.** Two structural items, measured on the thumbv8m target, neither of them a buffer: (1) +224 B for the new `PassState` field on `App` \u2014 136 B of it the `Connections` set (eight named cross-domain slots: two UI intents, the expiry intent, the active-route notice, the fault bit set, and the three deferred slots whose two halves are what make a later-to-earlier value land on the *next* pass), and 88 B the levels a stage detects an edge against (the last store revision seen, the revision the catalog announced, the last link state, the transfer state, the active route identity, the 12 B `Capabilities` the admission stage recalculates, and the re-entrancy flag); (2) +16 B in `CatalogState` for the catalog domain's admitted-intent slot (`Option` is 16 B \u2014 one identity and a tag), with its 4 B token source and the in-flight flag landing in existing padding. The test-only stage recorder is `#[cfg(test)]` and is not in this figure. Linked resident and `.uninit` are unchanged (304,824 B / 132,096 B on the pinned host, against the 320,688 B ceiling), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B against its 4,096 B limit \u2014 the field is written straight into its `.bss` slot like every other.", "_compile_note_dc7_1440": "**DC7 #1440 conformance gate: `app` 50,920 -> 50,904 B (-16 B), both profiles.** A saving, from deleting the `UiRuntime` -> `Recorder` connection the gate proved was destroying a rider request: the pass took the ride-close one-shot at stage 4 and dropped it at stage 7, where Recorder has no machine to act on it, so the ride was never finalized and no executor was told. Out of `Connections` go the `Slot` and the `Deferred`; with the last `KeepFirst` connection gone, `Merge` goes with them and both remaining deferred slots lose their merge field. The close is back on the legacy drain that performs it, and returns with Recorder's machine at #1397 S6. Linked resident and `.uninit` are unchanged within the ceilings (304,808 B / 132,096 B on the pinned host, against 320,688 B), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B of its 4,096 B limit.", - "_compile_note_s2_1485": "**#1397 S2 answered-domain seams: `app` 50,904 -> 50,928 B (+24 B), both profiles.** The four domains whose legacy answer is already terminal get their own pending state, so the rider's request stops living in `Activity` and starts living with its owner. Out of `Activity` go seven one-shot slots (`nav_request`, `detour_request`, `detour_commit`, `detour_cancel`, `dfu_request`, `nav_cancel`, `card_scan_request`) for -88 B; out of `App` go `card_free_bytes` (-16 B) and `HostPending`, whose persist half moved to `SettingsMachine` and whose remaining store-commit counter is now a bare `u32` (-8 B). In their place: `NavigatorMachine` (96 B \u2014 the two bounded planner requests, the per-family phase, and the `RerouteFreeze` that moved in from `App` so the domain is its only writer), `StorageInfo` (24 B \u2014 the free-space figure `card_free_bytes` used to hold, plus its token), `SettingsMachine` (12 B) and `DfuState` (8 B). The +24 B is padding plus the four token generations the epic's ownership rule requires: a domain that cannot validate a token cannot own an outcome (#1433 \u00a74.3). Linked resident 304,832 B and residual main stack 54,592 B on the pinned host (against 320,688 B and a 38,808 B floor); flash 1,449,328 B against 1,524,676 B; the arena, the poll frame (9,792 B) and the main task future (1,100 B) are untouched.", + "_compile_note_s2_1485": "**#1397 S2 answered-domain seams: `app` 50,904 -> 50,928 B (+24 B), both profiles.** The four domains whose legacy answer is already terminal get their own pending state, so the rider's request stops living in `Activity` and starts living with its owner. Out of `Activity` go seven one-shot slots (`nav_request`, `detour_request`, `detour_commit`, `detour_cancel`, `dfu_request`, `nav_cancel`, `card_scan_request`) for -88 B; out of `App` go `card_free_bytes` (-16 B) and `HostPending`, whose persist half moved to `SettingsMachine` and whose remaining store-commit counter is now a bare `u32` (-8 B). In their place: `NavigatorMachine` (96 B \u2014 the two bounded planner requests, the per-family phase, and the `RerouteFreeze` that moved in from `App` so the domain is its only writer), `StorageInfo` (24 B \u2014 the free-space figure `card_free_bytes` used to hold, plus its token), `SettingsMachine` (12 B) and `DfuState` (8 B). The +24 B is padding plus the four token generations the epic's ownership rule requires: a domain that cannot validate a token cannot own an outcome (#1433 \u00a74.3). Linked resident 304,832 B and residual main stack 54,592 B on the pinned host (against 320,688 B and a 38,808 B floor); flash 1,449,768 B against 1,524,676 B; the arena, the poll frame (9,792 B) and the main task future (1,100 B) are untouched.", "compile_time_allocations": { "framebuffer": 76800, "row_diff": 1284, @@ -200,7 +200,7 @@ "_compile_note_dc4_1437": "**DC4 #1437 domain boundaries: `app` 50,520 -> 50,680 B (+160 B), both profiles.** Itemised on the CI `embedded` run for this branch against its base (099bd1be), all four items structural and none of them a buffer: (1) +72 B for the three derived cache keys in `CatalogState` (`ride_profile_for`, `ride_preview_for`, `nav_preview_route`) widening from `Option` (8 B) to `Option` / `Option` (32 B) — a durable object identity plus a source and a view revision, which is what removes the catalog-index remap and with it the late-answer and replaced-bytes staleness holes; (2) +24 B for the three `Revision` counters those keys read (`source_revision`, `ride_track_view`, `nav_preview_view`); (3) +40 B for the new `WeatherDomain` field on `App` (a token source, the installed `WeatherData` identity/revision, the in-flight refresh token, the request flag and the last terminal result); (4) +24 B inside `RetentionMachine` (its own token source and the in-flight sidecar-write slot, so a failed metadata write can be re-queued for the id it was actually about). Linked resident clears the 320,688 B `resident_ram_max` ceiling by ~16 KB on both profiles and `.uninit` is untouched at 132,096 B, so nothing moves on glass. Re-verified against S3 #1446's placement-constructor merge: `app` reads 50,680 B on both profiles there too, and `init_idle`'s own frame is 76 B of the 4,096 B ceiling. `measured_resident` (320,616 B) is a record, not a gate, and is left as it stands: it predates this branch.", "_compile_note_dc5_1438": "**DC5 #1438 pass coordinator: `app` 50,680 -> 50,920 B (+240 B), both profiles.** Two structural items, measured on the thumbv8m target, neither of them a buffer: (1) +224 B for the new `PassState` field on `App` \u2014 136 B of it the `Connections` set (eight named cross-domain slots: two UI intents, the expiry intent, the active-route notice, the fault bit set, and the three deferred slots whose two halves are what make a later-to-earlier value land on the *next* pass), and 88 B the levels a stage detects an edge against (the last store revision seen, the revision the catalog announced, the last link state, the transfer state, the active route identity, the 12 B `Capabilities` the admission stage recalculates, and the re-entrancy flag); (2) +16 B in `CatalogState` for the catalog domain's admitted-intent slot (`Option` is 16 B \u2014 one identity and a tag), with its 4 B token source and the in-flight flag landing in existing padding. The test-only stage recorder is `#[cfg(test)]` and is not in this figure. Linked resident and `.uninit` are unchanged (304,824 B / 132,096 B on the pinned host, against the 320,688 B ceiling), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B against its 4,096 B limit \u2014 the field is written straight into its `.bss` slot like every other.", "_compile_note_dc7_1440": "**DC7 #1440 conformance gate: `app` 50,920 -> 50,904 B (-16 B), both profiles.** A saving, from deleting the `UiRuntime` -> `Recorder` connection the gate proved was destroying a rider request: the pass took the ride-close one-shot at stage 4 and dropped it at stage 7, where Recorder has no machine to act on it, so the ride was never finalized and no executor was told. Out of `Connections` go the `Slot` and the `Deferred`; with the last `KeepFirst` connection gone, `Merge` goes with them and both remaining deferred slots lose their merge field. The close is back on the legacy drain that performs it, and returns with Recorder's machine at #1397 S6. Linked resident and `.uninit` are unchanged within the ceilings (304,808 B / 132,096 B on the pinned host, against 320,688 B), the arena is untouched, and `init_idle`'s construction frame is unmoved at 76 B of its 4,096 B limit.", - "_compile_note_s2_1485": "**#1397 S2 answered-domain seams: `app` 50,904 -> 50,928 B (+24 B), both profiles.** The four domains whose legacy answer is already terminal get their own pending state, so the rider's request stops living in `Activity` and starts living with its owner. Out of `Activity` go seven one-shot slots (`nav_request`, `detour_request`, `detour_commit`, `detour_cancel`, `dfu_request`, `nav_cancel`, `card_scan_request`) for -88 B; out of `App` go `card_free_bytes` (-16 B) and `HostPending`, whose persist half moved to `SettingsMachine` and whose remaining store-commit counter is now a bare `u32` (-8 B). In their place: `NavigatorMachine` (96 B \u2014 the two bounded planner requests, the per-family phase, and the `RerouteFreeze` that moved in from `App` so the domain is its only writer), `StorageInfo` (24 B \u2014 the free-space figure `card_free_bytes` used to hold, plus its token), `SettingsMachine` (12 B) and `DfuState` (8 B). The +24 B is padding plus the four token generations the epic's ownership rule requires: a domain that cannot validate a token cannot own an outcome (#1433 \u00a74.3). Linked resident 304,832 B and residual main stack 54,592 B on the pinned host (against 320,688 B and a 38,808 B floor); flash 1,449,328 B against 1,524,676 B; the arena, the poll frame (9,792 B) and the main task future (1,100 B) are untouched.", + "_compile_note_s2_1485": "**#1397 S2 answered-domain seams: `app` 50,904 -> 50,928 B (+24 B), both profiles.** The four domains whose legacy answer is already terminal get their own pending state, so the rider's request stops living in `Activity` and starts living with its owner. Out of `Activity` go seven one-shot slots (`nav_request`, `detour_request`, `detour_commit`, `detour_cancel`, `dfu_request`, `nav_cancel`, `card_scan_request`) for -88 B; out of `App` go `card_free_bytes` (-16 B) and `HostPending`, whose persist half moved to `SettingsMachine` and whose remaining store-commit counter is now a bare `u32` (-8 B). In their place: `NavigatorMachine` (96 B \u2014 the two bounded planner requests, the per-family phase, and the `RerouteFreeze` that moved in from `App` so the domain is its only writer), `StorageInfo` (24 B \u2014 the free-space figure `card_free_bytes` used to hold, plus its token), `SettingsMachine` (12 B) and `DfuState` (8 B). The +24 B is padding plus the four token generations the epic's ownership rule requires: a domain that cannot validate a token cannot own an outcome (#1433 \u00a74.3). Linked resident 304,832 B and residual main stack 54,592 B on the pinned host (against 320,688 B and a 38,808 B floor); flash 1,449,768 B against 1,524,676 B; the arena, the poll frame (9,792 B) and the main task future (1,100 B) are untouched.", "compile_time_allocations": { "framebuffer": 76800, "row_diff": 1284, diff --git a/host/obc-host-core/tests/device_core_conformance.rs b/host/obc-host-core/tests/device_core_conformance.rs index c7bad667c..c3e80c76d 100644 --- a/host/obc-host-core/tests/device_core_conformance.rs +++ b/host/obc-host-core/tests/device_core_conformance.rs @@ -1499,8 +1499,9 @@ fn a_transfer_during_planning_withdraws_heavy_capability() { assert!(plan.effects.catalog.is_empty() && plan.effects.retention.is_empty(), "nor a store operation"); // The settings write is not heavy and is not withdrawn: the trusted-clock stamp this fixture // makes is a rider edit like any other, and a transfer holding the *store* has nothing to say - // about a settings revision. That distinction is what `Capabilities` is for. - let _ = plan.effects.settings.take(); + // about a settings revision. That distinction is what `Capabilities` is for — asserted, not + // merely tolerated, so a settings write that quietly stopped going out fails here. + assert!(plan.effects.settings.take().is_some(), "the settings write is unaffected by the transfer"); assert!(adapter.pending().holds(LegacyReply::RoutePlan), "and the running plan is untouched"); assert!(navigator.is_current(token), "its operation is still the current one"); @@ -2008,7 +2009,15 @@ fn the_conformance_replay_wake_profile_and_pass_cost() { /// `(passes, immediate, timed, sleep-until-event)` for the replay above. A ratchet, not a budget: /// the numbers move when the pass's wake decisions do, and #1397 compares against them. -const WAKE_PROFILE: (u32, u32, u32, u32) = (366, 6, 236, 124); +/// +/// #1397 S2 moved two passes from *sleep-until-event* to *timed* (236/124 → 238/122) and nothing +/// else. The cause is a settings write's failure now being raised into the fault connection at +/// stage 1 instead of posting its card directly, so the card — and the wake its 30 s timeout arms — +/// lands at stage 13 of that pass rather than before stages 2-12 run. Same pass, same card, same +/// rider-visible state (the disposition table is unchanged); only which side of `stage_plan` the +/// deadline appears on moved. The two figures the epic gates — 366 passes and 6 immediate wakes — +/// are untouched, which is the claim that matters: nothing here polls. +const WAKE_PROFILE: (u32, u32, u32, u32) = (366, 6, 238, 122); // ==================== the resource gate ==================== From 51b620a988574c6136e9c1a066d1e8873b37d8a7 Mon Sep 17 00:00:00 2001 From: timohueser Date: Mon, 24 Aug 2026 11:43:21 +0200 Subject: [PATCH 3/3] Trim revision references from the wake-profile comment Co-Authored-By: Claude Fable 5 --- .../obc-host-core/tests/device_core_conformance.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/host/obc-host-core/tests/device_core_conformance.rs b/host/obc-host-core/tests/device_core_conformance.rs index c3e80c76d..f0ee7cb5b 100644 --- a/host/obc-host-core/tests/device_core_conformance.rs +++ b/host/obc-host-core/tests/device_core_conformance.rs @@ -2008,15 +2008,13 @@ fn the_conformance_replay_wake_profile_and_pass_cost() { } /// `(passes, immediate, timed, sleep-until-event)` for the replay above. A ratchet, not a budget: -/// the numbers move when the pass's wake decisions do, and #1397 compares against them. +/// the numbers move when the pass's wake decisions do. /// -/// #1397 S2 moved two passes from *sleep-until-event* to *timed* (236/124 → 238/122) and nothing -/// else. The cause is a settings write's failure now being raised into the fault connection at -/// stage 1 instead of posting its card directly, so the card — and the wake its 30 s timeout arms — -/// lands at stage 13 of that pass rather than before stages 2-12 run. Same pass, same card, same -/// rider-visible state (the disposition table is unchanged); only which side of `stage_plan` the -/// deadline appears on moved. The two figures the epic gates — 366 passes and 6 immediate wakes — -/// are untouched, which is the claim that matters: nothing here polls. +/// Two replay passes are *timed* rather than *sleep-until-event* because a settings write's +/// failure is raised into the fault connection at stage 1, so its card — and the wake the card's +/// 30 s timeout arms — lands at stage 13 of that pass rather than before stages 2-12 run. The two +/// gating figures — 366 passes and 6 immediate wakes — are the claim that matters: nothing here +/// polls. const WAKE_PROFILE: (u32, u32, u32, u32) = (366, 6, 238, 122); // ==================== the resource gate ====================