Repository navigation
fix(dfu): retry a timed-out flash operation instead of faulting - #86
Conversation
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 34 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Retry state and initialization lib/sdk11/components/drivers_nrf/pstorage/pstorage_raw.c |
Adds retry counters, limits, and a single-shot timer. pstorage_init initializes the counters and creates the timer. |
Retry execution and completion lib/sdk11/components/drivers_nrf/pstorage/pstorage_raw.c, changelog.md |
Adds timed retries and scheduled re-kicks for busy results. Terminal errors are reported to the command owner, and successful flash completion resets retry counters. The changelog documents the retry behavior. |
Priority:
Estimated code review effort: 3 (Moderate) | ~20 minutes
Change: Bug fix
Sequence Diagram(s)
sequenceDiagram
participant pstorage
participant FlashRetryTimer
participant ApplicationScheduler
participant FlashOperation
participant app_notify
pstorage->>FlashRetryTimer: Start 100 ms retry
FlashRetryTimer->>ApplicationScheduler: Schedule retry kick
ApplicationScheduler->>pstorage: Run retry kick
pstorage->>FlashOperation: Re-issue queued command
FlashOperation-->>pstorage: Return command result
pstorage->>ApplicationScheduler: Schedule busy re-kick, up to five times
pstorage->>app_notify: Report terminal error
Suggested reviewers: oltaco, hathach
Merge Risk: 
a059c
-
No actionable merge-blocking issue is established for the flash-timeout retry change. It is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly and concisely describes the main change: retrying timed-out flash operations to prevent DFU faulting. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 1 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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.
A rabbit taps the timer twice,
Then waits beside the flash,
A busy call gets one more turn,
Retries bound the dash,
Success resets the counters clean,
And errors find their path.
Comment @coderabbitai help to get the list of available commands.
a059c49 to
f6dba81
Compare
Checklist
tools/build_all.py(or CI's board matrix) passesDescription of Change
A BLE OTA could validate and then leave the board in the bootloader instead of the new app: the SoftDevice reported
NRF_EVT_FLASH_OPERATION_ERRORwhen a flash operation would not fit the radio gaps (the activation settings save while the central is still connected), pstorage passed it up asNRF_ERROR_TIMEOUT, and the DFU callbacks fault on it.INFO_UF2.TXTshowedLast-Fault: id=0x4001 pc=0x0 info=0xd. It happens on OTAFIX 2.5 too. pstorage now re-issues a timed-out operation every 100 ms, up to 20 times, before reporting failure; a BUSY re-issue goes through the scheduler on its own bound, and any other rejection reports its own code.Tested with gcc 12.3 images and an injected flash error on the first completion of the activation settings save: master faults 8 of 8 (Wio Tracker L1 and RAK4631); this PR completes 50 of 50 on the L1 (S140 7.3.0) and 10 of 10 on the RAK4631 (S140 6.1.1). Without injection, the natural failure on the L1 was 3 of 20 full OTAs on master; an earlier version of this change (immediate retries) ran 30 full 700 KB OTAs on the L1 and 10 on the RAK4631 clean, with UF2 restores between them, and the final version passed 12 plain BLE OTAs. Serial DFU was not run on this branch.
Summary by CodeRabbit