feat(firmware): kick LAN:APPLY once on -200 LAN-not-initialized (closes #203)

[tylerkron]

Summary

• CheckWifiFirmwareStatusAsync's bounded chip-info retry (added in feat(firmware): split WiFi update planning from execution (closes #143) #198 for Retry LAN chip-info probing during WiFi update startup #144) only covers the transient post-PIC32-reboot window. A distinct steady-state failure — LAN:ENAbled? = 1 in saved settings but the WINC1500 state machine not yet INITIALIZED — makes GETChipInfo? return SCPI -200 instead of JSON, exhausting the retry budget and forcing a needless multi-minute WiFi reflash even when firmware is already current.
• DaqifiStreamingDevice.GetLanChipInfoAsync now throws a new LanNotInitializedException when it detects that specific -200 line (reusing the existing IsScpiErrorLine/TryParseScpiErrorCode helpers), instead of silently returning null like any other unparseable response.
• FirmwareUpdateService's retry loop catches that exception distinctly, sends SYSTem:COMMunicate:LAN:APPLY once per probe (new KickLanApplyOnNotInitialized option, default true, mirroring the PowerOnWifiModuleBeforeProbe pattern from feat(firmware): power on WINC before chip-info probe in CheckWifiFirmwareStatusAsync #320), and keeps retrying within the existing budget.
• If retries still exhaust, WifiFirmwareStatus.Reason reports the new WifiFirmwareStatusReason.LanNotInitialized instead of the generic ChipInfoUnavailable — same conservative "proceed with flash" behavior for callers, better diagnostics.

This implements options 1+3 from the issue (the issue's own text notes they "pair naturally" — option 3 alone wouldn't have fixed the actual reflash bug, and option 2 pushes the fix onto callers who'd have to remember to call it).

Test plan

• dotnet test — full suite passes (1495/1497, 2 pre-existing skips unrelated to this change)
• New unit tests: -200 detection at the DaqifiStreamingDevice.GetLanChipInfoAsync layer (GetLanChipInfoAsyncTests.cs), and end-to-end retry/kick/reason behavior at the FirmwareUpdateService layer (kick sent exactly once, recovers to UpToDate, reports LanNotInitialized on exhaustion, respects the new option flag)
• Verified against the real bench Nq1 (same hardware referenced in the issue, SN with ChipId 1377184 / FW 19.7.7) — GetLanChipInfoAsync still succeeds normally post-fix, confirming no regression on the healthy path. Attempted to reproduce the exact -200 steady-state via a fresh power cycle; it did not reproduce this run (the condition is timing-dependent per the issue itself), so the recovery path itself is validated by the deterministic unit tests above rather than live on hardware this session.

Closes #203.

[qodo-code-review]

PR Summary by Qodo

Fix WiFi probe: kick LAN:APPLY once on SCPI -200 (LAN not initialized)

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Detect SCPI -200 "LAN not initialized" during GETChipInfo? and surface it explicitly.
• Retry WiFi chip-info probe while sending a single best-effort LAN:APPLY kick per probe.
• Add option + status reason for better diagnostics and deterministic unit tests for behavior.

Diagram

graph TD
  A["FirmwareUpdateService"] --> B["TryGetLanChipInfoWithRetry"] --> C["ILanChipInfoProvider"] --> D["DaqifiStreamingDevice"] --> E{Parse chip info?}
  E -->|"JSON"| F["LanChipInfo"] --> G["WifiFirmwareStatus: UpToDate"]
  E -->|"No"| H{SCPI -200?}
  H -->|"Yes"| I["LanNotInitializedException"] --> J["Send \"LAN:APPLY\" (once)"] --> B
  H -->|"No"| K["Return null"] --> L["WifiFirmwareStatus: ChipInfoUnavailable"]
  I --> M["Exhaust retries"] --> N["WifiFirmwareStatus: LanNotInitialized"]

  subgraph Legend
    direction LR
    _svc["Component/Method"] ~~~ _dec{"Decision"}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Always send LAN:APPLY before probing

• ➕ Simpler control flow (no special-case exception or classification).
• ➕ May reduce first-probe failures from multiple causes, not just -200.
• ➖ More disruptive: APPLY can tear down/re-init the WINC even when already healthy/associated.
• ➖ Adds unnecessary SCPI traffic and potential side effects on every probe.

2. Treat -200 as a nullable chip-info (no exception) + internal flag

• ➕ Avoids using exceptions for control flow.
• ➕ Keeps provider API "null on failure" consistent.
• ➖ Harder to propagate a specific terminal reason without expanding return types broadly.
• ➖ Less explicit call-site handling; risks regressing into silent null handling over time.

3. Caller-managed recovery (expose helper to kick LAN:APPLY)

• ➕ Keeps provider simple; recovery logic lives where retries live.
• ➕ More flexible for different products/callers.
• ➖ Pushes correctness burden onto every caller; easy to forget and reintroduce reflashes.
• ➖ Scatters knowledge of SCPI -200 semantics across codebase.

Recommendation: Current approach is the best tradeoff: it isolates SCPI -200 detection at the parsing boundary (device/provider), enables targeted recovery (one-shot LAN:APPLY) only when needed, and preserves conservative behavior (still proceed to flash when probe can’t confirm) while improving diagnostics via WifiFirmwareStatusReason.LanNotInitialized.

Files changed (8) +371 / -15

Enhancement (3) +86 / -12

FirmwareUpdateService.csClassify not-initialized failures, kick LAN:APPLY once, and report new reason +55/-12

Classify not-initialized failures, kick LAN:APPLY once, and report new reason

• Updates the chip-info retry helper to return both chip info and whether the terminal failure was LanNotInitialized. Catches LanNotInitializedException to optionally send a best-effort one-shot LAN:APPLY (only if connected and enabled by option), continues retrying, and returns WifiFirmwareStatusReason.LanNotInitialized on exhaustion.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs

LanNotInitializedException.csIntroduce LanNotInitializedException for SCPI -200 chip-info responses +22/-0

Introduce LanNotInitializedException for SCPI -200 chip-info responses

• Adds a dedicated exception type representing the known steady-state condition where LAN is enabled but the WINC1500 state machine is not initialized. Intended to let callers differentiate this case from generic unparseable responses.

src/Daqifi.Core/Firmware/LanNotInitializedException.cs

WifiFirmwareStatus.csAdd WifiFirmwareStatusReason.LanNotInitialized +9/-0

Add WifiFirmwareStatusReason.LanNotInitialized

• Extends the status reason enum with a dedicated LanNotInitialized value to distinguish exhausted retries after a not-initialized (-200) response from generic chip-info unavailability.

src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs

Bug fix (1) +18 / -2

DaqifiStreamingDevice.csDetect SCPI -200 from GETChipInfo? and throw LanNotInitializedException +18/-2

Detect SCPI -200 from GETChipInfo? and throw LanNotInitializedException

• Changes LAN chip-info query to return parsed info when possible, otherwise scan for SCPI error lines and treat error code -200 as a distinct not-initialized condition. Throws LanNotInitializedException for -200 and continues returning null for generic unparseable responses.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Tests (2) +248 / -1

GetLanChipInfoAsyncTests.csAdd unit tests for LAN chip-info parsing and SCPI -200 handling +114/-0

Add unit tests for LAN chip-info parsing and SCPI -200 handling

• Introduces a test harness streaming device that returns canned text responses. Verifies valid JSON parsing, disconnected behavior, and that SCPI -200 triggers LanNotInitializedException while other SCPI errors still return null.

src/Daqifi.Core.Tests/Device/GetLanChipInfoAsyncTests.cs

FirmwareUpdateServiceTests.csTest retry + one-shot LAN:APPLY kick and LanNotInitialized terminal reason +134/-1

Test retry + one-shot LAN:APPLY kick and LanNotInitialized terminal reason

• Adds end-to-end tests proving the retry loop sends LAN:APPLY exactly once on LanNotInitializedException and can recover to UpToDate. Extends the fake streaming device to simulate not-initialized failures, count APPLY sends, and validate the option flag behavior and terminal status reason on exhaustion.

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

Documentation (1) +4 / -0

ILanChipInfoProvider.csDocument LanNotInitializedException on LAN chip-info API +4/-0

Document LanNotInitializedException on LAN chip-info API

• Adds XML documentation indicating that GetLanChipInfoAsync may throw LanNotInitializedException when the device reports SCPI -200 due to an uninitialized WINC state machine.

src/Daqifi.Core/Firmware/ILanChipInfoProvider.cs

Other (1) +15 / -0

FirmwareUpdateServiceOptions.csAdd KickLanApplyOnNotInitialized option (default true) +15/-0

Add KickLanApplyOnNotInitialized option (default true)

• Introduces a new configuration flag to gate sending LAN:APPLY after a SCPI -200 not-initialized chip-info response. Documents why it’s one-shot and how it avoids unnecessary reflashes while allowing opt-out to legacy behavior.

src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 WiFi chip-info probe: kick LAN:APPLY before declaring ChipInfoUnavailable 🎫 Retry LAN chip-info probing during WiFi update startup

1. APPLY ignores cancellation ✓ Resolved 🐞 Bug ☼ Reliability

Description

In FirmwareUpdateService.TryGetLanChipInfoWithRetryAsync, the LanNotInitializedException handler may
send the state-changing LAN:APPLY command after the caller has requested cancellation because it
does not re-check cancellation before device.Send(). This can reinitialize/disrupt the device’s
LAN/WiFi state even though the status probe was canceled.

Code

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[R1005-1018]

+                if (_options.KickLanApplyOnNotInitialized && !hasSentLanApply && device.IsConnected)
+                {
+                    hasSentLanApply = true;
+                    try
+                    {
+                        device.Send(ScpiMessageProducer.ApplyNetworkLan);
+                        _logger.LogDebug("Sent LAN:APPLY to initialize the WINC state machine after a not-initialized chip-info response.");
+                    }
+                    catch (Exception sendEx)
+                    {
+                        // Best-effort: falling through to the normal retry delay/loop
+                        // below still gives the device a chance to recover on its own.
+                        _logger.LogDebug(sendEx, "Failed to send LAN:APPLY after a not-initialized chip-info response; continuing retry loop without it.");
+                    }

Relevance

⭐⭐⭐ High

Cancellation-before-state-changing SCPI sends was accepted previously (ThrowIfCancellationRequested
pattern) in firmware flows.

PR-#320
PR-#315

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The code explicitly guards cancellation before other state-changing SCPI sends (TurnDeviceOn) but
not before the newly added LAN:APPLY send in the LanNotInitializedException handler, allowing a
cancellation race to produce side effects after cancellation.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[820-833]
src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[996-1019]
PR-#320

Agent prompt

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

### Issue description
`TryGetLanChipInfoWithRetryAsync` can send `SYSTem:COMMunicate:LAN:APPLY` even after the caller cancels, because the `LanNotInitializedException` catch block does not guard the state-changing `device.Send(...)` with a cancellation check.

### Issue Context
This method is otherwise cancellation-aware (it checks cancellation before attempts and during delays), and other state-changing sends (e.g., WINC power-on) explicitly check cancellation first. The new APPLY send should follow the same rule to avoid side effects after cancellation.

### Fix Focus Areas
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[996-1019]
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[820-833]

### Suggested fix
- Immediately before setting `hasSentLanApply = true` / calling `device.Send(ScpiMessageProducer.ApplyNetworkLan)`, add `cancellationToken.ThrowIfCancellationRequested();` (use the *caller* token, not the linked timeout token, so total-timeout doesn’t start throwing).
- (Optional but recommended) Add/extend a unit test that cancels during the LanNotInitialized failure path and asserts `LanApplySentCount == 0`.

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

---

2. LAN:APPLY missing LAN-enabled guard ✗ Dismissed 📎 Requirement gap ☼ Reliability

Description

The new LanNotInitializedException handler sends SYSTem:COMMunicate:LAN:APPLY without first
verifying LAN:ENAbled? = 1, which the compliance rule requires to reduce disruption risk. This can
cause LAN:APPLY to be issued in scenarios that may not match the intended guarded condition.

Code

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[R1005-1011]

+                if (_options.KickLanApplyOnNotInitialized && !hasSentLanApply && device.IsConnected)
+                {
+                    hasSentLanApply = true;
+                    try
+                    {
+                        device.Send(ScpiMessageProducer.ApplyNetworkLan);
+                        _logger.LogDebug("Sent LAN:APPLY to initialize the WINC state machine after a not-initialized chip-info response.");

Relevance

⭐⭐ Medium

No accepted precedent for LAN:ENAbled? guard; prior PRs send ApplyNetworkLan without checking
enabled state.

PR-#211
PR-#315

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 2 requires LAN:APPLY be issued only under a guard that includes verifying
LAN:ENAbled? = 1. The added handler sends ApplyNetworkLan after a LanNotInitializedException
but does not check LAN-enabled state (only option flag, one-shot tracking, and connection status).

On initial LAN chip-info failure, optionally kick SYSTem:COMMunicate:LAN:APPLY once before declaring ChipInfoUnavailable (guarded by LAN enabled and no prior success)
src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[996-1011]

Agent prompt

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

## Issue description
`FirmwareUpdateService.TryGetLanChipInfoWithRetryAsync` sends `SYSTem:COMMunicate:LAN:APPLY` when catching `LanNotInitializedException`, but it does not verify the rule’s required guard that LAN is enabled (`LAN:ENAbled? = 1`).

## Issue Context
PR Compliance ID 2 requires kicking `LAN:APPLY` only under a safe guard (LAN enabled and no prior success) to minimize disruption risk. The current code only checks `_options.KickLanApplyOnNotInitialized`, `!hasSentLanApply`, and `device.IsConnected`.

## Fix Focus Areas
- src/Daqifi.Core/Firmware/FirmwareUpdateService.cs[996-1019]

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

---

3. LanNotInitialized docs inaccurate ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

WifiFirmwareStatusReason.LanNotInitialized XML docs claim the status occurs "even after" a single
LAN:APPLY kick, but the code can return this reason when KickLanApplyOnNotInitialized is disabled
(or when APPLY cannot be sent). This misleads callers into believing the device was nudged when it
may not have been.

Code

src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs[R62-69]

+    /// <summary>
+    /// The WiFi module's saved settings report enabled (<c>LAN:ENAbled? = 1</c>) but
+    /// its state machine was still not initialized (SCPI <c>-200</c>) even after a
+    /// single <c>LAN:APPLY</c> kick and exhausting the retry budget. Distinct from
+    /// <see cref="ChipInfoUnavailable"/> so callers can tell "known not-yet-ready
+    /// state, already nudged" apart from a genuinely unresponsive device.
+    /// </summary>
+    LanNotInitialized,

Relevance

⭐⭐⭐ High

Team often accepts correcting misleading XML docs to match real behavior (doc mismatch fixes
accepted).

PR-#321
PR-#98

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The enum docs assert APPLY was sent, but the added test explicitly verifies the reason is still
LanNotInitialized when the option disables APPLY and no APPLY was sent.

src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs[62-68]
src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[2743-2774]

Agent prompt

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

### Issue description
The XML documentation for `WifiFirmwareStatusReason.LanNotInitialized` states the device remained uninitialized "even after a single LAN:APPLY kick", but the implementation/tests show the reason is returned even when `KickLanApplyOnNotInitialized` is `false` (no APPLY sent), and also when APPLY can’t be sent (e.g., disconnected).

### Issue Context
Callers may rely on generated docs/log interpretation to understand whether Core attempted a remediation (APPLY) before returning this status.

### Fix Focus Areas
- src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs[62-68]
- src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs[2743-2774]

### Suggested fix
Update the enum docs to describe the reason as: terminal SCPI `-200` / not-initialized responses after exhausting the retry budget, and *optionally* note that Core may attempt a one-shot `LAN:APPLY` when `KickLanApplyOnNotInitialized` is enabled and the device is connected.

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

---