perf(streaming): stop rebuilding the channel map and copying the buffer on every frame

[tylerkron]

What was wrong

A DAQiFi streaming at 1 kHz made the host do the same throwaway work a thousand times a second. For every frame, Core re-derived which analog channels were active — copying the whole channel list out under the device's lock, filtering it into a fresh list, and sorting that list — for a set that only changes when someone configures the device. Underneath it, the message consumer copied its entire accumulation buffer on every read, and then wrapped each frame twice more on its way to an event that, in practice, nobody had subscribed to.

None of it was visible as a bug. It showed up as an application that streams for hours quietly generating gigabytes of garbage, with the decode thread contending against EnableChannel for the same lock the whole time.

How it was fixed

The active-channel set is worked out once and cached, and rebuilt only when the device says the channel state actually moved; the parser is handed a view over the buffer instead of a copy; and the per-frame wrappers are built only when something is listening.

What a reviewer may want to push back on:

• A cache of "which channels are active" is only safe if it can never go stale, and IChannel.IsEnabled has a public setter. A caller writing channel.IsEnabled = true straight onto a channel is legal and no device API sees it — so a version counter bumped only from the device's own configure paths would leave the decoder mapping frame values onto the previously active channels, filing AI1's reading under AI0, silently and forever. So the channel types now raise an internal notification when their enabled state actually changes, the device subscribes to every channel it owns, and that is what moves the version. Tests cover all three routes in (device API, direct write, status repopulation) and the two lifecycle edges (a dropped channel stops moving the version; a reused one still moves it exactly once).
• The version only moves when the channel set really changes. A status message that describes the channels the device already has reuses those instances, so the membership check compares them by reference and order — not by (type, number), which would report "unchanged" for a set whose instances had been replaced and leave the cache writing samples into channels the device no longer has.
• The version is read before the snapshot, not after. Read afterwards, a change landing in between would stamp a pre-change snapshot with the post-change version — undetectable by any later frame. Read before, the worst case is one redundant rebuild.
• StreamMessageConsumer grew a second, internal event. MessageReceived carries a snapshot of everything buffered at the time of the read; taking that snapshot is the per-read copy. No subscriber inside Core has ever looked at it, so Core's three subscribers moved to a new internal MessageParsed, and the snapshot is now taken only when the public event has a subscriber. The public event's behaviour is unchanged for anyone using it, including when an internal MessageParsed handler throws — that is isolated, so it cannot withhold a message from an external subscriber.
• Two documented behaviour changes for subclasses. DaqifiDevice.OnMessageReceived is no longer called for a frame when MessageReceived has no subscribers (the frame would have to be wrapped to be passed to it, and that wrapper was the allocation); overrides that must see every frame should use the classified OnStatusMessageReceived/OnStreamMessageReceived, which stay unconditional. And StreamMessageConsumer.OnMessageReceived's rawData argument is empty in that same no-subscriber case. Both are stated in the XML docs.
• IMessageParser<T> gained a span overload as a default interface method, so existing parsers keep compiling and behave exactly as before; ProtobufMessageParser and LineBasedMessageParser implement it for real (the protobuf parser's body already worked in spans — only its entry point demanded an array).
• Item 1's proposed invalidation in the issue was not implementable as written (it invalidates from PopulateChannelsFromStatus / ChannelControlOperations only, which is exactly the hole above). The notification approach is the correction.

Verification

Measured, branch vs. origin/main, same harness, 16 analog + 16 digital channels, reproducible across runs:

	before	after
decode path, bytes/frame	2312	1648
decode path, µs/frame	1.79	1.36
decode path, gen0 collections / 200k frames	55	39
consumer path, bytes/frame	1385	1310

The 664 B/frame saved on decode accounts for the channel-snapshot array, the intermediate list and its growth steps, and the two per-frame wrappers — roughly seven objects per frame. At 1 kHz that is ~2.4 GB/hour of gen0 churn that no longer happens, which is the order the issue estimated. The 75 B/frame saved on the consumer path is, as expected, almost exactly the frame size: that copy was the wire throughput, duplicated. The remaining ~1.3 KB/frame is the generated DaqifiOutMessage itself, which the issue puts explicitly out of scope.

Tests — 39 new. Proven regression catchers by reverting each half of the fix: removing the enablement notification fails 10 tests (including the direct-write mapping cases and the warm-up guard), removing the repopulation version bump fails 2, removing the unsubscribe on repopulation fails 2, making the version bump unconditional again fails 1, restoring the unconditional buffer snapshot fails 1, and neutering the MessageParsed isolation fails 1. Full suite green on net9.0 (3014 Core + 95 Mcp) and net10.0 (3014), 0 failures, 0 warnings.

Bench (non-destructive), Nq1 fw 3.7.2 on /dev/cu.usbmodem1101 — example CLI built against this branch:

• --discover-serial found the unit (sn=9090539562006014104, fw 3.7.2) — this also exercises the changed subscription in SerialDeviceFinder.
• 3 s @ 500 Hz on channels 0,1,2 → 1186 samples, three analog values per row (this unit's known ~79% clock ratio).
• Channel mapping re-checked across three different masks at 200 Hz: mask 1 → 1 value/row, mask 3 → 2, mask 21 (channels 0, 2, 4) → 3, with distinct values per channel. That is the cached channel map being rebuilt correctly for each session on real hardware.
• --sd-list (47 files) and --sd-storage (7.80 GB, 0.0% used) — the text-exchange path, whose line collection also moved to the new event.
• Discovery, status, streaming and SD queries only. No reboot, no format, no delete, no SD:GET, no firmware, no LAN writes.

closes #490

Not merging — this is for your review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

perf(streaming): cache active channels and avoid per-read buffer/frame allocations

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Cache active analog-channel ordering and rebuild only when channel state changes.
• Parse from buffer views and snapshot raw bytes only when public subscribers exist.
• Avoid per-frame wrapper allocations by routing typed protobuf messages directly.

Diagram

graph TD
  ext["Inbound bytes / frames"] --> cons["StreamMessageConsumer"] --> ip["IMessageParser (span)"] --> dev["DaqifiDevice routing"]
  dev --> dec["StreamFrameDecoder"]
  dev --> ch["Channels (enablement notify)"] --> ver["ChannelStateVersion"] --> dec

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make channel enablement immutable (device-only setters)

• ➕ Eliminates the risk of cache staleness from direct IsEnabled writes
• ➕ Avoids internal notifier/event plumbing
• ➖ Breaking API/behavior change for consumers that set IsEnabled directly
• ➖ Doesn’t address buffer-copy/per-read allocation issues by itself

2. Replace StreamMessageConsumer buffering with System.IO.Pipelines

• ➕ Naturally supports zero-copy parsing and backpressure
• ➕ Can reduce custom buffer management and lock usage
• ➖ Much larger refactor and higher risk for a hot path
• ➖ More complexity for maintainers; requires broader test and perf validation

3. Expose a public channel-change token/event for general consumers

• ➕ Lets non-decoder consumers cache channel-derived views safely
• ➕ More explicit contract than an internal version property
• ➖ Increases public API surface area and long-term compatibility burden
• ➖ May be unnecessary if only internal consumers need it today

Recommendation: The PR’s approach is the best tradeoff: it removes the dominant per-frame/per-read allocations while keeping public semantics stable. The internal enablement notification + ChannelStateVersion specifically addresses the otherwise-fatal staleness risk caused by IChannel.IsEnabled’s public setter, without forcing a breaking API change. Pipelines/immutability are valid longer-term directions but are disproportionate for the targeted perf win here.

Files changed (22) +1446 / -64

Enhancement (12) +498 / -61

AnalogChannel.csRaise internal enablement-change notification on IsEnabled transitions +29/-2

Raise internal enablement-change notification on IsEnabled transitions

• Implements IChannelEnablementNotifier and updates IsEnabled to notify subscribers only on value changes, invoking outside the channel lock to avoid re-entrancy coupling.

src/Daqifi.Core/Channel/AnalogChannel.cs

DigitalChannel.csRaise internal enablement-change notification on IsEnabled transitions +26/-2

Raise internal enablement-change notification on IsEnabled transitions

• Mirrors AnalogChannel behavior by implementing IChannelEnablementNotifier, notifying on real transitions only and firing outside the lock.

src/Daqifi.Core/Channel/DigitalChannel.cs

IChannelEnablementNotifier.csIntroduce internal enablement notifier interface for channels +30/-0

Introduce internal enablement notifier interface for channels

• Adds an internal event-based hook so devices can observe direct IsEnabled writes and invalidate cached channel-derived views safely.

src/Daqifi.Core/Channel/IChannelEnablementNotifier.cs

IMessageParser.csAdd span-based parsing entry point with safe default fallback +24/-0

Add span-based parsing entry point with safe default fallback

• Extends IMessageParser with ParseMessages(ReadOnlySpan<byte>, ...) to enable zero-copy parsing from accumulation buffers while keeping existing parsers working via a default ToArray fallback.

src/Daqifi.Core/Communication/Consumers/IMessageParser.cs

LineBasedMessageParser.csOverride span parsing to avoid per-line and per-read allocations +17/-5

Override span parsing to avoid per-line and per-read allocations

• Implements the span-based parse path and decodes strings directly from slices rather than copying into intermediate arrays.

src/Daqifi.Core/Communication/Consumers/LineBasedMessageParser.cs

ProtobufMessageParser.csOverride span parsing to avoid StreamMessageConsumer buffer copies +17/-3

Override span parsing to avoid StreamMessageConsumer buffer copies

• Moves parsing entry to ReadOnlySpan while preserving existing logic, converting internal helpers to span-based scanning for frame boundaries.

src/Daqifi.Core/Communication/Consumers/ProtobufMessageParser.cs

StreamMessageConsumer.csAvoid buffer copies and add internal MessageParsed event +50/-8

Avoid buffer copies and add internal MessageParsed event

• Bulk-appends read bytes, parses from a span over the accumulation buffer, snapshots raw bytes only when public MessageReceived is subscribed, and introduces MessageParsed for allocation-free internal subscribers.

src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs

DaqifiDevice.csAdd ChannelStateVersion and optimize inbound routing allocations +153/-12

Add ChannelStateVersion and optimize inbound routing allocations

• Introduces an internal channel-state change token, wires device subscriptions to per-channel enablement changes, raises undifferentiated MessageReceived only when subscribed, and prefers MessageParsed + typed protobuf routing to avoid per-frame wrappers.

src/Daqifi.Core/Device/DaqifiDevice.cs

SerialDeviceFinder.csUse MessageParsed to avoid raw-buffer snapshot during discovery probe +5/-2

Use MessageParsed to avoid raw-buffer snapshot during discovery probe

• Switches the status-probe subscription to the cheaper parsed-only event, preventing unnecessary per-read raw buffer copies during discovery.

src/Daqifi.Core/Device/Discovery/SerialDeviceFinder.cs

StreamFrameDecoder.csCache active analog channel mapping keyed by ChannelStateVersion +115/-18

Cache active analog channel mapping keyed by ChannelStateVersion

• Adds a consumer-thread-only cache of the channel snapshot and sorted enabled analog channels; refreshes only when the host’s version changes and uses the cached mapping for decode and warmup/guard logic.

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

TextExchangeEngine.csUse MessageParsed for text exchanges to avoid raw-buffer snapshots +5/-2

Use MessageParsed for text exchanges to avoid raw-buffer snapshots

• Switches line-collection to the parsed-only event, avoiding per-read raw-buffer copies that the exchange does not use.

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

ProtobufProtocolHandler.csAdd typed Handle(DaqifiOutMessage) to avoid per-frame wrapper allocations +27/-7

Add typed Handle(DaqifiOutMessage) to avoid per-frame wrapper allocations

• Refactors HandleAsync to delegate to a new synchronous typed Handle method, routing protobuf messages without requiring an IInboundMessage wrapper when the caller already has a DaqifiOutMessage.

src/Daqifi.Core/Device/Protocol/ProtobufProtocolHandler.cs

Refactor (2) +6 / -0

DaqifiStreamingDevice.csExpose ChannelStateVersion via IDeviceOperationHost +3/-0

Expose ChannelStateVersion via IDeviceOperationHost

• Implements the new host seam property so StreamFrameDecoder can invalidate channel caches without resnapshotting per frame.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

IDeviceOperationHost.csExtend decoder host seam with ChannelStateVersion +3/-0

Extend decoder host seam with ChannelStateVersion

• Adds ChannelStateVersion to the internal host interface to support cache invalidation for channel-derived decode state.

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

Tests (8) +942 / -3

ChannelEnablementNotificationTests.csAdd tests for enablement-change notifications on channels +82/-0

Add tests for enablement-change notifications on channels

• Introduces coverage ensuring AnalogChannel/DigitalChannel raise an internal enablement-changed callback only on real value transitions, allow handler readback, and stop notifying after unsubscribe.

src/Daqifi.Core.Tests/Channel/ChannelEnablementNotificationTests.cs

StreamMessageConsumerBufferCopyTests.csAdd tests ensuring StreamMessageConsumer avoids per-read copies +256/-0

Add tests ensuring StreamMessageConsumer avoids per-read copies

• Validates that parsing uses the span entry point, raw-buffer snapshots occur only when MessageReceived has subscribers, events observe identical message ordering, and framing still reassembles across reads.

src/Daqifi.Core.Tests/Communication/Consumers/StreamMessageConsumerBufferCopyTests.cs

ChannelStateVersionTests.csAdd end-to-end tests for ChannelStateVersion invalidation +232/-0

Add end-to-end tests for ChannelStateVersion invalidation

• Covers version movement on status repopulation and direct IsEnabled writes, no movement on idempotent writes, correct subscription lifecycle on dropped/reused channels, and next-frame correctness through streaming decode.

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

DeviceAdministrationOperationsTests.csUpdate test host seam to include ChannelStateVersion +1/-0

Update test host seam to include ChannelStateVersion

• Extends the internal test host interface implementation to satisfy the new ChannelStateVersion contract used by the decoder cache.

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

LiveSampleStreamTests.csUpdate fake host to provide a meaningful ChannelStateVersion +8/-0

Update fake host to provide a meaningful ChannelStateVersion

• Adds a simple version token to the test double so future caching callers don’t pass tests with a lying fake while failing on real devices.

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

StreamFrameDecoderTests.csAdd decoder tests for channel-map caching and invalidation +184/-3

Add decoder tests for channel-map caching and invalidation

• Adds coverage that steady-state decoding snapshots channels once, rebuilds exactly once on state change, reflects direct enable/disable on next frame, preserves channel-number ordering, and keeps warmup suppression correct.

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

ProtobufProtocolHandlerTests.csAdd tests for typed ProtobufProtocolHandler.Handle routing +65/-0

Add tests for typed ProtobufProtocolHandler.Handle routing

• Validates the new typed Handle(DaqifiOutMessage) routes equivalently to HandleAsync and rejects null, preventing per-frame wrapper allocations while preserving classification semantics.

src/Daqifi.Core.Tests/Device/Protocol/ProtobufProtocolHandlerTests.cs

UndifferentiatedMessageRaiseTests.csAdd tests for conditional raising of DaqifiDevice.MessageReceived +114/-0

Add tests for conditional raising of DaqifiDevice.MessageReceived

• Ensures the undifferentiated (allocating) event is raised only when subscribed, while classified status/stream events remain unconditional.

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

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

1. MessageParsed blocks MessageReceived ✓ Resolved 🐞 Bug ☼ Reliability

Description

StreamMessageConsumer.OnMessageReceived invokes MessageParsed before the public MessageReceived
without isolating exceptions between them. If any MessageParsed subscriber throws, MessageReceived
will not fire for that message, which can break external consumers even though only an internal
subscriber failed.

Code

src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[R706-710]

    protected virtual void OnMessageReceived(IInboundMessage<T> message, byte[] rawData)
    {
+        MessageParsed?.Invoke(message);
        MessageReceived?.Invoke(this, new MessageReceivedEventArgs<T>(message, rawData));
    }

Relevance

●●● Strong

Team has precedent hardening StreamMessageConsumer callbacks so one subscriber can’t break
processing.

PR-#415

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

OnMessageReceived invokes MessageParsed and MessageReceived sequentially without isolation;
ProcessMessageBuffer only catches exceptions around the whole OnMessageReceived call per message, so
a throw in MessageParsed aborts the rest of OnMessageReceived for that message.

src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[648-692]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[706-710]

Agent prompt

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

### Issue description
`StreamMessageConsumer<T>.OnMessageReceived` calls `MessageParsed?.Invoke(...)` and then `MessageReceived?.Invoke(...)` back-to-back. An exception from any `MessageParsed` handler prevents the public `MessageReceived` event from being raised for that message.

### Issue Context
Dispatch exceptions are caught per-message in `ProcessMessageBuffer`, so a throwing `MessageParsed` handler won’t crash the thread, but it will still suppress delivery of the public event for the affected message(s).

### Fix Focus Areas
- src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[648-710]

### Suggested fix
Wrap `MessageParsed` and `MessageReceived` invocations in separate try/catch blocks (or implement per-subscriber isolation if that is the intended contract). For example:
- `try { MessageParsed?.Invoke(message); } catch (Exception ex) { SafeRaiseError(ex); }`
- `try { MessageReceived?.Invoke(...); } catch (Exception ex) { SafeRaiseError(ex); }`

This ensures an internal `MessageParsed` failure can’t prevent external `MessageReceived` delivery.

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

---

2. Version bumps every status ✓ Resolved 🐞 Bug ➹ Performance

Description

DaqifiDevice.PopulateChannelsFromStatus unconditionally increments _channelStateVersion, even when
the channel membership and enabled states are unchanged. This contradicts the documented
change-token semantics and can trigger unnecessary StreamFrameDecoder cache rebuilds on each status
repopulation.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R3955-3957]

+                // The membership itself changed, which no per-channel notification covers.
+                Interlocked.Increment(ref _channelStateVersion);
+

Relevance

●● Moderate

PR intent/tests suggest version should move on status repopulation; conditional bump may be debated
without clear precedent.

PR-#250
PR-#428

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ChannelStateVersion is documented as a change token for channel set/enabled state changes, but
PopulateChannelsFromStatus increments it unconditionally. StreamFrameDecoder rebuilds its cached
channel views whenever the version differs, and StatusChannelPopulator can repopulate while reusing
the same channel instances (often only updating scaling), so repeated status refreshes can churn the
version and rebuild caches unnecessarily.

src/Daqifi.Core/Device/DaqifiDevice.cs[274-301]
src/Daqifi.Core/Device/DaqifiDevice.cs[3922-3959]
src/Daqifi.Core/Device/Internal/StreamFrameDecoder.cs[444-501]
src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs[55-100]
src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs[157-165]

Agent prompt

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

### Issue description
`PopulateChannelsFromStatus` always increments `_channelStateVersion` after swapping `_channels`, even if the effective channel membership (type/number set) did not change and `IsEnabled` did not change. Since `StreamFrameDecoder` uses this version to decide when to rebuild cached channel views, this can cause avoidable rebuilds under repeated status refreshes.

### Issue Context
- The XML docs describe `ChannelStateVersion` as changing when the channel set or enabled states change.
- `StatusChannelPopulator` can reuse the same channel instances and only update scaling, meaning many status populations may not change membership/enablement.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[3922-3958]
- src/Daqifi.Core/Device/DaqifiDevice.cs[274-301]

### Suggested fix
Compute whether the *membership* actually changed before incrementing `_channelStateVersion`:
- Compare previous vs new channel identity sets (e.g., count and `(Type, ChannelNumber)` sequence) before clearing `_channels`.
- Increment `_channelStateVersion` only when membership differs.

Rely on the per-channel `EnablementChanged` subscription (already in this PR) to move the version when `IsEnabled` truly changes.

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

---

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Previous review results

Review updated until commit 3fd4594 ⚖️ Balanced

Results up to commit eda9c79 ⚖️ Balanced

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

1. MessageParsed blocks MessageReceived ✓ Resolved 🐞 Bug ☼ Reliability

Description

StreamMessageConsumer.OnMessageReceived invokes MessageParsed before the public MessageReceived
without isolating exceptions between them. If any MessageParsed subscriber throws, MessageReceived
will not fire for that message, which can break external consumers even though only an internal
subscriber failed.

Code

src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[R706-710]

    protected virtual void OnMessageReceived(IInboundMessage<T> message, byte[] rawData)
    {
+        MessageParsed?.Invoke(message);
        MessageReceived?.Invoke(this, new MessageReceivedEventArgs<T>(message, rawData));
    }

Relevance

●●● Strong

Team has precedent hardening StreamMessageConsumer callbacks so one subscriber can’t break
processing.

PR-#415

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

OnMessageReceived invokes MessageParsed and MessageReceived sequentially without isolation;
ProcessMessageBuffer only catches exceptions around the whole OnMessageReceived call per message, so
a throw in MessageParsed aborts the rest of OnMessageReceived for that message.

src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[648-692]
src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[706-710]

Agent prompt

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

### Issue description
`StreamMessageConsumer<T>.OnMessageReceived` calls `MessageParsed?.Invoke(...)` and then `MessageReceived?.Invoke(...)` back-to-back. An exception from any `MessageParsed` handler prevents the public `MessageReceived` event from being raised for that message.

### Issue Context
Dispatch exceptions are caught per-message in `ProcessMessageBuffer`, so a throwing `MessageParsed` handler won’t crash the thread, but it will still suppress delivery of the public event for the affected message(s).

### Fix Focus Areas
- src/Daqifi.Core/Communication/Consumers/StreamMessageConsumer.cs[648-710]

### Suggested fix
Wrap `MessageParsed` and `MessageReceived` invocations in separate try/catch blocks (or implement per-subscriber isolation if that is the intended contract). For example:
- `try { MessageParsed?.Invoke(message); } catch (Exception ex) { SafeRaiseError(ex); }`
- `try { MessageReceived?.Invoke(...); } catch (Exception ex) { SafeRaiseError(ex); }`

This ensures an internal `MessageParsed` failure can’t prevent external `MessageReceived` delivery.

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

---

2. Version bumps every status ✓ Resolved 🐞 Bug ➹ Performance

Description

DaqifiDevice.PopulateChannelsFromStatus unconditionally increments _channelStateVersion, even when
the channel membership and enabled states are unchanged. This contradicts the documented
change-token semantics and can trigger unnecessary StreamFrameDecoder cache rebuilds on each status
repopulation.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R3955-3957]

