fix(device): suppress firmware's malformed warmup stream frame (closes #351)

[tylerkron]

Summary

Closes #351. The firmware's fast streaming encoder (Nanopb_EncodeStreamingFast) emits a leading warmup frame carrying fewer analog values than the enabled channel mask — with a normal one-sample-period timestamp, so the desktop #573 timestamp-jump machinery would not catch it. Core decoded this frame unfiltered, so every per-channel consumer (IChannel.SampleReceived, the StreamSamplesAsync live stream) received a partial first DataSample, silently corrupting first-value baselining, gap detection, min/max, and calibration/export.

Fix

Gate the decode path at stream start (DaqifiStreamingDevice):

• StartStreaming() arms a first-full-frame guard.
• In the decode path, before timestamp/gap processing, drop any leading analog-bearing frame whose value count is below the enabled-analog count. The suppressed frame is a complete non-event — the next frame anchors the session clock (no spurious gap).
• Only leading short frames are suppressed; a mid-stream short frame stays best-effort mapped (existing resilience preserved).
• Bounded by MaxSuppressedWarmupFrames (5) so a genuinely short stream is never withheld indefinitely.
• Raw MessageReceived still fires, so hand-demuxing consumers are unaffected (the raw frame is genuinely short on the wire; only the decoded per-channel surface is gated).

