feat(firmware): share the LAN chip-info retry probe as public API (part of #269)

[tylerkron]

Part of #269 (item 5: "a GetLanChipInfoAsync(retries, delay) helper in Core would let every consumer share it").

Not merging — opened for your review.

Why

A single GetLanChipInfoAsync is not a reliable answer to "what WiFi firmware is on this device":

• right after a PIC32 update the application is up while WiFi is still finishing startup (Retry LAN chip-info probing during WiFi update startup #144), and
• a module whose state machine has not reached INITIALIZED answers SCPI -200 instead of JSON (WiFi chip-info probe: kick LAN:APPLY before declaring ChipInfoUnavailable #203).

Both clear on their own within seconds. Treating the first failure as the answer is what sends a caller into a needless multi-minute reflash of already-current firmware.

Core already had the bounded retry that handles this — but it was private to WifiModuleUpdater, so every consumer hand-rolled its own. daqifi-desktop wraps the call in a 3×/2s TryGetLanChipInfoAsync of its own, which is precisely the duplication #269 exists to remove.

What changed

• New public API (Daqifi.Core.Firmware): ILanChipInfoProvider.GetLanChipInfoWithRetryAsync(...) extension, plus LanChipInfoRetryOptions (budget) and LanChipInfoProbeResult (what was read + whether the terminal failure was specifically "WINC not initialized").
• WifiModuleUpdater now calls it instead of its own copy: TryGetLanChipInfoWithRetryAsync (~140 lines) is gone, replaced by a small BuildLanChipInfoRetryOptions() projection of FirmwareUpdateServiceOptions. One implementation, not two.
• Additive only. No interface changed, nothing removed from the public surface. LanChipInfoRetryOptions' defaults are the ones Core itself uses (3 attempts / 2 s apart / 8 s total / kick enabled), so GetLanChipInfoWithRetryAsync() with no arguments is "what Core does".

Behavior is deliberately unchanged, including the parts that are easy to lose in a move:

• attempt count clamped to at least 1 (a probe that queries nothing would be indistinguishable from a dead module);
• the wall-clock budget ends the probe with an unavailable result, not an exception — only the caller's cancellation propagates;
• the LAN:APPLY kick is sent at most once per probe and only to a connected device (repeated kicks re-init the WINC and risk an already-associated link);
• the cancellation check before that state-changing send;
• the not-initialized flag describes the terminal failure, reset by any other kind — a stale one would send a caller kicking APPLY at a module that failed for an unrelated reason.

The probe finds the device for the kick by provider as IStreamingDevice (in practice the provider is the device — DaqifiStreamingDevice implements both). A bare provider simply gets no kick, which is covered by a test.

Tests

+22 new LanChipInfoProviderExtensionsTests (18 with the original commit, +4 with the review fix below). Baseline measured on a clean origin/main worktree at 4b8eed2 (2690) rather than taken from notes; this branch is 2712 — exactly +22, zero losses. Full suite green on net9 + net10 (2712 passed / 2 skipped each, plus 23 Mcp on net9), 0 warnings.

The 22 existing FirmwareUpdateServiceTests assertions covering this retry (#144 / #203 / kick-once / cancel-race / total-timeout) were left untouched on purpose — they are the evidence that routing the updater through the shared helper changed nothing.

Mutation-checked rather than assumed to bite. Eight mutations of the new implementation, each run against the new tests:

mutation	result
kick fires on every attempt	2 tests fail
drop Math.Max(1, MaxAttempts)	1 fails
drop the not-initialized flag reset	1 fails
delay after the final attempt too	1 fails
drop the pre-kick cancellation check	1 fails
ignore IsConnected before kicking	1 fails
ignore KickLanApplyOnNotInitialized	1 fails

(An eighth — deleting the !hasSentLanApply guard outright — doesn't compile under TreatWarningsAsErrors, so it was re-expressed as "kick every attempt".) Implementation restored from a byte-identical backup and diffed before committing.

Bench (real Nq1, fw 3.7.2, USB /dev/cu.usbmodem1101, non-destructive)

Scratchpad harness with a ProjectReference straight at this branch's Core, so the new public API is what actually ran against hardware.

• Real WINC read through the new probe: chipId=1377184 fwVersion=19.7.7 buildDate=Mar 30 2022 — identical to the single-shot GetLanChipInfoAsync baseline taken immediately before it (1061 ms), so the wrapper returns the device's own answer unaltered.
• The retry loop drives real device I/O, not just a mock. A flaky decorator failed the first two queries; the loop retried and the third attempt reached the hardware and came back with the same chip info (attempts=3 realDeviceCalls=1, 1563 ms ≈ 2 × 400 ms delay + the device call). Per-attempt debug logging surfaced through the injected ILogger. This decorator is also the not-a-streaming-device path, so the "no kick available" branch ran on hardware too.
• Probe after SYSTem:POWer:STATe 1 + 1 s settle (what Core's own status check does): succeeded in 560 ms.
• No LAN:APPLY was sent in any run — the kick was left disabled, since the bench module is already initialized and repeated APPLYs are the WiFi-churn hazard the once-only guard exists for.

One honest observation, pre-existing and unchanged by this PR: a 900 ms TotalTimeout against a 20-attempt budget did not cut a single in-flight query short — the run returned successfully at 1059 ms. The device-side text exchange doesn't observe the linked token mid-flight on this path, so the budget bounds the loop between attempts rather than acting as a hard deadline. That is exactly what it was written for (attempts × per-attempt timeout + delays overrunning while a lock is held), and it is the same behavior main has today; the docs now say so explicitly.

🤖 Generated with Claude Code

---

Review fix (0d903a7) — the retry budget is normalized, not thrown

Qodo caught a real gap in the new public surface, confirmed red before fixing: LanChipInfoRetryOptions is a settable record, but RetryDelay went straight to Task.Delay and TotalTimeout straight to CancellationTokenSource. Three misconfigurations escaped a probe whose stated contract is that failures come back as a result:

option	before
TotalTimeout negative	ArgumentOutOfRangeException from the CTS ctor — despite the option's own doc promising "no budget, so no attempt"
RetryDelay negative	ArgumentOutOfRangeException from Task.Delay, partway through the loop
both Timeout.InfiniteTimeSpan	hung between attempts with nothing left to release it

Normalized in the same clamp-don't-throw spirit as the existing Math.Max(1, MaxAttempts), rather than validated — throwing would contradict the "failures are absorbed" contract, and FirmwareUpdateServiceOptions.Validate() throws because it is an up-front config object, not a per-call budget.

One deliberate asymmetry, now documented on both properties: Timeout.InfiniteTimeSpan keeps meaning "no ceiling" for TotalTimeout (that is what .NET and CancellationTokenSource already mean by it) but folds into "no pause" for RetryDelay, where an infinite pause inside a bounded retry has no reading other than the hang above. Every other negative becomes zero. Defaults are provably identity, so the shipped path is unchanged.

Both subtle branches mutation-checked: clamping the infinite budget to zero fails 2 tests; honoring an infinite RetryDelay fails 1 (the 10s hang returns).

Re-benched on the real Nq1 (fw 3.7.2, USB, non-destructive, kick disabled) — the defaults line matches the pre-fix baseline, the negative budget returns in 0 ms having made no attempt, and the bottom three all previously threw or hung:

[defaults]                  OK chipId=1377184 fw=19.7.7 build=Mar 30 2022  (1064 ms)
[negative-total-timeout]    UNAVAILABLE (notInitialized=False)             (0 ms)
[negative-retry-delay]      OK chipId=1377184 fw=19.7.7 build=Mar 30 2022  (1053 ms)
[infinite-delay-and-budget] OK chipId=1377184 fw=19.7.7 build=Mar 30 2022  (1060 ms)
[infinite-budget-only]      OK chipId=1377184 fw=19.7.7 build=Mar 30 2022  (1059 ms)

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Expose shared LAN chip-info retry probe as public firmware API

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add a public, bounded-retry LAN chip-info probe to avoid false "unavailable" firmware reads.
• Centralize WINC-not-initialized handling (single LAN:APPLY kick, total-time budget, caller
 cancellation).
• Refactor WifiModuleUpdater to reuse the shared probe and add focused unit coverage.

Diagram

graph TD
  C["Core consumers"] --> E["GetLanChipInfoWithRetryAsync"] --> P["ILanChipInfoProvider"] --> Q["GetLanChipInfoAsync"] --> R{"Read chip info?"}
  R -->|yes| OK["LanChipInfoProbeResult (success)"]
  R -->|no / exception| D{"LanNotInitialized?"}
  D -->|yes| K["Optional LAN:APPLY kick"] --> L["Retry loop (budgeted)"] --> OK2["LanChipInfoProbeResult (unavailable)"]
  D -->|no| L
  K --> SD["IStreamingDevice.Send"]
  subgraph Legend
    direction LR
    _api(["Public API"]) ~~~ _iface["Interface"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Introduce a dedicated LanChipInfoProbe service (non-extension)

• ➕ Clear discoverability via DI (registerable service) rather than extension method
• ➕ Easier to evolve with additional probe strategies/telemetry without expanding static helpers
• ➖ Heavier surface area and wiring than needed for a small, additive helper
• ➖ More churn for consumers that already hold ILanChipInfoProvider and just need retry semantics

2. Use a retry library (e.g., Polly) for the loop

• ➕ Declarative retry policies; standard primitives for backoff/jitter/timeout
• ➕ Reusable across other retry needs
• ➖ Adds a dependency and policy configuration complexity for a very bespoke semantic (terminal not-initialized flag + single LAN:APPLY kick + caller-vs-budget cancellation split)
• ➖ Less explicit control over the exact edge-case semantics that are critical here

Recommendation: The chosen approach (public extension + explicit options/result types) is the best fit: it is additive, keeps the exact semantics already relied on by Core, avoids new dependencies, and makes reuse trivial for any consumer that already has an ILanChipInfoProvider. The bespoke cancellation/timeout split and the single-kick behavior are easier to reason about and test in this explicit implementation than via a generic retry abstraction.

Files changed (3) +719 / -151

Enhancement (1) +253 / -0

LanChipInfoProviderExtensions.csAdd public GetLanChipInfoWithRetryAsync API with options and result +253/-0

Add public GetLanChipInfoWithRetryAsync API with options and result

• Adds LanChipInfoRetryOptions (retry budget) and LanChipInfoProbeResult (chip info + terminal not-initialized flag). Implements a bounded retry loop that absorbs failures into a result, distinguishes caller cancellation from internal total-time budget expiry, and optionally sends a single LAN:APPLY kick when the provider is a connected IStreamingDevice.

src/Daqifi.Core/Firmware/LanChipInfoProviderExtensions.cs

Refactor (1) +23 / -151

WifiModuleUpdater.csReuse shared LAN chip-info retry probe and remove duplicated loop +23/-151

Reuse shared LAN chip-info retry probe and remove duplicated loop

• Replaces the private TryGetLanChipInfoWithRetryAsync implementation with a call to the new shared extension method. Adds a small projection helper to map FirmwareUpdateServiceOptions into LanChipInfoRetryOptions, ensuring updater behavior remains consistent while centralizing the probe implementation.

src/Daqifi.Core/Firmware/WifiModuleUpdater.cs

Tests (1) +443 / -0

LanChipInfoProviderExtensionsTests.csAdd unit tests for bounded LAN chip-info probe behavior +443/-0

Add unit tests for bounded LAN chip-info probe behavior

• Introduces a focused test suite that pins retry semantics, total-time budget behavior, caller cancellation propagation, and the at-most-once LAN:APPLY kick logic. Includes scripted provider/device test doubles to simulate null responses, transient exceptions, and WINC-not-initialized errors.

src/Daqifi.Core.Tests/Firmware/LanChipInfoProviderExtensionsTests.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 options not validated ✓ Resolved 🐞 Bug ☼ Reliability

Description

GetLanChipInfoWithRetryAsync uses LanChipInfoRetryOptions.TotalTimeout and RetryDelay directly in
CancellationTokenSource and Task.Delay; invalid negative values (other than
Timeout.InfiniteTimeSpan) can throw ArgumentOutOfRangeException and violate the API’s “failures are
absorbed into a result” contract. Also, RetryDelay == Timeout.InfiniteTimeSpan can block between
attempts indefinitely when TotalTimeout is also infinite (or otherwise never cancels).

Code

src/Daqifi.Core/Firmware/LanChipInfoProviderExtensions.cs[R138-141]

+        using var timeoutCts = new CancellationTokenSource(totalTimeout);
+        using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource(
+            cancellationToken, timeoutCts.Token);
+        var linkedToken = linkedCts.Token;

Relevance

●●● Strong

Team often adds guards for invalid retry/timeout inputs to preserve “no-throw” contracts and avoid
surprises.

PR-#419
PR-#249

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new extension method constructs a timeout CTS from the consumer-supplied TotalTimeout and later
delays using the consumer-supplied RetryDelay without any range checks. Elsewhere in the repo,
similar timeouts explicitly validate “positive or Timeout.InfiniteTimeSpan”, and firmware-update
options enforce positive delays/timeouts, indicating this code path should also validate when
exposed publicly.

src/Daqifi.Core/Firmware/LanChipInfoProviderExtensions.cs[125-141]
src/Daqifi.Core/Firmware/LanChipInfoProviderExtensions.cs[229-244]
src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs[121-134]
src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs[435-489]
PR-#419

Agent prompt

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

### Issue description
`LanChipInfoProviderExtensions.GetLanChipInfoWithRetryAsync(...)` is a new public API that accepts `LanChipInfoRetryOptions` from external consumers, but it passes `TotalTimeout` into `new CancellationTokenSource(totalTimeout)` and `RetryDelay` into `Task.Delay(retryDelay, ...)` without validating supported ranges. Negative values outside the .NET “infinite sentinel” can throw `ArgumentOutOfRangeException`, and `RetryDelay == Timeout.InfiniteTimeSpan` can lead to an unbounded wait between attempts when there is no finite total-timeout to cancel it.

### Issue Context
The method documentation emphasizes that exhausted budgets return an “unavailable” result rather than throwing (except for caller cancellation). The current implementation can still throw for malformed option values, which is a likely scenario for config-driven consumers.

### Fix Focus Areas
- src/Daqifi.Core/Firmware/LanChipInfoProviderExtensions.cs[125-141]
- src/Daqifi.Core/Firmware/LanChipInfoProviderExtensions.cs[229-244]

### Suggested fix approach
- Add upfront validation/normalization of `RetryDelay` and `TotalTimeout`, mirroring repo patterns (e.g., allow `Timeout.InfiniteTimeSpan` as the only negative sentinel, otherwise require positive).
- Decide and codify semantics for `TotalTimeout <= TimeSpan.Zero` (the options XML remarks currently say “no budget => no attempt”); implement explicitly rather than relying on `CancellationTokenSource` edge cases.
- Consider adding unit tests covering:
 - `TotalTimeout` negative (invalid) => does not crash (either returns unavailable or throws a documented `ArgumentOutOfRangeException`, depending on chosen contract)
 - `RetryDelay` negative (invalid) => handled deterministically
 - `RetryDelay == Timeout.InfiniteTimeSpan` with `TotalTimeout == Timeout.InfiniteTimeSpan` => doesn’t hang forever (either reject or normalize)

ⓘ 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]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 0d903a7

[tylerkron]

Ready for review: Qodo re-reviewed 0d903a7 and reports Bugs (0) / Rule violations (0) with the one earlier finding marked resolved, CI build is green on that head, and origin/main (4b8eed2) is already an ancestor so the branch is up to date. Not merging — this is for your review.

[tylerkron]

Bench-validated on the real Nq1 (fw 3.7.2, USB, non-destructive) — 16/16 checks green on 0d903a7, and the same suite fails 3 checks on the pre-fix parent 72c2293.

This PR shipped with unit tests only, so I put the new public API in front of real firmware. LAN:APPLY was disabled (KickLanApplyOnNotInitialized = false) throughout — no state-changing command was ever sent to the WINC. The device's module read back chip=1377184 fw=19.7.7 build=Mar 30 2022 before and after the whole series, so nothing was disturbed.

What real hardware confirmed

Check	Result
GetLanChipInfoWithRetryAsync defaults vs. a single-shot GetLanChipInfoAsync	identical chip info, WasLanNotInitialized=false
TotalTimeout = -1s	returns unavailable in 0 ms, no throw, and provably makes no device attempt
TotalTimeout = 250 ms, MaxAttempts = 5	stops at 1007 ms instead of spending 5 × 2000 ms of device timeouts; expiry comes back as an unavailable result, not OperationCanceledException
caller's own CancellationToken	still escapes as OperationCanceledException
MaxAttempts=1 probe after the series	device still answering — no wedge

The part worth flagging

My first pass ran the negative/infinite RetryDelay cases straight at the device and they all passed — and they pass on the pre-fix commit too. The delay only runs between attempts, and the device answers on attempt 1, so those probes never reached the code this PR changes. Passing there was worth nothing.

So I drove real retries: a decorator that discards the first two answers while still performing all three real device round-trips.

• RetryDelay = Timeout.InfiniteTimeSpan + TotalTimeout = Timeout.InfiniteTimeSpan, 3 attempts → 3186 ms total, i.e. 3 × the ~1060 ms round-trip with zero added pause, recovering on the last attempt with the genuine fw=19.7.7. On 72c2293 the same probe is still pending after 30 s at attempt 1 — the hang is real and this is what catches it.
• RetryDelay = -5s, 3 attempts → completes in 3181 ms. On 72c2293 it throws ArgumentOutOfRangeException from Task.Delay at attempt 1, partway through a probe documented to absorb failures.
• TotalTimeout = -1s on 72c2293 throws ArgumentOutOfRangeException from the CancellationTokenSource constructor before any attempt.

Pre-fix 9 passed / 3 failed → post-fix 16 passed / 0 failed, same binary, same board, same session.

Not covered on hardware

The bench unit's WINC is initialized, so it answers with JSON rather than SCPI -200. The LanNotInitializedException classification path and the LAN:APPLY kick therefore have unit-test coverage only — reaching them on hardware needs a device whose state machine hasn't reached INITIALIZED, and forcing that state isn't something I'll do unattended.

Still not merging — for review.

[tylerkron]

Heads-up: this PR's failing build is not caused by anything in this PR. It is inherited from main.

After #444 and #445 merged back to back, origin/main (7965b26) itself fails 5 FirmwareUpdateServiceTests — I reproduced it on a clean checkout of main with no changes applied. The two PRs were each green on their own head but neither CI run saw the other, and main's CI is schedule-only so nothing caught it post-merge. Updating this branch to main (67f65d9) simply pulled the breakage in.

Fix is up as #451 (test-only). Once that lands, a gh run rerun here should go green with no change needed to this branch. Not rerunning yet — main is still red, so it would just fail again.

Nothing else changed here: still Qodo-clean with 0 unresolved threads, and origin/main is an ancestor of the head.