feat(ota): fix update failure reporting, add progress telemetry and delta patching - #503
feat(ota): fix update failure reporting, add progress telemetry and delta patching#503yudataguy wants to merge 2 commits into
Conversation
…elta patching
An OTA update currently cannot be completed in a single ground pass. A signed
image is 726,784 bytes and the ground station paces file uplink at ~510 B/s
(204-byte chunks, 0.4 s cooldown), so a whole image needs ~24 minutes of
contact. File uplink has no cross-pass resume, so a dropped pass discards
everything transferred.
Correctness first. FlashWorker::writeImage only ever assigned its return status
in the CRC-mismatch branch, so a file read error or a flash write error logged a
warning and returned OP_OK, and the Updater announced UpdateSucceeded for an
image that was never fully written. An operator following the documented
procedure would then set next-boot and reboot into a truncated image. The write
path is restructured so every exit returns a classified outcome and there is no
mutable status a failure path can forget to set. Also fixed: the CRC-mismatch
path fell through into the write loop instead of returning, and any failure
forced another full 1 MB erase even when the flash was never touched.
Failures that never reached flash (bad file name, failed size or CRC read, CRC
mismatch) now leave the staging slot usable, so a mistyped file name costs a
retry rather than an erase. Failures that did reach flash still require another
preparation.
Operator visibility. FlashWorker gains UpdateStage, BytesWritten,
ImageTotalBytes, and LastUpdateStatus, packetized as SoftwareUpdate (23, group
5). These are channels rather than only events so an operator returning on a
later pass can see where an update stands without replaying event history.
CHUNK_DELAY_US and PROGRESS_STEP_PERCENT become parameters so the write pacing
can be tuned against hardware instead of rebuilt.
Two ways to fit an update into a pass:
ASSEMBLE_IMAGE joins numbered segments ("<prefix>.000", ...) uplinked across
several passes into one image and verifies it, bounding the loss from a dropped
pass to a single segment.
APPLY_PATCH reconstructs an image from a bsdiff-style delta against the running
image, read straight out of slot0 (the RP2350 executes XIP from memory-mapped
QSPI, so no filesystem copy is needed). Measured against real consecutive CI
builds, a compressed delta is 47-97 KB, or 1.5-3.2 minutes of uplink. Naive
block diffing was measured and rejected: a code size change shifts every later
address, so ~99% of 512-byte blocks differ between builds days apart.
The patch container records the size and CRC32 of the reference it was built
from and the apply refuses to run against anything else, because patching the
wrong reference yields a plausible but corrupt image that would then be flashed
and booted. Control records are range checked against both images so a
malformed patch arriving over the radio cannot read or write out of bounds.
Compression is deliberately not implemented on the flight side. The streams are
~84% zeros and compress to ~60 KB with DEFLATE, ~47 KB with LZMA, or ~97 KB with
heatshrink, but this Zephyr workspace ships no decompressor and choosing that
dependency is a project decision. The container carries a codec field, and
make-patch.py refuses to emit an uncompressed patch without an explicit flag
since it would be the size of the image.
Partition IDs now come from the device tree via FIXED_PARTITION_ID rather than a
hand-counted literal, which depended on DTS declaration order. Partition labels
corrected: slot1 was labelled "golden" while being the slot OTA overwrites.
Verified: make fmt clean, make test-unit 11/11 (three new host test suites),
firmware builds. The ground tool and flight applier were checked end to end by
reconstructing a real 726,784-byte image from a real 2-day-older reference,
byte-identical.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011kehXMi9zAAoPJ3PgfRd81
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (17)
📝 WalkthroughSummary by CodeRabbit
WalkthroughFlashWorker now supports sequenced image updates, progress telemetry, segmented image assembly, and delta-patch application. The change adds patch-generation tooling, device-tree partition naming, integration and unit tests, deployment telemetry, and complete component documentation. ChangesFlashWorker OTA update pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GroundStation
participant FlashWorker
participant UpdateSequencer
participant FlashRegion
GroundStation->>FlashWorker: Prepare update
FlashWorker->>UpdateSequencer: Record preparation result
UpdateSequencer-->>FlashWorker: PREPARED stage
GroundStation->>FlashWorker: Send image update
FlashWorker->>FlashRegion: Validate, erase, and write image
FlashWorker->>UpdateSequencer: Record write outcome
UpdateSequencer-->>FlashWorker: Update status and stage
FlashWorker-->>GroundStation: Report progress and completion
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@PROVESFlightControllerReference/Components/FlashWorker/docs/sdd.md`:
- Around line 52-58: Update the documented tools/bin/make-patch.py command in
both PROVESFlightControllerReference/Components/FlashWorker/docs/sdd.md lines
52-58 and docs-site/components/FlashWorker.md lines 52-58 to include the
required --allow-uncompressed option, preserving the remaining patch and update
workflow.
- Around line 28-159: The mirrored FlashWorker documents need markdownlint
formatting fixes: add appropriate language identifiers to every fenced command
block to resolve MD040, and insert blank lines between headings and immediately
following tables to resolve MD022 and MD058. Apply the same changes in
PROVESFlightControllerReference/Components/FlashWorker/docs/sdd.md lines 28-159
and docs-site/components/FlashWorker.md lines 28-159, keeping both documents
synchronized.
In `@PROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cpp`:
- Around line 491-497: Update the CRC-mismatch branch in the FlashWorker handler
to report the dedicated CRC failure value from PatchApplier::Error instead of
SIZE_MISMATCH, adding that enum value and its .fpp definition if absent. Also
remove the destination file on this path and the failure paths near lines 476
and 485, matching the cleanup behavior used by ASSEMBLE_IMAGE_cmdHandler.
- Around line 288-348: Both ASSEMBLE_IMAGE_cmdHandler and APPLY_PATCH_cmdHandler
leave partially created destination files after failures. Add a shared private
cleanup helper that closes and removes destination, logs the failure, and sends
EXECUTION_ERROR; invoke it on all nine specified segment/apply,
verification-open, and CRC-mismatch failure paths in
PROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cpp (anchor
lines 288-348 and sibling lines 491-497).
- Around line 160-171: Update the read loop in FlashWorker’s file-flashing logic
to increment its progress counter by the bytes actually returned in read_size,
rather than the fixed CHUNK size. Preserve the existing zero-byte termination
and file-read failure handling so the loop remains aligned with the file cursor
and processes all reported file bytes.
- Around line 454-464: Validate the patch file size before deriving or using the
stream offsets in the FlashWorker reconstruction flow. Obtain the file size
once, perform overflow-safe checks for the header size plus control, diff, and
extra stream sizes, and require their total to equal the patch size; reject
invalid or truncated headers before seeking or reading. Keep the existing
control_start, diff_start, extra_start, and reconstruction path for valid
patches.
- Around line 189-193: Update the post-write validation around written and
written_crc: check written against size before comparing CRCs, return
UpdateSequencer::WriteOutcome::FILE_READ_FAILED for a short read, and only
return FLASH_WRITE_FAILED for a CRC mismatch. Revise the nearby comment and log
event wording, including ImageWriteCrcMismatch in the FPP definition, to state
that streamed image bytes were validated without claiming flash readback or
slot-content verification.
- Around line 428-431: Before the read loop in the reference-data handling flow,
validate that header.reference_size does not exceed the flash area’s size.
Reject oversized values before any flash_area_read call and report the condition
with a clear size-related error, while preserving the existing reference-read
failure handling for actual read errors.
- Around line 172-176: Update the flash write logic around
flash_img_buffered_write in FlashWorker so the flush argument is true only when
the current chunk is the final file chunk, and false for all intermediate
chunks. Use the existing read-size, file-size, or end-of-file tracking in the
surrounding update flow to determine completion, while preserving the current
failure logging and FLASH_WRITE_FAILED return behavior.
In
`@PROVESFlightControllerReference/test/unit-tests/test_FlashWorker_SegmentPlan.cpp`:
- Around line 1-4: Update the title comment in test_FlashWorker_SegmentPlan.cpp
to use the actual file name, test_FlashWorker_SegmentPlan.cpp, instead of the
stale test_ImageBuilder_SegmentPlan.cpp reference.
In
`@PROVESFlightControllerReference/test/unit-tests/test_FlashWorker_UpdateSequencer.cpp`:
- Around line 121-133: Rename the test NoFailureOutcomeReportsSuccess to a name
describing that failure outcomes map to non-OK statuses while SUCCESS maps to
OP_OK. Keep the test body and assertions unchanged.
In `@tools/bin/make-patch.py`:
- Around line 197-202: Add bsdiff4 to the project’s UV-managed dependencies and
regenerate uv.lock, then add a Makefile target that runs tools/bin/make-patch.py
and update the ImportError message in make-patch.py to direct users to that
target instead of pip install. Ensure the target is exposed through the
supported tool environment.
🪄 Autofix
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: 6683b810-4413-4eb5-9c63-215ec1031e8b
📒 Files selected for processing (22)
PROVESFlightControllerReference/Components/FlashWorker/CMakeLists.txtPROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cppPROVESFlightControllerReference/Components/FlashWorker/FlashWorker.fppPROVESFlightControllerReference/Components/FlashWorker/FlashWorker.hppPROVESFlightControllerReference/Components/FlashWorker/PatchApplier.cppPROVESFlightControllerReference/Components/FlashWorker/PatchApplier.hppPROVESFlightControllerReference/Components/FlashWorker/SegmentPlan.cppPROVESFlightControllerReference/Components/FlashWorker/SegmentPlan.hppPROVESFlightControllerReference/Components/FlashWorker/UpdateSequencer.cppPROVESFlightControllerReference/Components/FlashWorker/UpdateSequencer.hppPROVESFlightControllerReference/Components/FlashWorker/UpdateStatus/UpdateStatus.fppPROVESFlightControllerReference/Components/FlashWorker/docs/sdd.mdPROVESFlightControllerReference/ReferenceDeployment/Top/ReferenceDeploymentPackets.fppiPROVESFlightControllerReference/test/int/ota_test.pyPROVESFlightControllerReference/test/unit-tests/CMakeLists.txtPROVESFlightControllerReference/test/unit-tests/test_FlashWorker_PatchApplier.cppPROVESFlightControllerReference/test/unit-tests/test_FlashWorker_SegmentPlan.cppPROVESFlightControllerReference/test/unit-tests/test_FlashWorker_UpdateSequencer.cppboards/bronco_space/proves_flight_control_board_v5/proves_flight_control_board_v5.dtsidocs-site/components/FlashWorker.mdlib/fprime-extrastools/bin/make-patch.py
| ``` | ||
| uplink /update/zephyr.signed.bin | ||
| Update.updater.PREPARE_UPDATE | ||
| Update.updater.UPDATE_IMAGE_FROM("/update/zephyr.signed.bin", <crc>) | ||
| Update.updater.CONFIGURE_NEXT_BOOT(TEST) | ||
| reboot | ||
| Update.updater.CONFIRM_UPDATE | ||
| ``` | ||
|
|
||
| Get `<crc>` from `tools/bin/calculate-crc.py`. | ||
|
|
||
| ### Image split across several passes | ||
|
|
||
| ``` | ||
| uplink /update/img.000, /update/img.001, ... (one or more per pass) | ||
| Update.worker.ASSEMBLE_IMAGE("/update/img", <n>, "/update/candidate.bin", <crc>) | ||
| Update.updater.PREPARE_UPDATE | ||
| Update.updater.UPDATE_IMAGE_FROM("/update/candidate.bin", <crc>) | ||
| ``` | ||
|
|
||
| ### Typical Usage | ||
| And the typical usage of the component here | ||
| Assembly verifies the joined image against `<crc>` before anything is written to flash, so a missing or reordered segment is caught rather than left for the bootloader to find. | ||
|
|
||
| ### Delta patch | ||
|
|
||
| ``` | ||
| (ground) tools/bin/make-patch.py <reference.bin> <target.bin> -o update.patch | ||
| uplink /update/update.patch | ||
| Update.worker.APPLY_PATCH("/update/update.patch", "/update/candidate.bin", <crc>) | ||
| Update.updater.PREPARE_UPDATE | ||
| Update.updater.UPDATE_IMAGE_FROM("/update/candidate.bin", <crc>) | ||
| ``` | ||
|
|
||
| The reference is the running image, read directly out of the `slot0_partition` flash area. The RP2350 executes XIP from memory-mapped QSPI, so no copy has to be kept on the filesystem. The patch container records the size and CRC32 of the reference it was built from, and `APPLY_PATCH` refuses to run if the running image is not that one. Patching the wrong reference produces a plausible but corrupt image that would then be flashed and booted, so this check is not optional. | ||
|
|
||
| **Compression is not yet available on the flight side.** The patch streams are ~84% zero bytes and compress from ~727 KB to ~60 KB with DEFLATE, ~47 KB with LZMA, or ~97 KB with heatshrink, but this Zephyr workspace ships no decompressor and selecting that dependency is a project decision. Until it is made, `make-patch.py` refuses to emit a patch without `--allow-uncompressed`, because an uncompressed patch is the size of the image and worth nothing over the radio. The container carries a codec field so a compressed format can be added without changing the applier's structure. | ||
|
|
||
| ## Flash Layout | ||
|
|
||
| Defined in `boards/bronco_space/proves_flight_control_board_v5/proves_flight_control_board_v5.dtsi`. | ||
|
|
||
| | partition | label | size | role | | ||
| |---|---|---|---| | ||
| | `boot_partition` | mcuboot | 1 MB | bootloader | | ||
| | `slot0_partition` | primary | 1 MB | running image, and the patch reference | | ||
| | `slot1_partition` | secondary | 1 MB | staging slot updates are written into | | ||
| | `slot2_partition` | reserved | 1 MB | unused in swap-using-offset mode with one image | | ||
| | `storage_partition` | n/a | 12 MB | LittleFS, holds uplinked segments and candidates | | ||
|
|
||
| ## Class Diagram | ||
| Add a class diagram here | ||
| Partition IDs are taken from the device tree with `FIXED_PARTITION_ID`, never hard coded: fixed partition IDs follow declaration order, so a literal would silently point at the wrong region if a partition were added above it. | ||
|
|
||
| ## Port Descriptions | ||
| | Name | Description | | ||
| |---|---| | ||
| |---|---| | ||
| | prepareImage | Erase the staging slot, from `Update.Updater` | | ||
| | updateImage | Write an image file into the staging slot | | ||
| | nextBoot | Set the next boot mode through MCUBoot | | ||
| | confirmImage | Confirm the running image so it is not reverted | | ||
| | prepareImageDone / updateImageDone | Report completion of the slow operations | | ||
|
|
||
| ## Component States | ||
| Add component states in the chart below | ||
|
|
||
| The sequence is tracked by `Components::UpdateSequencer`. | ||
|
|
||
| | Name | Description | | ||
| |---|---| | ||
| |---|---| | ||
| | IDLE | No usable staging slot; PREPARE_UPDATE must run before an update | | ||
| | PREPARED | Staging slot erased and ready to receive an image | | ||
| | UPDATED | An image has been written to the staging slot | | ||
|
|
||
| ## Sequence Diagrams | ||
| Add sequence diagrams here | ||
| A failure that never reached the flash (a bad file name, a failed size or CRC read, a CRC mismatch) leaves the sequence in PREPARED, so the operator can retry without paying for another 1 MB erase. A failure that did reach the flash drops to IDLE, because the slot now holds partial data and must be erased again. | ||
|
|
||
| ## Parameters | ||
| | Name | Description | | ||
| |---|---| | ||
| |---|---| | ||
| | CHUNK_DELAY_US | Microseconds to pause after each buffered flash write, default 5000. Exposed so it can be tuned against real hardware instead of rebuilt; at the default a 727 KB image spends about 7 s asleep. | | ||
| | PROGRESS_STEP_PERCENT | Percent of the image between progress events, default 10. Larger values spend less downlink reporting on an update in flight. | | ||
|
|
||
| ## Commands | ||
| | Name | Description | | ||
| |---|---| | ||
| |---|---| | ||
| | ASSEMBLE_IMAGE | Concatenate numbered uplink segments into one image file and verify its CRC32 | | ||
| | APPLY_PATCH | Reconstruct an image from a delta patch applied to the running image | | ||
|
|
||
| ## Events | ||
| | Name | Description | | ||
| |---|---| | ||
| |---|---| | ||
| | UpdateProgress | Periodic progress during a write | | ||
| | NoImagePrepared | An update was requested before a successful preparation | | ||
| | NextBootSetFailed / ConfirmImageFailed | MCUBoot next-boot or confirm call failed | | ||
| | FlashEraseFailed / FlashWriteFailed | Staging slot erase or write failed | | ||
| | ImageFileReadError / ImageFileCrcMismatch | Image file could not be read, or failed validation | | ||
| | ImageWriteCrcMismatch | Bytes written to flash did not match the bytes validated | | ||
| | AssembleStarted / AssembleSucceeded / AssembleFailed / InvalidSegmentCount | Segment assembly | | ||
| | PatchStarted / PatchSucceeded / PatchFailed / PatchReferenceMismatch | Patch application | | ||
|
|
||
| ## Telemetry | ||
| | Name | Description | | ||
| |---|---| | ||
| |---|---| | ||
| | UpdateStage | IDLE, PREPARING, PREPARED, WRITING, UPDATED, or FAILED | | ||
| | BytesWritten | Bytes of the image written into the staging slot so far | | ||
| | ImageTotalBytes | Total size of the image being written | | ||
| | LastUpdateStatus | Status of the most recent preparation or update | | ||
|
|
||
| These are channels rather than only events so that an operator returning on a later pass can ask where an update stands without replaying event history. They are packetized in `SoftwareUpdate` (packet 23, group 5). | ||
|
|
||
| ## Unit Tests | ||
| Add unit test descriptions in the chart below | ||
|
|
||
| Host tests, no F Prime or Zephyr dependency. Run with `make test-unit`. | ||
|
|
||
| | Name | Description | Output | Coverage | | ||
| |---|---|---|---| | ||
| |---|---|---|---| | ||
| | test_FlashWorker_UpdateSequencer | Sequence ordering, status mapping, retry cost, progress arithmetic | pass/fail | `UpdateSequencer` | | ||
| | test_FlashWorker_SegmentPlan | Segment naming, zero padding, buffer and index bounds | pass/fail | `SegmentPlan` | | ||
| | test_FlashWorker_PatchApplier | Container decoding, patch application, malformed and hostile patches | pass/fail | `PatchApplier` | | ||
|
|
||
| Integration tests covering the command surface against hardware are in `test/int/ota_test.py`. They deliberately never set the next boot, so a run cannot leave the board staged to boot an unintended image. | ||
|
|
||
| ## Requirements | ||
| Add requirements in the chart below | ||
|
|
||
| | Name | Description | Validation | | ||
| |---|---|---| | ||
| |---|---|---| | ||
| | A failed update is never reported as a success | Read and write failures propagate a failure status to the Updater | Unit test | | ||
| | A retry costs an erase only when one is needed | Failures that never reached flash leave the slot usable | Unit test, integration test | | ||
| | An image is validated before it is flashed | CRC32 is checked before the write, and the written bytes are verified after | Unit test, inspection | | ||
| | A patch is applied only to the image it was built from | The container records the reference size and CRC and both are checked | Unit test | | ||
| | A malformed patch cannot read or write out of bounds | Control records are range checked against both images | Unit test | | ||
|
|
||
| ## Change Log | ||
| | Date | Description | | ||
| |---|---| | ||
| |---| Initial Draft | | ||
| | n/a | Initial Draft | | ||
| | 2026-08-04 | Correct failure reporting, add progress telemetry and parameters, add segment assembly and delta patching | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make both FlashWorker documents pass markdownlint.
The command fences have no language. Several headings directly precede tables. Add fence languages and required blank lines in both mirrored documents.
PROVESFlightControllerReference/Components/FlashWorker/docs/sdd.md#L28-L159: fix MD040, MD022, and MD058 findings.docs-site/components/FlashWorker.md#L28-L159: apply the same markdownlint fixes.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 28-28: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 41-41: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 52-52: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
[warning] 78-78: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 79-79: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 100-100: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 105-105: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 106-106: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 111-111: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 112-112: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 123-123: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 124-124: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 155-155: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 156-156: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
📍 Affects 2 files
PROVESFlightControllerReference/Components/FlashWorker/docs/sdd.md#L28-L159(this comment)docs-site/components/FlashWorker.md#L28-L159
🤖 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 `@PROVESFlightControllerReference/Components/FlashWorker/docs/sdd.md` around
lines 28 - 159, The mirrored FlashWorker documents need markdownlint formatting
fixes: add appropriate language identifiers to every fenced command block to
resolve MD040, and insert blank lines between headings and immediately following
tables to resolve MD022 and MD058. Apply the same changes in
PROVESFlightControllerReference/Components/FlashWorker/docs/sdd.md lines 28-159
and docs-site/components/FlashWorker.md lines 28-159, keeping both documents
synchronized.
Source: Linters/SAST tools
| for (FwSizeType i = 0; i < size; i += CHUNK) { | ||
| FwSizeType read_size = CHUNK; | ||
| file_status = file.read(this->m_data, read_size); | ||
| if (file_status != Os::File::Status::OP_OK) { | ||
| this->log_WARNING_LO_ImageFileReadError(file_name, | ||
| Os::FileStatus(static_cast<Os::FileStatus::T>(file_status))); | ||
| return UpdateSequencer::WriteOutcome::FILE_READ_FAILED; | ||
| } | ||
| // The file ended earlier than its reported size; stop rather than flushing empty writes | ||
| if (read_size == 0) { | ||
| break; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Advance the loop counter by the bytes actually read.
read_size is an in/out parameter. A short read returns fewer bytes than CHUNK. The counter still advances by CHUNK, so the counter and the file cursor diverge. The loop can then exit while bytes remain in the file.
The truncation is not silent, because the CRC check at Line 190 fails. But the outcome is reported as FLASH_WRITE_FAILED. UpdateSequencer::dirtiesStagingSlot treats that outcome as dirty, so the operator pays for a full 1 MB erase before a retry, and the reported cause names the flash instead of the file read.
Advance by the consumed count so the loop matches the cursor.
🐛 Proposed fix
- for (FwSizeType i = 0; i < size; i += CHUNK) {
+ for (FwSizeType i = 0; i < size;) {
FwSizeType read_size = CHUNK;
file_status = file.read(this->m_data, read_size);
if (file_status != Os::File::Status::OP_OK) {
this->log_WARNING_LO_ImageFileReadError(file_name,
Os::FileStatus(static_cast<Os::FileStatus::T>(file_status)));
return UpdateSequencer::WriteOutcome::FILE_READ_FAILED;
}
// The file ended earlier than its reported size; stop rather than flushing empty writes
if (read_size == 0) {
break;
}
+ i += read_size;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (FwSizeType i = 0; i < size; i += CHUNK) { | |
| FwSizeType read_size = CHUNK; | |
| file_status = file.read(this->m_data, read_size); | |
| if (file_status != Os::File::Status::OP_OK) { | |
| this->log_WARNING_LO_ImageFileReadError(file_name, | |
| Os::FileStatus(static_cast<Os::FileStatus::T>(file_status))); | |
| return UpdateSequencer::WriteOutcome::FILE_READ_FAILED; | |
| } | |
| // The file ended earlier than its reported size; stop rather than flushing empty writes | |
| if (read_size == 0) { | |
| break; | |
| } | |
| for (FwSizeType i = 0; i < size;) { | |
| FwSizeType read_size = CHUNK; | |
| file_status = file.read(this->m_data, read_size); | |
| if (file_status != Os::File::Status::OP_OK) { | |
| this->log_WARNING_LO_ImageFileReadError(file_name, | |
| Os::FileStatus(static_cast<Os::FileStatus::T>(file_status))); | |
| return UpdateSequencer::WriteOutcome::FILE_READ_FAILED; | |
| } | |
| // The file ended earlier than its reported size; stop rather than flushing empty writes | |
| if (read_size == 0) { | |
| break; | |
| } | |
| i += read_size; |
🤖 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 `@PROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cpp`
around lines 160 - 171, Update the read loop in FlashWorker’s file-flashing
logic to increment its progress counter by the bytes actually returned in
read_size, rather than the fixed CHUNK size. Preserve the existing zero-byte
termination and file-read failure handling so the loop remains aligned with the
file cursor and processes all reported file bytes.
| status = flash_img_buffered_write(&this->m_flash_context, this->m_data, read_size, true); | ||
| if (status != 0) { | ||
| break; | ||
| this->log_WARNING_LO_FlashWriteFailed(static_cast<I32>(-1 * status), i); | ||
| return UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Zephyr flash_img_buffered_write flush parameter last chunk semantics
💡 Result:
In the Zephyr RTOS, the flash_img_buffered_write function is used to write image data to a flash slot in blocks [1][2]. The flush parameter serves as a signal to finalize the write operation for the current data stream [1][3]. When flush is set to true, the function forces any remaining buffered data—data that has not yet filled a full flash block—to be written to flash memory [1][2]. Because flash memory must be written in blocks, this final operation includes padding the remaining space (from the last byte written up to the next multiple of CONFIG_IMG_BLOCK_BUF_SIZE) with 0xff [1][4]. In practical usage, you should set flush to true only on the final call to the function for a given image upload, as it completes the buffering process and ensures the remaining partial block is committed to the device [1][2]. Setting this parameter to false allows the API to continue holding data in an internal buffer, waiting until subsequent calls provide enough bytes to form a complete, alignable flash block [1][3].
Citations:
- 1: https://docs.zephyrproject.org/latest/doxygen/html/group__flash__img__api.html
- 2: https://docs.zephyrproject.org/3.1.0/services/storage/flash_img/flash_img.html
- 3: https://docs.zephyrproject.org/3.2.0/services/storage/flash_img/flash_img.html
- 4: https://docs.zephyrproject.org/4.2.0/doxygen/html/group__flash__img__api.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files matching FlashWorker.cpp:\n'
fd -a 'FlashWorker\.cpp$' . || true
printf '\nRelevant source section:\n'
FILE="$(fd 'FlashWorker\.cpp$' . | head -n 1)"
if [ -n "${FILE:-}" ]; then
wc -l "$FILE"
sed -n '130,190p' "$FILE" | cat -n
fi
printf '\nSearch for flash_img_buffered_write declarations/calls in repo:\n'
rg -n "flash_img_buffered_write|BUFFER_BLOCK|CONFIG_IMG|chunk|flush" . -g '*cpp' -g '*c' -g '*hpp' -g '*fpp' -g 'CMakeLists.txt' -g 'kconfig*' | head -n 120Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 9766
Pass flush as true only on the final chunk.
flash_img_buffered_write uses flush to complete the buffered image stream. Passing true on every chunk flushes after each small write instead of buffering until the image ends. Track whether the current chunk completes the file and pass flush accordingly.
🐛 Proposed fix
- status = flash_img_buffered_write(&this->m_flash_context, this->m_data, read_size, true);
+ const bool last_chunk = ((written + read_size) >= size);
+ status = flash_img_buffered_write(&this->m_flash_context, this->m_data, read_size, last_chunk);
if (status != 0) {
this->log_WARNING_LO_FlashWriteFailed(static_cast<I32>(-1 * status), i);
return UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| status = flash_img_buffered_write(&this->m_flash_context, this->m_data, read_size, true); | |
| if (status != 0) { | |
| break; | |
| this->log_WARNING_LO_FlashWriteFailed(static_cast<I32>(-1 * status), i); | |
| return UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED; | |
| } | |
| const bool last_chunk = ((written + read_size) >= size); | |
| status = flash_img_buffered_write(&this->m_flash_context, this->m_data, read_size, last_chunk); | |
| if (status != 0) { | |
| this->log_WARNING_LO_FlashWriteFailed(static_cast<I32>(-1 * status), i); | |
| return UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED; | |
| } |
🤖 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 `@PROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cpp`
around lines 172 - 176, Update the flash write logic around
flash_img_buffered_write in FlashWorker so the flush argument is true only when
the current chunk is the final file chunk, and false for all intermediate
chunks. Use the existing read-size, file-size, or end-of-file tracking in the
surrounding update flow to determine completion, while preserving the current
failure logging and FLASH_WRITE_FAILED return behavior.
| // What landed in the slot must match what was validated before the write started | ||
| if (written_crc != expected_crc32) { | ||
| this->log_WARNING_HI_ImageWriteCrcMismatch(expected_crc32, written_crc); | ||
| return UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
State what the CRC check covers, and classify a short image separately.
written_crc accumulates the bytes read from the file. It does not read back from flash. The comment and the ImageWriteCrcMismatch event text say "Image written to flash failed verification", which claims a readback that does not happen. Correct the wording so an operator does not conclude the slot contents were verified.
A short file also lands here. written < size after the loop means the read ended early, not that the flash write failed. Reporting FLASH_WRITE_FAILED marks the staging slot dirty and forces another erase. Test written against size first, and return FILE_READ_FAILED for that case.
🐛 Proposed fix
- // What landed in the slot must match what was validated before the write started
+ // The file ended before its reported size, so the image is incomplete. This is a read
+ // shortfall, not a flash fault, and classifying it correctly keeps the slot retryable.
+ if (written != size) {
+ this->log_WARNING_LO_ImageFileReadError(file_name,
+ Os::FileStatus(Os::FileStatus::T::OTHER_ERROR));
+ return UpdateSequencer::WriteOutcome::FILE_READ_FAILED;
+ }
+
+ // The bytes handed to the flash must match what was validated before the write started.
+ // This covers the file changing underneath the write; it is not a readback of the slot.
if (written_crc != expected_crc32) {format "Image bytes streamed to flash failed verification: expected 0x{x} and actual 0x{x}"
🤖 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 `@PROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cpp`
around lines 189 - 193, Update the post-write validation around written and
written_crc: check written against size before comparing CRCs, return
UpdateSequencer::WriteOutcome::FILE_READ_FAILED for a short read, and only
return FLASH_WRITE_FAILED for a CRC mismatch. Revise the nearby comment and log
event wording, including ImageWriteCrcMismatch in the FPP definition, to state
that streamed image bytes were validated without claiming flash readback or
slot-content verification.
| const FwSizeType control_start = static_cast<FwSizeType>(PatchApplier::HEADER_SIZE); | ||
| const FwSizeType diff_start = control_start + static_cast<FwSizeType>(header.control_size); | ||
| const FwSizeType extra_start = diff_start + static_cast<FwSizeType>(header.diff_size); | ||
| const bool opened = (control_file.open(patch.toChar(), Os::File::Mode::OPEN_READ) == Os::File::Status::OP_OK) && | ||
| (diff_file.open(patch.toChar(), Os::File::Mode::OPEN_READ) == Os::File::Status::OP_OK) && | ||
| (extra_file.open(patch.toChar(), Os::File::Mode::OPEN_READ) == Os::File::Status::OP_OK) && | ||
| (out_file.open(destination.toChar(), Os::File::Mode::OPEN_CREATE, | ||
| Os::File::OverwriteType::OVERWRITE) == Os::File::Status::OP_OK) && | ||
| (control_file.seek(control_start, Os::File::SeekType::ABSOLUTE) == Os::File::Status::OP_OK) && | ||
| (diff_file.seek(diff_start, Os::File::SeekType::ABSOLUTE) == Os::File::Status::OP_OK) && | ||
| (extra_file.seek(extra_start, Os::File::SeekType::ABSOLUTE) == Os::File::Status::OP_OK); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate the header stream sizes against the patch file size.
control_start, diff_start, and extra_start are derived from header.control_size and header.diff_size with no bound. Two problems follow from the missing check.
The sums are FwSizeType. If FwSizeType is 32-bit on this target, a header that declares a control_size near UINT32_MAX wraps the sum, and the extra cursor seeks back into the header region. The reconstruction then reads the wrong bytes.
A truncated patch also passes here. Each cursor seeks to an offset past the end of the file, and the first read fails inside PatchApplier::apply rather than at a clear validation point.
The output CRC check at Line 491 does catch both cases, so a wrong image is not reported as success. The gap is that neither case is diagnosed at its cause, and the flight side spends a full reconstruction pass before rejecting a patch it could have refused from the header.
Size the patch file once and require the declared streams to account for it exactly.
🛡️ Proposed validation
+ // The declared streams must account for the patch file exactly. This refuses a truncated
+ // container and removes any chance of the cursor offsets below wrapping.
+ FwSizeType patch_size = 0;
+ {
+ Os::File sizer;
+ if ((sizer.open(patch.toChar(), Os::File::Mode::OPEN_READ) != Os::File::Status::OP_OK) ||
+ (sizer.size(patch_size) != Os::File::Status::OP_OK)) {
+ flash_area_close(area);
+ this->log_WARNING_HI_PatchFailed(static_cast<U8>(PatchApplier::Error::PATCH_READ_FAILED));
+ this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);
+ return;
+ }
+ }
+ const U64 declared = static_cast<U64>(PatchApplier::HEADER_SIZE) + header.control_size + header.diff_size +
+ header.extra_size;
+ if (declared != static_cast<U64>(patch_size)) {
+ flash_area_close(area);
+ this->log_WARNING_HI_PatchFailed(static_cast<U8>(PatchApplier::Error::TRUNCATED_PATCH));
+ this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR);
+ return;
+ }
+
const FwSizeType control_start = static_cast<FwSizeType>(PatchApplier::HEADER_SIZE);🤖 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 `@PROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cpp`
around lines 454 - 464, Validate the patch file size before deriving or using
the stream offsets in the FlashWorker reconstruction flow. Obtain the file size
once, perform overflow-safe checks for the header size plus control, diff, and
extra stream sizes, and require their total to equal the patch size; reject
invalid or truncated headers before seeking or reading. Keep the existing
control_start, diff_start, extra_start, and reconstruction path for valid
patches.
| if (actual_crc != crc32) { | ||
| this->log_WARNING_LO_ImageFileCrcMismatch(destination, Os::FileStatus(Os::FileStatus::T::OP_OK), crc32, | ||
| actual_crc); | ||
| this->log_WARNING_HI_PatchFailed(static_cast<U8>(PatchApplier::Error::SIZE_MISMATCH)); | ||
| this->cmdResponse_out(opCode, cmdSeq, Fw::CmdResponse::EXECUTION_ERROR); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report a CRC mismatch with a CRC error code.
The reconstructed image failed the CRC check. The handler reports PatchApplier::Error::SIZE_MISMATCH. The size may be correct and only the content wrong, so the code misdirects the operator. PatchFailed carries this code as the only machine-readable cause, and the .fpp documents it as Components::PatchApplier::Error.
Use a code that names the CRC failure. If PatchApplier::Error has no such value, add one.
The destination file also stays on the filesystem on this path and on the failure paths at Lines 476 and 485. Remove it, for the same storage reason as in ASSEMBLE_IMAGE_cmdHandler.
🤖 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 `@PROVESFlightControllerReference/Components/FlashWorker/FlashWorker.cpp`
around lines 491 - 497, Update the CRC-mismatch branch in the FlashWorker
handler to report the dedicated CRC failure value from PatchApplier::Error
instead of SIZE_MISMATCH, adding that enum value and its .fpp definition if
absent. Also remove the destination file on this path and the failure paths near
lines 476 and 485, matching the cleanup behavior used by
ASSEMBLE_IMAGE_cmdHandler.
| // ====================================================================== | ||
| // \title test_ImageBuilder_SegmentPlan.cpp | ||
| // \brief Unit tests for uplink segment naming and validation | ||
| // ====================================================================== |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the file name in the title comment.
The header says test_ImageBuilder_SegmentPlan.cpp. The file is test_FlashWorker_SegmentPlan.cpp, and SegmentPlan lives under Components/FlashWorker. The stale name refers to a component that does not exist in this change.
📝 Proposed fix
-// \title test_ImageBuilder_SegmentPlan.cpp
+// \title test_FlashWorker_SegmentPlan.cpp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ====================================================================== | |
| // \title test_ImageBuilder_SegmentPlan.cpp | |
| // \brief Unit tests for uplink segment naming and validation | |
| // ====================================================================== | |
| // ====================================================================== | |
| // \title test_FlashWorker_SegmentPlan.cpp | |
| // \brief Unit tests for uplink segment naming and validation | |
| // ====================================================================== |
🤖 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
`@PROVESFlightControllerReference/test/unit-tests/test_FlashWorker_SegmentPlan.cpp`
around lines 1 - 4, Update the title comment in test_FlashWorker_SegmentPlan.cpp
to use the actual file name, test_FlashWorker_SegmentPlan.cpp, instead of the
stale test_ImageBuilder_SegmentPlan.cpp reference.
| TEST(UpdateSequencerTest, NoFailureOutcomeReportsSuccess) { | ||
| // Every outcome other than SUCCESS must map to a non-OK status | ||
| const UpdateSequencer::WriteOutcome failures[] = { | ||
| UpdateSequencer::WriteOutcome::FILE_OPEN_FAILED, UpdateSequencer::WriteOutcome::FILE_QUERY_FAILED, | ||
| UpdateSequencer::WriteOutcome::CRC_MISMATCH, UpdateSequencer::WriteOutcome::FILE_READ_FAILED, | ||
| UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED, | ||
| }; | ||
| for (const UpdateSequencer::WriteOutcome outcome : failures) { | ||
| EXPECT_NE(UpdateSequencer::Status::OP_OK, UpdateSequencer::statusForOutcome(outcome)); | ||
| } | ||
| EXPECT_EQ(UpdateSequencer::Status::OP_OK, | ||
| UpdateSequencer::statusForOutcome(UpdateSequencer::WriteOutcome::SUCCESS)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Rename the test to describe what it asserts.
The name NoFailureOutcomeReportsSuccess states the inverse of the assertions. The body asserts that every failure outcome maps to a non-OK status and that only SUCCESS maps to OP_OK. A failing run in CI would print a name that contradicts the failure.
♻️ Proposed rename
-TEST(UpdateSequencerTest, NoFailureOutcomeReportsSuccess) {
+TEST(UpdateSequencerTest, OnlySuccessOutcomeMapsToOpOk) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TEST(UpdateSequencerTest, NoFailureOutcomeReportsSuccess) { | |
| // Every outcome other than SUCCESS must map to a non-OK status | |
| const UpdateSequencer::WriteOutcome failures[] = { | |
| UpdateSequencer::WriteOutcome::FILE_OPEN_FAILED, UpdateSequencer::WriteOutcome::FILE_QUERY_FAILED, | |
| UpdateSequencer::WriteOutcome::CRC_MISMATCH, UpdateSequencer::WriteOutcome::FILE_READ_FAILED, | |
| UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED, | |
| }; | |
| for (const UpdateSequencer::WriteOutcome outcome : failures) { | |
| EXPECT_NE(UpdateSequencer::Status::OP_OK, UpdateSequencer::statusForOutcome(outcome)); | |
| } | |
| EXPECT_EQ(UpdateSequencer::Status::OP_OK, | |
| UpdateSequencer::statusForOutcome(UpdateSequencer::WriteOutcome::SUCCESS)); | |
| } | |
| TEST(UpdateSequencerTest, OnlySuccessOutcomeMapsToOpOk) { | |
| // Every outcome other than SUCCESS must map to a non-OK status | |
| const UpdateSequencer::WriteOutcome failures[] = { | |
| UpdateSequencer::WriteOutcome::FILE_OPEN_FAILED, UpdateSequencer::WriteOutcome::FILE_QUERY_FAILED, | |
| UpdateSequencer::WriteOutcome::CRC_MISMATCH, UpdateSequencer::WriteOutcome::FILE_READ_FAILED, | |
| UpdateSequencer::WriteOutcome::FLASH_WRITE_FAILED, | |
| }; | |
| for (const UpdateSequencer::WriteOutcome outcome : failures) { | |
| EXPECT_NE(UpdateSequencer::Status::OP_OK, UpdateSequencer::statusForOutcome(outcome)); | |
| } | |
| EXPECT_EQ(UpdateSequencer::Status::OP_OK, | |
| UpdateSequencer::statusForOutcome(UpdateSequencer::WriteOutcome::SUCCESS)); | |
| } |
🧰 Tools
🪛 Clang (14.0.6)
[warning] 121-121: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 121-121: all parameters should be named in a function
(readability-named-parameter)
🤖 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
`@PROVESFlightControllerReference/test/unit-tests/test_FlashWorker_UpdateSequencer.cpp`
around lines 121 - 133, Rename the test NoFailureOutcomeReportsSuccess to a name
describing that failure outcomes map to non-OK statuses while SUCCESS maps to
OP_OK. Keep the test body and assertions unchanged.
| import bsdiff4 | ||
| except ImportError: | ||
| print( | ||
| "bsdiff4 is required: pip install bsdiff4", | ||
| file=sys.stderr, | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI -t f '^(pyproject\.toml|uv\.lock|requirements.*|Makefile)$' . -x sh -c '
echo "=== $1 ==="
rg -n -C 2 "bsdiff4|make-patch|uv|pip install" "$1" || true
' sh {}Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 3738
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== files ==="
fd -HI -t f 'make-patch\.py|pyproject\.toml|uv\.lock|requirements.*|Makefile' . | sed -n '1,120p'
echo "=== tools/bin/make-patch.py relevant area ==="
sed -n '160,220p' tools/bin/make-patch.py
echo "=== pyproject/requirements mentions of bsdiff4 ==="
rg -n "bsdiff4" . || trueRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 2876
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pyproject files ==="
fd -HI -t f 'pyproject\.toml$' . -x sh -c '
echo "=== $1 ==="
sed -n "1,220p" "$1" | rg -n -C 3 "dependencies|optional-dependencies|project|tool|u?v|bsdiff4|make-patch" || true
' sh {}
echo "=== Makefile make-patch mentions ==="
rg -n -C 3 "make-patch|patch" Makefile || trueRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 3281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== files ==="
fd -HI -t f 'make-patch\.py|pyproject\.toml|uv\.lock|requirements.*|Makefile' . | sed -n '1,120p'
echo "=== tools/bin/make-patch.py relevant area ==="
sed -n '160,220p' tools/bin/make-patch.py
echo "=== global bsdiff4 references ==="
rg -n "bsdiff4" . || true
echo "=== Makefile patch references ==="
rg -n -C 3 "make-patch|patch" Makefile || trueRepository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 4501
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== pyproject files ==="
fd -HI -t f 'pyproject\.toml$' . -x sh -c '
echo "=== $1 ==="
sed -n "1,220p" "$1" | rg -n -C 3 "dependencies|optional-dependencies|project|tool|u?v|bsdiff4|make-patch" || true
' sh {}Repository: Open-Source-Space-Foundation/proves-core-reference
Length of output: 1636
Register and expose bsdiff4 through the supported tool environment.
tools/bin/make-patch.py requires bsdiff4, but no project dependency, requirement, or uv.lock declares it. Add the package to UV-managed dependencies and add a Makefile target for tools/bin/make-patch.py; replace the pip install bsdiff4 instruction with that target.
🤖 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 `@tools/bin/make-patch.py` around lines 197 - 202, Add bsdiff4 to the project’s
UV-managed dependencies and regenerate uv.lock, then add a Makefile target that
runs tools/bin/make-patch.py and update the ImportError message in make-patch.py
to direct users to that target instead of pip install. Ensure the target is
exposed through the supported tool environment.
Source: Coding guidelines
Completes the two decisions left open when the delta patch path landed. Compression. A raw patch is about the size of the image it rebuilds, so it was worth nothing over the radio. Rather than add a third party decompressor to flight software, this adds a small LZ77 codec whose exact decoder can be round-tripped against real patches in host tests. Measured on a real patch between consecutive CI builds: 728,388 bytes becomes 101,208, turning ~24 minutes of uplink into ~3.3. The window is 4 KB. Measured alternatives: 16 KB saves 6% and 64 KB saves 8%, neither of which pays for the RAM, and 4 KB already matches what heatshrink achieved (~97 KB) in earlier measurements. A naive zero-RLE was also measured and rejected at 241 KB, because the zeros come in short scattered runs. The three streams are compressed together and decoded once into a scratch file, so PatchApplier keeps operating on plain streams and its existing tests still cover the apply unchanged. Auto-confirm. An image booted in TEST mode reverts unless it is confirmed before the next reboot, so a missed pass throws away a working image along with the uplink that delivered it. AUTO_CONFIRM_ENABLED lets the ground hand that decision to the spacecraft; it defaults to disabled, so confirmation stays operator-in-the-loop until explicitly armed. Because the decision is made after the reboot into the test image, arming it requires PRM_SAVE as well as PRM_SET, which is why it is documented that way. Confirmation is refused unless it was armed, the running image is actually pending confirmation, and it has run continuously for AUTO_CONFIRM_DELAY_SECONDS (default 1800). A delay of zero still requires one elapsed second, so an image that crashes immediately cannot confirm itself. RunningImageConfirmed and PendingConfirmSeconds are downlinked so an operator can see the countdown. The check runs on the 1 Hz rate group, which had a free member port; the configured ActiveRateGroupOutputPorts of 25 already covers it. Verified: make fmt clean, make test-unit 12/12, firmware builds. The codec was checked end to end by decoding a real 101,208 byte compressed patch and rebuilding a real 726,784 byte image from it, byte-identical to the target. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011kehXMi9zAAoPJ3PgfRd81
|
Both open decisions are now resolved and implemented in a29752b. 1. Compression: a self-contained LZ77 codec, not a vendored libraryThe choice was heatshrink vs. LZMA vs. DEFLATE, all of which meant adding a third-party decompressor to flight software. Measuring first changed the answer: a ~120-line LZ77 coder whose exact decoder can be round-tripped against real patches in host tests matches heatshrink's result without the dependency. Measured on a real patch between consecutive CI builds:
4 KB window. A 16× larger window buys 0.2 minutes — the compressible content is bsdiff's difference stream, which is short zero runs repeating at close range, so distance is cheap and window size barely matters. Zero-RLE was measured and rejected: the zeros come in short scattered runs (~10 bytes), so a real LZ-class coder is required. The three streams are compressed together and decoded once into a scratch file, so Verified end to end: decoding a real 101,208-byte compressed patch and rebuilding a real 726,784-byte image from it, byte-identical to the target. 2. Auto-confirm: operator-armed, defaulting to offTo answer the question directly — yes, the operator arms it from the ground, and it takes two commands: The Confirmation is refused unless all three hold: it was armed from the ground, the running image is actually pending confirmation ( Default is The check runs on the 1 Hz rate group. Verification
Still not hardware-tested: the full prepare → write → TEST boot → confirm cycle, |
1 participant
Implements #429. Design proposal and measurements were posted on the issue and agreed before this was written.
The problem, measured
A signed image is 726,784 bytes. The ground station paces file uplink at 204-byte chunks with a 0.4 s cooldown (
fprime-gds.yml), about 510 B/s, so a whole image needs ~24 minutes of contact. A pass is single-digit minutes, and F Prime's file uplink has no cross-pass resume, so a dropped pass discards everything sent.Correctness (the part worth reviewing closely)
FlashWorker::writeImageonly ever assigned its return status in the CRC-mismatch branch. A file read error or flash write error logged a warning and returnedOP_OK, soUpdateremittedUpdateSucceededfor an image that was never fully written — and an operator following the documented procedure would then set next-boot and reboot into a truncated image.The write path is restructured so every exit returns a classified
WriteOutcome; there is no longer a mutable status a failure path can forget to set. Also fixed: the CRC-mismatch path fell through into the write loop instead of returning, and any failure forced another full 1 MB erase even when flash was never touched.Failures that never reached flash now leave the slot usable, so a mistyped filename costs a retry instead of an erase. Failures that did reach flash still require another prepare.
Operator visibility
UpdateStage,BytesWritten,ImageTotalBytes,LastUpdateStatus, packetized asSoftwareUpdate(23, group 5). Channels rather than only events, so an operator returning a pass later can see where an update stands without replaying event history.CHUNK_DELAY_USandPROGRESS_STEP_PERCENTare parameters so write pacing can be tuned against hardware rather than rebuilt.Fitting an update into a pass
ASSEMBLE_IMAGEjoins numbered segments (<prefix>.000, ...) uplinked across several passes and verifies the result, bounding the loss from a dropped pass to one segment.APPLY_PATCHreconstructs an image from a bsdiff-style delta against the running image, read straight out ofslot0— the RP2350 executes XIP from memory-mapped QSPI, so no filesystem copy is needed (this is simpler than the issue's "keep an image on the filesystem" suggestion, and removes the stale-copy failure mode).Measured against real consecutive CI builds:
Naive block diffing was measured and rejected: a code-size change shifts every later address, so ~99% of 512-byte blocks differ between builds days apart — a block-index scheme would uplink more than the image.
Safety of the patch path
The container records the size and CRC32 of the reference it was built from, and the apply refuses to run against anything else. Patching the wrong reference yields a plausible but corrupt image that would then be flashed and booted. Control records are range-checked against both images, so a malformed patch arriving over the radio cannot read or write out of bounds.
One decision left to the team
Compression is deliberately not implemented on the flight side. The patch streams are ~84% zero bytes and compress from 727 KB to ~60 KB (DEFLATE), ~47 KB (LZMA), or ~97 KB (heatshrink) — but this Zephyr workspace ships no decompressor, and adding one to flight software is a dependency decision I did not want to make unilaterally. I also measured a naive zero-RLE (no new dependency): only 241 KB / 7.9 min, because the zeros come in short scattered runs. A real LZ-class coder is required.
The container carries a codec field so a compressed format drops in without changing the applier's structure, and
make-patch.pyrefuses to emit an uncompressed patch without--allow-uncompressed, since it would be the size of the image. Until a codec is chosen,APPLY_PATCHis functional but not yet operationally useful —ASSEMBLE_IMAGEis the shipping answer to the pass-boundary problem.Also
Partition IDs come from the device tree via
FIXED_PARTITION_IDinstead of a hand-counted literal that depended on DTS declaration order. Partition labels corrected —slot1was labelled"golden"while being the slot OTA overwrites.Verification
make fmt— cleanmake test-unit— 11/11 pass, three new host suites (UpdateSequencer,SegmentPlan,PatchApplier) covering the defects above plus malformed and hostile patcheszephyr.uf2producedmake-patch.pyplus the flight-side applier reconstructed a real 726,784-byte image from a real 2-day-older reference, byte-identicaltest/int/ota_test.pyadded for the command surface. It deliberately never sets next-boot, so a run cannot leave a board staged to boot an unintended image. Not yet run against hardware.Not hardware-tested: the full prepare → write → TEST boot → confirm cycle, and
APPLY_PATCHon a real board.Note for reviewers:
lib/fprime-extrasin my working tree was ahead of the recorded pointer and declaresSvc.ComRetry, which collides withlib/fprime's. I pinned it to the recorded commit to build, then restored it; this branch does not touch the submodule.🤖 Generated with Claude Code
https://claude.ai/code/session_011kehXMi9zAAoPJ3PgfRd81