refactor(device): split DaqifiStreamingDevice into focused collaborators (part of #344)

[tylerkron]

Why

DaqifiStreamingDevice had grown to 3,632 lines implementing five interfaces in one class. It's one of the two files in the repo where unrelated changes keep colliding — the SD work in #420 and any streaming change land in the same file for no reason other than that they happen to share a class.

This is the device half of #344. The firmware half is #419.

What

The four blocks the issue calls out move into internal collaborators. The device keeps every public member and forwards.

File	Lines	Job
DaqifiStreamingDevice	2,029	streaming, frame decode, channel/DIO/PWM/analog-out control, session tracking — plus the public surface, forwarding
SdCardOperations	1,357	ISdCardOperations and the shared-SPI-bus handover
DeviceDiagnosticsOperations	266	IDeviceDiagnostics
NetworkConfigurationOperations	214	the WiFi/LAN configuration half of INetworkConfigurable
IDeviceOperationHost	103	the seam the collaborators work through
LanChipInfoOperations	59	ILanChipInfoProvider

Was: one file, 3,632 lines.

Four SCPI line predicates that several of the split blocks shared also move into the existing ScpiResponseClassifier, which was already their home for the underlying rules.

The one design decision worth a look

The collaborators don't touch the transport. They go through IDeviceOperationHost, which DaqifiStreamingDevice implements explicitly — so it adds nothing to the public API — and every member of it forwards to the device member it names.

That indirection is the whole point. ExecuteTextCommandAsync, ExecuteRawCaptureAsync, Send and IsUsbConnection are all virtual, and subclasses override them to intercept device I/O — that's how the entire test suite stands in for hardware, and how an instrumented device would work in the field. A collaborator that reached for _transport directly would compile fine and silently step around every one of those overrides. Routing through the device's own virtual members keeps them in the path.

The evidence that it worked: the test suite needed no changes at all. Every existing double still intercepts every operation that moved.

How I know nothing changed

Rather than trust a reading of a 3,600-line move, I diffed the normalized statement multiset of the original file against the new ones — comments, usings and blank lines stripped, and the mechanical renames (_host. prefixes, the classifier qualifications) canonicalized away.

Nine statements exist in the original and not in the new files. All nine are structural, and here they are in full:

• 4 declarations of the moved predicates, whose accessibility changed from private static to internal static when they moved to ScpiResponseClassifier
• 2 one-line forwarders (IsScpiErrorLine, IsNonResultLine) that existed only to call the classifier, and their 2 bodies — call sites now call the classifier directly
• the class declaration, which gained IDeviceOperationHost

Zero logic statements were added, removed, or changed. Everything in the other direction is declarations, delegating one-liners, and seam forwarders.

The hazard that check does not catch

A statement-level diff says nothing about who raises an event. LowSdSpaceWarning is part of ISdCardOperations, and the space check that fires it moved into a collaborator — so the sender could have quietly become the collaborator. Every existing subscriber in the tests discards the sender ((_, e) => ...), so nothing would have failed.

OnLowSdSpaceWarning therefore stays on the device and the collaborator calls back into it. I added a test pinning sender to the device and mutation-verified it: pointing the raise at the collaborator makes it fail.

Bench evidence — Nyquist 1, FW 3.7.2, /dev/cu.usbmodem1101

Example CLI built against this working copy. Everything below runs through a collaborator that didn't exist before this PR.

Path	Collaborator	Result
Streaming, 10 Hz, 3 s, ch 0+1	(device, unchanged)	23 samples, exit 0 — run before and after the SD work
SD LIST	SdCardOperations	32 files with sizes and dates
SD storage	SdCardOperations	free 7,799,934,976 / total 7,800,356,864
SD download	SdCardOperations	4,639 bytes, parsed to 232 samples
SD log start/stop	SdCardOperations	3 s session, file appeared in the listing
SD delete	SdCardOperations	33 → 32 files, confirmed by re-listing
Diagnostics	DeviceDiagnosticsOperations	all 8 queries succeeded
LAN chip info	LanChipInfoOperations	id 1377184, fw 19.7.7, build Mar 30 2022

The download number is the useful one: 4,639 bytes / 232 samples is byte-for-byte what the same file produced on the pre-refactor build in #420's bench run.

One aside, not caused by this PR: the first download attempt failed after a streaming run, because on FW 3.7.2 a stream re-partitions the buffer pool and leaves the SD read buffer too small. A reboot restores it. That's the firmware limitation documented in #420, and it's visible here only because #420's idle window now reports it in 20 seconds instead of hanging.

WiFi leg

Also run over TCP at 192.168.1.30, after the unit was power-cycled: connect, initialize, stream at 10 Hz for 3 s on channels 0+1, stop, disconnect — clean throughout, with samples arriving at a steady cadence and no errors. Device still answering ping afterwards.

That's deliberately a small run. This is a pure refactor with no transport-specific logic in it, so the WiFi leg only has to show the delegation didn't disturb the TCP path; the USB evidence above is what actually exercises the moved code, including a download that came back byte-identical to the pre-refactor build.

What I did not do

The file is 2,029 lines, not under 800. The four blocks named in the acceptance criteria are all extracted and delegating, but they don't add up to that target on their own. What's left is streaming control, the frame-decode pipeline, session tracking/restore, and channel/DIO/PWM/analog-out control.

The obvious next extraction is the channel/DIO/PWM/analog-out block (~680 lines) — it's the same shape of work, mostly command senders. I stopped short of it deliberately: it needs the seam widened to carry channel-collection access and the channels lock, and doing that in the same PR would have meant a much larger diff verified less carefully. The issue asks for one extraction per PR, and this is one. Frame decode is the riskier one after that — it's the hot path and it owns events.

FirmwareUpdateService is untouched; that's #419.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Split DaqifiStreamingDevice into internal operation collaborators

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Extract SD, network, LAN chip info, and diagnostics logic into internal collaborators.
• Preserve public API by forwarding through an explicit IDeviceOperationHost seam.
• Centralize shared SCPI line predicates in ScpiResponseClassifier; add sender-behavior test.

Diagram

graph TD
  A["DaqifiStreamingDevice"] --> B["IDeviceOperationHost"] --> C["SdCardOperations"]
  B --> D["NetworkConfigOps"]
  B --> E["LanChipInfoOps"]
  B --> F["DiagnosticsOps"]
  C --> G["ScpiResponseClassifier"]
  F --> G

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Partial split (SD-only first)

• ➕ Reduces blast radius; easier review and bisecting if regressions appear
• ➕ Validates the host-seam approach on the riskiest subsystem first
• ➖ Prolongs merge-conflict pressure in DaqifiStreamingDevice for other work streams
• ➖ Ends up doing the same plumbing twice across multiple PRs

2. Make collaborators implement public interfaces directly

• ➕ Less forwarding boilerplate inside DaqifiStreamingDevice
• ➕ Potentially clearer ownership boundaries in code navigation
• ➖ Would widen or alter the public API surface (types become visible/constructible)
• ➖ Increases risk of breaking sender/virtual override semantics if events/I/O bypass the device

3. Inject transport into collaborators instead of a host seam

• ➕ Simpler collaborator signatures (direct access to transport)
• ➕ Potentially fewer forwarding methods
• ➖ Bypasses device virtual overrides used by tests/instrumentation
• ➖ Higher regression risk: behavior changes compile-clean but alter interception paths

Recommendation: The chosen approach (internal collaborators + explicit IDeviceOperationHost forwarding) is the best fit because it preserves the public surface and, critically, keeps device virtual overrides in the I/O path for tests and instrumented subclasses. The main thing to scrutinize in review is that every moved operation routes through the host seam (not direct transport access) and that any externally observed semantics (event sender, cached state like SdCardFiles/IsLoggingToSdCard, cancellation boundaries) remain identical.

Files changed (8) +2240 / -1786

Refactor (7) +2220 / -1786

DaqifiStreamingDevice.csDelegate SD/network/LAN/diagnostics to internal collaborators via host seam +183/-1786

Delegate SD/network/LAN/diagnostics to internal collaborators via host seam

• Adds internal collaborator fields and initializes them during streaming-device setup. Replaces large in-class implementations of SD card, network configuration, LAN chip info, and diagnostics with forwarding members while explicitly implementing IDeviceOperationHost to keep virtual I/O overrides in the call path.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

SdCardOperations.csIntroduce SdCardOperations collaborator owning SD state + SPI handover +1357/-0

Introduce SdCardOperations collaborator owning SD state + SPI handover

• Adds an internal SD operations implementation extracted from DaqifiStreamingDevice, including shared SPI bus handover (PrepareSdInterface/PrepareLanInterface), SD file listing/download behaviors, and SD-scoped state (logging flag, cached file list, download gate). All device interactions route through IDeviceOperationHost, including raising the device-owned LowSdSpaceWarning event.

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

NetworkConfigurationOperations.csExtract WiFi/LAN configuration logic into NetworkConfigurationOperations +214/-0

Extract WiFi/LAN configuration logic into NetworkConfigurationOperations

• Adds an internal collaborator for the configuration half of INetworkConfigurable, preserving the persisted/apply ordering and cancellation boundary semantics. Owns and returns a cloned last-known NetworkConfiguration while routing all commands through IDeviceOperationHost.

src/Daqifi.Core/Device/Network/NetworkConfigurationOperations.cs

LanChipInfoOperations.csExtract LAN chip info query/parsing into LanChipInfoOperations +59/-0

Extract LAN chip info query/parsing into LanChipInfoOperations

• Adds an internal ILanChipInfoProvider implementation that issues the chip-info query via the host text-exchange primitive and parses the JSON response. Preserves the special handling for SCPI -200 to throw LanNotInitializedException instead of returning null.

src/Daqifi.Core/Device/LanChipInfoOperations.cs

DeviceDiagnosticsOperations.csExtract IDeviceDiagnostics implementation into DeviceDiagnosticsOperations +266/-0

Extract IDeviceDiagnostics implementation into DeviceDiagnosticsOperations

• Adds a collaborator implementing IDeviceDiagnostics using host-mediated text exchanges and tolerant parsing. Adds explicit handling for error-only responses and uses ScpiResponseClassifier helpers to detect failures cleanly.

src/Daqifi.Core/Device/Diagnostics/DeviceDiagnosticsOperations.cs

IDeviceOperationHost.csAdd internal IDeviceOperationHost seam for collaborator-to-device calls +103/-0

Add internal IDeviceOperationHost seam for collaborator-to-device calls

• Introduces an internal interface exposing connection state, transport facts, streaming control, text exchange/raw capture primitives, feature gating, SD transfer timeouts, and a callback to raise LowSdSpaceWarning with the device as sender. Designed for explicit implementation to avoid widening the public API and to preserve virtual override interception.

src/Daqifi.Core/Device/Internal/IDeviceOperationHost.cs

ScpiResponseClassifier.csCentralize shared SCPI response predicates +38/-0

Centralize shared SCPI response predicates

• Adds helper methods to detect whether a response contains SCPI error lines and whether a response is error-only (non-empty but entirely error/status lines). This consolidates line predicates previously duplicated across the split blocks.

src/Daqifi.Core/Device/ScpiResponseClassifier.cs

Tests (1) +20 / -0

SdCardOperationsTests.csAssert LowSdSpaceWarning sender remains the device after collaborator split +20/-0

Assert LowSdSpaceWarning sender remains the device after collaborator split

• Adds a regression test ensuring LowSdSpaceWarning is raised with the device instance as the event sender, preventing a silent behavior change if a collaborator were to raise the event in its own name.

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. Unused members break build ✓ Resolved 🐞 Bug ≡ Correctness

Description

After delegating SD/network functionality to collaborators, DaqifiStreamingDevice still declares
several SD/network constants and the _sdDownloadGate field that are no longer referenced (each
identifier appears only in its declaration). With TreatWarningsAsErrors enabled for Daqifi.Core,
these unused-member compiler warnings will fail compilation.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1795-1816]

