fix(sdcard): run the SD bus switch as a prepare phase inside the exchange lock

[tylerkron]

Follow-up to #400 (merged as c2d5da8). This fix was ready minutes after that merge landed, so it needs its own PR.

The problem this fixes

#400 went through several review rounds on where the SD card's SPI bus switch should happen relative to the text exchange. The last move — hoisting PrepareSdInterface() and its settle delay above ExecuteTextCommandAsync — closed a stale-terminator window but opened a different one, and that is what merged.

ExecuteTextCommandCoreAsync runs its setup action while holding the device-wide _textExchangeLock. The bus switch used to run inside that lock, because it lived inside the setup action. Hoisting it moved it outside:

thread A: PrepareSdInterface()                 <- outside the lock now
thread B: acquires _textExchangeLock, runs some other text exchange,
          whose finally calls PrepareLanInterface()
thread A: acquires the lock, sends LIST        <- now running against LAN interface state

The device answers a LIST it cannot service, and the listing fails or retries for no reason. Concurrent text operations are an expected scenario — _textExchangeLock exists precisely because of #186 — so this is reachable, not theoretical.

Why not just move it back

Moving the switch back inside the setup action reinstates the window #400 spent two rounds closing: the settle wait between the switch and the LIST sits after the exchange's stale-line boundary, so a late reply to an earlier command arriving during it is captured as part of this response — and for the listing that means a stale SYSTem:ERRor? reply can pass for the end-of-listing terminator, letting a silent device read as a healthy empty card. That is the original #396 bug.

Both properties are wanted, so this splits the setup instead of choosing between them.

The fix

The existing virtual ExecuteTextCommandAsync gains an optional prepareAsync phase that runs inside the lock and before the consumer swap:

acquire lock -> validate -> prepare (SPI switch + settle) -> consumer swap -> stale-line boundary -> sends

• Inside the lock, so no competing exchange can interleave between the switch and the sends.
• Ahead of the boundary, so the settle wait is not a window in which the device appears to be answering. The setup action stays gap-free — two Send() calls, no awaits.
• Before the consumer swap, so anything the device emits in reply to the prepare phase goes to the protobuf consumer, exactly as it did before the exchange began.

GetSdCardFilesAsync and both DeleteSdCardFileAsync call sites share one PrepareSdInterfaceAndSettleAsync.

Subclassing impact — this one does break overrides, on purpose

The first version of this PR added a parallel ExecuteTextCommandWithPrepareAsync and claimed nothing broke. That was wrong, and Qodo caught it: the parallel method called the core directly, so a subclass overriding ExecuteTextCommandAsync silently stopped intercepting SD LIST and DELETE — no compile error, no runtime signal. The three extra overrides my own test fakes suddenly needed were the tell; anything outside this repo would just have stopped seeing SD traffic.

So there is one seam, and the honest statement of the cost:

• Callers are unaffected. prepareAsync is optional and sits after cancellationToken (CA1068 suppressed, matching the convention in IFirmwareUpdateService), so existing positional calls still compile.
• Overriders must widen their signature. C# requires an override to match the full parameter list, so every existing ExecuteTextCommandAsync(Action, int, int, CancellationToken) override fails with CS0115 until Func<CancellationToken, Task>? prepareAsync = null is appended. Ten in this repo; any downstream subclass — desktop, the Avalonia port, consumer test doubles — will hit the same.

That break is the point. A compile error that says "add this parameter" is strictly better than an override that quietly stops intercepting, which is the defect class this whole series has been retiring. An override that accepts the parameter and ignores it is still correct for a fake, since there is no real SPI bus to switch.

Only the Action overload takes the parameter. The Func<CancellationToken, Task> overload has no caller needing a prepare phase, and adding it there would break another eight overrides for no benefit.

Known gap, deliberately not addressed here

The restore side is still unsynchronized: PrepareLanInterface() runs in each SD method's finally, outside the lock, so a competing exchange's restore can still fire during another exchange. That predates #400 — it is on main today and is not something these changes introduced. Fixing it needs a symmetric finalize seam that still runs when the exchange itself throws, which is a larger change than belongs in a follow-up. Flagging it explicitly rather than leaving it implied; happy to file it separately.

Tests

Full suite green on net9.0 and net10.0 (1961 passed, 0 failed), build clean with 0 warnings.

Three new tests in DaqifiDeviceStaleTextLineTests:

• prepare runs before the setup action;
• prepare runs inside the exchange — asserted through the exchange's own re-entrancy guard, so it is deterministic rather than a thread race: a nested exchange started from within prepare must be refused, and if the phase were ever hoisted back outside the lock this test would silently start passing for the wrong reason, so it is written to fail in that case;
• a call carrying a prepare phase is still caught by a subclass override, which fails if the seam ever splits into two virtuals again.

Every test double now honors the prepare phase rather than ignoring the argument.

Bench

DAQiFi Nyquist 1, firmware 3.7.2, /dev/cu.usbmodem1101, example CLI built against this branch: listing returns all 31 files across repeated runs; --sd-storage and a 10 Hz two-channel stream unaffected. Nothing destructive — no format, delete, flash, reboot or WiFi change.

The delete path is deliberately not bench-verified: confirming it on hardware means deleting a file from the shared bench card. Its prepare phase is the identical helper the listing uses.

Refs #396. Follow-up to #400.

Not merging — opened for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Fix SD card SPI switch by adding prepare phase inside text exchange lock

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Add a prepare phase to the text exchange to serialize device state changes under the exchange
 lock.
• Move SD card LIST/DEL operations to use the prepare phase (SPI switch + settle) without widening
 stale-line boundary.
