fix(device): parse protobuf field 22 so analog IsEnabled tracks the device (closes #409)

[tylerkron]

Summary

• Core previously never read analog_in_port_enabled (protobuf field 22) — analog channel IsEnabled was only ever set by Core's own EnableChannel/DisableChannel calls, so it could silently drift from the device's actual state (another session enabling a channel, state surviving a reconnect, a partially-applied command).
• PopulateAnalogChannels now parses field 22 and resyncs IsEnabled from it on every status frame that reports one — both for newly-created channels and for existing channel instances reused across a repopulation.
• Firmware that doesn't populate the field (pre-v3.5.0) sends an empty byte string, which is treated as "not reported" rather than "everything disabled" — existing/default behavior is preserved in that case.

This closes scope item 7 of #390, split out because #390 was closed by #404 with item 7 deliberately deferred. It directly addresses the staleness #404 introduced: DeviceCapabilities.CurrentMaximumRateHz is computed by the device from its enabled set, and Core's cached copy is only as good as Core's own view of that set.

Bench verification

The issue explicitly flagged the wire encoding of field 22 as unverified, so I confirmed it against a real Nq1 (fw 3.7.2) before committing to an implementation:

• My first assumption (one byte per channel, matching sibling fields like analog_in_port_type) was wrong. The bench showed the real encoding is a bit-packed mask, same layout Core already sends outbound via EnableAdcChannels: enabling channels 0 and 2 on a 16-channel device produced AnalogInPortEnabled = [5, 0] (2 bytes, little-endian, bit n = channel n), not a 16-byte array.
• Verified the drift-resync path directly: forced Core's local channel 5 IsEnabled = true without telling the device (no SCPI sent), then requested a fresh status frame — Core's view snapped back to false, matching the device's real state.

Test plan

• dotnet test — full suite passes (2184 tests, 2 pre-existing skips)
• New unit tests in ChannelPopulationTests.cs covering: single-byte mask, multi-byte mask (channel 9+), missing field 22 (old firmware) defaults to disabled, repopulation resyncs from device rather than preserving Core's stale value, truncated mask treats missing bytes as disabled
• Bench-tested against a real DAQiFi Nq1 (fw 3.7.2) over USB/serial, per above

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Sync analog channel IsEnabled from device-reported enabled bitmask (field 22)

🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Parse protobuf field 22 (analog_in_port_enabled) to resync analog channel IsEnabled from device
 state.
• Treat empty enabled mask as “not reported” for older firmware to preserve prior behavior.
• Add unit coverage for single/multi-byte masks, repopulation resync, and truncated/missing masks.

Diagram

graph TD
  FW{{"Device firmware"}} --> MSG("Status frame") --> POP["PopulateChannelsFromStatus"] --> ANA["PopulateAnalogChannels"] --> BIT["IsChannelBitSet"] --> CH["AnalogChannel.IsEnabled"]
  TEST["ChannelPopulationTests"] --> POP

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Split commanded vs reported enable state

• ➕ Preserves both Core’s requested state and the device’s actual state for diagnostics/UX.
• ➕ Avoids ambiguity when device doesn’t report field 22 but Core did issue enable commands.
• ➖ Requires API/model expansion (new properties or tri-state), increasing surface area and migration cost.
• ➖ More complex state reconciliation logic across reconnects/sessions.

2. Treat empty mask as “all disabled”

• ➕ Simpler logic: mask always considered authoritative.
• ➕ No special-case branch for old firmware.
• ➖ Breaks backward compatibility with firmware that never populates field 22 (empty ByteString).
• ➖ Would incorrectly disable channels in Core’s view after status refresh.

Recommendation: Current approach is the best fit: it makes IsEnabled reflect device truth when the device reports it, while preserving prior behavior for older firmware by treating an empty mask as “not reported”. The “commanded vs reported” split is a viable future enhancement if the product needs to expose divergence, but it’s heavier than needed to fix the drift/staleness bug described in #409.

Files changed (2) +153 / -3

Bug fix (1) +32 / -3

DaqifiDevice.csResync analog IsEnabled from protobuf field 22 enabled bitmask +32/-3

Resync analog IsEnabled from protobuf field 22 enabled bitmask

• Updates analog channel population to read AnalogInPortEnabled (field 22) as a little-endian bitmask and apply it to both newly created and reused analog channels. Introduces a helper to safely test channel bits (out-of-range treated as disabled) and gates resync on mask presence to avoid misinterpreting older firmware’s empty value.

src/Daqifi.Core/Device/DaqifiDevice.cs

Tests (1) +121 / -0

ChannelPopulationTests.csAdd tests for analog enabled-mask parsing and resync behavior +121/-0

Add tests for analog enabled-mask parsing and resync behavior

• Adds unit tests asserting IsEnabled is derived from the bit-packed AnalogInPortEnabled mask, including multi-byte masks. Covers backward compatibility (missing/empty field), repopulation resync of existing channel instances, and truncated masks defaulting missing bytes to disabled.

src/Daqifi.Core.Tests/Device/ChannelPopulationTests.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. Deadlock guard ineffective ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The test claims a CancellationTokenSource is a “deadlock safety net”, but cancellation cannot
interrupt a synchronous/blocking PopulateChannelsFromStatus call, so the resync task may never
observe cancellation and the test can still hang when it waits for the worker to stop.

Code

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[R265-267]

+            // Safety net so a regression that reintroduces a deadlock fails the test instead of
+            // hanging CI indefinitely.
+            using var stop = new CancellationTokenSource(TimeSpan.FromSeconds(30));

Relevance

●●● Strong

Team previously accepted bounding hangs with WaitAsync/[Fact Timeout] instead of unbounded awaits
(PR #364, #198).

PR-#364
PR-#198

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new code introduces a CTS-based “deadlock safety net” and only checks
stop.IsCancellationRequested in the loop; canceling the token cannot unblock a synchronous
PopulateChannelsFromStatus call if it is stuck, so the worker may not terminate and the test can
still hang when awaiting it.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[265-285]
src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[300-304]
PR-#364

Agent prompt

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

### Issue description
`EnableChannel_ConcurrentWithStatusResync_AlwaysSendsMaskIncludingJustEnabledChannel` introduces a CancellationTokenSource as a “deadlock safety net”, but the worker loop performs a synchronous `device.PopulateChannelsFromStatus(...)` call that cannot be canceled mid-call. If that call blocks (e.g., due to a regression), the token won’t be observed and the test can still hang when it waits for the worker to end.

### Issue Context
Cancellation tokens only help if the code under test cooperates (checks the token) or if the awaiting side uses a bounded wait. A thread blocked in `lock`/deadlock will not be released by `CancellationTokenSource.Cancel()`.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[265-304]

### Suggested fix
- Keep the CTS, but **also bound the wait for the worker** during cleanup, e.g.:
 - `await resyncTask.WaitAsync(TimeSpan.FromSeconds(5));` and fail the test on timeout, OR
 - `var completed = await Task.WhenAny(resyncTask, Task.Delay(...)); Assert.Same(resyncTask, completed);`
- Optionally pass `stop.Token` to `Task.Run(..., stop.Token)` and/or add an explicit timeout/failure message so CI fails deterministically instead of hanging.

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

---

2. Busy-spin status resync test ✓ Resolved 🐞 Bug ➹ Performance

Description

The new concurrency regression test runs PopulateChannelsFromStatus in a tight loop with no
yield/backoff, which can unnecessarily peg a CPU core and increase scheduling/lock contention,
making test runtime more variable (especially under parallel CI). This is avoidable without
weakening the race-coverage intent by adding a minimal yield/backoff and/or a bounded stress
duration.

Code

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[R265-272]

+            using var stop = new CancellationTokenSource();
+            var resyncTask = Task.Run(() =>
+            {
+                while (!stop.IsCancellationRequested)
+                {
+                    device.PopulateChannelsFromStatus(allDisabledStatus);
+                }
+            });

Relevance

●●● Strong

Team historically adds sleeps/timeouts to background polling loops to reduce CI flakiness/CPU (e.g.,
PR #403).

PR-#403
PR-#37
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test’s background task loops until cancellation and repeatedly calls
PopulateChannelsFromStatus without any delay/yield, which is a classic busy-spin pattern that can
drive high CPU usage during the test run.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[265-272]

Agent prompt

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

### Issue description
`EnableChannel_ConcurrentWithStatusResync_AlwaysSendsMaskIncludingJustEnabledChannel` starts a background task that repeatedly calls `PopulateChannelsFromStatus` in a tight `while` loop with no yielding or throttling. This can consume significant CPU during the test run and increase contention, making CI slower/flakier.

### Issue Context
This is a stress-style concurrency test; it should still exercise interleavings, but it doesn’t need to busy-spin at maximum speed to do so.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[265-272]

### Suggested fix
Update the loop to include a minimal yield/backoff while still keeping high concurrency pressure, e.g.:
- Use `SpinWait` (`var sw = new SpinWait(); sw.SpinOnce();`) inside the loop, or
- Call `Thread.Yield()` each iteration, or
- Convert the delegate to `async` and `await Task.Yield()` occasionally.

Optionally pass `stop.Token` into `Task.Run(..., stop.Token)` and/or add a safety timeout (e.g., `CancelAfter`) so the test can’t hang indefinitely if a regression deadlocks.

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

---

3. IsEnabled resync race ✓ Resolved 🐞 Bug ☼ Reliability

Description

PopulateAnalogChannels now overwrites AnalogChannel.IsEnabled from inbound status frames, while
DaqifiStreamingDevice enable/disable APIs also write IsEnabled and then recompute/send the ADC
enable mask without synchronizing with status processing. If a status frame arrives mid
enable/disable, Core can send a mask computed from a clobbered intermediate IsEnabled state, leaving
device enable state incorrect and causing local IsEnabled to flip-flop.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1909-1912]

+                    if (enabledIsReported)
+                    {
+                        existingAnalog.IsEnabled = IsChannelBitSet(analogInPortEnabled, i);
+                    }

Relevance

●●● Strong

Team repeatedly accepts channel-state thread-safety/race fixes (locks/atomic updates) in
PopulateChannelsFromStatus + management APIs.

PR-#250
PR-#314
PR-#196

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Status processing always calls PopulateChannelsFromStatus on the consumer thread, and the PR added a
write to existingAnalog.IsEnabled when AnalogInPortEnabled is present; channel-management APIs
concurrently mutate IsEnabled and then compute/send the enable mask from current IsEnabled values.
Since status messages can be periodic during streaming, the two paths can overlap and produce
incorrect masks.

src/Daqifi.Core/Device/DaqifiDevice.cs[1698-1710]
src/Daqifi.Core/Device/DaqifiDevice.cs[1906-1914]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1178-1227]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1268]
src/Daqifi.Core/Device/DeviceMetadata.cs[70-74]
PR-#57

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` now updates `AnalogChannel.IsEnabled` from the device-reported enabled mask. This introduces a new concurrent writer to `IsEnabled` (consumer/status thread) that can race with caller-thread operations like `SetChannelsEnabled(...)`/`DisableAllChannels()` in `DaqifiStreamingDevice`, which (1) mutate `IsEnabled` and then (2) recompute and send the ADC enable mask.

Because these operations don’t share a lock/critical section, a status frame can interleave between the `IsEnabled` mutation and `SendAdcEnableMask()`’s recomputation, resulting in an incorrect mask being sent (and inconsistent local vs device state).

### Issue Context
- Status frames can occur during streaming (periodic status emissions), so overlap with enable/disable calls is realistic.
- This PR added the status-path `IsEnabled` assignment, creating the new race.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1866-1926]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1183-1227]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1268]

### Suggested remediation
- Introduce a shared critical section between status repopulation and channel enable/disable operations.
 - Option A: Add a `protected` helper on `DaqifiDevice` like `WithChannelsLock<T>(Func<T>)` / `WithChannelsLock(Action)` that locks the same `_channelsLock` used by `PopulateChannelsFromStatus`.
 - In `SetChannelsEnabled`/`DisableAllChannels`, perform **(a)** `IsEnabled` mutations and **(b)** recomputation of ADC/DIO outbound state under that lock, but **send SCPI commands outside the lock** using computed values to avoid holding the lock across I/O.
- Alternatively, add a dedicated lock for enable-state+mask recomputation that is acquired by both the status-path update and the channel-management API.

ⓘ 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.}

Previous review results

Review updated until commit 3a0eaf6

Results up to commit 9473540 ⚖️ Balanced

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

1. IsEnabled resync race ✓ Resolved 🐞 Bug ☼ Reliability

Description

PopulateAnalogChannels now overwrites AnalogChannel.IsEnabled from inbound status frames, while
DaqifiStreamingDevice enable/disable APIs also write IsEnabled and then recompute/send the ADC
enable mask without synchronizing with status processing. If a status frame arrives mid
enable/disable, Core can send a mask computed from a clobbered intermediate IsEnabled state, leaving
device enable state incorrect and causing local IsEnabled to flip-flop.

Code

src/Daqifi.Core/Device/DaqifiDevice.cs[R1909-1912]

+                    if (enabledIsReported)
+                    {
+                        existingAnalog.IsEnabled = IsChannelBitSet(analogInPortEnabled, i);
+                    }

Relevance

●●● Strong

Team repeatedly accepts channel-state thread-safety/race fixes (locks/atomic updates) in
PopulateChannelsFromStatus + management APIs.

PR-#250
PR-#314
PR-#196

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Status processing always calls PopulateChannelsFromStatus on the consumer thread, and the PR added a
write to existingAnalog.IsEnabled when AnalogInPortEnabled is present; channel-management APIs
concurrently mutate IsEnabled and then compute/send the enable mask from current IsEnabled values.
Since status messages can be periodic during streaming, the two paths can overlap and produce
incorrect masks.

src/Daqifi.Core/Device/DaqifiDevice.cs[1698-1710]
src/Daqifi.Core/Device/DaqifiDevice.cs[1906-1914]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1178-1227]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1268]
src/Daqifi.Core/Device/DeviceMetadata.cs[70-74]
PR-#57

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` now updates `AnalogChannel.IsEnabled` from the device-reported enabled mask. This introduces a new concurrent writer to `IsEnabled` (consumer/status thread) that can race with caller-thread operations like `SetChannelsEnabled(...)`/`DisableAllChannels()` in `DaqifiStreamingDevice`, which (1) mutate `IsEnabled` and then (2) recompute and send the ADC enable mask.

