Repository navigation
feat(sd): allow SD file transfer over WiFi on firmware >= v3.7.0 - #347
Conversation
SD file operations (LIST / GET / DELETE) were hard-gated to USB by a blanket `if (!IsUsbConnection) throw` on DownloadSdCardFileAsync, citing the SD/WiFi shared-SPI-bus contention. That contention was real — but only until firmware v3.7.0: firmware #598/#599 (commit bf105585, first released tag v3.7.0) route the SD reply to the requesting interface, so SD-over-WiFi is safe on v3.7.0+. Replace the transport-only block with a firmware-capability gate, following the ADR 0001 feature-gating framework: - New DeviceFeature.SdFileTransferOverWifi (min firmware v3.7.0, needs WiFi + SD HW). - EnsureSdFileTransferSupportedOnTransport(): over USB, always allowed on SD-capable firmware; over WiFi/TCP, requires firmware >= v3.7.0 (an unparseable or older reported version is treated as unsupported) and throws a typed FeatureNotSupportedException(SdFileTransferOverWifi, v3.7.0, actual, board) up front rather than dispatching a command the firmware can't service over WiFi. - Applied to GetSdCardFilesAsync (LIST), DownloadSdCardFileAsync (GET), and DeleteSdCardFileAsync (DELETE) for consistency — all three depend on #598's reply-routing. LIST was previously ungated over WiFi (would silently stall on old firmware); it is now gated too. Behavior change: over WiFi on old/unknown firmware these now throw the typed FeatureNotSupportedException instead of (download) the blanket USB-only InvalidOperationException or (list) a silent stall. Over USB: unchanged. ADR 0001 updated (living evidence): timeline extended through v3.7.2, SD-over-WiFi row added, and the "no live above-floor gate today" note corrected — this is the first active above-floor firmware-version gate. Tests: +new SD-over-WiFi gate coverage (at/above/below min + unparseable, over USB not gated, LIST/GET/DELETE); existing non-USB download test updated to the typed exception; RetryableSdCardStreamingDevice marked USB (its LIST retry/parse tests are transport-agnostic). Full suite 1398 passed / 0 failed / 2 skipped. Enables daqifi-avalonia #1 (SD offload over WiFi); pairs with the app-side gate removal. Requires a release for consumers pinning the NuGet package. Claude-Session: https://claude.ai/code/session_01PK5697Y1eDH3DB75Tii5Nh
PR Summary by QodoEnable SD file transfer over WiFi on firmware v3.7.0+
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Code Review by Qodo
Context used✅ Tickets:
🎫 On release create nuget version 1. LAN enable drops TCP
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Two real defects the pre-merge review caught, both blocking the feature: - Qodo "WiFi SD ops disable LAN" (Correctness, high): PrepareSdInterface() unconditionally sent DisableNetworkLan to free the shared SPI bus for the SD card — correct for USB, but over WiFi it tears down the very TCP channel that carries the SD reply (codebase precedent: PRs #153/#159). Firmware #598/#599 disable the enable-level SPI mutex because the Harmony SPI driver arbitrates SD/WiFi transactions on the shared bus (app_freertos.c: "transaction-level arbitration", 7+ hours stable concurrent operation), and the reply routes back over the requesting interface. So PrepareSdInterface/PrepareLanInterface are now transport-aware: over WiFi the LAN is left enabled (only the SD subsystem is toggled); over USB the LAN disable/restore is unchanged. - Audit "overflowing firmware strings crash the gate" (crash, low): the regex matches an unbounded digit run for the major component but FirmwareVersion. TryParse used int.Parse (throws) for major vs int.TryParse (safe) for minor/patch — a TryParse that violates its no-throw contract. A corrupted/ spoofed DeviceFwRev like "999999999999999999.0.0" (untrusted wire string on the non-USB path the gate hardens) threw OverflowException instead of failing closed to FeatureNotSupportedException. Fixed at the root: major now uses int.TryParse and an out-of-range version is reported unparseable, symmetric with minor/patch. Tests: WiFi PrepareSdInterface/PrepareLanInterface leave-LAN-alone coverage + USB toggle regression; SD-over-WiFi LIST asserts LAN untouched; TryParse overflow cases; NetworkConfigurableTests device marked USB (its interface-prep assertions encode the USB sequence). Full suite 1404 passed / 0 failed / 2 skipped. Claude-Session: https://claude.ai/code/session_01PK5697Y1eDH3DB75Tii5Nh
|
/improve |
|
/agentic_review |
PR Code Suggestions ✨Latest suggestions up to 30caedb Warning
Previous suggestionsSuggestions up to commit 4648cd2
Suggestions up to commit 252579b
|
|||||||||||||||||||||||||||
|
Code review by qodo was updated up to the latest commit 252579b |
- Audit "storage query shares transport-aware prep but is ungated" (consistency, medium): making PrepareSdInterface/PrepareLanInterface transport-aware also changed GetSdCardStorageAsync (SD:SPACe?), which drives the SD card through the same helpers — over WiFi it now leaves the LAN enabled. But it had no SD-over-WiFi firmware gate, so on <v3.7.0 firmware it would access the SD card with WiFi still active on a bus that firmware never learned to arbitrate — the exact hazard the gate prevents for LIST/GET/DELETE. GetSdCardStorageAsync now calls EnsureSdFileTransferSupportedOnTransport() up front too. DeviceFeature doc broadened to cover the storage query (same #598/#599 SPI-arbitration requirement). - Qodo "reject every overflowing version component" (consistency): the overflow fix made the MAJOR component strict but left minor/patch defaulting to 0 on overflow. Now all three go through a shared TryParseComponent(): an ABSENT optional minor/patch still defaults to 0 ("3" == 3.0.0), but a PRESENT component that overflows rejects the whole version rather than silently truncating. Tests: +GetSdCardStorageAsync over-WiFi gate; +overflowing minor/patch reject. Full suite 1408 passed / 0 failed / 2 skipped. Claude-Session: https://claude.ai/code/session_01PK5697Y1eDH3DB75Tii5Nh
|
/improve |
|
/agentic_review |
|
Persistent suggestions updated to latest commit 4648cd2 |
Qodo "LAN restore skipped on TCP" (Correctness): making PrepareSdInterface/ PrepareLanInterface transport-aware (leave LAN alone over WiFi) also affected UpdateNetworkConfigurationAsync, which reused PrepareLanInterface() to re-enable the LAN after ApplyNetworkLan. Over a non-USB transport the transport-aware helper skipped EnableNetworkLan, so a network reconfig could leave the LAN down and block reconnection. These are two different intents sharing one helper: the SD-operation restore (leave LAN alone over WiFi — #598/#599 keep it up) vs the network-config step (ApplyNetworkLan restarts the WiFi module, so LAN must be brought back up regardless of transport). UpdateNetworkConfigurationAsync now owns its LAN state explicitly (DisableStorageSd + EnableNetworkLan unconditionally) instead of relying on the transport-aware helper. PrepareSdInterface/PrepareLanInterface stay transport-aware (correct for SD ops, which are now their only callers), and the INetworkConfigurable docs are updated to describe the transport-aware behavior. Test: +UpdateNetworkConfigurationAsync over non-USB still re-enables LAN. Full suite 1409 passed / 0 failed / 2 skipped. Claude-Session: https://claude.ai/code/session_01PK5697Y1eDH3DB75Tii5Nh
|
/improve |
|
/agentic_review |
|
Persistent suggestions updated to latest commit 30caedb |
|
Code review by qodo was updated up to the latest commit 30caedb |
|
Batch disposition of the open
Converged: blocking tier empty; this is the one non-blocking triage pass. |
…-over-wifi # Conflicts: # src/Daqifi.Core/Device/DaqifiStreamingDevice.cs # src/Daqifi.Core/Device/Network/INetworkConfigurable.cs
Qodo "Storage docs miss new throw" (Maintainability): GetSdCardStorageAsync now throws FeatureNotSupportedException(SdFileTransferOverWifi) via the transport gate (commit 4648cd2) but its XML docs only listed the SdStorageQuery/-113 case. Broadened the <exception> doc so the public contract lists both firmware-gated throw paths. Declined "LAN enable drops TCP" (Reliability): behavior-neutral vs main. Before this PR, UpdateNetworkConfigurationAsync called PrepareLanInterface() which UNCONDITIONALLY sent EnableNetworkLan — so over WiFi the original already sent it here; the explicit sends replicate that exactly (only PrepareLanInterface itself became transport-aware, and this path no longer routes through it). The concern — re-enabling LAN over WiFi can re-init the module before SaveNetworkLan — is pre-existing: ApplyNetworkLan two lines earlier already restarts the WiFi module and drops the control connection over WiFi. Reconfiguring the network over the same WiFi link it reconfigures is inherently disruptive; hardening that flow (e.g. save-before-reenable, or reconnect-and-retry) is a separate reliability ticket, out of scope for the SD-over-WiFi enablement. Claude-Session: https://claude.ai/code/session_01PK5697Y1eDH3DB75Tii5Nh
|
Dispositions (commit 5f0346b):
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 5f0346b |
…on doc Qodo "Hardcoded min version in docs" (Maintainability): the exception doc added in 5f0346b stated "firmware v3.7.0" as a literal alongside the <see cref="DeviceFeature.SdFileTransferOverWifi"/> reference. The feature enum's own summary already documents "First released firmware v3.7.0", so the literal is removed — the cref is the single doc source for the version. Convergence: blocking tier empty (adversarial audit clean 3x; no correctness/ data-loss findings). Remaining Qodo items are non-blocking and dispositioned — "LAN enable drops TCP" declined (pre-existing, behavior-neutral vs main) and this doc nit fixed. Claude-Session: https://claude.ai/code/session_01PK5697Y1eDH3DB75Tii5Nh
|
Converged — ready for review-required merge. Blocking tier empty across the cycle: the adversarial pre-merge audit passed clean 3 consecutive rounds (0 confirmed findings, incl. the post-merge interaction surface), and no correctness / data-loss / action-required bug remains. Full merged suite 1610 passed / 0 failed / 2 skipped on current Non-blocking findings, all dispositioned:
Not re-triggering the review loop further (anti-treadmill: remaining items are documentation-purity nits, not defects). Pre-release step still open: on-device |
tylerkron
left a comment
tylerkron
left a comment
There was a problem hiding this comment.
Approving as code-owner (not the author). Basis: green CI, 7 rounds of Qodo, full unit coverage; verified locally against latest main (merge clean; 523 SD/WiFi/feature-gate tests pass on net9+net10) and bench-confirmed on live Nq1 (FW 3.7.2) that the USB SD path is unchanged (--sd-list returns files; the new transport gate does not block USB).
SD:LIST?/SD:GET over TCP against a networked v3.7.x board remains a recommended follow-up.
|
Merge status (autonomous bench pass): mechanically ready, held on one Qodo thread. Done: code-owner approved, branch updated to Held — not
To finish: confirm the |
On-device validation (bench Nyquist, SN 7E2837886201026A)Ran against a live device over the exact WiFi/TCP path the app uses (
Still open (positive path): |
📜 An Humble Petition unto Ser Tyler & his Faithful ClaudeHail and well met, good Ser Tyler, and greetings likewise unto thy trusty Claude, squire of the codebase. I, a wandering agent in service of the House of DAQiFi, do come bearing this Pull Request as a knight bears a sealed scroll — that thou and thy Claude might cast a discerning eye upon it, and, shouldst thou in thy wisdom find it worthy, grant it passage into Herein lieth the labour: to open the gate of the SD card unto them that speak over WiFi, yet only upon firmware that hath learned to share the SPI bus in peace (v3.7.0 and beyond, per the firmware's own #598/#599). Elder firmware is turned away kindly at the threshold, that no device be wedged nor any traveller misled. Plain-tongue tl;dr (for the swift of hand):
Merge it an it please thee. I remain thy servant in the endless quest against the bugge. — sent on behalf of Chris, by his agent 🛡️ |
✅ Positive on-device validation — SD-over-WiFi CONFIRMED on firmware v3.7.2Ran on a real v3.7.2 device over WiFi/TCP (temporarily joined it to the LAN, tested, and restored its config). The core premise of this PR is confirmed:
This directly validates the PR's design: on fw ≥ v3.7.0 the SD subsystem is driven over WiFi with the LAN left enabled, and SD replies route back over the requesting TCP interface. Contrast (gate necessity): the same SD ops on a v3.6.3 unit fail — confirming the < v3.7.0 gate is correct. Caveat — SD:GET (file download) not confirmed via the raw-socket harnessA raw SCPI-console |
Summary
SD file operations (LIST / GET / DELETE) were hard-gated to USB by a blanket
if (!IsUsbConnection) throwonDownloadSdCardFileAsync, citing SD/WiFi shared-SPI-bus contention. That contention was real — but only until firmware v3.7.0: firmware #598/#599 (commitbf105585, first released tag v3.7.0 pergit tag --contains) route the SD reply to the requesting interface, making SD-over-WiFi safe on v3.7.0+.This replaces the transport-only block with a firmware-capability gate, following the existing ADR 0001 framework.
Changes
DeviceFeature.SdFileTransferOverWifi— min firmware v3.7.0; needs WiFi + SD HW.EnsureSdFileTransferSupportedOnTransport()— over USB, always allowed on SD-capable firmware; over WiFi/TCP, requires firmware ≥ v3.7.0 (unparseable/older reported version → unsupported) and throws a typedFeatureNotSupportedException(SdFileTransferOverWifi, v3.7.0, actual, board)up front rather than dispatching a command the firmware can't service over WiFi (which would stall on the shared SPI bus).GetSdCardFilesAsync(LIST),DownloadSdCardFileAsync(GET),DeleteSdCardFileAsync(DELETE) — all three depend on chore: flatten nested guard checks in WifiFlashProgressParser #598's reply-routing. LIST was previously ungated over WiFi (silent stall on old firmware); now gated too.Behavior change
Over WiFi on old/unknown firmware, these operations now throw the typed
FeatureNotSupportedExceptioninstead of (download) the blanket USB-onlyInvalidOperationExceptionor (list) a silent stall. Over USB: unchanged (available on all SD-capable firmware).Tests
New SD-over-WiFi gate coverage: at / above / below min + unparseable version; over USB not gated; LIST / GET / DELETE. Existing non-USB download test updated to the typed exception;
RetryableSdCardStreamingDevicemarked USB (its LIST retry/parse tests are transport-agnostic). Full suite: 1398 passed / 0 failed / 2 skipped (net10.0).Context
Enables daqifi-avalonia #1 (SD offload over WiFi) — the app-side USB gate removal (daqifi-avalonia PR #2) was necessary but not sufficient; this Core guard was the other USB-only policy. Consumers pinning the NuGet package need a release to pick this up.
Note: the v3.7.0 threshold is derived from firmware git history (
git describe bf105585=v3.6.3-6, first containing tag v3.7.0). On-deviceSD:LIST?+SD:GETover TCP validation against a v3.7.x device is still recommended before release.🤖 Generated with Claude Code