fix(firmware): take the device back out of WiFi update mode when a flash fails (part of #269) - #445
Conversation
…ash fails (part of #269) `UpdateWifiModuleAsync` puts the device into LAN firmware-update mode, which bridges USB straight through to the WINC and bypasses the SCPI console. Only the *successful* path walked it back out, so a failed or canceled flash left the module unreachable until someone power-cycled the device. That is the gap `daqifi-desktop` compensates for with its own recovery `finally` (`ResetLanAfterUpdate` + a raw transparent-mode exit) around Core's call — one of the workarounds #269 exists to move into Core. Both failure exits now run a best-effort bridge exit before reporting the failure. It is the managed-connection twin of the already-shipped `WifiBridgeActivator.Deactivate`: `SYSTem:USB:SetTransparentMode 0`, the same 100 ms pause, then `LAN:APPLY` to kick the WiFi manager out of its bridge-mode state machine. The pause is now a single shared constant so the two paths that walk a device out of bridge mode cannot pace it differently. Deliberately not the success path's full `LAN:ENAbled`/`APPLY`/`SAVE` restore: persisting a network configuration off the back of a flash that did not complete is not this step's job. It never throws, so the caller still sees the original failure, and it runs on a token of its own — on the cancellation path the caller's token is canceled by definition, and that is exactly when the device most needs the exit. Bounded by the post-flash reconnect budget, which is the same physical operation and already tunable. No new options and no public API change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoRestore device LAN mode after failed/canceled WiFi module update
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…lf done Two review findings on the failure-path bridge exit. Arming `mayBeInLanUpdateMode` before the whole prepare step meant an `EnsureDeviceConnected` failure — a device that was never connected, so never bridged — still ran the recovery, whose reconnect loop then waited out the full ReconnectingAfterFlash budget (45s by default) for a transport that was never gone. An immediate "device must be connected" failure became a 45-second one. It is now armed inside the prepare step, immediately after the `LAN:FWUpdate` send. That is as early as it can honestly be: `Send` is synchronous and no await separates it from the assignment, so nothing can interleave between them, and everything above the send fails with the command definitively un-sent. The original reason for arming early — a cancel or state timeout landing while the device is still acting on a command it already received — is unaffected, because that window opens after the send, not before it. Second, the recovery budget was observed by the pause between the two exit commands, so it could send `SetTransparentMode 0` and then not send `LAN:APPLY`. That half state is the one outcome worse than not starting: the console is handed back while the WiFi manager is left in its bridge-mode state machine, so the device answers SCPI while its module still is not reachable. The budget now bounds only the reconnect wait — the one step here that can take unbounded time — and the two-command exit runs to completion once the transport is back. The un-cancelled tail is a fixed pause plus two synchronous writes, so the helper stays bounded. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both Qodo findings addressed in 2. "Recovery armed too early" — valid, fixed as suggestedReal regression, and worse than the summary states. Armed inside the prepare delegate now, immediately after 1. "Missing pre-send cancel check" — the race is real, the prescribed fix inverts the helper's purposeThe suggestion is to add
What the finding does correctly identify is an inconsistency: the budget was observed by the pause between the two commands but not by the sends around it, so the sequence could be torn in half. It could send Fixed by resolving the inconsistency the other way: the budget now bounds the reconnect wait and nothing else — the only step here that can take unbounded time — and the two-command exit runs to completion once the transport is back. The un-cancelled tail is a fixed 100 ms plus two synchronous writes, so the helper stays bounded. Note that The previous test that pinned the torn-in-half outcome was rewritten to pin the new intent, and a second test now covers the case the budget genuinely exists for. Tests+2 net (2693 → 2695), zero losses. FULL suite green net9 + net10 (2695 passed / 2 skipped each, +23 Mcp on net9), 0 warnings.
Mutation-checked, 4/4 caught: arm before the prepare step again (1 fail — the disconnected-device test), put
Bench (real Nq1, fw 3.7.2, USB, non-destructive) — 5/5Harness Deliberately not sent: Not merging — for review. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 1967c2e |
|
Ready for review — not merging. Verified on the exact head Full suite green net9 + net10 (2695 passed / 2 skipped each, +23 Mcp on net9), 0 warnings. Bench re-run on the real Nq1 (fw 3.7.2, USB, non-destructive) 5/5 with a negative control. |
1 participant
Part of #269 (item 2 — removes the consumer-side recovery workaround).
Not merging — this is for your review.
The bug
UpdateWifiModuleAsyncsendsSYSTem:COMMUnicate:LAN:FWUpdate, which bridges USB straight through to the WINC and bypasses the device's SCPI console. Only the successful path walked it back out. A failed flash, a state timeout, or a cancel left the module unreachable until someone power-cycled the device.That is precisely why
daqifi-desktopstill wraps Core's call in its own recoveryfinally(SerialStreamingDevice.ResetLanAfterUpdate+ a raw transparent-mode exit) — one of the workarounds #269 exists to move into Core.The fix
Both failure exits of
WifiModuleUpdater.RunUpdateAsyncnow run a best-effort bridge exit before reporting the failure. It is the managed-connection twin of the already-shippedWifiBridgeActivator.Deactivate— same two commands, same order, same pause:The pause is now one shared
internalconstant rather than two copies, so the raw-serial path and the managed path cannot pace the same firmware transition differently.Design calls worth reviewing:
LAN:ENAbled/APPLY/SAVErestore. Persisting a network configuration off the back of a flash that did not complete is not this step's job; the job is only to make the device answerable again. This also keeps the failure path from adding WiFi connect churn beyond the singleAPPLYthat leaves bridge mode.ReconnectingAfterFlash→VerifyingTimeout). Same physical operation — waiting for the serial transport to come back — and already tunable by a host that knows its re-enumeration is slow.No new options. No public API change.
WifiBridgeActivator.InterCommandDelaywent fromprivatetointernal.Tests
+3 (2690 → 2693). Full suite green on net9.0 and net10.0: 2693 passed / 2 skipped each, +23 Mcp on net9, 0 warnings.
FirmwareUpdateException/Programmingunchanged.LAN:FWUpdatebut before prep's disconnect → recovery still runs on its own token;DisconnectCalls == 0pins that the exit is not conditional on having been disconnected.ConnectAttempts == 0, immediate failure. (Added by the review fix.)The existing happy-path test already asserts the success sequence by exact equality, so "no bridge exit on success" stays pinned there.
Mutation-checked, 6/6 caught: recovery never runs (3 fail), skip recovery when the caller canceled (2), drop the inter-command pace (1), swap the two commands (3), send the full
ENAbled/APPLY/SAVEinstead (2), and an unbounded recovery budget — which hangs the suite rather than failing a test, since the reconnect loop then has nothing to stop it. Source restored from a byte-identical backup (sha verified) before committing.Note for anyone repeating this: deleting the recovery calls outright does not compile under
TreatWarningsAsErrors(CS0219, the flag becomes write-only), so that mutation had to be expressed as an always-true guard inside the helper.Bench (real Nq1, fw 3.7.2, USB, non-destructive)
Scratchpad harness
ProjectReference'd at this branch's Core.SYSTem:NOTAREALCOMMAND?→-113,"Undefined header". Without it the clean queues below would be worthless evidence.SYSTem:USB:SetTransparentMode 0with the 102 ms pace →scpiErrors=none. This is the already-off no-op path, i.e. exactly what the conservative arming hits on a device that never entered bridge mode.finally, or a retried update) →scpiErrors=none.id=1377184 fw=19.7.7 build=Mar 30 2022. Device still answering, final queue clean.Deliberately not exercised, and the harness gates on it rather than assuming: the trailing
LAN:APPLYwas skipped because the probe found the WINC powered, whereAPPLYstarts a real WiFi association and connect-churn is a known way to wedge this bench unit.LAN:APPLYis unchanged shipped behavior on both the success path andWifiBridgeActivator.Deactivate; the ordering and pacing around it are pinned by the unit tests above.SetTransparentMode 1was never sent — it is unrecoverable over a managed connection, which is also why "wrong order fails" cannot be shown non-destructively.Merge safety
Verified with
git merge-treeagainst both open loop PRs: clean against #443 and clean against #444. Hunks were mapped first and deliberately placed clear of both.Review fix (
1967c2e)Both Qodo findings addressed — full reasoning in this comment.
"Recovery armed too early" — valid, fixed as suggested.
mayBeInLanUpdateModewas armed before the prepare step, so a device that was never connected — and therefore never bridged — still ran the recovery, whose reconnect loop then waited out the fullReconnectingAfterFlashbudget (45 s by default). An immediate "device must be connected" failure became a 45-second one. Armed inside the prepare delegate now, immediately after theLAN:FWUpdatesend — as early as it can honestly be, sinceSendis synchronous and noawaitseparates it from the assignment."Missing pre-send cancel check" — the race is real, the prescribed fix inverts the helper's purpose. Guarding the sends with the budget token could only ever turn a recovery that had already got the transport back into a device left bridged. The genuine problem the finding surfaced is that the budget was observed by the pause between the two commands but not by the sends around it, so the sequence could be torn in half — console handed back, WiFi manager still in its bridge-mode state machine. The budget now bounds the reconnect wait and nothing else, and the two-command exit runs to completion once the transport is back; the un-cancelled tail is a fixed 100 ms plus two synchronous writes.
Tests: +2 net (2693 → 2695), zero losses. FULL suite green net9 + net10 (2695 passed / 2 skipped each, +23 Mcp on net9), 0 warnings. Mutation-checked 4/4 on the fix itself.
git merge-treere-verified clean against #443 and #444.Bench re-run (real Nq1, fw 3.7.2, USB, non-destructive) — 5/5. Negative control (
-113,"Undefined header") first, then the pacedSetTransparentMode 0exit and a repeat of it bothscpiErrors=none(elapsed 102 ms), WINC chip info identical before/after, final queue clean.