Because these operations don’t share a lock/critical section, a status frame can interleave between the `IsEnabled` mutation and `SendAdcEnableMask()`’s recomputation, resulting in an incorrect mask being sent (and inconsistent local vs device state).

### Issue Context
- Status frames can occur during streaming (periodic status emissions), so overlap with enable/disable calls is realistic.
- This PR added the status-path `IsEnabled` assignment, creating the new race.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[1866-1926]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1183-1227]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1234-1268]

### Suggested remediation
- Introduce a shared critical section between status repopulation and channel enable/disable operations.
 - Option A: Add a `protected` helper on `DaqifiDevice` like `WithChannelsLock<T>(Func<T>)` / `WithChannelsLock(Action)` that locks the same `_channelsLock` used by `PopulateChannelsFromStatus`.
 - In `SetChannelsEnabled`/`DisableAllChannels`, perform **(a)** `IsEnabled` mutations and **(b)** recomputation of ADC/DIO outbound state under that lock, but **send SCPI commands outside the lock** using computed values to avoid holding the lock across I/O.
- Alternatively, add a dedicated lock for enable-state+mask recomputation that is acquired by both the status-path update and the channel-management API.

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

---

Results up to commit 42fc002 ⚖️ Balanced

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

1. Busy-spin status resync test ✓ Resolved 🐞 Bug ➹ Performance