• Extend unit tests to assert prepare ordering and non-reentrancy under the exchange lock.

Diagram

graph TD
  A["SD card ops"] --> B["ExecuteTextCommandWithPrepareAsync"] --> C["Text exchange core"]
  C --> S[("Transport stream")] --> P["Protobuf consumer"] --> T["Text consumer"]
  C -. "holds" .-> L(("Text exchange lock"))
  subgraph Legend
    direction LR
    _cmp["Component"] ~~~ _lock(("Lock")) ~~~ _io[("I/O stream")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Move SD switch back into setupAction

• ➕ No new API surface area
• ➖ Reintroduces the stale-line/terminator window by placing the settle delay after the stale-line boundary

2. Wrap SD switch + commands in an SD-specific lock

• ➕ Avoids changing the text-exchange API
• ➖ Doesn't guarantee correct interaction with the existing exchange lock/consumer swap boundary
• ➖ Still risks widening the stale-line boundary unless integrated at the exchange-core level

3. Make settle delay non-awaiting (busy wait/synchronous delay)

• ➕ Could keep setupAction gap-free without a new seam
• ➖ Blocks threads, harms responsiveness, and still leaves interleaving risk unless done under the exchange lock

Recommendation: Keep the new ExecuteTextCommandWithPrepareAsync seam. It is the minimal change that simultaneously (1) prevents interface interleaving by keeping the SD SPI switch serialized under _textExchangeLock, and (2) preserves the stale-line boundary guarantees by running the awaited settle delay before the consumer swap and boundary capture.

Files changed (4) +192 / -29

Bug fix (2) +99 / -28

DaqifiDevice.csIntroduce ExecuteTextCommandWithPrepareAsync and integrate into exchange core +61/-0

Introduce ExecuteTextCommandWithPrepareAsync and integrate into exchange core

• Adds a new protected ExecuteTextCommandWithPrepareAsync overload that accepts an async prepare phase. Extends ExecuteTextCommandCoreAsync to optionally run prepare inside the device-wide text-exchange lock and before the consumer swap/stale-line boundary, with debug logging for timing.

src/Daqifi.Core/Device/DaqifiDevice.cs

DaqifiStreamingDevice.csRun SD SPI switch + settle as exchange prepare phase for list/delete +38/-28

Run SD SPI switch + settle as exchange prepare phase for list/delete

• Reworks GetSdCardFilesAsync and DeleteSdCardFileAsync to use ExecuteTextCommandWithPrepareAsync so the SD interface switch + settle delay executes under the exchange lock without widening the stale-line boundary. Introduces a shared PrepareSdInterfaceAndSettleAsync helper used by both listing and delete paths (including retry).

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Tests (2) +93 / -1

DaqifiDeviceStaleTextLineTests.csAdd tests for prepare-phase ordering and lock containment +51/-1

Add tests for prepare-phase ordering and lock containment

• Adds unit tests asserting that the new prepare phase runs before the setup action and executes inside the exchange critical section (validated via the non-reentrancy guard). Extends the testable device shim with a helper that exposes ExecuteTextCommandWithPrepareAsync.

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

SdCardOperationsTests.csUpdate SD card test doubles to support prepare-phase exchange +42/-0

Update SD card test doubles to support prepare-phase exchange

• Adds overrides for ExecuteTextCommandWithPrepareAsync in multiple test device fakes so SD card operations can use the new prepare seam while keeping existing canned-response behavior. Ensures prepare is executed before the simulated exchange response collection.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.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. Prepare seam bypasses overrides ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

GetSdCardFilesAsync/DeleteSdCardFileAsync now dispatch through ExecuteTextCommandWithPrepareAsync,
so subclasses that previously overrode ExecuteTextCommandAsync will not intercept SD operations
unless they also override the new method. This can silently break test fakes/instrumented devices
that rely on overriding ExecuteTextCommandAsync to capture SCPI sends or return canned responses.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1671-1672]

+                    lines = await ExecuteTextCommandWithPrepareAsync(
+                        PrepareSdInterfaceAndSettleAsync,

Relevance

●● Moderate

Mixed history: API-compat concerns rejected (#329/#388) but backward-compat shim accepted (#349).

PR-#329
PR-#388
PR-#349

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The SD operations were changed to call the new virtual method, and the base class introduces that
new method as a separate override point; repo test fakes had to add overrides for it to keep
intercepting SD behavior, demonstrating the dispatch change.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1632-1684]
src/Daqifi.Core/Device/DaqifiDevice.cs[792-805]
src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs[1985-1999]

Agent prompt

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

### Issue description
`DaqifiStreamingDevice` SD operations now call `ExecuteTextCommandWithPrepareAsync(...)` instead of `ExecuteTextCommandAsync(...)`. Any downstream subclass that overrides only `ExecuteTextCommandAsync` (common for mocks, test fakes, or instrumentation) will no longer affect SD operations, leading to unexpected behavior changes.

### Issue Context
This PR adds a new protected virtual method as a new dispatch point. In-repo fakes had to add overrides to keep tests working, which is a concrete sign that existing overrides are bypassed.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[752-805]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1664-1684]
- src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs[1985-2000]

### Suggested fix
Choose one (ordered by robustness):
1) Refactor to a single overridable exchange hook used by *both* `ExecuteTextCommandAsync` and `ExecuteTextCommandWithPrepareAsync` so derived classes have one stable override point.
2) If keeping two virtual seams, explicitly document in XML docs (and optionally in code comments) that subclasses overriding `ExecuteTextCommandAsync` must also override `ExecuteTextCommandWithPrepareAsync` to affect SD operations; consider adding a protected virtual shim that delegates between them where safe.

ⓘ 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 a002460