Scoped deliberately to the reproduced, headline bug (partial-channel warmup frame). The stale-prior-session leftover-frame port (recommendation #1, timestamp-jump based) is a distinct, larger concern and is left for a follow-up.

Tests

• 8 new xUnit tests in DaqifiStreamingDeviceDecodeTests covering: warmup suppression + subsequent full frame, no-clock-anchoring, raw passthrough, full-first-frame passthrough, digital-only stream, per-session re-arm, and the suppression cap.
• Reworked the prior Decode_FewerValuesThanChannels test to exercise the mid-stream short-frame path (its actual intent) now that the leading short frame is suppressed.
• Full suite green on net9.0 + net10.0 (1730 passed).

Bench validation

Nq1, FW 3.7.2, USB/serial. A minimal harness consuming Core's decoded StreamSamplesAsync stream (channels 0+1 @ 50 Hz):

Decoded frames (deviceTimestamp -> channels that produced a decoded sample):
  ts=2715962758 channels=[0,1] count=2   <-- FIRST frame now full (was 1 value)
  ts=2716802758 channels=[0,1] count=2
  ts=2717642758 channels=[0,1] count=2
  ...
PASS: first decoded frame carried the full complement (2 channels).

Exactly inverts the issue's evidence (analog=[1] first).

---

🤖 Not merging — opened for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Suppress malformed warmup streaming frames in device decode (fixes #351)

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Suppress leading analog frames with fewer values than enabled channels to prevent partial first
 samples.
• Ensure suppressed warmup frames don’t anchor timestamp/gap detection; mid-stream short frames
 remain best-effort.
• Add targeted xUnit coverage for warmup suppression, raw passthrough, re-arming, and suppression
 cap.

Diagram

graph TD
A["StartStreaming"] --> B["DecodeStreamFrame"] --> C{"Warmup guard?"}
C -->|"suppress"| D["Drop frame"]
C -->|"decode"| E["Timestamp + gap"] --> F["Decode analog/digital"] --> G["Channel consumers"]
H["Firmware stream frame"] --> B

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Fix in firmware encoder

• ➕ Eliminates malformed frames at the source for all clients
• ➕ Avoids client-side heuristics/caps entirely
• ➖ May require firmware release coordination and device updates
• ➖ Doesn’t protect existing deployed firmware versions

2. Timestamp-jump-based suppression at session start

• ➕ Doesn’t depend on channel/value counts
• ➕ Can address other stale-frame/session-boundary artifacts
• ➖ This warmup frame has a normal one-period timestamp, so it won’t be detected reliably
• ➖ More complex interaction with existing gap/timestamp machinery; higher regression risk

3. Emit partial samples but mark as incomplete

• ➕ Preserves visibility that a warmup frame occurred
• ➕ Could help diagnostics/telemetry
• ➖ Requires new surface area/contract changes for all consumers
• ➖ Still risks subtle consumer misuse if incomplete flag is ignored

Recommendation: Keep the current decode-gating approach: it is narrowly scoped to the reproduced failure mode (leading short analog frame), prevents downstream silent corruption, and correctly avoids anchoring session time on suppressed frames by dropping before timestamp/gap processing. The bounded suppression cap is a pragmatic safety valve. Consider a follow-up firmware fix and/or broader session-boundary cleanup (e.g., leftover-frame handling) separately to avoid conflating behaviors.

Files changed (2) +264 / -8

Bug fix (1) +68 / -4

DaqifiStreamingDevice.csGuard stream start against leading short analog frames (issue #351) +68/-4

Guard stream start against leading short analog frames (issue #351)

• Introduces a per-session warmup guard armed in StartStreaming that suppresses leading analog-bearing frames with fewer values than enabled analog channels, capped at 5 frames. Moves channel snapshot earlier and drops malformed frames before timestamp reconstruction/gap detection so suppressed frames are a complete non-event, while leaving mid-stream short frames best-effort mapped. Adds a helper to count enabled analog channels.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Tests (1) +196 / -4

DaqifiStreamingDeviceDecodeTests.csAdd warmup-frame suppression coverage and re-scope short-frame test to mid-stream +196/-4

Add warmup-frame suppression coverage and re-scope short-frame test to mid-stream

• Renames and reworks the prior short-frame test to explicitly validate mid-stream best-effort mapping after a full frame clears the warmup guard. Adds a dedicated test region covering warmup suppression behavior, including clock anchoring, raw MessageReceived passthrough, digital-only streams, per-session re-arming, and the suppression cap.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceDecodeTests.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. Warmup drops digital payload ✓ Resolved 🐞 Bug ≡ Correctness ⭐ New

Description

When a leading short-analog warmup frame is suppressed, DecodeStreamFrame returns early before
timestamp/gap processing and before DecodeDigital runs, so any DigitalData in that same frame is
ignored. If the device emits combined analog+digital stream messages, the initial decoded digital
state/edge can be silently lost even though the raw frame is still re-raised.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R491-500]

+            if (_awaitingFirstFullAnalogFrame && (hasFloat || hasRawAnalog))
+            {
+                var analogValueCount = hasFloat ? message.AnalogInDataFloat.Count : message.AnalogInData.Count;
+                var enabledAnalogCount = CountEnabledAnalogChannels(channels);
+                if (enabledAnalogCount > 0 && analogValueCount < enabledAnalogCount
+                    && _suppressedWarmupFrameCount < MaxSuppressedWarmupFrames)
+                {
+                    _suppressedWarmupFrameCount++;
+                    return;
+                }

Relevance

⭐⭐ Medium

Team fixes digital-decode bugs, but warmup guard aims to drop whole frame; no history on mixed
analog+digital warmup.

PR-#279
PR-#280
PR-#353

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The warmup guard returns before the digital decode block, so digital payloads in suppressed frames
never reach per-channel decoding. The repo’s SD-card test builder demonstrates that a stream message
can legally carry both analog values and digital data in the same protobuf message, making this a
reachable behavior change.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[469-530]
src/Daqifi.Core.Tests/Device/SdCard/SdCardTestFileBuilder.cs[73-109]

Agent prompt

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

### Issue description
`DecodeStreamFrame` suppresses leading short-analog warmup frames by incrementing `_suppressedWarmupFrameCount` and `return`ing early. This prevents the later `DecodeDigital(...)` block from running, so any `DigitalData` contained in the suppressed frame is never decoded into per-channel digital samples.

### Issue Context
The message model and repo utilities explicitly allow a single `DaqifiOutMessage` to carry both analog and digital payloads. The current suppression is intentionally a “complete non-event” for timestamp/gap anchoring, but that also makes digital decoding a casualty.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[469-530]

### What to change
Adjust suppression so it does **not** skip digital decoding when `message.DigitalData.Length > 0`.

Concrete options:
1) **Suppress only analog decode**: keep timestamp/gap processing and allow `DecodeDigital(...)` to run, but skip `DecodeAnalog(...)` for suppressed warmup frames.
2) If you must preserve “next frame anchors the clock”, then suppress analog but still decode digital using a timestamp strategy you deem correct (e.g., process timestamp for this frame, decode digital, and then re-`Reset` timestamp/gap state so the next frame re-anchors), and add a unit test for combined warmup frames.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Guard suppresses post-enable frames ✓ Resolved 🐞 Bug ≡ Correctness

Description

StartStreaming always arms _awaitingFirstFullAnalogFrame, but DecodeStreamFrame only clears it when
an analog-bearing frame arrives, so a digital-only start leaves the guard armed. If analog channels
are enabled later during the same streaming session and the first analog-bearing frames are short,
up to MaxSuppressedWarmupFrames (5) frames will be dropped even though they are no longer “leading”
at session start.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R486-498]

+            if (_awaitingFirstFullAnalogFrame && (hasFloat || hasRawAnalog))
+            {
+                var analogValueCount = hasFloat ? message.AnalogInDataFloat.Count : message.AnalogInData.Count;
+                var enabledAnalogCount = CountEnabledAnalogChannels(channels);
+                if (enabledAnalogCount > 0 && analogValueCount < enabledAnalogCount
+                    && _suppressedWarmupFrameCount < MaxSuppressedWarmupFrames)
+                {
+                    _suppressedWarmupFrameCount++;
+                    return;
+                }
+
+                _awaitingFirstFullAnalogFrame = false;
+            }

Relevance

⭐⭐ Medium

No prior evidence for this specific guard/state bug; team often accepts streaming-correctness fixes
(e.g., PRs 279,353).

PR-#279
PR-#353

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The guard is armed on every StartStreaming, but DecodeStreamFrame only clears it inside the
(hasFloat||hasRawAnalog) block, so digital-only frames never transition it; since channel
enable/disable is allowed via SetChannelsEnabled during streaming, analog can become enabled later
in the same session while the guard is still armed.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[304-328]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[464-525]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1069-1118]