+        // -----------------------------------------------------------------
+        // Delegation to the operation collaborators.
+        //
+        // Each block below was lifted out of this class wholesale (#344); what
+        // remains is the public surface, unchanged, forwarding to the object
+        // that now owns the implementation. The collaborators reach back
+        // through IDeviceOperationHost, implemented explicitly at the bottom of
+        // this file, so every call still passes through this device's own
+        // virtual members and any subclass override of them.
+        // -----------------------------------------------------------------
+
+        /// <summary>WiFi/LAN configuration (<see cref="INetworkConfigurable"/>).</summary>
+        private NetworkConfigurationOperations _networkOperations = null!;
+
+        /// <summary>SD card operations (<see cref="ISdCardOperations"/>) and the shared-SPI handover.</summary>
+        private SdCardOperations _sdCardOperations = null!;
+
+        /// <summary>WiFi module chip info (<see cref="ILanChipInfoProvider"/>).</summary>
+        private LanChipInfoOperations _lanChipInfoOperations = null!;
+
+        /// <summary>Device diagnostics (<see cref="IDeviceDiagnostics"/>).</summary>
+        private DeviceDiagnosticsOperations _diagnosticsOperations = null!;

Relevance

●●● Strong

TreatWarningsAsErrors enforced; team fixes/removes warning-causing dead code to keep build clean
(PRs #225, #258).

PR-#225
PR-#258
PR-#163

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

DaqifiStreamingDevice still contains SD/network-specific constants and _sdDownloadGate, while
SD/network behavior is forwarded to collaborators and the collaborators now own equivalent
constants/gates; with warnings treated as errors, leaving these declarations unused is a
compile-time failure risk.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[34-101]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1795-1840]
src/Daqifi.Core/Device/SdCard/SdCardOperations.cs[31-89]
src/Daqifi.Core/Device/Network/NetworkConfigurationOperations.cs[22-35]
src/Daqifi.Core/Daqifi.Core.csproj[3-8]

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` delegates SD/network functionality to collaborator classes, but it still contains several SD/network-only constants/fields that now have no references. In this repo, `TreatWarningsAsErrors` is enabled, so unused-member warnings (e.g., CS0169/CS0414) become build errors.

## Issue Context
The SD/network implementation moved into `SdCardOperations` and `NetworkConfigurationOperations`, which now define and use the equivalent constants/gate. The remaining declarations in `DaqifiStreamingDevice` are dead.

## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[34-101]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1795-1840]

## What to change
- Remove the now-unused declarations from `DaqifiStreamingDevice`:
 - `WIFI_MODULE_RESTART_DELAY_MS`
 - `SD_INTERFACE_SETTLE_DELAY_MS`
 - `SD_LIST_MAX_RETRIES`
 - `SD_LIST_COMPLETION_TIMEOUT_MS`
 - `ScpiErrorCodeUndefinedHeader`
 - `_sdDownloadGate`
- Ensure no other newly-unused members/usings remain in `DaqifiStreamingDevice` after the extraction (quick scan/search), then rebuild.

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

Round 1 addressed — finding accepted, stated mechanism corrected (40707ce).

The six members Qodo named were exactly the six that were dead after the extraction, and they're removed. A refactor that leaves orphaned declarations behind isn't finished.

They were not breaking the build, though. From-scratch rebuild on the reviewed commit: 0 Warning(s), 0 Error(s). A positive control adding private int _mutantNeverUsed; and private int _mutantAssignedNeverRead = 7; to the same class did fail the build with CS0169 and CS0414 — so the diagnostics are live and TreatWarningsAsErrors does escalate them. The orphans just weren't shapes those rules cover: five were private const (no diagnostic exists for an unreferenced constant), and _sdDownloadGate is a readonly field initialized with an object creation, which CS0414 deliberately doesn't report. Full evidence on the thread.

Also did the wider scan the finding asked for: no other unused members; System.Diagnostics looked dead but is still needed for Trace; every moved constant is referenced at its new home.

Device file: 2,029 → 1,973 lines. Statement-multiset verification unchanged — same nine structural differences, zero logic changes, with only in NEW dropping by exactly six as the duplicate declarations went away. Suite green: 2,356 tests across net9.0 and net10.0.

The WiFi bench leg stays deferred — the bench unit's WiFi needs a power cycle and the hardware is unattended.

/agentic_review

[tylerkron]

WiFi leg complete — the last outstanding item on this PR.

Ran over TCP at 192.168.1.30 against a freshly power-cycled unit: connect, initialize, stream 10 Hz for 3 s on channels 0+1, stop, disconnect. Clean throughout — steady sample cadence, no errors. Device still answering ping on the way out.

Kept it to a single connect on purpose. This is a pure refactor with no transport-specific logic, so the WiFi leg only needs to show the delegation didn't disturb the TCP path; the USB run is where the moved code actually gets exercised, including the SD download that came back byte-for-byte identical to the pre-refactor build.

PR body updated — the deferred-WiFi caveat is replaced with the result. No code changed since the last review.

/agentic_review