Repository navigation
fix(firmware): device discovery kept probing the bus during a manual HID flash - #178
Conversation
|
/agentic_review |
PR Summary by QodoPause discovery throughout manual HID firmware flashes
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Round 1 triage — 1 finding, accepted and fixed in
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit e5e6418 |
e5e6418 to
107956a
Compare
|
Rebased onto No conflicts. The one shared-state risk I went looking for is absent: #172's new Re-measured on the new base Round 2 was clean on |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 107956a |
…HID flash Discovery was paused for an *auto* firmware update but not for a *manual* HID bootloader flash, because a manual flash never sets ConnectionManager.DeviceBeingUpdated and so is invisible to IsFirmwareUpdateInProgress. Two ways the bus came back live mid-write: - Another device's auto-update finishing while the user was in the firmware dialog picking a .hex restarted serial AND WiFi discovery unconditionally, leaving both running when they hit Upload. - ConnectHid's finally restarted discovery the moment the dialog closed, even though the flash lease is released asynchronously and can outlive the dialog (closing it mid-flash only cancels). The pause is now a three-reason guard, IsDiscoveryPausedForFirmware: a coordinator auto-update, an open HID firmware dialog, or a bootloader write still in flight on the app-global watcher. Every reason can hold discovery down on its own, and each one calls the restart when it clears, so whichever clears last is the one that actually brings discovery back. Also fixes an ordering bug in BootloaderWatcher.ResumeAfterFlashAsync: it cleared _flashingPath before awaiting the target's re-grab, so the watcher advertised "flash over" while the HID handle was still being reopened. It now clears in a finally after the re-grab — late enough to cover the handoff, unconditional enough that a failed re-grab cannot strand the marker set. Ported from daqifi-desktop 52257bdf (their issue #777). closes #168
…ing lease Qodo round 1, one High finding, accepted. PrepareFlashAsync hands out an independent lease per call but the watcher recorded a single _flashingPath, so disposing the first of two overlapping leases cleared the shared state, resumed HID discovery and released the connection dialog's guard while the second write was still on the bus. Not reachable through today's UI — the only caller, FirmwareDialogViewModel, sits behind a modal opened from another modal — but IsFlashInProgress is now a published contract read by a component that holds no lock, so the state has to mean "any lease outstanding", not "the most recent one". _flashingPath becomes a counted set of paths plus a single-field total for the lock-free reader. The re-grab is likewise deferred to the last lease for that path: reopening the handle while another write to the same device is in flight would fight it. Behaviour is identical for one flash, which is the only case that is reachable today. Also, per the same finding, the existing overlap test only proved a second prepare was allowed; it now disposes both leases, and a new test covers the disposal ordering it did not. Plus: the "pause is in place" test now reads the guard from *inside* the transport drain, using the fact that an async method runs synchronously up to its first suspension. Reading it after the drains could not tell "flag raised before the drains" from "flag raised after them", which is the property the fix actually claims.
…very pause Qodo round 3, one Medium finding, accepted. PrepareFlashAsync raises the flash marker before awaiting the target hold's release — deliberately, so an auto-update ending during that release cannot see a gap. But a throwing ReleaseAsync exits without returning a lease, and the lease is the only thing that ever clears the marker. IsFlashInProgress then stays true forever. That was survivable before this PR, when a stranded marker only paused the watcher's own HID discovery. It is not now: the connection dialog keys its serial and WiFi discovery on the same flag, so a single failed release would leave the whole bus unscanned for the life of the process, over a flash that never started. The release is now wrapped: on failure the marker is rolled back, discovery resumes if nothing else needs it paused, and the exception is rethrown — so the caller still sees the failure, and no rising edge is ever announced for a flash that did not begin.
107956a to
c60e1ac
Compare
Round 3 triage — 1 new finding, accepted and fixed in
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit c60e1ac |
…lls back Qodo round 4, one finding, accepted — and a consequence of round 3's own fix. IsFlashInProgress is polled, not only subscribed to. Between PrepareFlashAsync raising the marker and the rollback that a throwing ReleaseAsync now triggers, it reads true, and the connection dialog's discovery guards can refuse a restart on the strength of it — a coordinator auto-update ending inside that window is exactly the case. Rolling the marker back silently left that refusal with nothing to retry it, so serial and WiFi discovery stayed stopped over a flash that never began: the same permanent stranding round 3 set out to remove, moved one level up. The edge is now announced from a finally, so it fires on the failure path as well as the happy one. No rising edge is announced when the preparation throws (the rethrow precedes it), so this is an unpaired falling edge — which is precisely the signal a subscriber needs: whatever refused you is gone, ask again.
Round 4 triage — 1 finding, accepted and fixed in
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit cffbed6 |
|
Qodo-clean, CI green — ready for review Verified independently on head Extra checks beyond the boilerplate:
|
This branch deletes `AbstractStreamingDevice.Write(string)` and updated every test double that overrode it — every one that existed at 4a8c338, its branch point. #178 landed on main afterwards and added a fourth: `ConnectionDialogFirmwarePauseTests.TestDevice`, whose `public override bool Write(string command) => throw new NotSupportedException();` had nothing left to override once the two were merged. Git called the merge CLEAN because it is — textually. The two changes touch disjoint files and only fail together at compile time, so the merge queue was the first thing to see it: CS0115 in the `Test` step of run 33671611557. The double is a placeholder for `ConnectionManager.DeviceBeingUpdated`; the six tests that construct it never write, send or connect. Deleting the override leaves it satisfying the base's two remaining abstract members (`ConnectionType`, `SendMessage`), exactly like the three doubles this branch already updated. #178's subject — discovery staying paused across a manual HID flash — is untouched. Also merges current origin/main into the branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Not merging — for review. closes #168 (parity gap #5 from the 2026-09-01 audit; upstream
52257bdf, their issue #777).What was wrong
While a DAQiFi board is being written, nothing may touch the bus. The serial finder opens and probes every DAQiFi COM port each cycle and the WiFi finder UDP-broadcasts; either can starve a bootloader's HID I/O mid-write. The app knew this and paused discovery — but only for an auto firmware update.
A manual HID flash never sets
ConnectionManager.DeviceBeingUpdated, so it is invisible toIsFirmwareUpdateInProgress, which was the only thing the connection dialog gated on. Two concrete ways the bus came back live in the middle of a flash:.hex.OnFirmwareUpdateInProgressChangedrestarted serial and WiFi discovery unconditionally — so by the time the user hit Upload, every COM port was being probed and UDP broadcasts were going out.ConnectHid'sfinallyrestarted both transports the instant the modal closed. But closing mid-flash only cancels: the watcher's flash lease is released asynchronously and can outlive the dialog, so discovery came back while the HID write was still unwinding.Bus contention during a flash is intermittent, but when it lands it strands the update — and this is a multi-device bench workflow.
What changed
The pause is now a three-reason guard.
ConnectionDialogViewModel.IsDiscoveryPausedForFirmwareis true when any of these holds:ConnectionManager.IsFirmwareUpdateInProgress_hidFirmwareDialogOpenIBootloaderWatcher.IsFlashInProgressBoth
StartWiFiDiscoveryandStartSerialDiscoveryconsult it, so a restart requested while any reason is active is refused. The mirror-image risk — a guard that refuses restarts leaving discovery dead for the rest of the dialog's life — is handled by having every reason callRestartDiscoveryAfterFirmwarePause()when it clears: whichever clears last is the one that actually restarts. That is also why the dialog now subscribes to the watcher's newFlashInProgressChanged: when the write outlives the dialog, its falling edge is the only trigger left.ConnectHid's body moved intoRunWithHidFlashQuiescedAsync, which raises the flag before awaiting the drains — an auto-update can finish while we are still tearing discovery down, and would otherwise restart the very thing being torn down.Plus one ordering bug in
BootloaderWatcher.ResumeAfterFlashAsync. It cleared its flash marker at the top, beforeawait hold.BeginHoldAsync()— so the watcher advertised "flash over" while the HID handle was still being reopened, which is exactly the window a resumed COM-port probe would land in. The marker is now released in afinallyafter the re-grab: late enough to cover the handoff, unconditional enough that a re-grab failure cannot strand it set (which would pause discovery permanently).New on
IBootloaderWatcher:IsFlashInProgressandFlashInProgressChanged.BootloaderWatcheris the only implementor.FirmwareDialogViewModelneeded no change — it already hands off throughPrepareFlashAsync.And one thing beyond the upstream port, from review.
PrepareFlashAsynchands out an independent lease per call, but the watcher recorded a single_flashingPath— so if two flashes ever overlapped, disposing the first lease would clear the shared state, resume HID discovery and open the dialog's guard while the second write was still on the bus. That is not reachable through today's UI (the only caller sits behind a modal opened from another modal), butIsFlashInProgressis now a published contract read by a component holding no lock, so the state has to mean "any lease outstanding". It is now a counted set of paths plus a single-field total for the lock-free reader, and the post-flash re-grab is likewise deferred to the last lease for that path. Behaviour is byte-for-byte identical for a single flash — the only case reachable today — so this cannot regress the path I am not allowed to bench-test.And one failure-path fix, also from review. The flash marker is raised before the target's hold is released, deliberately, so an auto-update ending during that release cannot see a gap. The cost was that a throwing
ReleaseAsyncexits without returning a lease — and the lease is the only thing that ever clears the marker. That stranding pre-dates this PR, but its blast radius did not: before, a stranded marker only paused the watcher's own HID discovery; now the connection dialog keys serial and WiFi on the same flag, so one failed release would leave the whole bus unscanned for the life of the process, over a flash that never started. The release is now wrapped, the marker rolled back on failure, discovery resumed if nothing else needs it paused, and the exception rethrown — so the caller still sees the failure.That fix then needed one of its own, which review also caught.
IsFlashInProgressis polled, not only subscribed to: between the increment and the rollback it reads true, and the dialog's guards can refuse a restart on the strength of it — a coordinator auto-update ending inside that window is exactly the case. Rolling back silently left that refusal with nothing to retry it: the same permanent stranding, moved one level up. The edge is now announced from afinallyso it fires on the failure path too. No rising edge is announced there (the rethrow precedes it), so it is an unpaired falling edge — which is precisely the signal a subscriber needs: whatever refused you is gone, ask again.Verification
19 new xUnit tests, 0 failures. The guard is a small state machine, so it is pinned directly rather than described.
Every figure in this section was measured against base
4a8c338— currentmainat the time of writing, which this branch is rebased onto — where the suite goes 133 → 152.Every claim below was checked by reverting the fix and re-running, not by reading:
IsDiscoveryPausedForFirmwarereduced to the connection-manager term alone (whatmaindoes)maindoes)The_pause_is_in_place_before_the_transports_finish_draining)Disposing_one_of_two_overlapping_leases_leaves_the_flash_in_progress)A_preparation_that_fails_to_release_the_hold_does_not_strand_the_pause)Being precise about the four dialog tests that stayed green, since "5 of 9" on its own would flatter them: two of them (
Nothing_is_paused…,An_auto_update_pauses_discovery) cover the auto-update reason, whichmainalready had.Closing_the_dialog_unsubscribes…guards a handlermaindoes not have, so it cannot fail there. AndThe_pause_clears_when_the_auto_update_finishes_after_the_bootloader_writepasses vacuously againstmain— with only one reason, of course it clears; it is there to catch the new failure mode this PR could introduce, a guard that refuses restarts forever. Its mirror (…_when_the_bootloader_write_finishes_after_the_auto_update) does go red, and it is the one that proves the watcher's falling edge is wired up.Two of these are windows rather than states, and a window can only be tested by suspending the code inside it. The re-grab test parks the fake hold's
BeginHoldAsyncon aTaskCompletionSourceand readsIsFlashInProgresswhile the re-grab is suspended. The drain-ordering test exploits the fact that an async method runs synchronously up to its first real suspension: it parks the discovery-loop task, and control returning from the call is itself proof that the window is open. Neither uses a sleep.CI: on the earlier pushes, Desktop-macOS pass, Android head pass (the head this Mac has no workload for, so CI is the only place it is proven), CI self-tests pass, and the Linux
Library + Desktop head + capture harnessjob pass with the full suite green — worth naming, because the dispatcher this PR's test seam works around behaves differently on Linux and macOS, and both hosts agree.Gate (both sides at
4a8c338, see above). Desktop250 → 250warnings, iOS Releaseiossimulator-arm64251 → 251, 0 errors on both. Measured against a merge base built in this same worktree with the same discipline —Daqifi.Avalonia/obj,Daqifi.Avalonia.Desktop/obj,Daqifi.Avalonia.iOS/objandthird_party/oxyplot-avalonia/OxyPlot.Avalonia/objall cleared before each side, so the vendored OxyPlot's 20 warnings are emitted on both. The comparison that actually matters is the distinct first-party warning-site set (file(line,col): warning CODE,third_party/excluded): 182 on both sides, the only difference being two pre-existing warnings inConnectionDialogViewModel.cswhose line numbers moved.packages.lock.jsonuntouched (restored without-r).Not verified by hardware, and why
No flash was performed. A firmware write is on this session's destructive denylist, so the acceptance criterion in #168 — watch discovery state across a real HID flash on two boards — is not met by this PR. What is established is the state machine (unit-tested, both directions) and the reasoning above. A human should still run one real manual flash with a second board attached before trusting it in the field.
Two smaller things the tests cannot reach:
_hidFirmwareDialogOpenis set around the real modal in production; the tests driveRunWithHidFlashQuiescedAsyncwith a delegate standing in for it.FirmwareDialogViewModeldisposing the watcher's lease. That path is unchanged and untested here; the watcher tests exercise the lease directly.Test seam worth flagging in review
ConnectionDialogViewModel's UI marshal moved from a direct call to a private_marshalToUiThreadfield (still initialised to the same static method — production behaviour is identical). This is not tidiness: outside a running Avalonia app,Dispatcher.UIThreadbinds to whichever thread first touches it and is never pumped, so the production marshal's blockingInvokenever returns when reached from any other thread, and xUnit gives no thread affinity. Without the seam these tests would deadlock the test host rather than fail. Reached by reflection, so noInternalsVisibleToand no.csprojedit. That was originally to stay off the one file #172, #173 and #174 were all editing; those have since merged andDaqifi.Avalonia.csprojnow carriesInternalsVisibleTo, so the seam could be converted — but it works as it is, and swapping it now would cost a review round for no behaviour change. Happy to convert it if you would rather these tests used the same mechanism as #172's.Collision check
Diffed #172, #173 and #174 before starting, when all three were open.
mainthen moved five times while this PR was in review, so the check was redone against each: this branch is rebased onto4a8c338and re-measured there, and none of #172, #173, #174, #176 or #177 touchesConnectionDialogViewModel.cs,BootloaderWatcher.csorIBootloaderWatcher.cs.Three specific interactions I went looking for, none of which bites:
OnDeviceConnectionLostearly-returns onIsDeviceBeingUpdated) has the same blind spot this PR fixes —DeviceBeingUpdatedis never set by a manual flash. It does not matter there, because a manually flashed board is a sitting bootloader that was never connected as anIStreamingDevice, so it is not inConnectedDevicesand that handler never runs for it. Nothing in this PR is made redundant by fix(devices): a dropped WiFi device is never torn down, and Core's background failures are invisible #172, and nothing in fix(devices): a dropped WiFi device is never torn down, and Core's background failures are invisible #172 needs changing.DeviceBeingUpdated, and these tests set it on the process-wideConnectionManager.Instance— which xUnit could run in parallel. No race: fix(devices): a dropped WiFi device is never torn down, and Core's background failures are invisible #172 builds its own manager through the internal test constructor and never touchesInstance.ViewModelBase. This PR references none of them; the test doubles here implementIAppLoggerdirectly rather than reusing the (now deleted)NoOpLogger.All three heads plus the test project build and pass on the rebase.
No
.portomatic/map/edit; the@port:markers for the new members are in the code.🤖 Generated with Claude Code