refactor(firmware): split FirmwareUpdateService into focused collaborators (part of #344)

[tylerkron]

Why

FirmwareUpdateService had grown to 2,656 lines. It held two completely independent jobs in one class — flashing the PIC32 over its HID bootloader, and flashing the WiFi module via an external tool — plus the shared retry/state machinery both lean on. It's one of the two biggest collision points in the repo: unrelated changes keep landing in the same file.

This is the firmware half of #344. The DaqifiStreamingDevice half is separate.

What

Same public API, same behavior, same tests — the code just lives in sensible places now.

File	Lines	Job
FirmwareUpdateService	359	public facade: argument checks, the lock that serializes device I/O, disposal
FirmwareUpdateContext	404	shared state machine, progress, retries, per-state timeouts
Pic32BootloaderSession	445	the low-level HID conversation (enumerate, connect, erase, program, verify, jump)
Pic32FirmwareUpdater	731	PIC32 flow orchestration, diagnostics, post-failure cleanup
WifiModuleUpdater	813	WINC version probe + external flash-tool run
WifiFlashProgressParser	154	lifted out of its nested position

Was: one file, 2,656 lines.

How

Rather than trust my own reading, I diffed the normalized statement multiset of the original file against all six new files — stripping comments and canonicalizing the mechanical renames. Every remaining difference is structural (signatures, field declarations, wiring). Zero logic statements were added, removed, or changed.

One test added. The StateChanged event's sender used to be an implicit this on a field-like event, so it couldn't be wrong. It's now forwarded from the context, and nothing asserted it — so I pinned it, and mutation-verified the test actually fails when the sender is wrong.

WifiFlashProgressParser was internal nested inside a public class, so it was never externally visible. Moving it to top-level internal is not a public API change; only the in-assembly name moved, which is why three test references were updated.

No changes outside src/Daqifi.Core/Firmware/** and its tests.

Bench evidence

Nyquist 1, FW 3.7.2 / HW 2.0.0, on /dev/cu.usbmodem1101 and over WiFi at 192.168.1.30. Example CLI built against this working copy.

PIC32 bootloader diagnostics loop — the meaningful test for this refactor, and non-destructive by design:

baseline   connected=True  status=Connected
forceboot  sending SYSTem:FORceBoot -> HID bootloader enumerated
health     CheckBootloaderHealthAsync -> bootloader version = '1.4'
reset      ResetBootloaderAsync (JMP_TO_APP) -> OK
recover    reconnected=True  status=Connected
recover    LAN chip info: id=1377184 fw=19.7.7 build=Mar 30 2022
result     PASSED (no flash touched)

That exercises the whole new PIC32 stack end to end on real hardware: RunBootloaderDiagnosticAsync (facade lock + Idle gate), Pic32FirmwareUpdater.RunHealthCheckAsync / RunSoftResetAsync, and Pic32BootloaderSession's enumerate / connect / version / jump. Worth noting the run emitted no state-transition events — that's correct, and confirms the diagnostics still deliberately leave CurrentState at Idle.

Streaming, to show the shared assembly is undisturbed:

• USB, 10 Hz, 3 s, channels 0+1 — 23 samples, exit 0
• WiFi, 10 Hz, 3 s, channels 0+1 — 21 samples, exit 0
• USB again after the bootloader reboot — streams normally, exit 0

(Sample counts run below the nominal rate because of this unit's known FW 3.7.2 clock offset, not anything here.)

Not validated on hardware

No erase and no program was run — deliberately. There's no macOS recovery path if a flash goes wrong, so the actual UpdateFirmwareAsync and UpdateWifiModuleAsync write paths were not exercised on the bench. Their correctness here rests on the statement-level diff plus the existing unit tests, not on hardware. The WiFi external-tool path in particular is Windows-only today and cannot run on this machine at all.

Two things worth a reviewer's eye

1. WaitingForBootloaderTimeoutDetailProvider is a settable property on the context, assigned once after construction. It's a mild smell — if it were ever null, the WaitingForBootloader timeout message would quietly lose its diagnostic detail. The cause is a real cycle: the context must exist to build the session, but the detail lives on the session. An existing test covers the message text, so a regression would be caught. I left it rather than add a lazier indirection.

2. WifiModuleUpdater is 813 lines, marginally over the issue's "~800" target. The obvious next seam is the external-tool orchestration — which is exactly the code WiFi (WINC) flash depends on a Windows-only external tool — blocks cross-platform #271 replaces when it introduces the IWincFlasher seam for the native cross-platform flasher. Splitting it here would be churn I'd immediately redo, so I left it for that PR.

Test suite

4,701 tests green across net9.0 and net10.0 (+1 from the sender guard). Build clean — 0 warnings, and TreatWarningsAsErrors is on.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Refactor firmware updates: extract PIC32/WiFi updaters behind FirmwareUpdateService facade

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Split the 2.6k-line FirmwareUpdateService into focused internal collaborators without API changes.
• Centralize state/progress/retry/timeout plumbing in FirmwareUpdateContext shared by both flows.
• Add test to pin StateChanged sender and update parser references after extraction.

Diagram

graph TD
  S["FirmwareUpdateService"] --> C("FirmwareUpdateContext")
  S --> P("Pic32FirmwareUpdater") --> B("Pic32BootloaderSession") --> E{{"Device I/O"}}
  S --> W("WifiModuleUpdater") --> F("WifiFlashProgressParser")
  W --> E
  C --> P
  C --> W
  subgraph Legend
    direction LR
    _fac["Facade"] ~~~ _coll("Collaborator") ~~~ _ext{{"External/IO"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Use partial classes to split the file only

• ➕ Minimizes constructor wiring changes and event-forwarding concerns
• ➕ Keeps all code in one type while reducing file size/conflicts
• ➖ Still couples two independent flows in one class/type
• ➖ Harder to test and reason about responsibilities; less clear boundaries

2. Introduce internal interfaces + DI for collaborators

• ➕ Improves test isolation and mockability of PIC32/WiFi paths independently
• ➕ Enables future swapping/feature flags per updater
• ➖ Adds abstraction/registration overhead for a refactor whose goal is primarily file decomposition
• ➖ May be overkill if collaborators are not intended for reuse outside the service

3. Adopt a dedicated state machine library

• ➕ Declarative transitions/timeouts can become easier to audit and visualize
• ➕ Can standardize retry/timeout behavior across modules
• ➖ Adds dependency and migration risk for a behavior-preserving refactor
• ➖ Refactor goal is separation of concerns, not new behavior/configuration

Recommendation: The chosen approach (public facade + focused internal collaborators + shared context) is the best fit: it reduces collision hot-spots, makes PIC32 vs WiFi responsibilities explicit, and keeps the external API stable. The only notable risk introduced by this split is wiring/forwarding (e.g., StateChanged sender), and the added test appropriately guards that.

Files changed (7) +2661 / -2363

Refactor (6) +2610 / -2360

FirmwareUpdateService.csReduce FirmwareUpdateService to facade and delegate to new collaborators +63/-2360

Reduce FirmwareUpdateService to facade and delegate to new collaborators

• Turns FirmwareUpdateService into a public facade that validates inputs, serializes operations via the existing lock, forwards StateChanged from the shared context, and delegates PIC32/WiFi operations to extracted updaters. Removes the large inlined implementations of both update flows and the nested WiFi progress parser while preserving the existing public API surface.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs

FirmwareUpdateContext.csAdd shared state/progress/retry/timeout context for update flows +404/-0

Add shared state/progress/retry/timeout context for update flows

• Introduces FirmwareUpdateContext to own the firmware update state machine (allowed transitions), progress reporting, retry wrappers, per-state timeout execution helpers, and exception/recovery-message construction. Also centralizes StateChanged raising while forwarding the sender as the public service instance.

src/Daqifi.Core/Firmware/FirmwareUpdateContext.cs

Pic32BootloaderSession.csExtract low-level PIC32 HID bootloader session operations +445/-0

Extract low-level PIC32 HID bootloader session operations

• Adds Pic32BootloaderSession to encapsulate HID enumeration/targeting, connect-with-retry, version/erase/program/verify/jump exchanges, and bootloader-search diagnostics used for timeout messaging. Keeps per-run targeting/poll state localized to the session and reused by the PIC32 updater.

src/Daqifi.Core/Firmware/Pic32BootloaderSession.cs

Pic32FirmwareUpdater.csExtract PIC32 firmware update orchestration and diagnostics +731/-0

Extract PIC32 firmware update orchestration and diagnostics

• Adds Pic32FirmwareUpdater to orchestrate the PIC32 update state flow (prepare → wait → connect → erase → program → CRC verify → jump → complete) and to host standalone diagnostics (health check and soft reset). Preserves prior cleanup behavior on failures/cancellation by re-erasing when in flash-touching states, using the shared context for state transitions/timeouts.

src/Daqifi.Core/Firmware/Pic32FirmwareUpdater.cs

WifiModuleUpdater.csExtract WiFi module (WINC) update and status-check flow +813/-0

Extract WiFi module (WINC) update and status-check flow

• Adds WifiModuleUpdater to probe current WiFi firmware status, switch the device into LAN update mode, run the external flash tool with prompt-handling and retry logic, validate success based on tool output markers, then reconnect and restore LAN settings. Uses FirmwareUpdateContext for state transitions, timeouts, and consistent failure reporting.

src/Daqifi.Core/Firmware/WifiModuleUpdater.cs

WifiFlashProgressParser.csMove WiFi flash progress parser to top-level internal type +154/-0

Move WiFi flash progress parser to top-level internal type

• Extracts the previously nested WiFi flash progress parser into its own internal class for reuse by WifiModuleUpdater. Keeps the same parsing behavior: ignores image-build percents, tracks block-address advancement across write/read/verify phases, and enforces monotonic progress updates.

src/Daqifi.Core/Firmware/WifiFlashProgressParser.cs

Tests (1) +51 / -3

FirmwareUpdateServiceTests.csAdd coverage for StateChanged sender and update parser references +51/-3

Add coverage for StateChanged sender and update parser references

• Adds a test asserting StateChanged events use the FirmwareUpdateService instance as the sender now that events are raised from an internal context. Updates existing tests to instantiate WifiFlashProgressParser as a top-level type instead of the previous nested class.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Retry loop can no-op ✓ Resolved 🐞 Bug ≡ Correctness

Description

FirmwareUpdateContext.ExecuteWithRetryAsync returns successfully without running action when
maxAttempts is 0 or negative, silently skipping the intended operation. While current call sites
pass validated retry counts, this helper is now shared plumbing and a future/unvalidated caller
could accidentally bypass critical firmware steps without any failure signal.

Code

src/Daqifi.Core/Firmware/FirmwareUpdateContext.cs[R194-224]

+    internal async Task ExecuteWithRetryAsync(
+        string operation,
+        int maxAttempts,
+        TimeSpan retryDelay,
+        Func<CancellationToken, Task> action,
+        Func<Exception, bool> isTransient,
+        CancellationToken cancellationToken)
+    {
+        for (var attempt = 1; attempt <= maxAttempts; attempt++)
+        {
+            cancellationToken.ThrowIfCancellationRequested();
+
+            try
+            {
+                await action(cancellationToken).ConfigureAwait(false);
+                return;
+            }
+            catch (Exception ex) when (attempt < maxAttempts && isTransient(ex))
+            {
+                Logger.LogWarning(
+                    ex,
+                    "Operation '{Operation}' failed on attempt {Attempt}/{MaxAttempts}; retrying in {DelayMs} ms.",
+                    operation,
+                    attempt,
+                    maxAttempts,
+                    retryDelay.TotalMilliseconds);
+
+                await Task.Delay(retryDelay, cancellationToken).ConfigureAwait(false);
+            }
+        }
+    }

Relevance

●●● Strong

Team often adds fail-fast guards for non-positive inputs (e.g., timeout/port/range validation) in
similar flows.

PR-#249
PR-#187
PR-#349

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The retry helper’s only execution path is the for loop; if maxAttempts is 0, the loop body never
runs and the method completes successfully without invoking action. Existing bootloader retry
counts are validated to be >= 1, which reduces current blast radius but doesn’t remove the
helper-level bug for future call sites.

src/Daqifi.Core/Firmware/FirmwareUpdateContext.cs[194-224]
src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs[392-406]
src/Daqifi.Core/Firmware/Pic32BootloaderSession.cs[206-244]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`FirmwareUpdateContext.ExecuteWithRetryAsync(...)` can return without executing the provided `action` when `maxAttempts` is 0 or negative. This is a silent-success failure mode that can skip critical operations.

### Issue Context
Today, current call sites appear to pass validated retry counts (e.g., `FirmwareUpdateServiceOptions.Validate()` enforces `HidConnectRetryCount` and `FlashWriteRetryCount` >= 1), but this helper is generic shared infrastructure and should defend its own contract.

### Fix Focus Areas
- src/Daqifi.Core/Firmware/FirmwareUpdateContext.cs[194-224]

### What to change
- Add an upfront guard, e.g. `if (maxAttempts < 1) throw new ArgumentOutOfRangeException(nameof(maxAttempts), ...)`.
- (Optional) Add a small unit test for `maxAttempts == 0` to ensure it throws (or, if you prefer clamping semantics, explicitly clamp to 1 and document it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[tylerkron]

Qodo round 1 addressed — 1 bug, 0 rule violations

Checked all three surfaces (inline review comments, the issue-comment summary, and reviewThreads via GraphQL) — one finding total, no hidden inline items.

Retry loop can no-op — agreed, fixed in f65dc7d.

ExecuteWithRetryAsync returned successfully without ever invoking action when maxAttempts was 0 or negative, because the for body simply never ran. For a helper that carries erase / program / verify, that is a silent success which skipped a flash-critical step with no failure signal.

Added an upfront ArgumentOutOfRangeException guard. Throwing rather than clamping to 1, per Qodo's primary suggestion: a caller asking for zero attempts has a bug and should hear about it. The message names the operation so the failure points at the right step.

Worth recording: this is pre-existing behavior, not a regression from the split. The identical unguarded loop is on origin/main in FirmwareUpdateService.ExecuteWithRetryAsync; this PR relocated it verbatim. That's consistent with the statement-level multiset diff in the PR description, which found zero logic changes. So adding this guard is a deliberate, narrow exception to the pure-refactor claim — flagged here and in the commit message rather than quietly folded in.

Three tests added: maxAttempts 0 and -1 (throws, and the action provably never runs), plus a maxAttempts == 1 boundary case so the new check can't drift into rejecting the valid minimum.

Suite now 4,707 green across net9.0 and net10.0; build still clean with TreatWarningsAsErrors.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit f65dc7d