refactor(device): extract the session snapshot/restore decision into a collaborator (part of #344)

[tylerkron]

Part of #344 — not merging, this is for review.

Problem

DaqifiStreamingDevice still carried the whole #379 reconnect-snapshot mechanism inline: a private nested StreamingSessionSnapshot class, CaptureSessionSnapshot, and a RestoreSessionSnapshotAsync that mixed deciding what to restore with applying it. That's ~120 lines of the god-class doing work that is really just set arithmetic over a channel list.

Fix

New Device/Internal/StreamingSessionSnapshot (with a SessionRestorePlan result), following the exact decision/effect split #439 established for SessionCommandInterpreter:

• Moves out (pure): recording which channels were enabled at the drop, and matching that set against the channels the reconnected device is now reporting to produce the channel objects to enable plus whether to resume.
• Stays on the device (effects, and their order matters): clearing IsStreaming, the two cancellation checks, DisableAllChannels() before re-applying, EnableChannels(...), and StartStreaming().

Deliberately host-free — no IDeviceOperationHost member added, no new field, no constructor wiring. That keeps the public surface and the internal seam untouched, and it keeps this slice's hunks disjoint from the still-open #440.

One small tightening that came with the move: the enabled set is now private to the snapshot (EnabledChannelCount is all that's exposed) instead of a publicly-readable HashSet, so identity matching can't be bypassed.

DaqifiStreamingDevice.cs: 1196 → 1148 lines.

Evidence this changed nothing

DeviceReconnectTests is left completely untouched on purpose — all 44 of its tests are the "the extraction changed nothing" evidence, and editing them would weaken that.

Tests: +13 new StreamingSessionSnapshotTests. FULL suite green on net9 + net10 — 2667 passed / 2 skipped each (+23 Daqifi.Mcp.Tests on net9; Mcp is net9-only), 0 warnings.

Bench (real Nq1, fw 3.7.2, USB, non-destructive — 12/12 checks passed). A physical unplug isn't available unattended, so instead of skipping hardware validation the harness drove the protected capture/restore members directly on a live device and tore the session down the way re-initialization does:

• Established a live session through raw Send (mask 3 @ 200 Hz) — 395 samples each on Analog0/Analog1 over 2.5 s.
• CaptureSessionSnapshot(), then StopStreaming() + DisableAllChannels(): 0 channels enabled, 0 samples arriving.
• RestoreSessionSnapshotAsync(ResumeStreaming: true) → returned true, IsStreaming back to true, exactly Analog0,Analog1 re-enabled, and 395 samples each flowing again with nothing on any other channel. That last part is the bit a mock can't show: the plan handed back the device's live channel objects, so the enable mask actually reached the firmware.
• RestoreSessionSnapshotAsync(ResumeStreaming: false) → returned false, channels restored, IsStreaming stayed false, and 0 samples — the stream really was not restarted.

(158 Hz effective vs 200 Hz requested is the known fw 3.7.2 clock mismatch, not a regression.)

closes nothing on its own — part of #344.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Refactor device reconnect snapshot/restore into StreamingSessionSnapshot collaborator

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Extract pure session snapshot/restore planning out of DaqifiStreamingDevice.
• Keep device-side restore effects/ordering unchanged while delegating restore decisions.
• Add focused unit tests pinning snapshot capture and restore planning behavior.

Diagram

graph TD
  A["DaqifiStreamingDevice"] --> D[("Channels snapshot")] --> B["StreamingSessionSnapshot"]
  A["DaqifiStreamingDevice"] --> E[("ReconnectOptions")] --> B["StreamingSessionSnapshot"]
  B["StreamingSessionSnapshot"] --> C[("SessionRestorePlan")] --> A["DaqifiStreamingDevice"]

Loading

High-Level Assessment

The decision/effect split is the right approach here: it isolates the set/identity arithmetic into a pure collaborator (now directly unit-tested) while keeping the device’s side-effect ordering and cancellation points intact. Alternatives like adding a host dependency or introducing a broader strategy interface would increase wiring and surface area without clear benefit for this slice.

Files changed (3) +335 / -60

Refactor (2) +149 / -60

DaqifiStreamingDevice.csDelegate reconnect snapshot/restore decisions to StreamingSessionSnapshot +12/-60

Delegate reconnect snapshot/restore decisions to StreamingSessionSnapshot

• Removes the nested StreamingSessionSnapshot type and inlines no longer perform set matching in the device. CaptureSessionSnapshot now calls StreamingSessionSnapshot.Capture(...), and RestoreSessionSnapshotAsync builds and applies a SessionRestorePlan (disable-all first, enable planned channels, optionally restart streaming).

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

StreamingSessionSnapshot.csIntroduce StreamingSessionSnapshot + SessionRestorePlan collaborator +137/-0

Introduce StreamingSessionSnapshot + SessionRestorePlan collaborator

• Adds a new internal snapshot class that captures enabled channel identities and whether streaming was active at drop time. Adds SessionRestorePlan and PlanRestore(...) to compute which current channel objects to re-enable and whether to resume streaming, keeping the enabled set private and validating inputs.

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

Tests (1) +186 / -0

StreamingSessionSnapshotTests.csAdd unit tests for snapshot capture and restore planning +186/-0

Add unit tests for snapshot capture and restore planning

• Introduces a new test suite for StreamingSessionSnapshot covering capture semantics (enabled-only, defensive copy), identity matching (type + number), intersection behavior when channel sets change, resume policy gating, reusability across attempts, and null-argument validation.

src/Daqifi.Core.Tests/Device/Internal/StreamingSessionSnapshotTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[tylerkron]

Ready for review: Qodo re-review at 79cf570 reports Bugs (0) / Rule violations (0) / Requirement gaps (0) with 0 review threads (verified via GraphQL reviewThreads, not just the headline), CI build is green on that head, and the branch still contains origin/main (MERGEABLE, blocked on approval only). Not merging — for your review.