+                // The membership itself changed, which no per-channel notification covers.
+                Interlocked.Increment(ref _channelStateVersion);
+

Relevance

●● Moderate

PR intent/tests suggest version should move on status repopulation; conditional bump may be debated
without clear precedent.

PR-#250
PR-#428

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

ChannelStateVersion is documented as a change token for channel set/enabled state changes, but
PopulateChannelsFromStatus increments it unconditionally. StreamFrameDecoder rebuilds its cached
channel views whenever the version differs, and StatusChannelPopulator can repopulate while reusing
the same channel instances (often only updating scaling), so repeated status refreshes can churn the
version and rebuild caches unnecessarily.

src/Daqifi.Core/Device/DaqifiDevice.cs[274-301]
src/Daqifi.Core/Device/DaqifiDevice.cs[3922-3959]
src/Daqifi.Core/Device/Internal/StreamFrameDecoder.cs[444-501]
src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs[55-100]
src/Daqifi.Core/Device/Internal/StatusChannelPopulator.cs[157-165]

Agent prompt

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

### Issue description
`PopulateChannelsFromStatus` always increments `_channelStateVersion` after swapping `_channels`, even if the effective channel membership (type/number set) did not change and `IsEnabled` did not change. Since `StreamFrameDecoder` uses this version to decide when to rebuild cached channel views, this can cause avoidable rebuilds under repeated status refreshes.

