Stop failed DFU sessions wedging the radio or failing silently - #2467
Open
fluffyspace wants to merge 2 commits into
Open
Stop failed DFU sessions wedging the radio or failing silently#2467fluffyspace wants to merge 2 commits into
fluffyspace wants to merge 2 commits into
Conversation
WritePacketHandler spins until SystemTask disables sleeping, with no upper bound. This runs on the BLE host task, so if the wake lock never arrives the host task never returns: GATT operations stop being served, disconnect events are never processed, and the watch cannot advertise again until it is rebooted. A failed update then looks like a watch whose Bluetooth has died. Cap the wait at 1s and continue without the lock. Racing the sleep timer is a far better failure mode than a radio that needs a reboot to recover.
DfuService silently drops any control point command it cannot handle in its current state, and silently drops data packets the same way. A host that reconnects to a watch left parked mid-transfer by an interrupted update therefore gets no reply at all: from its side, Start DFU simply never answers, which is indistinguishable from a watch that is not listening. Every subsequent attempt looks like an unexplained hang until the watch is rebooted or the 10s inactivity timer happens to reset the state machine. The protocol already defines ErrorCodes::InvalidState, and it was never used. Reply with [Response, opcode, InvalidState, state] instead of returning silently; the fourth byte names the state the watch is actually in, which makes the failure diagnosable from the host without a debugger. Stray data packets report once per session rather than per packet, so the notification path cannot be flooded by a host that keeps sending.
|
Build size and comparison to main:
|
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.
Two independent DFU robustness fixes, one per commit, so either can be taken alone. Both came out of debugging repeated OTA failures on real hardware; the underlying data-corruption bug is #2466, and these are about the surrounding failure modes being invisible or unrecoverable.
1. Bound the wait for the sleep wake lock
WritePacketHandlerspins untilSystemTaskdisables sleeping, with no upper bound:This runs on the BLE host task. If the wake lock never arrives, the host task never returns: GATT operations stop being served, disconnect events are never processed, and the watch cannot advertise again. The status bar still shows Bluetooth as connected, but the watch is invisible to every scanner until it is rebooted — a failed update looks like the radio has died.
Capped at 1 s, then continue without the lock. Racing the sleep timer is a much better failure mode than a radio that needs a reboot to recover.
2. Answer commands that arrive in the wrong state
Every wrong-state branch in
ControlPointHandlerreturns silently, and so do stray data packets. A host that reconnects to a watch left parked mid-transfer by an interrupted update gets no reply at all — Start DFU simply never answers, which is indistinguishable from a watch that is not listening. Every retry looks like an unexplained hang until the watch is rebooted or the 10 s inactivity timer happens to reset the state machine.ErrorCodes::InvalidStateis already defined in the protocol and was never used. These branches now reply[Response, opcode, InvalidState, state], the fourth byte naming the state the watch is actually in, which makes the failure diagnosable from the host without a debugger.Stray data packets report once per session rather than per packet, so a host that keeps sending cannot flood the notification path. The flag is cleared in
Reset().Testing
Tested on a PineTime (bootloader 1.0.1, InfiniTime 1.16.0), alongside #2466: the firmware builds and runs with these changes, and OTA updates complete normally with them in place.
To be precise about what I did not verify: I did not stage a mid-transfer reconnect to observe an
InvalidStateresponse arrive at the host. The silent-return behaviour it replaces is plain in the code, and my host client decodes the response, but the new reply path itself is unexercised — worth a reviewer's eye.The unbounded wake-lock wait, by contrast, was hit repeatedly in practice: a watch showing Bluetooth as connected while being invisible to every scanner, recoverable only by reboot.
Happy to split this into two PRs if you would prefer them reviewed separately.