refactor(device): extract status-frame channel mapping into a collaborator (part of #344)

[tylerkron]

Problem

DaqifiDevice is now the largest file in the repo at 4,398 lines — larger than either class #344 originally named. The status comment on #344 flagged this as item 4 and left the scope question open; this PR answers it by taking the smallest, safest bite rather than by re-scoping the whole issue.

The status-frame-to-channel mapping is the most self-contained block in the file: protobuf fields in, channel instances out. It owns no lock, raises no event, and holds no device state — it is translation sitting inside a connection-lifecycle class.

Fix

Moved to StatusChannelPopulator (internal, Device/Internal/). The device keeps everything that is not mapping — the channels lock, the list swap, the timestamp-frequency update, and the ChannelsPopulated event. The mapping still runs inside the lock, exactly where it did before.

No public API change. PopulateChannelsFromStatus keeps its signature, its virtual-ness, and its documented behaviour.

The seam is two constructor arguments: the logger, and a Func<string> for the device name. A delegate rather than a captured string because the name can change during the device's lifetime, and a warning naming the wrong device is worse than one naming none. The field is readonly, so nullable analysis turns "a future constructor forgot to build it" into a compile error rather than an NRE on the first status frame.

DaqifiDevice: 4,398 → 4,193 lines.

Verifying it is a move, not a rewrite

Same method as #419/#422/#432: a normalized statement multiset diff (comments and doc-comments stripped, whitespace canonicalized) of the original block against the device's remainder plus the new file. Every surviving residue is structural and enumerated:

Residue	Why
3 × SafeLog(... LogWarning ...)	Name → _deviceName()
foreach (var existing in _channels) + index write	loop now walks the existing parameter; local renamed to channel
2 × analogCount/digitalCount = Populate*(…, updatedChannels)	parameter renamed to destination
private int PopulateDigitalChannels	now private static — it touches no instance state
namespace / usings / class / ctor / fields / SafeLog twin / Populate entry point	new file scaffolding

SafeLog is a local twin rather than a shared helper, matching the existing convention — MessageProducer and DaqifiStreamingDevice (as SafeTrace) already each carry their own, and DaqifiDevice's stays private to that class.

Tests

Zero edits to existing tests — that is the evidence behaviour is unchanged. ChannelPopulationTests' 44 cases still drive the same mapping through the device and still pass untouched.

Adds 23 cases against the collaborator directly, which is what the extraction newly makes possible (no device, no connection, no channels lock): constructor guards, analog-then-digital ordering, in-place instance reuse, the enabled-mask resync and the empty-mask ambiguity (#409), the PWM-capable channel set, assumed-resolution and non-finite-scaling fallbacks, short calibration arrays, that the device name is read at populate time rather than at construction, and that a throwing consumer logger still leaves the population complete.

• FULL suite green on net9.0 and net10.0: 2,552 passed, 2 skipped (was 2,529 + 23 new), plus Daqifi.Mcp.Tests 23/23.
• Release build of the solution: 0 warnings, 0 errors on both TFMs.

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

Channel population is exercised on every connect, so this is directly bench-observable:

• Status frame from the live device populates analogIn=16 digital=16 — the real 16+16 Nyquist channel description, not a fixture.
• --channels 3 (ch 0,1) → 2 analog values per frame; --channels 5 (ch 0,2) → 2; --channels 11 (ch 0,1,3) → 3. The decode maps values onto channels by IsEnabled in channel order, so a broken enabled-mask resync would mis-map or drop values here — it doesn't.
• Nothing driven, no NVM write, no reboot.

Not merging — for review.

Part of #344 (item 4 in its status comment). Deliberately does not touch DaqifiStreamingDevice, so it does not conflict with #432.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Refactor device status-frame channel mapping into StatusChannelPopulator

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Extract status-frame → channel-instance mapping from DaqifiDevice into an internal collaborator.
• Preserve PopulateChannelsFromStatus public/virtual behavior and lock-scoped population semantics.
• Add focused unit tests for mapping, reuse behavior, sanitization, and logging safety.

Diagram

graph TD
  M{{"Status frame"}} --> D["DaqifiDevice"] --> P["StatusChannelPopulator"] --> C["IChannel list"]
  D --> E["ChannelsPopulated event"]
  P --> L["ILogger"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep mapping methods inside DaqifiDevice

• ➕ No new class/constructor wiring
• ➕ All logic remains in one file for debugging
• ➖ Continues growth of an already very large class
• ➖ Harder to unit test mapping in isolation
• ➖ Mixes translation concerns into lifecycle/state class

2. Extract to static helper methods (e.g., Device/StatusChannelMapping.cs)

• ➕ No per-device instance to hold
• ➕ Straightforward call sites
• ➖ Harder to supply device identity context for logging without extra parameters
• ➖ Encourages hidden global-style dependencies (logger/device name) passed around repeatedly
• ➖ Less explicit seam for mocking/testing than a collaborator

3. Split DaqifiDevice via partial class

• ➕ Minimizes new types and preserves internal access to fields
• ➕ Mechanical separation without changing call graph
• ➖ Does not create a true seam for isolated tests
• ➖ Still couples translation logic to device internals and file-level complexity remains distributed rather than reduced

Recommendation: The collaborator extraction used here is the best trade-off: it creates a clear seam for isolated tests, keeps device ownership of locking/state/eventing, and preserves runtime behavior by still running mapping under the same lock. A static helper or partial-class split would reduce file size but would not provide as clean a testable boundary or would keep translation logic tightly coupled to the device class.

Files changed (3) +569 / -218

Refactor (2) +320 / -218

DaqifiDevice.csDelegate status-frame channel mapping to StatusChannelPopulator +13/-218

Delegate status-frame channel mapping to StatusChannelPopulator

• Adds a cached StatusChannelPopulator instance and wires it in all constructors with the device logger and a name delegate. Replaces the in-method mapping logic in PopulateChannelsFromStatus with a single populator call while retaining the channels lock, list swap, timestamp-frequency update, and ChannelsPopulated event behavior.

src/Daqifi.Core/Device/DaqifiDevice.cs

StatusChannelPopulator.csIntroduce internal collaborator for status-frame → channel mapping +307/-0

Introduce internal collaborator for status-frame → channel mapping

• Adds an internal sealed class that performs the prior PopulateChannelsFromStatus mapping: indexes existing channels for identity-based reuse, populates analog and digital channels in order, parses the analog enabled mask, applies sanitization for invalid calibration/scaling/port-range/resolution values, and logs warnings via a SafeLog wrapper to prevent logger failures from aborting population.

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

Tests (1) +249 / -0

StatusChannelPopulatorTests.csAdd isolated unit tests for StatusChannelPopulator mapping contract +249/-0

Add isolated unit tests for StatusChannelPopulator mapping contract

• Introduces a new test suite that exercises channel population ordering, instance reuse, enabled-mask handling, PWM capability flags, resolution/scaling sanitization, and SafeLog behavior. Also validates that the device name is read via delegate at populate-time and that null constructor inputs throw.

src/Daqifi.Core.Tests/Device/Internal/StatusChannelPopulatorTests.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 review came back clean (0 bugs, 0 rule violations, 0 requirement gaps, no unresolved threads) and CI build is green on net9 + net10. Not merging — awaiting your review.