### Issue Context
- The XML docs describe `ChannelStateVersion` as changing when the channel set or enabled states change.
- `StatusChannelPopulator` can reuse the same channel instances and only update scaling, meaning many status populations may not change membership/enablement.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[3922-3958]
- src/Daqifi.Core/Device/DaqifiDevice.cs[274-301]

### Suggested fix
Compute whether the *membership* actually changed before incrementing `_channelStateVersion`:
- Compare previous vs new channel identity sets (e.g., count and `(Type, ChannelNumber)` sequence) before clearing `_channels`.
- Increment `_channelStateVersion` only when membership differs.

Rely on the per-channel `EnablementChanged` subscription (already in this PR) to move the version when `IsEnabled` truly changes.

ⓘ 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 3fd4594

[tylerkron]

Qodo-clean, CI green — ready for review.

Round 2 on head 3fd4594: Bugs (0) / Rule violations (0) / Skill insights (0), both round-1 findings struck ✓ Resolved, 0 unresolved threads. 16 SHA references to 3fd4594 vs 1 to the previous head, so the review is on the current code. Settle re-check at +10 min: summary byte-identical, comment count unchanged, threads still 0. CI build SUCCESS; MERGEABLE.

Both round-1 findings were valid and taken. The second one's suggested fix was not — comparing (type, number) sequences would have called a set "unchanged" whose instances had been replaced, which is exactly the stale-cache corruption this PR exists to prevent; the check compares by reference instead. Details in the thread.

Bench re-run after those production fixes (non-destructive, Nq1 fw 3.7.2 on /dev/cu.usbmodem1101): discovery found the unit, 3 s @ 500 Hz on channels 0-2 gave 1185 samples with three analog values per row, channel masks 1/3/21 gave 1/2/3 values per row respectively, --sd-list 47 files. Allocation numbers re-measured and unchanged: 2312 → 1648 bytes/frame decode, 1.79 → 1.35 µs/frame across three runs each.