Description

The new concurrency regression test runs PopulateChannelsFromStatus in a tight loop with no
yield/backoff, which can unnecessarily peg a CPU core and increase scheduling/lock contention,
making test runtime more variable (especially under parallel CI). This is avoidable without
weakening the race-coverage intent by adding a minimal yield/backoff and/or a bounded stress
duration.

Code

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[R265-272]

+            using var stop = new CancellationTokenSource();
+            var resyncTask = Task.Run(() =>
+            {
+                while (!stop.IsCancellationRequested)
+                {
+                    device.PopulateChannelsFromStatus(allDisabledStatus);
+                }
+            });

Relevance

●●● Strong

Team historically adds sleeps/timeouts to background polling loops to reduce CI flakiness/CPU (e.g.,
PR #403).

PR-#403
PR-#37
PR-#226

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The test’s background task loops until cancellation and repeatedly calls
PopulateChannelsFromStatus without any delay/yield, which is a classic busy-spin pattern that can
drive high CPU usage during the test run.

src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[265-272]

Agent prompt

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

### Issue description
`EnableChannel_ConcurrentWithStatusResync_AlwaysSendsMaskIncludingJustEnabledChannel` starts a background task that repeatedly calls `PopulateChannelsFromStatus` in a tight `while` loop with no yielding or throttling. This can consume significant CPU during the test run and increase contention, making CI slower/flakier.

### Issue Context
This is a stress-style concurrency test; it should still exercise interleavings, but it doesn’t need to busy-spin at maximum speed to do so.

### Fix Focus Areas
- src/Daqifi.Core.Tests/Device/DaqifiStreamingDeviceChannelManagementTests.cs[265-272]

### Suggested fix
Update the loop to include a minimal yield/backoff while still keeping high concurrency pressure, e.g.:
- Use `SpinWait` (`var sw = new SpinWait(); sw.SpinOnce();`) inside the loop, or
- Call `Thread.Yield()` each iteration, or
- Convert the delegate to `async` and `await Task.Yield()` occasionally.

Optionally pass `stop.Token` into `Task.Run(..., stop.Token)` and/or add a safety timeout (e.g., `CancelAfter`) so the test can’t hang indefinitely if a regression deadlocks.

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

---

[tylerkron]

Addressed the IsEnabled resync race flagged above: SetChannelsEnabled/DisableAllChannels now mutate IsEnabled and compute the outbound ADC/DIO mask atomically under the same lock that guards the status-driven resync (new DaqifiDevice.WithChannelsLock), with the SCPI send kept outside the lock. Added a regression test that hammers a concurrent status resync against repeated EnableChannel calls — verified it fails without the lock and passes with it.

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 42fc002

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 3a0eaf6