Repository navigation
fix(firmware): a running upload can be cancelled, and it says what it is doing - #256
Conversation
… is doing Issue #241. Both cancel commands were implemented, correct and bound by no view; FirmwareUpdateStatusText was written 18 times and read zero times. - FirmwareDialog.axaml: the upload scrim covers the dialog's own Cancel button, so a stalled bootloader flash could only be escaped by killing the app. Adds the only binding of CancelUploadFirmwareCommand. - DevicesPanePrototype.axaml / DevicesMobileView.axaml: bind CancelFirmwareUploadCommand and FirmwareUpdateStatusText, both gated on IsFirmwareUploading so the line is unambiguously the running flash. - Coordinator: open the run before the first status write, and move the WiFi-only "Preparing..." message in from the view model, so both land while the line is on screen. - DaqifiViewModel: delete its three status writes. All three ran with IsFirmwareUploading false and could never have been displayed.
…l button The scrim's Cancel is reachable only through a reflection binding in AXAML, so nothing in the build can see it break — the exact gap CaptureDialogs() was added for (#213). dialog-firmware-uploading renders it through the real dialog and the real view-model and asserts the button by AutomationId. Five determinism runs agreed 25/25, and the same capture reproduced the previous manifest on all 24 existing hashes byte for byte, so the recording is additive and this change moved nothing that was already gated.
|
/agentic_review |
PR Summary by QodoMake firmware uploads cancellable and show live status
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
Self-review of the previous commit. Moving BeginUpload() above the first status write put the uploading flag up before three statements that were still outside the try — so a throw from any of them (the status setter is bound, and a faulting binding handler throws through it) would leave IsFirmwareUploading raised with no finally to lower it: the pane stuck "uploading" behind a Cancel button with nothing left to cancel. Everything after BeginUpload now sits inside the try, in both entry points. Against the previous commit the new test fails with the exception escaping UploadFirmwareAsync entirely, which is the wedge.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 50cbc67 |
…ount The ledger's rows 1 and 2 still read as open gaps and its "deliberately not fixed here" note still blocks on #239, which merged this morning. Struck both rows with what actually changed; the 2026-09-04 measurement counts above them are left as recorded, since the file is a dated measurement. Also: the test file's class comment said two writes were moved and two deleted. One of the two was not moved — the run is opened earlier so the write it already made lands inside. Four writes failed the invariant, in three different ways.
|
/agentic_review |
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
|
Code review by qodo was updated up to the latest commit e2d49c2 |
…dy hidden Qodo, round 3. CancelUpload gates on the cancellation source rather than on IsFirmwareUploading, and must keep doing so — a flag with no source announcing a cancellation it cannot deliver is issue #234. But now that the status line is rendered only while the flag is up, those two conditions part in the one window that is not atomic: a Cancel that has already read a live source, arriving after the run's completion has lowered the flag, wrote "Canceling firmware update..." into a hidden control, where the next run would reveal it. The announcement is now separately gated on the flag. The cancel itself is not: a run whose flag has come down must still be stopped. A_cancel_arriving_after_the_flag_is_down_writes_nothing forces that window through the download seam. Without the gate it fails with exactly the recorded write ("Canceling firmware update...", WasUploading=False).
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 49be77c |
Shepherd verification — head
|
| pane | control | rendered |
|---|---|---|
| desktop drawer | CANCEL UPDATE |
IsEffectivelyVisible=True IsEnabled=True Bounds=110x23, Command non-null |
| desktop drawer | status line | IsEffectivelyVisible=True Bounds=375x14, Text="Waiting for WiFi module to enter update mode..." |
| mobile pane | CANCEL UPDATE |
IsEffectivelyVisible=True IsEnabled=True Bounds=293x44, Command non-null |
| mobile pane | status line | IsEffectivelyVisible=True Bounds=293x16, same text |
Both bindings resolve against the compiled view models — a reflection binding that renders is exactly that proof. CancelFirmwareUpload / CancelUploadFirmware were searched as bare substrings across every .cs and .axaml, which covers the [RelayCommand]-generated …Command spelling and every binding form by construction: each has exactly one binding site, and every theme resource the new controls use (PillButton, DarkSecondaryButton, TextSecondary) was already in use in the same file.
The two captures are attached below (desktop drawer first, then mobile). The harness edit is reverted; git status is clean.
The new capture gate is deterministic here
Five capture runs, byte-compared: 25/25 identical on all five, and run 1 reproduced all 25 committed hashes in baselines/macos-arm64.sha256, including the 24 that predate this PR. The manifest diff is +1 / −0 lines — no unexplained second baseline. Recorded on the same Mac as the author's, so the independent-host leg is CI's Desktop head on macOS job, which is green on this SHA. Note that a small edge-column delta in desktop-7/-8/-9 would be #201, not a regression from this PR; none appeared.
Every number in the body re-derived
- 18 → 16 write sites — hand-counted.
origin/main: 15 inFirmwareUpdateCoordinator+ 3 inDaqifiViewModel= 18. This branch: 16 + 0 = 16. Exact. - 512 → 520 — from CI's own logs, not the author's word: run on
4b37a22reportsPassed: 512, run33991618437on49be77creportsPassed: 520. Locally 520/520 too. The new file holds 6[Fact]+ 1[Theory]× 2[InlineData]= 8 cases, so +8 reconciles exactly. - "6 of 8 fail against
origin/main" — I copied the test file onto a throwaway branch at4b37a22and ran it: 8 of 8 fail, not 6. The body's claim is conservative rather than inflated (it says six were run pre-fix and all six failed; it never claims the other two pass on main). Worth stating in the stronger form. - Warnings 191/191 desktop, 192/192 iOS Release — confirmed from CI's published MSBuild summaries on both heads (
4b37a22vs49be77c), and the warning sets diff to zero: the only differences are three warnings whose line numbers shifted by the diff's own insertions. These are all-project totals, ~20 of themthird_partyAVP1002— not a first-party count. Reproduced locally: desktop, iOS restore, iOS Releaseiossimulator-arm64all green;packages.lock.jsonuntouched. - Zero declarations removed. Every deleted C# line is a statement, so the fix(layering): a view-model was hand-writing SCPI bytes at the transport #189 family (a member
maingained an override for) does not apply.
Merge state
git merge-base origin/main HEAD is 4b37a22 — this branch's base is today's main tip, so it already carries #239, #240, #244, #245, #246 and #247, and git log origin/main --not HEAD is empty. The merge result is bit-identical to 49be77c, which CI built green across all six real jobs and which I rebuilt locally (desktop + iOS Release) with the suite at 520/520.
Qodo
Clean on the current head. The Code Review by Qodo comment reads "Review updated until commit 49be77c ⚖️ Balanced" (a real review, not a Skipped doc-only round) with Bugs (0) / Rule violations (0) / Skill insights (0); its one Medium finding is struck through and resolved. GraphQL reviewThreads shows 1 qodo-authored thread, 0 unresolved — filter validated against #196, which yields 3 qodo-authored threads by the same substring match. Re-checked both surfaces 15 minutes after the round landed: the summary comment was byte-identical and the thread count unchanged.
Qodo-clean, CI green — ready for review at 49be77c281d941b69c20e37cacdd563077f39afd.
What remains unverified, precisely: no live firmware upload was exercised — unattended flashing is on this environment's destructive denylist, so no PIC32 or WINC image was written to a board and no device was rebooted. That leaves untested whether Core's flasher honours the token promptly mid-transfer, which is a property of Core and of #239 rather than of this diff. Everything above is a unit test, a headless render of the real UI, or a reading of CI's own output.
…ched hunk The fix moves exactly two of the manifest's screens, mobile-landscape-3-storage and mobile-portrait-3-storage -- the two orientations of the one view that changed. Every other screen is byte-for-byte unchanged against the pre-fix capture, and that pre-fix capture matched the then-committed manifest on every screen it lists, so the clipped empty state is what was shipping rather than an artefact of the harness. Five --determinism runs agreed after the change and --check-captured passes against the new manifest. Both the manifest lines and the README paragraph are placed to miss the other open PRs writing to these files: three of them are at the head and foot of the README's baselines list, so this entry goes between the #251 and #213 paragraphs, and the manifest lines touched are neither the one #256 inserts after nor the ones #258 and #263 rewrite. The paragraph states no screen count, because #256 changes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
closes #241
Not merging — for review.
What was wrong
A firmware upload could not be stopped, and while it ran the app would not say what it was doing.
Both halves were absences in XAML, which is why nothing caught them: views here carry no
x:DataType, so bindings resolve by reflection and a member nothing binds is indistinguishable from a member nothing has.FirmwareDialog.axamlraises a modal scrim over the whole bootloader dialog — including its own Cancel button — for as long asIsFirmwareUploadingis true. Under it sat a progress bar and two labels and nothing else.FirmwareDialogViewModel.CancelUploadFirmwareCommandwas implemented, had aCanExecutewired through[NotifyCanExecuteChangedFor], and was bound by no view. A flash that hung meant killing the app.DaqifiViewModel.CancelFirmwareUploadCommand, which both the desktop drawer and the mobile pane could have shown and neither did.FirmwareUpdateStatusTextwas assigned 18 times and read zero times, in C# or XAML — whileIFirmwareUpdateHostdocuments it as "Status line shown beneath the firmware progress bars". During a multi-minute flash the only signal a user had was a percentage.I re-derived all three claims rather than taking the issue's word for them. All three held; the 18 write sites are 15 in
FirmwareUpdateCoordinatorand 3 inDaqifiViewModel, and the interface declares the property{ set; }only, so it is structurally unreadable through the seam.How it is fixed
The cancel is pure wiring — three bindings, no new C#, no new command, no new state:
FirmwareDialog.axamlDevicesPanePrototype.axamlDevicesMobileView.axamlEach command's existing
CanExecuteisIsFirmwareUploading, so it is enabled for precisely as long as an upload is running. #239 (merged earlier today) is what makes this safe to bind: before it, the WiFi-only path cancelled a differentCancellationTokenSourcethan the flash received, so a Cancel button would have armed a control that lies.The status text is rendered, not deleted — and its dead writes are deleted. I checked the "already shown elsewhere" half of the question rather than only the "unreachable" half. The 16 messages that survive narrate stages (
Downloading latest firmware package...,Waiting for WiFi module to enter update mode...,Failed to enter WiFi update mode. Reconnect the device and try again.) and appear nowhere else in the UI: the panes show a percentage, and the outcome dialogs speak only after the run is over. That is a lost capability, not a duplicated one.Both the status line and the cancel button are gated on
IsFirmwareUploading. That single decision is what makes the whole thing honest — the line is unambiguously what the running flash is doing, and it can never be left showing a finished run's last message next to a failure dialog that contradicts it. Outcomes stay where they already are: the success/failure dialogs,HasErrorOccured,IsUploadComplete.It also creates an invariant the writes must satisfy — no status write reaches a hidden line — and four of them did not:
stateDiagram-v2 [*] --> Idle Idle --> Running: BeginUpload() — CTS created, IsFirmwareUploading = true Running --> Canceling: CancelUpload() — "Canceling firmware update..." Canceling --> Idle: EndUpload() — CTS dropped, flag false Running --> Idle: EndUpload() — success or failure note right of Running Status line VISIBLE and Cancel ENABLED (both bind IsFirmwareUploading) All 16 remaining writes happen here end note note left of Idle Status line hidden. A write here reaches nobody — which is what the two deleted DaqifiViewModel writes were. end noteCancelUploadis the one write a run does not make: it gates on the cancellation source, not on the flag, and must keep doing so (a flag with no source announcing a cancellation it cannot deliver is #234). Its announcement is separately gated on the flag, so the two conditions parting in the non-atomic window cannot put text on a hidden line. That one came from Qodo; see Verification.So the C# change is net-subtractive in write sites, 18 to 16:
UploadFirmwareAsyncnow callsBeginUpload()before"Preparing firmware update..."(a moved line). That message is the one that tells a user the click registered, and it was being written to a hidden control. Everything afterBeginUpload()also moved inside the existingtry— see the wedge under Verification."Preparing WiFi firmware update..."moves fromDaqifiViewModelintoUpdateWifiModuleOnlyAsync, which is where the flag and the token are actually owned.DaqifiViewModel.TryGetLanChipInfoAsync(the probe hard-returns whenIsFirmwareUploadingis true, so this can never be visible; the coordinator's identical message covers the during-a-flash case that can), and"WiFi firmware update canceled."in its cancel catch (the coordinator'sfinallyhas already cleared the flag by then).DaqifiViewModelno longer writes the property at all. It now has exactly one writer, the coordinator, through the host seam.Verification
No live firmware upload was exercised, and could not be: unattended firmware flashing is on this environment's destructive denylist, so no PIC32 or WINC image was written to a board. Everything below is a unit test or a headless render of the real UI. What that leaves unverified is narrow but real: that Core's flasher honours the token promptly mid-transfer, which is a property of Core and of #239, not of this diff.
New tests: 8.
Daqifi.Avalonia.Tests/Device/Firmware/FirmwareUploadAffordanceTests.cs. Suite is 512 → 520, all green. Six of the eight were run againstorigin/mainbefore the fix was written, and all six failed. The other two exist because of defects this branch itself introduced — one found by self-review, one by Qodo — and each fails against the commit that introduced it. Both are described below.Two things came out of running them unfixed that I did not have going in:
The_combined_update_...failed with the whole disconnected-device run producing exactly one status write,"Preparing firmware update...", with the flagFalse— i.e. the message was not merely unrendered, it was the only one that path ever produced.The_wifi_only_update_...failed with"Downloading WiFi firmware package..."as the first write the coordinator made, confirming the view-model's"Preparing..."never reached the coordinator's run at all.A wedge I introduced and then removed, worth stating plainly. Moving
BeginUpload()above the first status write raisedIsFirmwareUploadingwhile three statements were still outside thetry. The status setter is bound, so a faulting binding handler throws through it — and a throw there would have left the flag raised with nofinallyto lower it: the pane stuck "uploading" behind a Cancel button with nothing left to cancel. Everything afterBeginUpload()now sits inside thetry, in both entry points.A_throw_from_the_first_status_write_still_ends_the_runpins it, and against the intermediate commit it fails with the exception escapingUploadFirmwareAsyncentirely, which is exactly the wedge. Found by self-review, not by a reviewer.A second one, from Qodo, and it was right. Making the status line visible only while the flag is up turned a previously-harmless disagreement into a real one:
CancelUploadgates on the cancellation source, so a Cancel that has already read a live source and arrives after the run's completion has lowered the flag would write"Canceling firmware update..."into a hidden control — where the next run would reveal it. The announcement is now separately gated on the flag; theCancel()itself is not, because a run whose flag has come down must still be stopped.A_cancel_arriving_after_the_flag_is_down_writes_nothingforces that window open through the download seam, and without the gate fails with exactly the predicted write,("Canceling firmware update...", WasUploading=False).The XAML facts are deliberately textual — they assert the binding exists in the view and that the member it names is public and readable on the view model. That pair is what a reflection binding needs and is the pair nothing else in the build checks.
Answering "what does a cancel do today": the issue's phrasing left three possibilities. It is no control — not a control that does nothing, and not one that cancels the wrong thing. Both commands and both
CanExecutepredicates were correct on arrival; #239's five tests already pin the token routing, and I confirmed they still pass unchanged. Before-start, after-completion and gone-away are all covered there and were not weakened.A permanent visual gate, because this class of break is invisible to the build.
tools/parity-auditgaineddialog-firmware-uploading, which boots the real app headless, constructs the realFirmwareDialogViewModel, and asserts the Cancel button byAutomationId(IsVisible=True Bounds=185, 242, 130, 30 Text="Cancel Upload"). Its download service is stubbed so the capture never opens a socket. Five determinism runs agreed 25/25 byte-identical, and the same capture reproduced the committed manifest on all 24 existing hashes byte for byte — so this change moved nothing that was already gated, and the recording is purely additive.expected=in the macOS CI step and the README are updated in the same commit, as that README asks.What was NOT rendered, stated plainly. Only the bootloader dialog is in that gate. The two device-pane blocks are not: the firmware section lives inside the device settings drawer, which the harness never opens (it sweeps the shell's four drawers, not the pane's), and
DevicesMobileViewhas no captured screen at all. Seeding a selected device to reach either is more than this change should carry, and the harness explicitly forbids starting real discovery to get one. Those three bindings rest on the textual + reflection pair above, and on the 24 unchanged baseline hashes showing they cost no layout when hidden. If a reviewer wants that drawer gated, it is a good separate ticket.Build gate, measured against my own merge-base (
origin/main, same worktree, same-t:Rebuilddiscipline,third_party/oxyplot-avalonia/objcleared on both sides):iossimulator-arm64These are MSBuild all-project totals, not first-party-only (~20 are
third_partyAVP1002). I also diffed the warning sets, not just the totals: nothing added, nothing removed on either head.packages.lock.jsonunchanged after every restore. The Android head is not buildable on this machine (workload absent) — CI covers it, and did: the first push went green on all six checks, includingDesktop head on macOS, which is where the new baseline hash was reproduced on a runner that is not my laptop.The instrument that filed the issue now reports it clean.
python3 tools/parity-audit/coverage.py --bindings ../daqifi-desktopreturns sections B and C populated as before, with no firmware member in either — all three of the issue's rows are gone.On the shape of the diff
It is net-additive overall (+643/−25), and that deserves a word. The additions are: one test file (432 lines, over half of it fakes), one capture scenario plus its offline stub (76), and XAML bindings whose comment blocks are longer than the controls they explain. The part that changes app behaviour is 5 files and +110/−13, of which the C# is one moved line, one moved message, two deletions, one three-line guard, and an indentation change from widening a
try. No upload-state manager, no progress abstraction, no status-sink interface — one command per button, and a property that was already there.What the fix looks like