refactor(device): extract the stream frame decode path into a collaborator (part of #344)

[tylerkron]

Problem

DaqifiStreamingDevice is still 1,756 lines against #344's ~800 target. Its own status comment names the frame-decode block as the next extraction and flags it as the riskiest one: it is the hot path, it owns three public events, and it is where a mistake is silent — samples stamped with times that never happened, or a raw frame handed to consumers twice.

#432 (merged) took the channel/DIO/PWM block and widened the IDeviceOperationHost seam with SnapshotChannels(), which is what made this one possible without duplicating that work.

What changed

The whole path from "a frame arrives" to "per-channel samples" moves into a new internal StreamFrameDecoder (Device/Internal/): the cross-session leftover gate, the #351 warmup-frame guard, timestamp reconstruction, gap detection, and the analog/digital unpacking — plus the session-scoped state that BeginSession resets together and the two per-session counters.

DaqifiStreamingDevice: 1,756 → 1,368 lines. Public API unchanged; DiscardedStreamFrameCount and DecodeFailureCount now delegate.

What deliberately did not move

StreamFrameDiscarded, GapDetected, and the raw-frame re-raise stay on the device, reached through four new IDeviceOperationHost members:

• The two events' sender has to remain the device a subscriber attached to. A collaborator raising them in its own name would be a silent, compile-clean behavior change — the same reasoning already recorded on RaiseLowSdSpaceWarning, and the same class of bug refactor(firmware): split FirmwareUpdateService into focused collaborators (part of #344) #419 had to add a guard test for.
• The raw re-raise has to be base.OnStreamMessageReceived, so the base implementation and any subclass between it and the streaming device still see the frame. Calling the override would recurse; there's a comment at the call site saying so.
• SafeTrace stays too — it's still used by the session-command tracking, so the isolating try/catch around each subscriber stays with it.

The decoder owns the discard counter and increments it before asking the device to raise the event, preserving the documented guarantee that a handler reading DiscardedStreamFrameCount already sees the frame it's being told about.

Verifying it's a pure move

Same method as #419/#422/#432/#433: a normalized statement multiset diff of what the device lost against what the decoder plus the delegating call sites gained (comments stripped, mechanical renames canonicalized). Every surviving residue is structural — new-file scaffolding, the four seam forwarders, the two one-line delegations, and the split of the old RaiseStreamFrameDiscarded(reason, frame, counts) into "decoder counts and builds the args" / "device raises". No decode statement is lost or altered.

Also fixed a pre-existing doc/member mismatch carried along by the move: the <summary> describing DecodeAnalog was attached to CountEnabledAnalogChannels. Each now documents itself.

Tests

Zero edits to existing tests — DaqifiStreamingDeviceDecodeTests (38 cases) still drives the same pipeline through the device, and that is the evidence the extraction changed nothing.

+15 new cases against the collaborator directly, covering what only a direct test can see: the order and multiplicity of the calls back into the host. Through the device those callbacks are invisible — a frame re-raised twice, or a discard counted after the event, looks identical from the outside until a consumer trips over it. The fake host records the call sequence, and its non-decode members throw, so a future change that makes the decoder reach for device I/O fails loudly.

Mutation-verified rather than eyeballed:

Mutation	Result
Count the discard after raising the event	DiscardIsCountedBeforeTheEventIsRaised fails
Re-raise the raw frame unconditionally (drop the else)	2 tests fail, one of them a pre-existing device-level test
BeginSession skips the gap-detector reset	BeginSession_ResetsTheGapDetector fails

FULL suite green net9 + net10 (2,544 passed, 2 skipped, was 2,529) plus Daqifi.Mcp.Tests (23). Release solution build 0 warnings on both TFMs.

Bench (real Nq1, fw 3.7.2, USB, non-destructive)

A unit test cannot show that the device's own stream still flows through the moved code, so this ran against the board:

Session 1 — channels 0,1,2 @ 200 Hz, 3 s

• rawFrames=475, decodedCh0=475 — every delivered frame reached both consumer paths exactly once. This is the raw re-raise multiplicity contract, on hardware.
• discarded=1, reason=PartialAnalogFrame[an=1/en=3] — the firmware's malformed leading warmup frame (Streaming: malformed first sample (partial analog channels) — Core lacks desktop's first-frame/leftover-frame protection (#573) #351) was caught by the moved guard, reported with the counts the decision was made on, and withheld from raw consumers.
• All three channels decoded 475 samples each in ascending channel order, with plausible per-channel voltages.
• decodeFailures=0, gaps=0. Every event's sender was asserted to be the device — it was.

Session 2 — channel 0 only @ 100 Hz, 3 s (same device instance)

• discarded=0, decodeFailures=0 — BeginSession reset both counters, and no leftover frame from session 1 tripped the gate.
• rawFrames=238 vs decodedCh0=237: one frame arrived after the stop command landed and was re-raised but not decoded — exactly the if (!IsStreaming) branch, working on hardware.
• ch1=0, ch2=0 decoded — the disable reached the device and the enabled-channel snapshot the decode maps against is still correct; a broken snapshot would have mis-mapped values into the disabled channels.

Only channel enable/disable and stream start/stop. No NVM write, no reboot, no SD, nothing driven.

Scope

Part of #344 — does not close it. Remaining: DaqifiStreamingDevice is still above the ~800 target (the SD-card block at 1,357 lines is now the dominant remaining piece, extracted but not yet under target), DaqifiDevice, and WifiModuleUpdater once #271 settles.

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Extract streaming frame decode pipeline into StreamFrameDecoder

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Move stream-frame screening, timestamping, gap detection, and per-channel decode into
 StreamFrameDecoder.
• Keep public events and raw-frame re-raise on DaqifiStreamingDevice via new IDeviceOperationHost
 forwards.
• Add focused unit tests to pin host-callback order/multiplicity and session reset behavior.

Diagram

graph TD
  A["DaqifiStreamingDevice"] --> B["StreamFrameDecoder"] --> C["Channel samples"]
  B --> H["IDeviceOperationHost"] --> R["Raw MessageReceived"]
  B --> H --> E["Device events"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Inject explicit callbacks instead of expanding IDeviceOperationHost

• ➕ Keeps the host interface smaller and more specific to decoding
• ➕ Allows passing only what the decoder needs (actions for discard/gap/raw/failure)
• ➖ Adds constructor plumbing and more moving pieces than a single seam
• ➖ Harder to evolve consistently if additional host interactions are needed later

2. Let decoder raise events directly (passing device as sender)

• ➕ Avoids adding event-raising members to IDeviceOperationHost
• ➕ Keeps all decode-related logic co-located
• ➖ Would require exposing/handing event delegates to the decoder, weakening encapsulation
• ➖ Higher risk of subtle behavior drift (sender identity, recursion hazards with raw re-raise)

Recommendation: The chosen approach (decoder + widened IDeviceOperationHost seam) is the best tradeoff for this extraction: it keeps public event semantics and raw-frame routing on the device while cleanly isolating the decode pipeline and its session-scoped state. The added unit tests appropriately target the main regression risk: callback ordering/multiplicity and counter/event guarantees.

Files changed (4) +1055 / -409

Enhancement (1) +38 / -0

IDeviceOperationHost.csExpand host seam with stream decode/event forwarding hooks +38/-0

Expand host seam with stream decode/event forwarding hooks

• Adds four members used by StreamFrameDecoder to raise StreamFrameDiscarded, GapDetected, raw-frame re-raise (via device base), and stream decode failure reporting, with documentation emphasizing sender/behavior preservation.

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

Refactor (2) +546 / -409

DaqifiStreamingDevice.csDelegate streaming hot path to StreamFrameDecoder and forward host callbacks +38/-409

Delegate streaming hot path to StreamFrameDecoder and forward host callbacks

• Removes in-class frame decoding/session state and replaces it with a StreamFrameDecoder collaborator. DiscardedStreamFrameCount and DecodeFailureCount now delegate to the decoder, BeginStreamingSession resets via decoder, and IDeviceOperationHost is extended/implemented to forward discard/gap/raw/failure surfaces while preserving sender identity and base.OnStreamMessageReceived behavior.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

StreamFrameDecoder.csIntroduce StreamFrameDecoder collaborator for streaming frame processing +508/-0

Introduce StreamFrameDecoder collaborator for streaming frame processing

• Adds a new internal decoder that encapsulates frame gating, warmup-frame suppression, timestamp reconstruction, gap detection, and analog/digital sample decoding. Owns per-session state and counters, and reports events/raw frames back through IDeviceOperationHost while keeping best-effort per-frame isolation for decode failures.

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

Tests (1) +471 / -0

StreamFrameDecoderTests.csAdd unit tests pinning decoder-to-host callback behavior +471/-0

Add unit tests pinning decoder-to-host callback behavior

• Introduces a focused StreamFrameDecoder test suite that validates raw-frame re-raise behavior, discard counting/order, decode-failure isolation, warmup suppression bounds, gap detection, and session reset semantics using a FakeHost that records host callback ordering.

src/Daqifi.Core.Tests/Device/Internal/StreamFrameDecoderTests.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. Misleading thread-safety docs ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

StreamFrameDecoder’s XML remarks claim its unsynchronized session fields are only ever touched on
the message-consumer thread, but BeginSession is invoked from public StartStreaming() and raw Send()
command tracking paths. This inaccurate contract can mislead future changes into assuming thread
confinement when it isn’t guaranteed by the current API surface.

Code

src/Daqifi.Core/Device/Internal/StreamFrameDecoder.cs[R34-37]

+    /// Not thread-safe by design, matching the code it was moved from: frames arrive on the single
+    /// message-consumer thread, which is also the thread that repopulates channels, so the
+    /// unsynchronized session fields are only ever touched from there. The two counters are
+    /// interlocked because they are read from arbitrary threads through the device's public

Relevance

●●● Strong

Team routinely updates misleading XML docs to match real behavior and prevent drift/confusion.

PR-#357
PR-#348

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The decoder’s comments explicitly claim thread confinement for unsynchronized session fields, but
the device calls the decoder’s BeginSession from publicly callable control paths (StartStreaming and
raw Send tracking), which are not inherently restricted to the message-consumer thread.

src/Daqifi.Core/Device/Internal/StreamFrameDecoder.cs[29-38]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[315-342]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[521-548]

Agent prompt

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

## Issue description
`StreamFrameDecoder`’s XML remarks state its unsynchronized session fields are only touched by the single message-consumer thread, but `BeginSession(...)` is called from `DaqifiStreamingDevice.StartStreaming()` and from the raw `Send()` command-tracking path. This makes the documentation/contract inaccurate and risks future contributors relying on incorrect thread-confinement assumptions.

## Issue Context
- `StreamFrameDecoder` owns session state (timestamp processor, gap detector, frame gate, warmup guard) and resets it in `BeginSession`.
- `DaqifiStreamingDevice.StartStreaming()` and `TrackStreamingStart()` both call `BeginStreamingSession()`, which calls `_frameDecoder.BeginSession(...)`.

## Fix Focus Areas
- src/Daqifi.Core/Device/Internal/StreamFrameDecoder.cs[29-38]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[315-342]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[530-548]

### Suggested fix
Choose one:
1) **Documentation-only (minimal):** Update the XML remarks to remove/soften the claim that fields are only touched from the consumer thread, and explicitly state that `BeginSession` must be serialized with `ProcessFrame` by the owning device (or that it is expected to be called when no frames are concurrently being processed).
2) **Enforcement (stronger):** Add a lightweight synchronization strategy (e.g., a private lock) around `BeginSession` and `ProcessFrame` (or ensure `BeginSession` is marshaled onto the same consumer thread) so the documentation can remain true.

ⓘ 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 0e11946

[tylerkron]

Ready for review: Qodo came back clean on the latest commit (0 bugs, 0 rule violations, 0 skill insights; the earlier Misleading thread-safety docs finding is resolved) and CI build is green on net9 + net10. Not merging — awaiting your review.

Note on the one red CI run here (0e11946): it was an unrelated flake, not a regression. SerialDeviceFinderTests.DiscoverAsync_HungPort_TimesOutAndStillReturnsHealthyDevices failed on net9 only while net10 passed in the same run, and 0e11946 changes zero non-comment lines — the two commits before it on this branch both passed. That test races a 300 ms PortProbeHardTimeoutMs against a probe that has to be scheduled onto the thread pool by Task.Run, so under runner contention the timeout can win and the healthy port gets abandoned, producing exactly the observed Assert.Single() Failure: The collection was empty. Re-ran the failed job and it is green; the PR was not modified. Worth tightening that test's timing assumption separately if it recurs.