dfu: reboot when a started transfer goes silent - #48
Open
fkallay1 wants to merge 1 commit into
Open
Conversation
An interrupted transfer left the bootloader waiting forever for the rest of an image that
will never arrive, with the application already erased so there was nothing to fall back
to. Nothing supervised a live transfer: the only DFU timer was APP_TIMER_MODE_SINGLE_SHOT
and its handler returns as soon as tud_mounted(), and the watchdog is only fed if an
application had already started one. Recovery therefore depended entirely on the host
coming back, or on someone pressing reset.
The same tick now drives two independent timeouts:
- enumeration: expires after the timeout_ms passed to bootloader_dfu_start() and leaves
DFU, exactly as before;
- stall: DFU_STALL_MS of no DFU activity, once a transfer has actually started ->
NVIC_SystemReset(), so the bootloader comes back with a clean DFU state.
They are counted separately because the enumeration window wants to be generous while stall
detection wants to be short, and one shared interval cannot be both.
Two details make this safe, and both were found by getting them wrong first:
- The serial erase loop reports each page as activity. Erasing the region costs ~90 ms
per page - about 10 s for a 460 KB image and ~17 s for a 768 KB one - with no packets
arriving at all, so without this a healthy flash of a large image looks exactly like a
stalled one.
- The stall check measures elapsed time via app_timer_cnt_get() rather than counting
handler invocations. A long blocking erase stops app_timer from running and it then
catches up with a burst of immediate calls; counting invocations, only the first of that
burst sees the activity flag and the rest run the counter straight past the threshold.
That rebooted the device at the exact moment a 17 s erase finished, destroying a
perfectly healthy flash.
Verified on a ProMicro nRF52840, verdicts read from the device rather than from the DFU
client: a normal flash (27.7 s, ~10 s of it erasing) does not trip the stall; a deliberately
padded 786432-byte image whose erase runs ~17.3 s completes normally; an abandoned transfer
self-resets at 15.07 s and USB is back 0.22 s later; a retry inside the window still
succeeds without any reboot; and a full BLE OTA is unaffected, since the OTA path creates no
such timer.
Side effect worth having in a bootloader: the image gets 568 bytes smaller, from 32924 to
32356 on promicro_nrf52840, which sits at 84.8% of its 38 KB region. The old code passed the
runtime timeout_ms into APP_TIMER_TICKS(), so that macro's 64-bit multiply and divide had to
be evaluated at runtime and pulled libgcc's __udivmoddi4 (716 bytes) plus the ldiv0/idiv0
stubs into the image. Both APP_TIMER_TICKS() uses here now take a compile-time constant
(DFU_TICK_MS, DFU_STALL_MS) so the division folds, and the enumeration deadline divides a
uint32 by a constant, which the compiler turns into a multiply and shift - nothing references
the 64-bit divider any more. Confirmed by diffing the symbol tables: __udivmoddi4,
__aeabi_ldiv0 and __aeabi_idiv0 are gone, against 20 bytes of newly referenced
app_timer_cnt_get()/app_timer_cnt_diff_compute().
Re-verified on the final branch, alone and merged, on a ProMicro nRF52840 with verdicts read over
SWD: this branch alone self-resets 16.4 s after the abort, and a healthy 29 s flash - about 10 s of
it erasing - does not trip it. Merged with the recovery change: 16.1 s, USB back 0.2 s later. With
all four: 15.0 s, USB back 0.2 s later. Merged with the retry fix, the retry lands 3/3 and the
healthy flash still does not trip it.
On a XIAO nRF52840 Sense (S140 7.3.0, no debugger): 15.5 s after the abort. That board also showed
what the absence of this costs. With only the recovery change installed, an aborted transfer left
the bootloader waiting with no self-reset after 30 s - and because nothing could open a new session
either, it accepted nothing at all until someone physically pressed reset. This timeout clears that
state on its own in 15 s.
Worth knowing for host tooling: with this installed, a client has roughly 15 s to retry before the
device resets itself. After the reset the retry succeeds against the recovered bootloader, so
nothing is lost - but a harness that pauses in between will see the reset rather than its own retry.
Note for the nRF52832 forced-startup path: DFU_SERIAL_STARTUP_INTERVAL is unchanged and
still yields a one-tick deadline, so that behaviour is equivalent.
Owner
|
Thanks for your PRs, they won't make it into the next release but I will consider them for the next release. |
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
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.
An interrupted transfer left the bootloader waiting forever for the rest of an image that
will never arrive, with the application already erased so there was nothing to fall back
to. Nothing supervised a live transfer: the only DFU timer was APP_TIMER_MODE_SINGLE_SHOT
and its handler returns as soon as tud_mounted(), and the watchdog is only fed if an
application had already started one. Recovery therefore depended entirely on the host
coming back, or on someone pressing reset.
The same tick now drives two independent timeouts:
DFU, exactly as before;
NVIC_SystemReset(), so the bootloader comes back with a clean DFU state.
They are counted separately because the enumeration window wants to be generous while stall
detection wants to be short, and one shared interval cannot be both.
Two details make this safe, and both were found by getting them wrong first:
per page - about 10 s for a 460 KB image and ~17 s for a 768 KB one - with no packets
arriving at all, so without this a healthy flash of a large image looks exactly like a
stalled one.
handler invocations. A long blocking erase stops app_timer from running and it then
catches up with a burst of immediate calls; counting invocations, only the first of that
burst sees the activity flag and the rest run the counter straight past the threshold.
That rebooted the device at the exact moment a 17 s erase finished, destroying a
perfectly healthy flash.
Verified on a ProMicro nRF52840, verdicts read from the device rather than from the DFU
client: a normal flash (27.7 s, ~10 s of it erasing) does not trip the stall; a deliberately
padded 786432-byte image whose erase runs ~17.3 s completes normally; an abandoned transfer
self-resets at 15.07 s and USB is back 0.22 s later; a retry inside the window still
succeeds without any reboot; and a full BLE OTA is unaffected, since the OTA path creates no
such timer.
Side effect worth having in a bootloader: the image gets 568 bytes smaller, from 32924 to
32356 on promicro_nrf52840, which sits at 84.8% of its 38 KB region. The old code passed the
runtime timeout_ms into APP_TIMER_TICKS(), so that macro's 64-bit multiply and divide had to
be evaluated at runtime and pulled libgcc's __udivmoddi4 (716 bytes) plus the ldiv0/idiv0
stubs into the image. Both APP_TIMER_TICKS() uses here now take a compile-time constant
(DFU_TICK_MS, DFU_STALL_MS) so the division folds, and the enumeration deadline divides a
uint32 by a constant, which the compiler turns into a multiply and shift - nothing references
the 64-bit divider any more. Confirmed by diffing the symbol tables: __udivmoddi4,
__aeabi_ldiv0 and __aeabi_idiv0 are gone, against 20 bytes of newly referenced
app_timer_cnt_get()/app_timer_cnt_diff_compute().
Re-verified on the final branch, alone and merged, on a ProMicro nRF52840 with verdicts read over
SWD: this branch alone self-resets 16.4 s after the abort, and a healthy 29 s flash - about 10 s of
it erasing - does not trip it. Merged with the recovery change: 16.1 s, USB back 0.2 s later. With
all four: 15.0 s, USB back 0.2 s later. Merged with the retry fix, the retry lands 3/3 and the
healthy flash still does not trip it.
On a XIAO nRF52840 Sense (S140 7.3.0, no debugger): 15.5 s after the abort. That board also showed
what the absence of this costs. With only the recovery change installed, an aborted transfer left
the bootloader waiting with no self-reset after 30 s - and because nothing could open a new session
either, it accepted nothing at all until someone physically pressed reset. This timeout clears that
state on its own in 15 s.
Worth knowing for host tooling: with this installed, a client has roughly 15 s to retry before the
device resets itself. After the reset the retry succeeds against the recovered bootloader, so
nothing is lost - but a harness that pauses in between will see the reset rather than its own retry.
Note for the nRF52832 forced-startup path: DFU_SERIAL_STARTUP_INTERVAL is unchanged and
still yields a one-tick deadline, so that behaviour is equivalent.
Related to #41, which asks for a maximum time in DFU because a failed update left a node stuck in
DFU somewhere hard to reach. This covers the case that report describes - an update that started and
then went silent - by rebooting 15 s after the last activity rather than waiting indefinitely.
Two limits worth stating plainly rather than letting the issue look solved. It supervises a transfer
that has actually begun, so it is not a general time limit on an idle DFU session. And if the update
had already erased the application there is no firmware left to fall back to, so the device comes
back in DFU rather than in the old application - #47 is what makes sure it comes back on a transport
that is reachable.
One of four independent DFU fixes, listed in the order they matter:
Any subset can be taken, in any order. Verified: all four merge cleanly in five different orders and
in all twelve ordered pairs, and each was flashed and tested on hardware on its own as well as in
combination - 26 of 26 checks on a ProMicro nRF52840 with verdicts read over SWD, plus a XIAO
nRF52840 Sense on a different SoftDevice with no debugger.