fix(firmware): stop reflashing supported WiFi modules when the release lookup fails (part of #269)

[tylerkron]

Part of #269. Not merging — for review.

The problem

CheckWifiFirmwareStatusAsync had exactly one way to form a version opinion: compare the module's reported firmware against the latest GitHub release. When that lookup failed — offline bench, blocked egress, rate limit, GitHub down — it returned LatestReleaseUnavailable with no opinion at all, and UpdateWifiModuleAsync's version gate fell through to its default: arm:

default:
    // ... LatestReleaseUnavailable ... — proceed with the flash conservatively.
    return false;

So a network failure triggered a multi-minute, destructive WINC reflash of a module whose own reported version was already fine. "Conservative" was doing a lot of work in that comment: on a network outage, flashing is the expensive guess, not the safe one.

This is the same failure class PowerOnWifiModuleBeforeProbe (#301) and KickLanApplyOnNotInitialized (#203) already exist to prevent — each closed a path where a probe that couldn't answer sent callers into a needless reflash. This closes the remaining one, which is the only one that isn't the device's fault.

The fix

#269's comment thread resolved the policy: model "minimum supported version" as the canonical concept in Core and own the 19.7.7 constant in the SDK, since it's a firmware-contract fact. That question is answerable with no network at all — the device already told us its version.

• FirmwareUpdateServiceOptions.MinimumSupportedWifiFirmwareVersion, defaulting to the new DefaultMinimumSupportedWifiFirmwareVersion const. Validated at construction, so a typo'd minimum throws instead of silently degrading the check back to "no version opinion" — the exact invisible failure the option exists to remove.
• WifiFirmwareStatus.MinimumSupportedVersion and MeetsMinimumSupportedVersion, populated on every result path.
• The update flow's version gate honors a true verdict and skips the flash.

MeetsMinimumSupportedVersion is a three-state bool? deliberately: false means "read the device and it is below the bar"; null means "could not tell". Collapsing those into one false is the conflation that made an unreadable module indistinguishable from an outdated one — and it's load-bearing here, because null still proceeds to flash. Only LatestReleaseUnavailable can reach the gate with a non-null verdict; every other fall-through reason leaves the device version unknown or unparseable.

IsUpToDate is untouched — it keeps its strict latest-release meaning and all its current values. The new answer sits beside it rather than redefining it, so nothing that reads the status record today changes behavior. The two genuinely disagree (a module can be supported but not newest), which is why they're modeled separately. A caller that wants to reflash regardless already has skipVersionCheck: true.

This also removes the reason daqifi-desktop hard-codes its own MinimumWifiFirmwareVersion = "19.7.7" in FirmwareUpdateCoordinator.WifiFirmwareNeedsFlash.

Bench validation (real Nq1, fw 3.7.2, USB, non-destructive)

Ran CheckWifiFirmwareStatusAsync against the live device with a download service that throws on every call — the field failure, reproduced:

Reason                       = LatestReleaseUnavailable
CurrentChipInfo.FwVersion    = 19.7.7
CurrentChipInfo.BuildDate    = Mar 30 2022
LatestRelease                = (null - lookup failed)
IsUpToDate                   = False
MinimumSupportedVersion      = 19.7.7
MeetsMinimumSupportedVersion = True
=> version gate verdict: SKIP flash (module is supported)

Before this change the same offline call produced no version opinion and the gate said PROCEED to flash. The bench module reports exactly 19.7.7, so this also exercises the inclusive boundary (>=) against real hardware rather than a fixture.

Same device, minimum raised to 99.0.0:

MinimumSupportedVersion      = 99.0.0
MeetsMinimumSupportedVersion = False
=> version gate verdict: PROCEED to flash

— proof the verdict is driven by the configured bar and not hard-coded to true. Only chip-info reads plus the WINC power-on Core's probe already performs; no flash, no NVM write, no reboot. Device answered SCPI normally afterward.

UpdateWifiModuleAsync itself was not run on the bench — that's the destructive path. Its gate is covered by unit tests.

Tests

+14 tests. Two mutation checks rather than eyeballing coverage:

• Inverting the gate to == false fails both new update-flow tests (the skip stops happening, and the below-minimum module stops being flashed).
• Widening it to != false — treating null as "meets" — fails 9 tests including the new chip-info-unavailable guard, confirming the null/false distinction is genuinely load-bearing and already fenced by the existing suite.

FULL suite green net9 + net10: 2,543 passed, 2 skipped, each TFM. Daqifi.Mcp.Tests 23 passed. Release solution build 0 warnings on both TFMs.

Not in scope

The rest of #269 — the port-release wait, the LAN-update-mode prep/recovery sequences, and the single-entry-point orchestration — all touch the destructive flash path and need a supervised bench session. This PR is the piece that is fully validatable non-destructively.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Avoid reflashing supported WINC modules when GitHub release lookup fails

🐞 Bug fix 🧪 Tests ✨ Enhancement 🕐 40+ Minutes

AI Description

• Introduce a minimum supported WINC firmware policy independent of GitHub availability.
• Populate WiFi status with a three-state minimum-version verdict to avoid unsafe fallbacks.
• Skip WINC flashing when the device meets the minimum but latest-release lookup is unavailable.

Diagram

graph TD
  A["UpdateWifiModuleAsync"] --> B["CheckStatusAsync"] --> E["WifiFirmwareStatus"] --> F{"Skip flash?"}
  B --> C["Device chip info"]
  B --> D["GitHub release lookup"]
  F -->|"Yes"| H["Complete (no flash)"]
  F -->|"No"| G["WINC flash tool"] --> H

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Skip flashing on LatestReleaseUnavailable unconditionally

• ➕ Simplest behavior change; minimal new API surface
• ➕ Avoids reflashing on network outage
• ➖ May leave truly unsupported/outdated modules unflashed when offline
• ➖ Loses a clear policy boundary for manufacturing/field checks

2. Cache the last known latest-release version locally

• ➕ Avoids GitHub dependency during intermittent outages
• ➕ Preserves 'newest available' semantics when cache is fresh
• ➖ Cache staleness can reintroduce incorrect decisions
• ➖ Requires cache invalidation strategy and persistence decisions

3. Bundle a recommended/required WiFi version with shipped firmware artifacts

• ➕ Eliminates network dependency while keeping a 'current' target
• ➕ Version policy can be updated with firmware bundle releases
• ➖ Couples policy to distribution pipeline and artifact management
• ➖ Still needs a separate notion of minimum support vs newest

Recommendation: The PR’s approach is the best tradeoff: a minimum-supported version is a stable firmware-contract policy that can be answered offline, while still allowing flashing to proceed when the device is below the bar or unreadable (null verdict). Keeping IsUpToDate tied to the latest-release lookup preserves existing semantics and avoids silently redefining current behavior for callers that truly want the newest.

Files changed (4) +522 / -3

Enhancement (2) +72 / -0

FirmwareUpdateServiceOptions.csIntroduce configurable minimum supported WINC firmware version with validation +37/-0

Introduce configurable minimum supported WINC firmware version with validation

• Adds DefaultMinimumSupportedWifiFirmwareVersion (19.7.7) and a MinimumSupportedWifiFirmwareVersion option used to judge offline support. Extends Validate() to throw when the configured minimum is not parseable, preventing silent degradation back to an unknown verdict.

src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.cs

WifiFirmwareStatus.csExpose minimum-supported version and three-state support verdict in WiFi status +35/-0

Expose minimum-supported version and three-state support verdict in WiFi status

• Adds MinimumSupportedVersion and MeetsMinimumSupportedVersion (bool?) to distinguish supported, unsupported, and unknown outcomes. Documents the distinction between IsUpToDate (latest-release dependent) and the network-independent minimum-supported check.

src/Daqifi.Core/Firmware/WifiFirmwareStatus.cs

Bug fix (1) +53 / -2

WifiModuleUpdater.csCompute minimum-supported verdict early and skip flash when only GitHub lookup failed +53/-2

Compute minimum-supported verdict early and skip flash when only GitHub lookup failed

• Parses the configured minimum supported version up front and carries it through every status return path. Computes a three-state minimum-supported verdict from device-reported firmware before attempting the latest-release lookup, so the verdict survives lookup failures. Updates the version gate to treat MeetsMinimumSupportedVersion == true as sufficient to skip flashing when the latest-release verdict is unavailable.

src/Daqifi.Core/Firmware/WifiModuleUpdater.cs

Tests (1) +397 / -1

FirmwareUpdateServiceTests.csAdd coverage for minimum-supported WiFi verdict and release-lookup failure paths +397/-1

Add coverage for minimum-supported WiFi verdict and release-lookup failure paths

• Adds tests ensuring UpdateWifiModuleAsync skips flashing when GitHub lookup fails but the device meets the minimum supported version. Adds regression tests verifying flashing still occurs when the device is below the minimum or chip info is unavailable, plus checks for default/validation behavior and status fields across all status reasons. Extends the FakeFirmwareDownloadService to simulate both null-return and exception-throwing release lookup failures.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

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

Previous review results

Review updated until commit e5c41a3 ⚖️ Balanced

Results up to commit b92809e ⚖️ Balanced

No changes from previous review

Results up to commit 0b17b1f ⚖️ Balanced

No changes from previous review

[tylerkron]

Ready for review: Qodo review came back clean (0 bugs, 0 rule violations, 0 requirement gaps, no unresolved threads) and CI build is green on net9 + net10. Not merging — awaiting your review.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Ready for review (supersedes the earlier note, which pinned 0bd9337).

This PR was BEHIND main and therefore not mergeable as it stood — the ruleset requires branches to be up to date, so you would have hit a required update before you could merge it. Brought it up to date with main via gh pr update-branch; the state is now BLOCKED, i.e. merge-ready pending your approval.

The merge was checked for conflict first: main's only new commit (cf5e757, the #435 frame-decode extraction) touches Device/*, this PR touches Firmware/* — zero file overlap, and GitHub reported MERGEABLE.

Current head 0b17b1f:

• Qodo clean — Bugs (0) / Rule violations (0) / Requirement gaps (0), 0 unresolved threads, re-reviewed against this SHA.
• CI build green on net9 + net10.
• Full suite re-run locally on the merged tree (which had never been built before this): 2,581 passed, 2 skipped on each TFM, plus Daqifi.Mcp.Tests 23, 0 warnings.
• Bench re-validated on the real Nq1 over USB, non-destructive (connect/stream/disconnect only; no NVM write, no reboot, no SD) — main's StreamFrameDecoder refactor and this PR's firmware changes had never run together on hardware. Example CLI built against the merged core: 3 connect → populate → stream → disconnect cycles, all exit 0, each reporting analogIn=16 digital=16 fw=3.7.2, 30 CSV rows each at 20 Hz × 2 s — identical run to run, no wedge across repeated open/close of the port.

The PR's own change was not benched, deliberately: it is about the GitHub release lookup failing — a host-side network condition, not a device one — and the surrounding WINC paths can only be exercised by a destructive flash, so hardware could not observe it either way.

The only new commit since 0bd9337 besides the merge is a SESSION_LOG.md entry; source and tests are unchanged. Not merging — awaiting your review.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Ready for review (supersedes the earlier note, which pinned 0b17b1f).

This PR had gone CONFLICTING again — the previous update brought it up to cf5e757, then #437 merged at 23:40, three minutes after that note was posted. The ruleset requires branches to be up to date, so you would have been blocked here.

Diagnosed before anything was touched: git merge-tree --write-tree showed the only conflicting path is SESSION_LOG.md, an append collision. Zero source overlap — this PR touches Firmware/*, #437 landed Device/DaqifiDevice.cs + Device/Internal/LifecycleGate.cs.

Resolved chronologically, keeping both entry blocks in full (this branch's entry is from the 23:37 fire, after main's last #437 note at 23:28, so it sorts last). Verified lossless mechanically rather than by eye: every non-blank line of both parents is still present — 0 missing against each. Merged-vs-branch shows exactly main's LifecycleGate delta and nothing else; merged-vs-main shows exactly this PR's own Firmware/* files and nothing else.

Current head e5c41a3:

• Qodo clean — 0 unresolved threads, re-reviewed against this SHA.
• CI build green on net9 + net10.
• Full suite re-run locally on the merged tree, which had never been built before this: 2,606 passed, 2 skipped on each TFM, plus Daqifi.Mcp.Tests 23. Release solution build 0 warnings both TFMs.
• Bench: 2 non-destructive cycles on the real Nq1 (connect → status → stream → disconnect; no NVM write, no reboot, no SD), example CLI built against the merged core — both exit 0, analogIn=16 digital=16 fw=3.7.2, 30 CSV rows at 20 Hz x 2 s, output comparable to the refactor(device): extract device administration into a collaborator (part of #344) #436 merged tree run on the same board. This checks that the merged tree runs, not this PR's change.
• The PR's own change is still deliberately unbenched, for the same reason as before: it is about the GitHub release lookup failing, a host-side network condition rather than a device one, and the surrounding WINC paths can only be exercised by a destructive flash. Hardware cannot observe it either way.

Only the merge commit is new since 0b17b1f; source and tests are unchanged.

mergeable: MERGEABLE, state BLOCKED — merge-ready pending your approval.

One thing worth your attention: this is the second time this PR has been unblocked from a SESSION_LOG.md conflict, and whichever of #434/#436 you merge first will conflict the other on that same file again. Qodo flagged the pattern independently on #436. The fix is structural (per-fire journal files, or journal updates on main only) and needs a window when no open PR has a SESSION_LOG.md delta — flagging it as your scheduling call rather than changing the loop's own convention unilaterally. Not merging — awaiting your review.