Agent prompt

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

### Issue description
`_awaitingFirstFullAnalogFrame` is armed unconditionally in `StartStreaming()`, but it is only evaluated/cleared when an analog-bearing frame arrives in `DecodeStreamFrame()`. This means a streaming session that begins with only digital frames keeps the warmup guard armed; if analog channels are later enabled mid-session and the first analog-bearing frames are short, those frames will be suppressed (up to the existing cap of 5).

### Issue Context
Channel enablement can happen during a streaming session (no guard that requires stopping/restarting). The warmup guard was described/scoped as a *stream start* mitigation, so retaining it until “first analog-bearing frame” can cause suppression that is temporally far removed from session start.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[304-328]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[464-525]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1069-1118]

### Suggested fix approach
- In `StartStreaming()`, arm `_awaitingFirstFullAnalogFrame` only when there is at least one **currently enabled** analog channel (e.g., `CountEnabledAnalogChannels(SnapshotChannels()) > 0`).
- Optionally add a unit test covering: start streaming with digital-only enabled, then enable analog mid-stream and verify the first short analog frame is **not** treated as a warmup-frame suppression case (or, if you want that behavior, codify it explicitly and document it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Previous review results

Review updated until commit d94a274

Results up to commit abc32e6 ⚖️ Balanced

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Guard suppresses post-enable frames ✓ Resolved 🐞 Bug ≡ Correctness

Description

StartStreaming always arms _awaitingFirstFullAnalogFrame, but DecodeStreamFrame only clears it when
an analog-bearing frame arrives, so a digital-only start leaves the guard armed. If analog channels
are enabled later during the same streaming session and the first analog-bearing frames are short,
up to MaxSuppressedWarmupFrames (5) frames will be dropped even though they are no longer “leading”
at session start.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R486-498]

+            if (_awaitingFirstFullAnalogFrame && (hasFloat || hasRawAnalog))
+            {
+                var analogValueCount = hasFloat ? message.AnalogInDataFloat.Count : message.AnalogInData.Count;
+                var enabledAnalogCount = CountEnabledAnalogChannels(channels);
+                if (enabledAnalogCount > 0 && analogValueCount < enabledAnalogCount
+                    && _suppressedWarmupFrameCount < MaxSuppressedWarmupFrames)
+                {
+                    _suppressedWarmupFrameCount++;
+                    return;
+                }
+
+                _awaitingFirstFullAnalogFrame = false;
+            }

Relevance

⭐⭐ Medium

No prior evidence for this specific guard/state bug; team often accepts streaming-correctness fixes
(e.g., PRs 279,353).

PR-#279
PR-#353

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The guard is armed on every StartStreaming, but DecodeStreamFrame only clears it inside the
(hasFloat||hasRawAnalog) block, so digital-only frames never transition it; since channel
enable/disable is allowed via SetChannelsEnabled during streaming, analog can become enabled later
in the same session while the guard is still armed.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[304-328]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[464-525]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1069-1118]

Agent prompt

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

### Issue description
`_awaitingFirstFullAnalogFrame` is armed unconditionally in `StartStreaming()`, but it is only evaluated/cleared when an analog-bearing frame arrives in `DecodeStreamFrame()`. This means a streaming session that begins with only digital frames keeps the warmup guard armed; if analog channels are later enabled mid-session and the first analog-bearing frames are short, those frames will be suppressed (up to the existing cap of 5).

### Issue Context
Channel enablement can happen during a streaming session (no guard that requires stopping/restarting). The warmup guard was described/scoped as a *stream start* mitigation, so retaining it until “first analog-bearing frame” can cause suppression that is temporally far removed from session start.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[304-328]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[464-525]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1069-1118]

### Suggested fix approach
- In `StartStreaming()`, arm `_awaitingFirstFullAnalogFrame` only when there is at least one **currently enabled** analog channel (e.g., `CountEnabledAnalogChannels(SnapshotChannels()) > 0`).
- Optionally add a unit test covering: start streaming with digital-only enabled, then enable analog mid-stream and verify the first short analog frame is **not** treated as a warmup-frame suppression case (or, if you want that behavior, codify it explicitly and document it).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 565a9ba

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit d94a274

[tylerkron]

✅ Ready for review — Qodo is clean (0 bugs / 0 rule violations, both prior findings resolved) and CI is green on d94a274. Not merging; leaving for your review.