fix(ui): D5 — a sheet's owed base draw survives until a frame pays it - #1590
Conversation
`handle` opened by calling `settle()` just to ask whether a page slide was in flight. `settle` is also the edge `tick_timers` reads to arm the base draw the settling frame owes, and input runs before the tick in one pass — so a gesture landing at or after `slide start + SLIDE_MS`, inside an ordinary double-tap, retired the slide silently and the tick then assigned `needs_base = false`. The settling frame either left the outgoing page's ink in the 4 px margin either side of the sheet (and, on the quick drawer, the 32 rows the editor gives back going 136 -> 104) or, with no render key moved, was never asked for at all and the pages stayed half-slid. Input now asks a pure `slide_running(now_ms)`; only the tick retires a slide. `needs_base` becomes a debt: `tick_timers` ORs into it, `slide_to` and `swapped_in` arm it eagerly as before, and nothing clears it but a frame that actually drew the base — `UiRuntime::spend_base_draw`, called from `render_scene_map_rain_timed` under `!sheet_only`. `Screen::draw` stays `&self` and side-effect-free; the mutation lives at the frame boundary. A pass that ticks and drops its frame keeps the debt. `ContextDrawerScreen::uncovered` is deleted: the debt subsumes it, since `swapped_in`'s `needs_base: true` now survives to the first base draw on its own. Also registers `Msg::QuickBrightness` in `fixed_buffer_captions_fit` — the brightness editor's title writes it into a discarded `String<24>` and was never bounded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…draw The sheet-to-sheet swap had no differential-repaint coverage: both drawer replays are the quick drawer's, and both reach the screen below only through a page slide. `the_sheet_swap_puts_back_the_band_the_taller_sheet_held` drives the real Down+Back chord on the riding Map, wraps to the Map display row and presses it — the 244 px sheet is replaced by the 156 px one — and holds the swap to one base draw, on the frame the press produced, reaching into the 88 rows above row 164 that only the taller sheet covered. Dropping `needs_base: true` from `swapped_in` loses 20,416 pixels at (11, 76). `drawer_pages_replay` gains one keyed step at 2_080 — the exact end of the 180 ms slide out of the brightness editor — so a gesture lands on the frame the slide settles on. Restoring `settle` at the top of `handle` loses 258 pixels at (3, 24): the outgoing page's ink in the sheet's inset margin. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe change replaces per-frame drawer redraw flags with persistent base-render debt. Quick and context drawers preserve debt across transitions and dropped frames. ChangesDrawer base-render debt
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The drawers now retain required base redraws until the base is actually rendered, preventing stale pixels and half-slid pages; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant Input
participant ContextDrawerScreen
participant UiRuntime
participant render_scene_map_rain_timed
Input->>ContextDrawerScreen: query slide_running()
ContextDrawerScreen->>ContextDrawerScreen: retain transition state
render_scene_map_rain_timed->>UiRuntime: render frame
UiRuntime->>ContextDrawerScreen: tick timers and spend base draw
ContextDrawerScreen-->>UiRuntime: clear base-render debt
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Linked Issues checkExplanation The changes address issue Full details: Out of Scope Changes checkExplanation The changes remain within issue ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
timohueser
left a comment
timohueser
left a comment
There was a problem hiding this comment.
Adversarial review — D5 (#1589), head 65a3615
Reviewed by reading the complete diff (7 files, +463/−65) plus the surrounding old and new
code, and by running read-only probes in the implementer's worktree. No blocking finding.
What I probed, and what it showed
1. The debt's lifecycle, end to end
Arm sites. Three, and only three: context_drawer.rs:793 / quick_drawer.rs:375
(slide_to), context_drawer.rs:595 (swapped_in), and the tick's OR at
context_drawer.rs:743 / quick_drawer.rs:328. Nothing else writes needs_base in either
drawer. Confirmed by grep over both files.
Clear sites. One: ui_runtime.rs:308 spend_base_draw, reached only from
app.rs:3489 under !sheet_only. Screen::clear_base_debt (screen/mod.rs:1272) has no
other caller in the tree. Confirmed by grep across firmware/ and apps/.
(a) Is there a frame that draws the base without going through
render_scene_map_rain_timed? No. Every public render entry — render_frame,
render_frame_with_rain, render_map, render_map_timed, render_map_rain_timed,
render_scene_map_timed — funnels into render_scene_map_rain_timed, and the base's draw
happens only inside that function's one draw loop (app.rs:3472-3480). The other render
entry point, App::render_overlay (app.rs:3495), paints only the hold bulge and the
Recalculating banner and never the base — so the board's freeze branch
(ride.rs:2661-2682), which calls it alone, correctly clears nothing.
I checked every host:
| Host | Base-draw call | Resident? |
|---|---|---|
| board | ride.rs:2800 render_map_rain_timed |
yes (ride.rs:936) |
| sim GUI | gui.rs:833 → map_file.rs:149 render_scene_map_rain_timed |
yes (gui.rs:474) |
| sim headless / snapshot sweep | main.rs:1506 render_frame |
no (main.rs:1368, deliberately) |
| web demo | demo.rs:338 render_frame |
yes (demo.rs:475) |
dirty_parity candidate / reference |
dirty_parity.rs:474 render_map |
yes / no |
The snapshot host declares no resident frame, so sheet_only is false on every frame there,
the base is drawn every frame, and the debt is discharged every frame. That is why nothing in
this slice can move a snapshot pixel, and the reported 312/312 is consistent rather than
lucky.
(b) Can spend_base_draw run on a frame that did not actually draw the base? No. The
guard is not a re-derivation — it is the same local. let sheet_only = ui.sheet_only();
at app.rs:3466 is computed once, read at app.rs:3474 to skip the base's draw, and read
again at app.rs:3489 to clear. Same expression, same pass, same inputs, no mutation in
between. This is the tightest possible form of that guard and it is worth keeping that way.
I chased the one way the two could still disagree — a clipped base draw that pays a debt
it did not honour. cv.set_clip(render_clip) can narrow the base's primitives. On the board,
clip is dirty.region only when needs_map is false (ride.rs:2758). A region survives
take_dirty only when region ticks were the sole dirt, and both drawers return
region: None on every tick (context_drawer.rs:753/757, quick_drawer.rs:339/343), so any
frame that arms the debt also sets the full-frame map_dirty and folds the region away.
Behind that, advance_timers skips the base entirely while it is frozen
(ui_runtime.rs:236, first = base + base_frozen()), so the base cannot contribute a region
under a sheet either. And every latched retry drops the region outright
(ride.rs:2570-2574). Closed.
(c) Does sheet_only consult needs_base, and does that converge? Yes —
ui_runtime.rs:296 is && !self.stack.iter().skip(base + 1).any(|s| s.needs_base()). That
is the linkage the whole model hangs on, and it is what makes the swap's uncovering draw
happen at all: swapped_in arms the debt during input, so sheet_only is false on the swap
frame, so the base is drawn, so the debt is cleared. It converges in exactly one frame and
cannot oscillate, because the clear is unconditional given !sheet_only and the only
re-arming inputs (sliding > 0 || settled) are false on a landed sheet.
The energy question behind it: can the debt stay armed forever and hold sheet_only false,
turning every open step back into a map render? No. A debt survives only across passes that
render nothing, and a pass renders nothing only when nothing is dirty — in which case the
board also builds no Reader (the whole map arm is inside else if dirty.map). Every arming
event sets changed on the same tick, so a render is always demanded, and the first render
pays.
2. slide_running purity
Old gate: settle(now) then slide_ms.is_some(). settle clears the slide when
now - started >= SLIDE_MS, so the old predicate is
slide.is_some() && now - started < SLIDE_MS.
New gate: slide_ms.is_some_and(|s| now.wrapping_sub(s) < SLIDE_MS). Algebraically identical,
same wrapping_sub, same < vs >= polarity, in both drawers
(context_drawer.rs:657, quick_drawer.rs:232). No mutation. Gesture acceptance at the
boundary instant is bit-identical, including within a multi-gesture batch (a second gesture in
the same pass reads the same now_ms and gets the same answer either way).
I also checked what now observes a slide that stays Some between input and tick: nothing.
Neither drawer's render key carries slide state (context_drawer.rs:609,
quick_drawer.rs:201), and sheet_height / visible_height / draw run after the tick in
the pass order (pass.rs:361-362 then the render). So no key moves and no frame is drawn from
the un-retired state.
3. Wake
next_wake_ms in both drawers is derived from opening and sliding only
(context_drawer.rs:751-758, quick_drawer.rs:336-344) and never reads needs_base. Nothing
else in advance_timers reads it. sheet_only is the sole reader of needs_base in the tree
(plus base_needs_reader, transitively). The claim that the wake is unaffected holds, and
skipping the wake-profile isolations is justified.
4. The board's dropped-frame paths
No board file changed, and none needs to. The board's sheet_only local at ride.rs:2759 is
sampled for logging only. The discharge happens inside the app's render, which the board
reaches through render_map_rain_timed at ride.rs:2800. Path by path:
ride.rs:2704(reader build failed),:2726(scratch arena held),:2777(weather bind
failed) — all threereturn Nonebefore the render closure runs, so the debt survives
for the retry, whichpending_map_redrawforces full-frame. Correct.ride.rs:2888(present did not reach glass) — here the debt is cleared, and the PR's
justification is right for a reason worth stating:MapDisplay::render_frame
(map_plane.rs:183) renders into an owned, residentFrame64, so the drawn base is
still in the framebuffer when the push fails, andpresent_framere-arms a full push on
failure (map_plane.rs:216-222). Even if the retry re-renders sheet-only, the base pixels
it leaves standing are the ones this frame drew. Correct.
5. uncovered deletion
The D4c behaviour survives, and is strictly better. Under the old two-flag model, a swap frame
that was dropped left uncovered already taken and needs_base reassigned to false on the
next tick — the band was never put back. That is the second hole #1589 names, and deleting the
field is what fixes it rather than what risks it. The one-and-only-one property is now pinned
twice: the_display_row_swaps_the_sheet_and_back_lands_on_the_map (unit) and
the_sheet_swap_puts_back_the_band_the_taller_sheet_held (pixel). I support the disposition.
6. The new parity test
I checked its constants against the draw rather than taking them: MAP has 5 rows and
MAP_DISPLAY 3, and Page::Root.height is SHEET_PAD*2 + rows*ROW_H = 24 + 220 = 244 and
24 + 132 = 156; the card is laid out at rect(4, top, …) (context_drawer.rs:821), so on
a 320 px panel the two top rows are 76 and 164 exactly as the test states. The
assertions are not tautological: the named mutant (dropping needs_base: true from
swapped_in) makes the swap frame sheet-only, features_tried 0, paid.len() 0 — and
drive_replay's own full-frame comparison fails one step earlier still.
7. Tautology audit on the four new tests
a_press_as_the_slide_lands_does_not_spend_the_base_draw_it_owes(both drawers): the
clear_base_debt()inside the slide loop is what makes this real — without it the final
needs_base()would be trivially true fromslide_to. Withsettlerestored at the top of
handle, the settling tick seessettled == falseandsliding == 0, so both halves fail.
I did not run the mutant (file edits are out of scope for this review), but the failure
follows from the code with no room for doubt.the_base_draw_a_sheet_owes_outlives_a_pass_that_drew_no_frame: fails under the tick
assigning rather than ORing. Real.- The replay's mutant claim also holds analytically: at 2 080 ms with
settlerestored, the
2 060 ms render has already paid the debt, the tick arms nothing, the frame renders
sheet-only, and the outgoing page's ink in the 4 px margin stands against a reference that
redrew it. every_quick_drawer_string_fits_the_sheet_in_every_language: I cross-checked the string set
against the draw.draw_root(:493-501),draw_brightness(:508),draw_power_confirm
(:535/:542) anddraw_powering_off(:565) between them draw exactly the eightMsg
values the test measures, in exactly the fonts it uses. Nothing is missed. The centred budget
is right by construction: a string of width p centred at 120 on a card spanning 4…236
clears the edge by116 − p/2, which is ≥ 8 exactly when p ≤ 216.
8. Exclusions
All met. Seven files, none of them board, display, render, ports, i18n catalog, settings, or
snapshot recipe. No constant moved. No copy changed. Screen::draw is untouched and still
&self. No new DrawerKey field. sheet_only's three exclusions are unchanged. The
snapshot manifest is not in the diff.
9. Checks I ran
Read-only, in the implementer's worktree:
| Probe | Result |
|---|---|
cargo test -p obc-app --lib |
974 passed |
cargo test -p obc-app --test i18n |
8 passed |
cargo test -p obc-sim --test dirty_parity |
10 passed, including both new/amended cases |
I did not re-run the sweep, rebuild the board, or re-derive resources, per the review model.
Findings
Nit 1 — settle's new doc overstates who calls it
firmware/obc-app/src/screen/context_drawer.rs:802 and
firmware/obc-app/src/screen/quick_drawer.rs:384 both now read "The tick calls this and
nothing else does". A test calls it directly: context_drawer.rs:1455,
d.settle(1_000 + SLIDE_MS);. The intent is clear and the production claim is true — but the
sentence as written is falsifiable by grep, which is the kind of comment that ages badly.
Suggest "nothing in production calls this but the tick".
Nit 2 — the i18n row's failure mode is not quite what the comment says
firmware/obc-app/tests/i18n.rs:109-110: "an over-length caption blanks the whole title
rather than truncating." At this call site the write! is four fragments — the caption, a
literal space, the number, a literal % — and heapless's push_str is atomic per fragment.
So a caption that fits 24 bytes but leaves no room for " 100%" yields a truncated title
(HELLIGKEIT with no percentage), not a blank one. The blank-title claim is only true when
the caption alone overflows.
The budget itself is correct — 24 − len(" 100%") = 19 — and the row is a real guard, so
this is prose only. It also matches the file's existing header prose, so it is house style;
worth one word of precision if the file is touched again.
Nit 3 — the sheet budgets restate the draw's literals
firmware/obc-app/src/screen/quick_drawer.rs:979-985 derives card_w from W - 8 and
title_room from W - 4 - 14 - MIN_CLEAR, with 4, 14 and 8 restated from
draw (:403) and draw_brightness (:509) rather than imported. If the sheet's 4 px inset
or the title's 14 px offset ever moves, the test keeps passing against a stale budget; the
assert_eq!((centred_room, title_room), (216, 214)) pins the arithmetic, not its source.
I am not asking for a change: there are no named constants to import, and D4a's
every_label_and_choice_fits_the_sheet_in_every_language (context_drawer.rs:1487) establishes exactly this convention with the same literals.
Recording it so that whoever next
names those insets as consts knows two tests want them.
Deviation dispositions
1. The amended sheet-swap test is the_display_row_swaps_the_sheet_and_back_lands_on_the_map,
not the issue's a_sheet_swap_asks_for_the_base_once. Accepted. The issue cited
context_drawer.rs:1579-1581, which is this test's second-half assertion — the name in the
issue was simply wrong. The rewrite does not weaken it: the original's claim ("no frame after
it does: the swap costs exactly one map draw") is still asserted at the end, now after
clear_base_debt(), and a strictly new assertion ("a tick that drew no frame does not put the
band back") sits in front of it. Net strengthening.
2. One more amendment than the issue listed —
a_page_slide_asks_for_the_base_and_a_settled_page_stops_asking re-expressed against
clear_base_debt() in three places. Accepted, and it was unavoidable. Under the debt model
the original's three assert!(!d.needs_base()) lines would be false by design. I checked each
one: every negative assertion the original pinned is still present, just moved to after the
draw pays, and one new positive assertion was added at each site. Nothing was deleted and
nothing was loosened. The PR body should have listed it — it lists it now in prose, which is
enough.
3. ContextDrawerScreen::uncovered deleted (the issue's vetoable disposition). Supported.
The field's whole job — making the swap cost exactly one base draw — is done strictly better by
a debt, and keeping it would preserve the very hole D4c's own review note opened. If the owner
vetoes, the veto costs a field and a second reason for one fact; I would not spend it.
Verdict: APPROVE
The model is sound and the seam is in the right place. The clear is guarded by the same
local as the draw, not by a re-derivation — that is what makes "a frame that drew the base"
un-fakeable, and it is the detail the whole slice rests on. Every host reaches it, the three
board paths that drop a frame all return before it, and the fourth (a failed present) is
covered by the framebuffer being resident. The debt cannot strand: sheet_only reads
needs_base, so an armed debt forces the very draw that pays it, in one frame, with no
oscillation and no standing energy cost.
The three findings are documentation and convention nits. None of them blocks; none of them
needs a re-review round. Land it.
The settle doc now scopes its only-caller claim to production (a test calls it directly), and the brightness-title guard row states the real failure mode — per-fragment truncation, not a blanked title. Review findings on #1590. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Closes #1589. Slice D5 of the two-drawer UI epic #1515 — the last one.
The defect, and its two faces
Both drawers opened
handlewithself.settle(cx.now_ms)for one reason only: to ask whether apage slide was still in flight. But
settleis not a question — it retires the slide, and theedge it returns is exactly what
tick_timersreads to decide that the settling frame owes thescreen below a draw. Input runs before the tick inside one pass. So a gesture landing at or after
slide start + 180 ms— well inside an ordinary double-tap — retired the slide silently, and thetick then found no edge and assigned
needs_base = false.Two things fell out of that one line:
either side of the sheet, and the ink they leave there stood until the next frame that drew the
base. The quick drawer leaving the brightness editor also gives back the 32 rows the sheet loses
going 136 → 104 — about 7,400 px of stale parchment over the map.
returned
ScreenTick::idle(): nothing was asked for and the two pages stayed half-slid. Thatis reachable whenever the stealing gesture moves no render key — a Select-hold on the sheet, or
any Up/Down on D4d's one-row
ROUTE_PLAN, wherestep_selection(0, n, 1) == 0.A second, narrower hole had the same shape: the tick spent the edge whether or not that pass
rendered anything, and the board can tick and then drop its frame (the scratch arena is held, a
weather bind failed).
What changed
Input asks; only the tick retires.
handlenow calls a pureslide_running(now_ms). Behaviourat the gate is identical —
now - started >= SLIDE_MSstill means "not running", so the gesture isaccepted exactly as before — but the
settlededge is no longer consumable by anything but thetick.
needs_basebecomes a debt, not a per-frame flag.tick_timersORs into it.slide_toandswapped_instill arm it eagerly. Nothing clears it but a frame that actually drew the base:App::render_scene_map_rain_timedcallsui.spend_base_draw()after the draw loop under!sheet_only, which is the one place that answer exists.Screen::drawstays&selfandside-effect-free; the mutation lives at the frame boundary beside
render_clip.take(). A pass thatticks and drops its frame leaves the debt armed for the pass that renders.
ContextDrawerScreen::uncoveredis deleted. With a debt,swapped_in'sneeds_base: truealready survives to the first base draw, which is the whole job the second flag did. One field out,
one reason instead of two. This is a recorded vetoable disposition — D4c introduced the field in
#1586 and the owner may prefer it kept for one more slice.
The parity step
The sheet-to-sheet swap had no differential-repaint coverage: both drawer replays in
dirty_parity.rsare the quick drawer's, and both reach the screen below only through a page slide.the_sheet_swap_puts_back_the_band_the_taller_sheet_helddrives the real Down+Back chord on theriding Map, wraps to the Map display row and presses it — the 244 px sheet is replaced by the 156 px
one — and holds the swap to one base draw, on the frame the press produced, reaching into the 88
rows above row 164 that only the taller sheet covered. The full-frame reference is the oracle for
those rows. Dropping
needs_base: truefromswapped_inloses 20,416 pixels at (11, 76).drawer_pages_replaygains one keyed step at 2,080 ms — the exact end of the 180 ms slide out ofthe brightness editor — so a gesture lands on the frame the slide settles on, and the existing
rendered_base(1_900, 2_300) > 0assertion tightens to name that frame. Restoringsettleat thetop of
handleloses 258 pixels at (3, 24): the outgoing page's ink in the sheet's inset margin.The overflow pin
The context sheet's copy has been measured in all four languages since D4a. The quick drawer's
was measured by nobody, and it is the sheet with the least room — its captions are centred on a
232 px card, so an overrun is clipped at both ends.
every_quick_drawer_string_fits_the_sheet_in_every_languagemeasures the four root captions and both BLE states, the two lines of the guarded confirmation and
the brightness title in
Font::Label, and the terminal line inFont::Body, against the sheet's own216 px centred budget (214 px for the left-inset title). It pins the two constraints that have
nothing left, by name rather than in PR prose:
es "BLUETOOTH INACTIVO"is exactly 216 px, zerospare, and
en "POWERING OFF..."/de "SCHALTET AUS..."are 210 px in Body, 6 px spare. Alonger translation of any of the eight now fails here instead of on the panel.
fixed_buffer_captions_fitgains one row forMsg::QuickBrightness: the brightness editor writes itplus
" 100%"into aheapless::String<24>and discards the result, so an over-length caption wouldrender fully blank. It was never registered when D2 added it. Current worst is
de "HELLIGKEIT"at10 bytes — a guard, not a fix.
No copy change, no i18n catalog change, no snapshot recipe change, no board / display / render
crate change, no constant change.
Documentation
No public documentation changed.
docs/content/software/ui.mdalready describes the behaviour thisslice makes true where it was not ("the screen below a sheet is held still… the map is drawn once
when the shorter display sheet replaces the taller sheet").
Checks run
cargo test -p obc-appcargo test -p obc-simdirty_parity10/10cargo clippy -p obc-app --all-targets -- -D warningscargo clippy -p obc-sim --all-targets -- -D warningscargo fmt --check(root + the three standalone roots)tools/check_one_home.pytools/check_render_keys.pytools/check_screen_vocabulary.pypython3 -m unittest discover -s firmware/tools/testspython3 -m unittest discover -s tools/testsfirmware/ui-snapshots.sha256. Noupdatestep.resource_baseline.jsonresource guards passed;allocation report matches baselineDeliberately omitted, per CLAUDE.md's verification budget:
obc test full(nothing cross-cuttingchanged),
obc suites check(no suite, workflow or registry change — every test lands in an existingfile), wake-profile isolations (
next_wake_msis computed from the same terms and isNoneon theaffected frame either way; what this restores is a repaint, not a wake),
docs/build_docs.py --check-links(no doc touched), the iOS / web / desktop surfaces, any on-device / flashing / HIL /on-glass work (owner ruling, 2026-08-30), and any routine mutant demonstration. CI is the gate.
Resource figures
Exact-match gate, met:
compile_time_allocations.apppoll_frame_measured.uninitsize_of::<Screen>()size_of::<ContextDrawerScreen>()uncoveredlanded in paddingsize_of::<QuickDrawerScreen>()size_of::<RenderKey>()size_of::<UiRuntime>()VERSION/MIN_SUPPORTEDTwo link-level records moved, and they are reported as cross-checkout records rather than as a
result of this slice, because proving a cause would need a base rebuild the budget forbids:
measured_resident(.bss 299,864 + .data 5,800)measured_flashresidual_stack_measuredfirmware/tools/resource_baseline.jsonwas last re-pinned at0280e271, which predates D4b(#1583), D4c (#1586), D4d (#1588) and a long run of other merged app work — so the recorded base is
several epics behind the head, and the deltas are cumulative across all of it. This slice adds no
resident state at all: the debt is the
boolthat was already there, andContextDrawerScreengivesone up. The non-gating figures did not change size class, and every exact-match gate passed.
Notes
swapped_in'snow_mssits with the sheet, unchanged; no newDrawerKeyfield and no render-keychange — the debt is never a drawn fact.
than at the present:
ride.rs:2726and:2777return before rendering, so the debt survives; a:2888present failure happens after the plane already holds the drawn base, and the retryre-presents that same plane.
References #1515.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests