fix(api): retire four downstream workarounds — SD stall typing, empty-transfer guard, ResetAll semantics, Verifying overload (closes #398)

[tylerkron]

Retires all four gaps from the tracking issue in one PR (they are small and unrelated except in provenance). All line numbers in the issue were verified at v1.3.0; everything here was re-located on current main.

closes #398

---

Gap 1 — SD download stalls are typed

The three bare TimeoutException throw sites in SdCardFileReceiver now throw SdCardTransferStalledException, a new member of the existing SdCardOperationException hierarchy, carrying FileName, BytesReceived, Timeout, and a Reason:

Reason	Meaning
NoDataReceived	The read returned zero bytes while the stream still reports itself readable.
TransportClosed	The stream is no longer readable — retrying on it cannot succeed.
TransferTimeout	The overall transfer deadline elapsed.

The old message asserted "Transport stream closed before receiving the EOF marker" for any zero-byte read, which is wrong on the transport it most often fires on. That claim is gone.

One honest caveat, established on the bench (see below): on macOS, SerialStream.ReadAsync does not return 0 on ReadTimeout — it blocks straight past it. So TransferTimeout is the reason that fires on this platform, and NoDataReceived covers the transports where a zero-length read really is what you get. The enum doc says so rather than over-claiming, because a zero-length read on a stream-oriented network transport can also mean the peer closed the connection and the stream gives no way to tell. In every case the consumer action is the same — the download did not complete — which is the point of typing it.

Gap 2 — SdCardEmptyTransferException can honor its own precondition

Its XML doc always claimed it is "never a valid download for a file the directory listing reports as non-empty", but the receiver had no access to the listed size, so a genuinely 0-byte file (routinely left on a FAT card by an interrupted logging session) raised the identical exception as a wedged SD subsystem — and told the user to power-cycle.

The listed size is now plumbed through:

• Firmware already emits "<path> <size>" per listing entry; the parser was discarding the size. SdCardFileInfo gains SizeInBytes (long?) and SdCardFileListParser retains it. Anything that is not a plain non-negative integer parses as null ("unknown") rather than a guess.
• SdCardFileReceiver.ReceiveAsync gains an optional listedFileSizeBytes. A marker-only transfer throws only when the listing said the file was non-empty; a listed 0-byte file returns 0 bytes as a legitimate empty download.
• DownloadSdCardFileAsync looks the size up from the last GetSdCardFilesAsync listing. With no listing available the conservative DownloadSdCardFileAsync silently returns a 0-byte success on an empty (marker-only) transfer #264 behavior is unchanged (throw, so the existing GET retry still covers a wedged subsystem). An ambiguous name — the listing keeps only leaf names, so the same name can appear twice from different directories — is also treated as unknown, because an over-confident size would wave through the very failure the guard exists to catch.
• The exception exposes ListedSizeInBytes so the distinction is visible even in the throwing case.

Gap 3 — ResetAll() no longer discards device clock configuration

ResetAll() cleared _deviceTickPeriods alongside session baselines, and the loss was silent: GetTickPeriod falls back to the 50 MHz default while this firmware clocks at 42 MHz, scaling every reconstructed timestamp by ~1.19 with no exception, log, or warning. Reset(deviceId) has always preserved frequencies; ResetAll() now matches — frequency is static device configuration, not session state. It still clears every session baseline, which is its actual job. SetTimestampFrequency(deviceId, 0) remains the explicit way to drop one.

That alone retires the consumer-side "have I applied it yet" gate flag that daqifi-desktop and daqifi-avalonia independently invented. The fallback is also made observable, so the failure stops being invisible:

• ITimestampProcessor.HasTimestampFrequency(deviceId) — check up front. (GetTickPeriod alone cannot tell an unconfigured device from one that genuinely reports the fallback frequency.)
• TimestampResult.UsedFallbackTickPeriod — reported per message, read from the same lookup that chose the tick period so the two cannot disagree.

Per the settled convention on #80, local time and the omitted rollover +1 are unchanged and deliberately untouched.

Gap 4 — FirmwareUpdateState.Verifying no longer covers two opposite severities

The post-WINC-flash serial reconnect gets its own state, FirmwareUpdateState.ReconnectingAfterFlash, so Verifying now means only the PIC32 flash CRC check. BuildRecoveryGuidance keys on the same discriminator, so a WiFi flash that succeeded completely stops telling the user:

Flash verification failed — the device's flash CRC did not match the firmware image…

and instead says the firmware flashed and verified fine and only the reconnect timed out. Consumers discriminate on FailedState — a structural value — instead of on Operation, which was only ever the human-readable UI progress string. The step keeps its previous timeout budget (VerifyingTimeout), so a host that tuned that for a slow re-enumeration keeps the tuning.

---

Behavior changes (deliberate — please weigh these)

1. The SD download path throws SdCardTransferStalledException where it previously threw TimeoutException. These are unrelated types; a consumer catching TimeoutException on this path stops matching. The issue asked for the new type to live in the SdCardOperationException hierarchy, so this is unavoidable.
2. The WiFi update flow reports ReconnectingAfterFlash where it previously reported Verifying in both StateChanged and progress events. A UI mapping states to labels needs the new case.
3. ResetAll() no longer clears device frequencies. Anyone relying on it as a full wipe should call SetTimestampFrequency(id, 0).
4. A listed 0-byte file now downloads successfully instead of throwing.
5. Additive-but-recompile-requiring: a new member on ITimestampProcessor (breaking for any external implementer of the interface), a new optional parameter on SdCardFileInfo/TimestampResult/SdCardEmptyTransferException constructors and on ReceiveAsync, and a new FirmwareUpdateState enum member.

docs/ and the MCP server (src/Daqifi.Mcp) were checked — neither references the changed exception types, reset semantics, or firmware states, so no updates were needed.

Tests

23 new xUnit tests across the four gaps. Full suite green on net9.0 and net10.0: 1963 passed, 2 skipped (the two pre-existing real-hardware transport tests), 0 warnings.

Bench verification

DAQiFi Nyquist 1, firmware 3.7.2, /dev/cu.usbmodem1101. Non-destructive only — no flash, no SD:FORmat/delete, no reboot, no WiFi reconfiguration. Example CLI and a throwaway probe both built against this branch's Core.

• Gap 2 — validated against real firmware. SD:LIS? returned 31 files, 31/31 with a parsed size, including exactly one genuine zero-byte file (log_20260728_190448.bin). That is precisely the file that used to raise "the device's SD subsystem may not be ready; retry or power-cycle the device". The device also reported timestamp_freq = 42000000 Hz, the value gap 3 exists to protect.
• Gap 1 — validated on a real SerialStream. Pointing SdCardFileReceiver at a live SerialPort.BaseStream that receives nothing produces SdCardTransferStalledException, Reason=TransferTimeout, is SdCardOperationException=True, is TimeoutException=False. This run is also what established the macOS ReadAsync behavior noted above: it blocked past the 500 ms ReadTimeout and only ended at the 3 s transfer deadline.
• Gap 3 — not bench-validated. The board produced 0 stream samples on every attempt, including via the unmodified example CLI at 100 Hz on channels 0+1 and via a probe that enabled an analog channel and sent SYSTem:POWer:STATe 1 first. A device-state condition; the documented fix is a power-cycle/reboot, which is prohibited here. Unit tests cover the reset semantics and the fallback signal.
• Gap 4 — not bench-testable non-destructively (it needs a real firmware flash). Unit tests only, including a WiFi-flash run whose post-flash reconnect times out and which asserts both the state and the corrected guidance.
• Note for reliability: SD download against a wedged card hangs indefinitely and ignores cancellation — no consumer-side watchdog can rescue it #399: SD GET never returned data on this board for any file, including the 0-byte one and a 186-byte one — the read blocked until my own 25 s probe leash. This is the known low-heap SD condition, not a regression from this PR (the read loop is unchanged apart from the exception type, and the stock CLI on this branch fails identically). But it does show Core's hardcoded 30-minute deadline is the only bound on that path in practice, which is exactly what that ticket owns.

Siblings

Deliberately stayed in lane; textual conflicts with these are expected, semantic overlap should not exist:

• reliability: SD download against a wedged card hangs indefinitely and ignores cancellation — no consumer-side watchdog can rescue it #399 owns hang/timeout/cancellation bounding on the SD download path. This PR adds no deadlines — only exception typing. (See the note above, which is evidence for that ticket.)
• api: connectivity guards throw untyped InvalidOperationException, forcing clients to message-match #395 owns the untyped InvalidOperationException connectivity guards. Gap 1 is a distinct site inside SdCardFileReceiver that api: connectivity guards throw untyped InvalidOperationException, forcing clients to message-match #395 does not cover.
• api: a timed-out SD LIST is indistinguishable from an empty SD card #396 owns the SD LIST timeout-vs-empty ambiguity. Gap 2 is the GET sibling of it; this PR keeps to the GET path, though it does add a size field to the shared listing parser.

Retiring these lets the linked desktop workarounds be simplified or deleted: daqifi-desktop#793 (gaps 1–2), #794 (gap 3), #790 (gap 4).

Not merging — opened for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix: type SD stalls, honor listed sizes, preserve timestamp freq, split WiFi reconnect state

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Replace SD download timeout/string matching with a typed stalled-transfer exception and reasons.
• Plumb SD listing sizes through parsing and downloads to legitimize true 0-byte files.
• Preserve per-device timestamp frequencies across ResetAll and expose fallback tick-period usage.
• Split WiFi post-flash reconnect into its own firmware update state with correct failure guidance.

Diagram

graph TD
  A["Consumer app"] --> B["DaqifiStreamingDevice"] --> C["SdCardFileReceiver"] --> D["SdCardTransferStalledException"]
  B --> E["SdCardFileListParser"] --> F["SdCardFileInfo(SizeInBytes)"]
  A --> G["TimestampProcessor"] --> H["TimestampResult(UsedFallback)"]
  A --> I["FirmwareUpdateService"] --> J["FirmwareUpdateState(ReconnectingAfterFlash)"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Model SD download outcomes as a result type (no exceptions)

• ➕ Avoids exception-driven control flow for expected stalls/retries
• ➕ Encodes partial progress and retryability explicitly (e.g., union of Completed/Empty/Stalled/Closed/TimedOut)
• ➖ Breaking API shift for existing consumers that already catch SdCardOperationException
• ➖ More plumbing across async call chains; exceptions are still useful for unrecoverable states

2. Keep FirmwareUpdateState unchanged; add a sub-reason to FirmwareUpdateException

• ➕ Avoids expanding the public enum and potential downstream switch updates
• ➕ Keeps progress UI states stable while still enabling correct recovery guidance
• ➖ Progress/event streams still lack a structural discriminator unless consumers parse exception details
• ➖ State machine remains semantically overloaded (same problem, different location)

3. On missing timestamp frequency, log/throw instead of adding a fallback flag

• ➕ Forces configuration correctness; prevents silent-but-wrong timestamps
• ➕ Simplifies consumer logic (no need to inspect UsedFallbackTickPeriod)
• ➖ Too disruptive for existing integrations that rely on default behavior
• ➖ Requires a logging/telemetry contract and/or a new exception surface

Recommendation: The PR’s direction is the best incremental fix: it preserves backward-compatible behavior where needed (conservative empty-transfer handling when size is unknown, timestamp fallback still available) while adding strong structural signals (typed stall reasons, UsedFallbackTickPeriod, dedicated firmware reconnect state) that let consumers stop matching on strings or compensating for silent defaults. The alternatives either impose larger breaking API shifts (result types / throwing on missing frequency) or keep semantic overload in place (exception sub-reason without a state split).

Files changed (19) +1083 / -57

Enhancement (7) +265 / -13

ITimestampProcessor.csAdd HasTimestampFrequency and clarify ResetAll contract +23/-3

Add HasTimestampFrequency and clarify ResetAll contract

• Introduces HasTimestampFrequency to make silent fallback detectable and updates ResetAll docs to specify it clears session baselines while preserving configured device frequencies.

src/Daqifi.Core/Device/ITimestampProcessor.cs

SdCardEmptyTransferException.csInclude listed size in empty-transfer exception and message +41/-4

Include listed size in empty-transfer exception and message

• Adds ListedSizeInBytes and updates messaging to incorporate listing evidence when available, while documenting the ambiguity between true empty files and wedged transfers when size is unknown.

src/Daqifi.Core/Device/SdCard/SdCardEmptyTransferException.cs

SdCardFileInfo.csExtend SD file info with nullable SizeInBytes +23/-1

Extend SD file info with nullable SizeInBytes

• Adds SizeInBytes (long?) to carry listing-reported file size, and validates that provided sizes are non-negative to prevent invalid metadata from propagating.

src/Daqifi.Core/Device/SdCard/SdCardFileInfo.cs

SdCardTransferStallReason.csAdd enum for SD stall classification +36/-0

Add enum for SD stall classification

• Introduces SdCardTransferStallReason with NoDataReceived, TransportClosed, and TransferTimeout, documenting platform/transport nuances (notably SerialStream.ReadAsync returning 0 on timeout).

src/Daqifi.Core/Device/SdCard/SdCardTransferStallReason.cs

SdCardTransferStalledException.csAdd typed SD stalled-transfer exception in SdCardOperationException hierarchy +99/-0

Add typed SD stalled-transfer exception in SdCardOperationException hierarchy

• Adds SdCardTransferStalledException carrying FileName, BytesReceived, Reason, and optional Timeout, with reason-specific messages and inner exception propagation for timeout cancellations.

src/Daqifi.Core/Device/SdCard/SdCardTransferStalledException.cs

TimestampResult.csAdd UsedFallbackTickPeriod to timestamp results +24/-3

Add UsedFallbackTickPeriod to timestamp results

• Extends TimestampResult (and CreateFirstMessage) to include a UsedFallbackTickPeriod boolean so consumers can detect when default tick period was applied due to missing device configuration.

src/Daqifi.Core/Device/TimestampResult.cs

FirmwareUpdateState.csAdd ReconnectingAfterFlash and clarify Verifying semantics +19/-2

Add ReconnectingAfterFlash and clarify Verifying semantics

• Refines Verifying documentation to mean only true flash verification, and adds a new ReconnectingAfterFlash enum value explicitly described as a benign/environmental failure mode distinct from bad flash CRC.

src/Daqifi.Core/Firmware/FirmwareUpdateState.cs

Bug fix (5) +200 / -38

DaqifiStreamingDevice.csPass listed SD file sizes into downloads to distinguish true empty files +54/-7

Pass listed SD file sizes into downloads to distinguish true empty files

• Enhances DownloadSdCardFileAsync docs and behavior by looking up the last SD listing’s SizeInBytes and passing it to SdCardFileReceiver. Adds TryGetListedFileSize with case-insensitive matching and ambiguity detection (duplicate leaf names => unknown).

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

SdCardFileListParser.csParse and retain SD listing size token +29/-2

Parse and retain SD listing size token

• Keeps the firmware-emitted size token after the path, parsing only plain non-negative integers and returning null for malformed/extra columns. Constructs SdCardFileInfo with the parsed SizeInBytes.

src/Daqifi.Core/Device/SdCard/SdCardFileListParser.cs

SdCardFileReceiver.csType SD transfer stalls and gate empty-transfer exception by listed size +64/-21

Type SD transfer stalls and gate empty-transfer exception by listed size

• Replaces TimeoutException throw sites with SdCardTransferStalledException and classifies zero-byte reads as NoDataReceived vs TransportClosed based on stream readability. Adds listedFileSizeBytes to treat marker-only transfers as legitimate only when listing reports size=0, otherwise throw SdCardEmptyTransferException including the listed size.

src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs

TimestampProcessor.csPreserve configured frequencies on ResetAll and surface fallback usage +32/-5

Preserve configured frequencies on ResetAll and surface fallback usage

• Implements HasTimestampFrequency and updates ProcessTimestamp to compute a consistent UsedFallbackTickPeriod flag alongside the tick period used. Changes ResetAll to clear only session baselines (_deviceStates) while preserving per-device tick periods.

src/Daqifi.Core/Device/TimestampProcessor.cs

FirmwareUpdateService.csIntroduce ReconnectingAfterFlash state for WiFi post-flash reconnect +21/-3

Introduce ReconnectingAfterFlash state for WiFi post-flash reconnect

• Transitions the post-WiFi-flash reconnect step out of Verifying into a dedicated ReconnectingAfterFlash state, updates allowed state transitions, and adds distinct recovery guidance for reconnect timeouts versus CRC verification failures.

src/Daqifi.Core/Firmware/FirmwareUpdateService.cs

Tests (5) +604 / -6

SdCardFileListParserTests.csAdd coverage for parsing SD listing size tokens +78/-0

Add coverage for parsing SD listing size tokens

• Adds tests asserting that size tokens (including tab-separated and zero) are parsed into SizeInBytes, and that malformed/extra/overflow tokens yield null rather than guessed values.

src/Daqifi.Core.Tests/Device/SdCard/SdCardFileListParserTests.cs

SdCardFileReceiverTests.csUpdate SD receiver tests for typed stalls and size-aware empty transfers +223/-4

Update SD receiver tests for typed stalls and size-aware empty transfers

• Replaces TimeoutException expectations with SdCardTransferStalledException and verifies stall reasons (NoDataReceived, TransportClosed, TransferTimeout). Adds extensive tests for marker-only transfers gated by listed size and introduces a ClosableStream test helper.

src/Daqifi.Core.Tests/Device/SdCard/SdCardFileReceiverTests.cs

SdCardOperationsTests.csTest size-informed download behavior and listing propagation +111/-1

Test size-informed download behavior and listing propagation

• Adds tests ensuring listed 0-byte files download without retries, listed non-empty marker-only transfers retry then throw with ListedSizeInBytes, and ambiguous/no prior listing falls back to conservative behavior. Updates the test device stub to return seeded listing lines.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs

TimestampProcessorTests.csExpand ResetAll semantics and fallback observability tests +118/-1

Expand ResetAll semantics and fallback observability tests

• Updates ResetAll tests to require preserving timestamp frequencies and adds new tests for clearing session baselines, HasTimestampFrequency semantics, and UsedFallbackTickPeriod reporting during processing.

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

FirmwareUpdateServiceTests.csAssert post-flash reconnect uses new firmware update state +74/-0

Assert post-flash reconnect uses new firmware update state

• Adds tests verifying successful WiFi updates emit ReconnectingAfterFlash and do not misuse Verifying, and that a post-flash reconnect timeout fails with FailedState=ReconnectingAfterFlash and appropriate recovery guidance.

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

Documentation (1) +10 / -0

ISdCardOperations.csDocument new SD download exception semantics +10/-0

Document new SD download exception semantics

• Updates DownloadSdCardFileAsync XML docs to describe size-aware SdCardEmptyTransferException behavior and the new SdCardTransferStalledException with reason discrimination.

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs

Other (1) +4 / -0

FirmwareUpdateServiceOptions.csMap ReconnectingAfterFlash timeout to existing Verifying budget +4/-0

Map ReconnectingAfterFlash timeout to existing Verifying budget

• Adds state-timeout mapping so ReconnectingAfterFlash uses VerifyingTimeout, preserving existing host tuning for slow serial re-enumeration.

src/Daqifi.Core/Firmware/FirmwareUpdateServiceOptions.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. ReceiveAsync breaks positional calls ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

SdCardFileReceiver.ReceiveAsync inserts listedFileSizeBytes before the existing CancellationToken
parameter, so any downstream code that previously passed a CancellationToken positionally as the 5th
argument will no longer compile/bind. Because this is a public API, the parameter should be appended
(or the old signature preserved) to avoid breaking callers unnecessarily.

Code

src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs[R78-85]

    public async Task<long> ReceiveAsync(
        Stream destinationStream,
        string fileName,
        IProgress<SdCardTransferProgress>? progress = null,
        TimeSpan? timeout = null,
+        long? listedFileSizeBytes = null,
        CancellationToken cancellationToken = default)
    {

Relevance

●●● Strong

Team previously accepted keeping CancellationToken positional to avoid source-breaking API changes.

PR-#198

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The PR changes the public ReceiveAsync signature to include a new parameter (listedFileSizeBytes)
before CancellationToken, which breaks any existing positional invocation that supplied a
CancellationToken as the last argument under the prior signature.

src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs[78-85]

Agent prompt

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

## Issue description
`SdCardFileReceiver.ReceiveAsync(...)` added `listedFileSizeBytes` *before* `CancellationToken cancellationToken`. This breaks existing positional calls that used the previous signature shape `(dest, fileName, progress, timeout, cancellationToken)` and also changes the method’s runtime signature.

## Issue Context
This is a public method on a public type (`SdCardFileReceiver`), so callers outside this repo may be impacted.

## Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/SdCardFileReceiver.cs[78-85]

### Suggested fix
Reorder parameters so the new optional argument is appended after `CancellationToken` (keeping `CancellationToken` effectively “in the same position” for positional callers), e.g.:
- `..., TimeSpan? timeout = null, CancellationToken cancellationToken = default, long? listedFileSizeBytes = null`

Then update internal call sites (they already use named arguments, so this should be low-impact).

ⓘ 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 2b906c5