Pin slot 0 as the recovery anchor and harden the boot hatch - #45
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe changes centralize resistive-button decoding, add boot-time recovery confirmation, introduce identity-aware OTA slot planning and structured outcomes, validate firmware descriptor fields, and document the anchored update flow. ChangesRecovery input and anchored OTA flow
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
The FreeInk SDK's RecoveryBoot (freeink-sdk 2f9f73d) treats slot 0 as a
recovery firmware that is "deliberately never reflashed". Our port of the
hatch matched the mechanism but not that convention: the SD updater wrote
whichever slot was inactive, so an update applied while running from slot 1
landed in slot 0 -- overwriting the very image Back+Up returns to, and (in a
mixed install) any CrossPoint recovery firmware parked there. Half of all
installs also ran from slot 0, where recover_to_slot0 is a deliberate no-op,
so the hatch was absent on those boots.
Updates now always target slot 1 and nothing writes slot 0. That cannot be
"dest = 1" alone: the slot we execute from can never be the write target, so
a second consecutive update would have to be refused outright. Instead a boot
that finds a trigger while running from slot 1 points otadata back at the
anchor and resets *without* consuming the trigger, and the anchor boot applies
the image into slot 1. One extra reboot; slot 0 still never written. The
hand-off terminates because the anchor is not the update slot, so its boot
takes the write path and clears the trigger either way.
Bouncing into a foreign anchor would move the user off their firmware and
strand the update on something that cannot consume FWUPDATE.BIN, so the anchor
is identified by the app descriptor's project_name and a non-match refuses
instead. The descriptor offset (0x50) is verified against a built firmware.bin
rather than inferred from the struct. Validation also moved ahead of the bounce
decision, so a corrupt image no longer costs a pointless trip through the
anchor to discover it was corrupt.
The combo itself was sampled at a single instant. A held combo is continuous
and the switch it arms is a reboot into another slot, so it now needs 3
consecutive in-band readings 4 ms apart (~12 ms of hold), giving up after
~32 ms. That is a shorter budget than RecoveryBoot's 16 polls / 5 confirms /
6 ms because most of theirs covers InputManager's debounce state machine
warming up, which reading the ADC directly does not have -- and the idle path
runs on every deep-sleep wake, where the latency is felt.
Detection no longer duplicates the ladder thresholds. The bands, HardwareButton,
and classify move to app_core::buttons, and recovery_combo_held is derived from
the same tables the input task reads, so a recalibration cannot silently move
the hatch off the documented buttons. The move is pure; no behavior changes.
Both hardened paths were unverifiable because the decisions were tangled with
flash and SD I/O, so they follow plan_switch behind sans-IO seams:
proto::ota::{plan_update_action, plan_recovery_switch, project_name} and
app_core::buttons::ComboConfirmer. fw is left as the I/O that answers them.
The reboot-crossing hand-off is now exercised by a simulated device carrying
real otadata sectors through successive boots, driving the actual plan_switch
and active_app_slot and asserting after each switch that the bootloader would
select the intended slot: staged-from-anchor lands in one reboot, staged-from-
update-slot bounces then lands with the trigger surviving the reset, a foreign
anchor refuses, the hand-off always terminates and bounces at most once, four
updates in a row never write slot 0, and the hatch still finds an intact anchor
after an update installed through the bounce. 31 new host tests (app-core
107->116, proto 127->142); fw gains none, correctly, since it is now only I/O.
Two fixes fell out of the refactor: the anchor probe is short-circuited so it
costs nothing when running from slot 0, and a NoUsableAnchor refusal now clears
the trigger instead of re-running the refusal on every boot.
Verified: tools/check.sh all (fmt, host clippy, host tests, X3 host tests,
emulator golden frames, firmware clippy and release builds for X4 and X3), plus
clippy -p fw --features ota-selftest and ota-selftest,device-x3 under
-D warnings, since the selftest path changed. Descriptor offset confirmed by
reading a built target/release-images/firmware.bin. Not run: builtin-custom-font
(no feature-gated code there changed). Still hardware-only, and listed as such
in docs/FLASHING.md: a live Back+Up press, and that the trigger file physically
survives the bounce reset on a real card -- both blocked on the missing card
reader, not on code.
Review findings on the anchor policy and the combo timing. The anchor check accepted slot 0 whenever the descriptor's project_name matched, but both device builds shipped the same name while taking different trigger filenames (FWUPDATE.BIN vs FWUPDX3.BIN) and driving different panels. An image for the other board therefore passed, and bouncing into it booted firmware for the wrong hardware that would never look for this board's trigger -- stranding the update. Worse, that state has no way out: the bounce leaves otadata on slot 0, and recover_to_slot0 is a no-op once slot 0 is active, so Back+Up cannot undo it. project_name now carries the board and an updater generation: "CalendulaOS <board> u<generation> (MarigoldOS)", as proto::ota::IDENTITY_X4 / IDENTITY_X3. fw selects one by feature and the strings live in proto beside the comparison that reads them back, so the firmware cannot stamp one identity while the updater expects another. The comparison moved to ota::anchor_can_apply_update and is exact, not a product prefix: a different board, an older updater generation, and the previous product-name-only descriptor are all refused. The hatch itself stays permissive (bootable image only, as upstream RecoveryBoot). That asymmetry is deliberate and now documented: the hatch is an explicit user action whose whole purpose can be falling back to CrossPoint or the stock app in a mixed install, while a bounce is automatic and unrequested, so it must be sure the anchor will finish the job. The combo's stated timing was off by one interval in the other direction from what the code did. N readings are separated by N-1 delays, so 3 confirmations at 4 ms observed 8 ms, not the documented 12, and 8 polls spanned 28 ms, not 32. The window mattered more than the poll count for a destructive combo, and the idle-boot cost is set by MAX_POLLS rather than CONFIRM_POLLS, so a fourth confirmation buys the intended 12 ms for free on the common path. Both windows are now derived constants (CONFIRM_WINDOW_MS, MAX_WINDOW_MS) asserted against a replay of the caller's poll loop, so prose can no longer drift from behaviour -- the literal figures were what went stale. prepare-release.sh's stamp check followed the rename, and deliberately requires the X4 identity rather than the product name: it verifies the X4 ELF, so a stray X3 build left in the shared target directory would otherwise have passed. Verified: tools/check.sh all (fmt, host clippy, host tests, X3 host tests, emulator golden frames, firmware clippy and release builds for X4 and X3), plus clippy -p fw --features ota-selftest and ota-selftest,device-x3 under -D warnings. Both boards' images rebuilt and read back at descriptor offset 0x50: X4 stamps "CalendulaOS X4 u1 (MarigoldOS)", X3 stamps the X3 identity, and neither ELF contains the other's. prepare-release.sh's grep re-run against a freshly built X4 ELF (it builds x4 itself before checking, so the shared-target staleness above does not affect it). 8 new host tests (proto 142->149, app-core 116->117). Unchanged from the previous commit: a live Back+Up press and the trigger surviving a bounce reset on a physical card remain hardware-only.
The stamp check tightened in 5c6c97c only went halfway. `grep -F "CalendulaOS X4"` is a substring match, so it accepted any generation -- an image stamped "CalendulaOS X4 u0 (MarigoldOS)" passed while the OTA bounce, which compares the identity exactly, would refuse to touch it. The comment claimed the release verified the descriptor identity; the code verified only that some string in the ELF contained a board prefix. Two changes. The expected value now comes from proto::ota::IDENTITY_X4 itself rather than a literal restated in the script -- a third copy of that string would have been free to drift from the one the firmware stamps and the updater compares, which is the same failure this whole identity mechanism exists to prevent. Extraction failing (a rename, a reformat) is a hard error, so the check cannot silently weaken to a no-op. And the comparison now reads the descriptor field the updater actually reads -- project_name, the 32 bytes at image offset 0x50 of firmware.bin -- instead of asking `strings` whether the ELF mentions it anywhere. That is the artifact being shipped, at the offset the bootloader and updater use. Verified by running the new block against real and forged images: the genuine X4 image is accepted; the X3 image (the stray-build case), an X4 image forged to generation u0, and the pre-identity product-name-only descriptor are each rejected, and the u0 case is confirmed to have passed the old check. The extraction guard fires when the constant cannot be read. bash -n clean; shellcheck is not installed on this machine, so it was not run. No Rust changed, so the Rust suite was not re-run beyond the commit and push hooks.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/FLASHING.md`:
- Around line 219-233: Update the refusal outcome in the Calendula flashing
documentation to state that the trigger file is consumed or cleared when the
update is refused. Clarify that the user must recopy the trigger file before
retrying, or apply the update through a computer or the OEM updater; do not
imply the original file remains available.
In `@fw/src/ota_update.rs`:
- Around line 303-309: Update the descriptor read failure branch in the OTA
validation flow to log the flash read error before returning false. Preserve the
existing refusal behavior, and use the same logging mechanism and context as the
other rejection paths.
In `@proto/src/ota.rs`:
- Around line 1185-1198: Replace the vacuous loop in
no_action_ever_selects_the_anchor_as_a_write_target with explicit assertions for
each UpdateAction variant, and rename the test to describe the selects_slot
behavior it actually verifies. Assert WriteUpdateSlot selects UPDATE_SLOT,
BounceToAnchor selects ANCHOR_SLOT, and NoUsableAnchor selects None; do not
claim that selects_slot proves the anchor is never a write destination.
In `@tools/prepare-release.sh`:
- Around line 45-50: Update the version verification in the release script to
inspect the app descriptor’s fixed-width version field rather than searching all
strings in the ELF. Reuse the same field-level extraction and validation
approach used by the identity check below, and retain the existing failure
message and exit behavior when the extracted version does not equal VER.
- Around line 63-70: Update the ACTUAL_ID extraction in the release validation
to stop at the first NUL byte, matching proto::ota::project_name() semantics.
Replace the current removal of all NUL bytes while preserving the exact
comparison against EXPECTED_ID and existing failure behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 243d862a-671c-424b-ab2d-fbaf2c0c9a37
📒 Files selected for processing (9)
app-core/src/buttons.rsapp-core/src/lib.rsdocs/FLASHING.mdfw/src/main.rsfw/src/ota_update.rsfw/src/tasks/display.rsfw/src/tasks/input.rsproto/src/ota.rstools/prepare-release.sh
Four review follow-ups on the anchor work, none changing device behaviour. prepare-release.sh checked the version by scanning the ELF for a matching string, which passes on any build that happens to contain one. Both stamps now read the app descriptor in firmware.bin through one desc_field helper: version at 0x30, project_name at 0x50. Field reads truncate at the first NUL, the rule project_name() applies, rather than deleting every NUL from the field. FLASHING.md said a refused update was "left for you to apply". It isn't — every non-bounce path clears the trigger, so a refusal that left it in place would repeat on every boot. Say so, and point at the fix that actually works: a computer or the OEM updater, both of which write slot 0 and restore an anchor. The anchor's descriptor read returned false without logging, alone among the rejection paths. It now logs the flash error like its neighbours. no_action_ever_selects_the_anchor_as_a_write_target asserted nothing in two of its three iterations, and could not have proved its name: the bounce selects the anchor on purpose. Renamed to what selects_slot actually returns, with a pointer to many_updates_in_a_row_never_write_the_anchor, which does prove it.
chongfun
force-pushed
the
recovery-hatch-hardening
branch
from
July 26, 2026 19:14
2ea72a6 to
577db31
Compare
The bootloader loads the slot otadata names only if that image verifies. When it does not, ESP-IDF falls forward to another app partition and boots it without rewriting otadata, so a running firmware that reads otadata can be reading a slot it is not executing. The updater trusted it. Given a slot 0 whose magic and identity survived but whose body did not — an interrupted flash — a bounce pointed otadata at the anchor, the bootloader rejected it and booted slot 1 anyway, and the next boot read "active = slot 0" while executing slot 1. That is the WriteUpdateSlot path: erase slot 1. It would have erased the running firmware mid-update, and with the anchor already corrupt, the last bootable image on the device. The anchor's own validity is the evidence that settles it. A slot only boots if its image verifies, so otadata naming an unbootable anchor is proof we are running somewhere else, and slot 1 is the only candidate left. plan_update_action now returns RunningSlotUnknown there and writes nothing. Because that inference is needed on the ordinary write path too, the anchor check no longer short-circuits on active == UPDATE_SLOT. Checking it properly means validating the whole image, not the first byte: validate_flash_image applies the same segment walk, XOR checksum and appended SHA-256 that a staged update gets. A partition has no length the way a file does, so the walk measures the image from its own structure and the partition size only bounds how far it may run. The Device simulator now models the fall-forward — running_slot() diverges from active() — and asserts on every write that we are not erasing the slot we run from. Reverting the guard alone fails two tests on that assertion. recover_to_slot0 stays magic-only on purpose: a foreign but working anchor should still be an escape. A corrupt one now costs a wasted reboot instead of setting the trap above.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
proto/src/ota.rs (1)
260-275: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winBound the segment walk with
checked_add:pos + data_lencan overflowusizeon the 32-bit target.
data_lenis an attacker/corruption-controlledu32read straight from the segment header (Line 270). On a 32-bit target (riscv32imc, and the host tests when overflow checks are on)pos + data_lenoverflows for large declared lengths, panicking in a checked build and wrapping in a release one — in the wrapping case the> limitguard silently passes and the walk proceeds until a read error. Same forpos + SEG_HEADER_LEN. This is externally influenced data on the flash/SD validation path, so it must not depend on overflow behaviour.As per coding guidelines, "Do not use
unwrap,expect, or panic-prone indexing on externally influenced data".🛡️ Proposed fix
for _ in 0..segment_count { - if pos + SEG_HEADER_LEN > limit { + if pos.checked_add(SEG_HEADER_LEN).is_none_or(|end| end > limit) { return Err(ImageError::BadSegments); } @@ - if pos + data_len > limit { + if pos.checked_add(data_len).is_none_or(|end| end > limit) { return Err(ImageError::BadSegments); }(
Option::is_none_oris stable since Rust 1.82; usemap_or(true, ...)if the toolchain is older.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proto/src/ota.rs` around lines 260 - 275, Update the segment-walking loop around the `pos` bounds checks to use checked arithmetic for both `pos + SEG_HEADER_LEN` and `pos + data_len`. Treat any overflow as `ImageError::BadSegments`, while preserving the existing limit validation and read behavior for valid lengths; avoid panic-prone arithmetic on header-controlled data.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/FLASHING.md`:
- Around line 222-233: Update the sentence introducing the list in
docs/FLASHING.md to say “four cases” so it matches the four bullets that follow;
leave the bullet content unchanged.
In `@proto/src/ota.rs`:
- Around line 1453-1475: The boot model’s ordering in `boot` does not match the
firmware’s write-then-remove-trigger behavior. Update `boot` so
`self.slots[UPDATE_SLOT as usize]` is written before clearing `self.trigger`,
and model trigger-removal failure by preserving the trigger and preventing the
switch when removal fails; otherwise soften the ordering comment to accurately
describe the model’s behavior.
In `@tools/prepare-release.sh`:
- Around line 71-81: Extend tools/prepare-release.sh:71-81 to extract
IDENTITY_X3 alongside IDENTITY_X4 and compare it with the X3 image’s desc_field
80 32 value, while retaining the existing X4 validation. Update
docs/FLASHING.md:326-334 only if needed to accurately describe the resulting
verification of both board identities; otherwise no direct documentation change
is required.
---
Outside diff comments:
In `@proto/src/ota.rs`:
- Around line 260-275: Update the segment-walking loop around the `pos` bounds
checks to use checked arithmetic for both `pos + SEG_HEADER_LEN` and `pos +
data_len`. Treat any overflow as `ImageError::BadSegments`, while preserving the
existing limit validation and read behavior for valid lengths; avoid panic-prone
arithmetic on header-controlled data.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: fc92ea7b-8d8d-4806-a390-239f7c749e7d
📒 Files selected for processing (9)
app-core/src/buttons.rsapp-core/src/lib.rsdocs/FLASHING.mdfw/src/main.rsfw/src/ota_update.rsfw/src/tasks/display.rsfw/src/tasks/input.rsproto/src/ota.rstools/prepare-release.sh
Whole-image validation proved the anchor's segment data, not its segment headers. The XOR checksum covers data only, so a corrupted load_addr passes it untouched — and an image without a SHA-256 trailer has nothing else covering that field. Believe such an anchor, bounce into it, and the bootloader refuses the mapping and falls forward to slot 1, leaving otadata naming slot 0 while we execute slot 1: the self-erasure this branch already fixed once, reached by a different door. A test asserts the premise, that the staged validator returns Ok for exactly this image. Resident images are now judged separately from staged ones. A staged image is about to be written to the inactive slot with the anchor intact behind it and the bootloader getting the last word, so it keeps the structural check it had. A resident image is being read as evidence about what the bootloader will do, and a wrong answer there is not a failed update, it is erasing ourselves — so it must carry the SHA-256 trailer that covers every byte, be stamped for this chip, and lay its segments out in a way that can actually be loaded: word-aligned, and for the flash-mapped ones, congruent with their offset modulo the MMU page. Every image this project builds already appends the hash. The load address rules come from the images rather than from first principles. Segment 3 of both real builds loads at address zero — esptool's padding — so the obvious "load address must be in a mapped window" rule would have rejected our own firmware and refused every update. Padding segments are copied, not mapped, so the congruence rule does not reach them. Checked against the built X4 and X3 images: both validate, and corrupting segment 0's load_addr in the real image is caught even with the hash resealed over it. The test builder gained the same shape, its ragged 513- and 1-byte segments being something no build emits. This narrows the gap rather than closing it. validate_flash_image is not esp_image_format.c and an anchor could still satisfy every check here and be refused by the bootloader for a reason we do not model. Closing it properly means asking the MMU which partition is mapped for execution instead of inferring it, which needs hardware to land safely; plan_update_action says so. What is here confines the gap to images that are bit-intact and stamped for this chip, which is not where corruption lands.
Everything here rested on reading otadata and reasoning about what the bootloader must have done with it. The hardware can just answer. Translating a mapped code address back to a flash offset through the flash MMU is what ESP-IDF's spi_flash_cache2phys() does, and what FreeInk's RecoveryBoot gets for free from esp_ota_get_running_partition(); esp-hal exposes no equivalent, so the arithmetic lives in proto::ota and fw::mmu supplies the one volatile read. otadata is demoted to a fallback. When the MMU answers, the question the update decision rests on is settled outright, and a disagreement is logged and resolved in the hardware's favour. When it does not answer, the previous inference stands and the anchor's validity still exposes a stale otadata. mark_running_slot_valid stopped deserving its name somewhere along the way: it marks the entry otadata selects, which after a fall-forward is a slot we are not running, so it would confirm an image that had just failed to boot. It now declines when the MMU disagrees. Measured on an X3 rather than modelled. Running a known build from slot 1, the bootloader reported segment 0 at paddr 0x650020 for vaddr 0x3c000020 and segment 4 at paddr 0x930020 for vaddr 0x422e0020, and the table read back 0x65 at index 0 and 0x96 at index 49 -- resolving rodata 0x3c00aab9 to 0x65aab9 and code 0x423175e0 to 0x9675e0, both inside slot 1, matching "Loaded app from partition at offset 0x650000". Unmapped entries read exactly 0x100. The tests assert those numbers, not a model of them. The fall-forward this all defends against was confirmed the same way. Erasing one sector of slot 0 and pointing otadata at it produced, on the device: E esp_image: Checksum failed. Calculated 0x98 read 0xb3 E boot: OTA app partition slot 0 is not bootable I boot: Loaded app from partition at offset 0x650000 mmu: executing from slot 1 ota: otadata requests slot 0 but slot 1 is executing; not marking it valid otadata still read seq 3 afterwards: the bootloader does not correct it. Without the check above, that boot would have read "active = slot 0", judged slot 1 idle, and erased the partition it was executing from, with the anchor already corrupt. The slot and otadata were restored byte-identical afterwards.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
proto/src/ota.rs (1)
292-370: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnchecked
pos + data_lenaddition can overflow/bypass segment bounds checks.
data_lenis fully attacker/corruption-controlled (u32::from_le_bytes(...) as usize, up to0xFFFF_FFFF). On a 32-bit target — this crate is compiled for the ESP32-C3 firmware whereusizeis 32 bits —pos + data_len(line 337) andpos + SEG_HEADER_LEN(line 325) can overflow. In a release build (no overflow-checks) this silently wraps, letting a corrupted/malicious segment header makepos + data_lenappear smaller thanlimitand bypass the bounds check outright; in a debug build it panics on externally-influenced flash/SD content, which thefwguideline explicitly disallows ("Do not useunwrap,expect, or panic-prone indexing on externally influenced data").The segment table is malformed or a segment runs past end-of-file. is exactly the case this check exists to catch — an overflow defeats that purpose for the largest, most obviously-malformed inputs.
🛡️ Use checked addition for both bounds checks
for _ in 0..segment_count { - if pos + SEG_HEADER_LEN > limit { - return Err(ImageError::BadSegments); - } + let seg_header_end = pos + .checked_add(SEG_HEADER_LEN) + .filter(|&p| p <= limit) + .ok_or(ImageError::BadSegments)?; let mut seg_header = [0u8; SEG_HEADER_LEN]; src.read_exact(&mut seg_header) .map_err(|_| ImageError::Read)?; sha.update(seg_header); - pos += SEG_HEADER_LEN; + pos = seg_header_end; let data_len = u32::from_le_bytes([seg_header[4], seg_header[5], seg_header[6], seg_header[7]]) as usize; - if pos + data_len > limit { - return Err(ImageError::BadSegments); - } + let seg_data_end = pos + .checked_add(data_len) + .filter(|&p| p <= limit) + .ok_or(ImageError::BadSegments)?;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@proto/src/ota.rs` around lines 292 - 370, Update both bounds checks in walk_image—covering SEG_HEADER_LEN and data_len—to use checked addition rather than direct pos + length arithmetic. Treat an addition overflow exactly like a segment exceeding limit by returning ImageError::BadSegments, while preserving the existing position updates and validation for non-overflowing values.fw/src/main.rs (2)
250-276: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winReplace
esp_hal::delay::Delaywith an async wait before polling inrecovery_combo_confirmed.
delay.delay_millis(ComboConfirmer::POLL_MS)blocks the CPU while polling the ADC/boards I/O path. Use an async Embassy timer such asembassy_time::Timer::after_millis(ComboConfirmer::POLL_MS).awaitso normal tasks remain scheduled while waiting.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fw/src/main.rs` around lines 250 - 276, Make recovery_combo_confirmed asynchronous and replace the blocking esp_hal::delay::Delay call with an awaited embassy_time::Timer::after_millis(ComboConfirmer::POLL_MS) between polling attempts. Update its callers to await the returned future while preserving the existing confirmation and failure behavior.Source: Coding guidelines
41-99: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun the required feature-combination compile/lint checks for this feature-gated change.
PROJECT_NAMEis correctly gated asproto::ota::IDENTITY_X4orproto::ota::IDENTITY_X3, but both affected feature combinations still need the required Cargo compile/lint run before merge.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fw/src/main.rs` around lines 41 - 99, Run the required Cargo compile and lint checks for both feature combinations affected by the PROJECT_NAME cfg gates: the default/X4 configuration and the device-x3 configuration. Confirm both pass before merging, without changing the implementation.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fw/src/mmu.rs`:
- Around line 11-18: Update the module-level documentation in mmu.rs to
accurately describe constant ownership: do not claim every constant lives in
proto::ota while MMU_TABLE remains local. Either revise the wording to exclude
or distinguish MMU_TABLE, or move MMU_TABLE into proto::ota and include it in
the shared assertions.
In `@proto/src/ota.rs`:
- Around line 1196-1204: Reuse the existing resident_image(true) fixture in
accepts_a_flash_image_with_unmapped_padding_segments instead of rebuilding the
identical image with build_image. Preserve the test’s length assertion and
validation behavior unchanged.
- Around line 345-347: Update the Rust toolchain configuration to pin a stable
Rust version of 1.87 or newer so the `u32::is_multiple_of` call in the OTA
segment-layout validation remains supported; otherwise replace that call with an
MSRV-compatible divisibility check while preserving the existing
`ImageError::BadSegmentLayout` behavior.
---
Outside diff comments:
In `@fw/src/main.rs`:
- Around line 250-276: Make recovery_combo_confirmed asynchronous and replace
the blocking esp_hal::delay::Delay call with an awaited
embassy_time::Timer::after_millis(ComboConfirmer::POLL_MS) between polling
attempts. Update its callers to await the returned future while preserving the
existing confirmation and failure behavior.
- Around line 41-99: Run the required Cargo compile and lint checks for both
feature combinations affected by the PROJECT_NAME cfg gates: the default/X4
configuration and the device-x3 configuration. Confirm both pass before merging,
without changing the implementation.
In `@proto/src/ota.rs`:
- Around line 292-370: Update both bounds checks in walk_image—covering
SEG_HEADER_LEN and data_len—to use checked addition rather than direct pos +
length arithmetic. Treat an addition overflow exactly like a segment exceeding
limit by returning ImageError::BadSegments, while preserving the existing
position updates and validation for non-overflowing values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e0ce2f72-7cbf-460b-b964-579e4e9fd3d7
📒 Files selected for processing (4)
fw/src/main.rsfw/src/mmu.rsfw/src/ota_update.rsproto/src/ota.rs
Knowing which slot is executing removed the self-erasure, and left a loop in its place. Slot 1 approves the anchor and bounces; the bootloader rejects the anchor and hands slot 1 back with otadata still naming slot 0; the next boot sees the same anchor, the same trigger, and bounces again. The reset repeats forever. plan_update_action only ever saw the running slot, so it could not tell that first bounce from this one. It now takes the requested slot too, and reads "otadata asks for the anchor while the update slot is executing" as proof that a bounce already failed: the anchor satisfied our checks and failed the bootloader's, which validate_flash_image cannot rule out and never will. It refuses and lets the trigger go, the same as for any other unusable anchor, and the state heals itself once slot 0 is reflashed and the two slots agree again. mark_running_slot_valid failed open the other way: with the running slot unknown it went ahead and blessed whatever otadata named, which is least trustworthy in exactly that case. The rule moved to proto::ota::may_mark_running_slot_valid, which requires proof, so the None cases have somewhere to be tested -- fw has no host tests, and that is why the rule did not have one before. The rest are review fixes. Both bounds checks in walk_image use checked addition, since pos + data_len wraps on the device's 32-bit usize and a release build does not trap, putting the sum back under the limit it was meant to enforce. prepare-release.sh builds the X3 image and verifies its identity as well; the release ships both boards and only the X4 was being checked. The boot model writes the image before consuming the trigger, as the firmware does, and says so accurately. FLASHING.md counts four refusal cases, and fw::mmu no longer claims every constant lives in proto while the table's address does not.
The MMU lookup was allowed to fail into otadata: `None => requested`. That gave back the self-erasure it was added to remove. otadata is wrong precisely when the bootloader fell forward, which is precisely when a write lands on the slot we are executing; so if the lookup fails while otadata names the anchor and the anchor passes our validator -- which it can, since that validator is not esp_image_format.c -- the plan comes out WriteUpdateSlot and erases the running firmware. It also contradicted running_slot's own documented contract. plan_update_action now takes Option<u32> and refuses when it is None, the same policy may_mark_running_slot_valid already applies. Putting the uncertainty in the type rather than an early return in fw is what makes it testable: fw has no host tests, and the None cases now have somewhere to live. Restoring the fallback turns the new test red with WriteUpdateSlot, which is the failure this commit exists to prevent. The boot model also reported the slot otadata named rather than the one it was running, so after a rejected bounce it claimed to be running the anchor the bootloader had just refused -- contradicting its own running_slot and the comment two lines above. It reports the running slot now, and the failed-bounce test expects slot 1. Also covered by the mark-valid step the model gained here: after a failed bounce the anchor's entry stays OTA_IMG_NEW, so a slot that would not boot is never recorded as having run. Confirmed on an X3 by reading otadata back after the fall-forward -- state NEW, not VALID.
The anchor was held to an exact identity while the image on the card was only checked for structure. So the one path that actually writes flash accepted anything shaped like an ESP32-C3 binary: an X4 image renamed to FWUPDX3.BIN, a foreign image, or -- worst -- a pre-anchor Calendula build, which still alternates slots and would overwrite slot 0 on its very next update, destroying the anchor the rest of this branch exists to protect. The staged image is now judged by the same descriptor identity, under a deliberately different rule. The anchor must match exactly, because it has to run *this* firmware's hand-off. A staged image is the thing replacing us, so it must be for this board and from a generation that keeps the anchor (u1 or later), and nothing more: requiring equality would refuse every future release on precisely the locked devices that depend on the SD path. Refusal is the documented outcome either way -- a computer or the OEM updater. chip_id moves out of the resident-only checks: an image built for another chip cannot run here whichever slot it sits in, and learning that before the write beats learning it after. The SHA trailer stays resident-only, since a staged image still has the bootloader to answer to with the anchor intact behind it. Verified against the built X4 and X3 images and against the pre-anchor build read off a real device: each board accepts its own and refuses the other's, and the old product-only identity is refused -- that being the case that would have eaten the anchor. Both refusals also stopped reporting things the firmware had not established. RunningSlotUnknown now covers an unresolved MMU lookup, where otadata may well name slot 1 and nothing is known about slot 0's bootability; NoUsableAnchor covers both an unusable anchor and a bounce the bootloader refused. They log running, requested and anchor_usable instead of a story. The comment claiming the lookup falls back to otadata went with them; it had been contradicting the code two lines below it since that fallback was removed.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fw/src/ota_update.rs`:
- Around line 266-285: Clarify the comment above requested and running so the
fail-closed “None” behavior explicitly refers to running, not requested.
Preserve requested’s unwrap_or(ANCHOR_SLOT) fallback and state that it mirrors
the bootloader behavior, while retaining the existing explanation that
running=None causes ota::plan_update_action to refuse the update.
In `@proto/src/ota.rs`:
- Around line 2077-2088: Move the unresolved-MMU-lookup `///` documentation from
above `the_shipped_identities_parse` to directly above
`an_unprovable_running_slot_writes_nothing`. Leave the identity-parsing test and
section separator without that unrelated documentation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 02554527-32d6-4318-9235-eb06f9297309
📒 Files selected for processing (5)
docs/FLASHING.mdfw/src/mmu.rsfw/src/ota_update.rsproto/src/ota.rstools/prepare-release.sh
- Clarify in fw/src/ota_update.rs that fail-closed None behavior applies to the running slot MMU lookup, and document that requested's unwrap_or(ANCHOR_SLOT) fallback mirrors bootloader behavior. - Move the unresolved-MMU-lookup doc comment in proto/src/ota.rs to directly above an_unprovable_running_slot_writes_nothing.
…neration - Update staged_image_is_installable to require candidate_generation == our_generation instead of allowing any generation >= 1. - Prevents installing a u2 image on a u1 anchor device that would cause a one-way upgrade where u2 running in slot 1 refuses to bounce to the u1 anchor on future updates. - Add lifecycle test u1_anchor_rejects_u2_installation_to_prevent_one_way_upgrade_deadlock covering the 4-step sequence.
…documentation - Update staged_image_is_installable() in proto/src/ota.rs to enforce exact string equality (project_name(candidate) == running_identity), rejecting numeric generation aliases like u01 that break slot-0 anchor validation on subsequent boots. - Add regression test u01_generation_alias_is_rejected_and_canonical_u1_is_accepted. - Remove superseded MIN_UPDATER_GENERATION constant. - Update docs/FLASHING.md to document that in-app updates must retain the current updater generation and that cross-generation updates require replacing/re-establishing the slot-0 anchor via the computer/OEM path.
A build from before the anchor has the old updater: it writes whichever slot is inactive. So it cannot be relied on to install the first anchor build where that build needs to be, and the outcome turns on something the user cannot see. If the old build was running from slot 1 the write lands in slot 0, the anchor gets the new identity and all is well. If it was running from slot 0 the write lands in slot 1, slot 0 keeps the old build, and since identity must now match exactly, every later in-app update is refused as NoUsableAnchor -- with nothing on the device able to repair it, because the updater never writes slot 0. FLASHING.md invited exactly that by opening with "once *any* build is running, it can update itself from the card". It now says which builds that means, and carries a section on migrating an older install: flash the first anchor build to 0x10000 from a computer or the OEM updater, then confirm the boot log says slot 0. The way out of a half-migrated device is the same flash, so that is written down too, along with the note that the first anchor release should say this rather than reading as an ordinary update. Several comments also still described implementations this branch replaced. plan_update_action documented a parameter it no longer takes and called asking the hardware for the running slot "the right long-term answer" -- which it now is, and does. What the anchor's validity decides changed with it: it is no longer evidence about which slot we are on, only about whether a bounce could finish the job, and the bootloader refusing one is handled after the fact instead of predicted. RunningSlotUnknown named only the case it was introduced for, and NoUsableAnchor only the case it started with. And "the inactive slot" survived in five places as the update's destination, which has not been true since slot 0 became the anchor.
… one The migration instructions read one number and drew a conclusion from it that does not follow. "Executing slot 1" was documented as meaning otadata still selects slot 1, with Back+Up as the fix -- but this branch's own hardware experiment produced the other cause of that reading: otadata selecting slot 0, the bootloader rejecting it, and slot 1 executing anyway. Back+Up does nothing there. recover_to_slot0 reads the selection, finds slot 0 already named, and declines to switch, so the user repeats a working recovery combo against an anchor that never booted. Distinguishing the two needs the requested slot as well, so mark_running_slot_valid now prints both on every boot, agreeing or not, and the guide reads the pair instead of one half of it: requests 0 executing 0 is done, requests 1 executing 1 is the combo's case, requests 0 executing 1 is a slot 0 that did not take and has to be written again. The refusal line below it loses the numbers it was duplicating. The comments explaining why the anchor is validated on every path still gave the old reason -- that an unbootable anchor proves otadata is lying about where we run. That inference was replaced by asking the MMU, and leaving it written next to the code invites someone to reinstate it. The anchor answers whether a bounce could finish the job; before a bounce that is the whole question, and on the ordinary write path it is corroboration, since an anchor the MMU says is executing and the validator says cannot boot is a disagreement to refuse on rather than resolve. Same correction in FLASHING.md and in proto, where Strictness::Resident described the cost of a wrong answer as erasing ourselves; it is now a stranded update or a needlessly refused one, in either direction.
…ng installed The migration check asked the user to read a log line the running firmware cannot print. Flashing 0x10000 writes slot 0 and leaves otadata alone, so a device selecting slot 1 goes on booting the old build -- and that build's mark_running_slot_valid says nothing unless it rewrites an entry. The one case the check existed for, "requests 1, executing 1", is exactly the case where the firmware that would report it is sitting unused in slot 0. So the step that was conditional on reading it is now unconditional: hold Back+Up at the first reset after the flash, every time. Builds from before the anchor carry the same hatch, and theirs switches otadata to slot 0 whenever slot 0 holds an image; if otadata already named slot 0 the hold does nothing. Confirmation moves to the second-stage bootloader's own loaded-partition line, which prints before any app and so does not care which firmware is executing -- 0x10000 is the anchor, and the "not bootable" line above it separates a hold that missed from a flash that did not take. The firmware's own pair is still documented, as the more convenient check once the new build is the one running. Also the last comment claiming an unbootable anchor proves the bootloader fell forward, in recover_to_slot0's note on why a magic-only check is enough. The conclusion it drew is still right -- a corrupt anchor costs a reboot, not an erase -- but for a different reason now: the executing slot comes from the MMU, and otadata naming the anchor while the update slot runs is read as a bounce already refused, so nothing writes or retries on the strength of it.
Owner
Author
|
@CodeRabbit review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/FLASHING.md`:
- Around line 295-297: Update the fenced code blocks in FLASHING.md around the
boot and ota output examples to include the text language tag, preserving their
existing contents and formatting.
- Around line 247-257: Rewrite the sentence beginning “A firmware that
believed…” in the explanatory paragraph so it has a complete main clause,
preserving the existing sequence about inferring slot 0, treating slot 1 as
idle, and erasing the executing image. Match the grammatical structure used by
the `plan_update_action` documentation without changing the surrounding
technical explanation.
In `@proto/src/ota.rs`:
- Around line 2162-2201: Align the test with what it actually exercises: rename
u1_anchor_rejects_u2_installation_to_prevent_one_way_upgrade_deadlock and
tighten its doc comment to describe the direct staged_image_is_installable
rejection plus the same-generation lifecycle check, without claiming the Device
flow proves the u2 deadlock scenario. Leave the existing Device behavior
unchanged unless you instead add per-image identity handling through
Device::boot.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 17f94733-0b04-47b3-adec-a52b7396a935
📒 Files selected for processing (3)
docs/FLASHING.mdfw/src/ota_update.rsproto/src/ota.rs
…naming - Add text language tags to code blocks in docs/FLASHING.md and fix sentence structure in explanatory paragraph. - Rename OTA test to differing_generation_staged_image_is_rejected_and_same_generation_lifecycle_succeeds and tighten its doc comment.
- Extend differing_generation_staged_image_is_rejected_and_same_generation_lifecycle_succeeds test in proto/src/ota.rs to verify post-bounce anchor execution, slot 1 write, trigger consumption, and final boot.
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Verification
Before requesting review, please confirm you have run the appropriate checks:
tools/check.sh fmtpassedtools/check.sh fastpassed (host Clippy and tests)tools/check.sh emulatorpassed (emulator tests and goldens)tools/check.sh firmwarepassed (firmware Clippy and release builds)tools/check.sh allpassed (required before the pull request is considered ready)Skipped checks
If you skipped any checks, please list them below and explain why:
Description
The FreeInk SDK treats slot 0 as a recovery firmware that is "deliberately never reflashed". Our port copied the mechanism but not that rule: the updater wrote whichever slot was inactive, so an update applied from slot 1 landed in slot 0 — overwriting the image Back+Up returns to. And half of all boots ran from slot 0, where the hatch is a deliberate no-op.
Slot 0 is now an anchor. Updates always target slot 1; nothing in the updater writes slot 0. That can't be
dest = 1alone, because the slot we execute from can never be the write target — a second consecutive update would have to be refused. Instead a boot that finds a trigger while running from slot 1 pointsotadataat the anchor and resets without consuming the trigger, and the anchor boot does the write. One extra reboot; slot 0 untouched.Images are judged by what they are, not just by their shape. Each build stamps
CalendulaOS <board> u<gen> (MarigoldOS)into its app descriptor, and both the anchor and a staged image must match the running firmware's exactly. Structure alone isn't enough: a pre-anchor build is a perfectly valid image that still alternates slots, so installing one would overwrite slot 0 on its very next update — destroying the thing this PR exists to protect.Existing installs need a migration, not an in-app update. A build from before the anchor writes whichever slot is inactive, so where the first anchor build lands depends on which slot that build happened to be running from — slot 0 if it was on slot 1, which is fine; slot 1 if it was on slot 0, which leaves the old identity in the anchor and every later in-app update refused. So the first anchor release has to go to
0x10000from a computer or the OEM updater.docs/FLASHING.mddocuments it as a migration boundary, and its release notes should too.A slot-resident image is additionally checked the way the bootloader would check it: SHA-256 trailer required (the XOR checksum can't see a corrupted
load_addr), chip id, segment layout.otadatais a request, not a report. When the selected image fails verification, ESP-IDF boots another partition and leavesotadataalone — so a firmware reading it can be reading a slot it isn't executing. Believing it meant erasing the slot we were running from. The firmware now asks the flash MMU which partition is mapped, the wayesp_ota_get_running_partition()does.otadatadoes not get to stand in for that: if the lookup cannot resolve, the update is refused rather than guessed at.Verified on an X3, not modelled. Erasing one sector of slot 0 and pointing
otadataat it produced:(Verbatim from that run; the last line's wording has changed since, and both slots are now printed on every boot.)
otadatastill read seq 3 afterwards — the bootloader does not correct it. The MMU constants were confirmed the same way, against the bootloader's own reported load addresses. Slot andotadatarestored byte-identical.Everything downstream fails closed. A bounce the bootloader refuses would otherwise repeat forever, so
otadataasking for the anchor while the update slot executes is read as a failed bounce and refused. Marking an app valid requires proof of the running slot. And when the MMU cannot resolve that slot at all, the update is refused outright rather than falling back tootadata— which is wrong in precisely the case where a write would erase the running firmware.The hatch itself. The combo was sampled at a single instant; it now needs 4 consecutive in-band readings 4 ms apart, giving up after 28 ms. Its ladder thresholds moved to
app_core::buttons, shared with the input task, so a recalibration can't silently move the hatch off the documented buttons.Testing. The decisions live behind sans-IO seams in
proto::otaandapp_core::buttons;fwis the I/O that answers them. A simulated device carries realotadatasectors across reboots, models the bootloader falling forward, and asserts on every write that it isn't erasing the slot it runs from. 74 new host tests (app-core 125→142, proto 127→184);fwgains none, correctly.Verified:
tools/check.sh all, plus Clippy and release builds for the default (X4) anddevice-x3configurations and bothota-selftestcombinations under-D warnings. Still hardware-only, and listed as such indocs/FLASHING.md: a live Back+Up press, and the trigger file surviving the bounce reset on a real card.Summary by CodeRabbit