fix(sd): make SD card operations work over the TCP/WiFi transport (closes #327) - #420
Conversation
…oses #327) The SD path was written for USB serial and carried three assumptions that a socket does not satisfy: - A file transfer waited for silence that a socket never reports. Serial has a per-read timeout; NetworkStream.ReadAsync ignores Socket.ReceiveTimeout and simply waits, so a device that stopped answering mid-file parked the download for its full 30-minute budget. The receiver now has an inactivity window (20 s) and gives up with an accurate, typed failure. - A zero-length read was classified by asking the stream whether it was still readable. NetworkStream.CanRead stays true after the peer's FIN, so a closed connection was reported as merely quiet. The caller, which knows its transport, now settles it. - Stopping an SD logging session re-enabled the LAN unconditionally. Over WiFi that re-initializes the module and drops the link the command arrived on, so it is now USB-only, mirroring PrepareLanInterface. Also aligns the download's interface-settle wait with the constant the other SD exchanges use, and corrects the docs and the logging-start message, which still claimed SD operations were impossible over a network connection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoFix SD downloads over TCP/WiFi with transport-aware stalls and idle timeout
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…nd 1) Adding optional parameters to the existing public (Stream, int) constructor changed its CLR signature, so a downstream app that upgrades the Daqifi.Core package without recompiling would hit MissingMethodException. The original constructor is restored verbatim and delegates to a new overload that carries the transport semantics. A reflection test pins the old signature; verified by mutation that it fails when the constructor is widened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 1 addressed. 1 — binary-breaking constructor: fixed (cf96926). Correct catch. The original 2 — per-read CTS allocation: keeping it, with reasoning on the thread. Reusing one CTS needs recreate-on-race handling because a cancelled CTS cannot be un-cancelled — a byte arriving exactly as the window closes would leave every later read failing instantly. Per-iteration allocation makes that unrepresentable, and it sits next to a socket read and a stream write that dominate it. The bench file was 2,551 bytes, i.e. three iterations. Full suite green: 2,355 tests on net9.0 and net10.0. /agentic_review |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit cf96926 |
…ture (Qodo round 2) Restoring the (Stream, int) constructor bought binary compatibility, but it delegated with idleTimeout: null, which resolves to the new 20-second default. That switched on an inactivity window for exactly the pre-compiled consumers the overload exists to protect: any transport that goes quiet without returning zero bytes, and honors cancellation, would start being abandoned mid-transfer where it used to run to the caller's own deadline. A silent behavior change is worse than the loud binary break the overload prevents. The legacy ctor now passes Timeout.InfiniteTimeSpan, restoring pre-PR semantics exactly. The inactivity window is opt-in on the new overload, which is where DaqifiStreamingDevice asks for it on both transports. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Round 2 addressed. 1 — legacy ctor enabled the idle timeout: fixed (4c7d8e6). Correct and worth naming precisely: restoring the CLR signature bought binary compatibility, then delegating with This does not weaken the PR: Two tests, and I'll be straight about what each buys: the behavioral one pins that the caller's deadline is still what ends a quiet transfer; the state one pins that the window is off at all. Mutation-verified by reverting the delegation — only the state test failed, because the behavioral test can't rule out a window longer than its own 400 ms deadline and the default is 20 s. So the state assertion is the one doing the catching. 2 — per-read CTS allocation: unchanged, restated on the thread. Reusing a CTS needs recreate-on-race handling since a cancelled CTS can't be un-cancelled; per-iteration allocation makes that unrepresentable. This PR's own review summary agrees, calling it "a reasonable tradeoff for correctness and clarity." Recording it as a documented disagreement rather than churning working code. Suite green: 2,357 tests on net9.0 and net10.0. No bench work this round — hardware is unattended. /agentic_review |
1 participant
Why
The firmware has served SD card listings and file reads over WiFi since v3.7.0, and desktop apps want to offer SD offload without asking people to plug in a USB cable. Issue #327 asked us to confirm Core can do that.
Most of it already could. The interface prep and the firmware-version gate were made transport-aware in earlier work. What was left was the part nobody had run over a network: the file transfer itself, which was written against a serial port and quietly assumed one.
What
Three things a socket does differently from a serial port, each of which the download got wrong:
A stalled transfer used to hang for half an hour. A serial port has a per-read timeout, so if the device goes quiet the read comes back empty within half a second and we notice. A socket has nothing like that —
NetworkStream.ReadAsyncignores the receive timeout and just waits. So a device that stopped part-way through a file left the download parked until the caller's whole 30-minute budget ran out. The receiver now watches for inactivity and gives up after 20 seconds with a message that says what happened and how many bytes made it.A closed connection was reported as "the device is just quiet". The old code asked the stream whether it was still readable and inferred the rest. A socket keeps reporting itself readable after the other end hangs up, so a dropped connection looked retryable when it wasn't. The download now tells the receiver which transport it is on, because that is the only place the answer exists.
Stopping an SD logging session cut the WiFi link. It re-enabled the LAN interface unconditionally, which restarts the WiFi module. Over USB that is correct and necessary; over WiFi it drops the very connection the command arrived on. It is now USB-only, matching what the interface-restore helper already did.
Plus some tidying found along the way: the download's settle wait now uses the same constant as the other SD operations instead of its own shorter one, and the docs and the logging-start error no longer claim SD operations are impossible over a network.
Nothing public was removed and no existing signature changed.
SdCardFileReceiverkeeps its original(Stream, int)constructor untouched — it now delegates to a new overload that carries the transport semantics. That distinction matters because Core ships as a NuGet package: adding optional parameters to the existing constructor would have kept every source caller compiling while breaking already-built consumers at runtime. A reflection test pins the old signature, mutation-verified to fail if it is ever widened.How it was checked
16 new unit tests cover the network cases: 1024-byte chunked delivery, the end-of-file terminator split at every offset of a chunk boundary, throttled delivery, the inactivity window, the zero-length-read classification on both transports, and a full download over a simulated WiFi device. Full suite green — 2,354 tests on net9.0 and net10.0.
Bench: Nyquist 1, firmware 3.7.2
Over USB — everything works, byte for byte.
sha256[0:16]=9DD0AE41B7FCF5C4, 0.63 sOver WiFi (192.168.1.30) — the transport works; this firmware build does not finish the job.
The truncation and the starved transfer are both the known firmware limitation on 3.7.2: the TCP write buffer can shrink to ~1400 B after a streaming session re-partitions the pool, and the SD reply writer gives up on a chunk rather than reporting it. Both are fixed on the firmware main branch after this release (#748 bounds the drains by the runtime buffer size, #750 makes the reply timeout terminal). They are not something Core can paper over.
What this PR does change about that experience: before, the starved WiFi download was a 30-minute freeze with nothing to show for it. Now it is a 20-second, clearly worded failure that reports the 51 bytes it did receive. That is what makes the firmware problem visible instead of looking like a hung app.
Answering the issue's fourth question
Sequencing while a WiFi stream is or was active is already correct and needs no
SYSTem:STReam:INTerfacehandling. The firmware picks where an SD reply goes from the interface the command arrived on (wifi_tcp_server_ContextIsTcp), not from the stream-interface setting, so the two are independent. All four SD operations already stop streaming first. The end-of-listing probe is also safe over TCP: SCPI replies and SD replies go into the same TCP buffer, and the listing command blocks until the listing has been handed over, so the terminator cannot overtake it.Deliberately left out
FormatSdCardAsynchas no transport gate. It is destructive and untestable on the bench, and it does not prepare the SD interface on either transport today — worth its own look.