refactor(device): extract channel/DIO/PWM/analog-out control into a collaborator (part of #344)

[tylerkron]

Problem

DaqifiStreamingDevice is 2,189 lines and still hosts five responsibilities. #422 extracted four of them; its status comment named the channel / DIO / PWM / analog-out block as the next one, and said why it was deliberately left behind:

It needs the IDeviceOperationHost seam widened for channel-collection access and the channels lock — a materially larger diff that would have been verified less carefully alongside the rest.

This is that extraction, on its own, with that verification.

Fix

ChannelControlOperations (internal, Device/Internal/) now owns:

• channel enable/disable and the ADC-bitmask / global-DIO-enable derivation (EnableChannel(s), DisableChannel, DisableAllChannels, SetChannelsEnabled, ComputeAdcEnableMask, ComputeDioEnableState, EnsureChannelBelongs)
• DIO direction and level
• PWM enable / duty cycle / frequency, plus the PwmFrequencyHz bookkeeping and the skip-if-unchanged cache (chore: small API polish batch (PWM frequency idempotence, SetFriendlyNameAsync, SCPI error-code parsing consolidation) #345)
• the analog output

DaqifiStreamingDevice keeps the public surface and forwards, exactly as it does for the SD, network, LAN-info and diagnostics collaborators.

The seam widening this needed. IDeviceOperationHost gains SnapshotChannels() and WithChannelsLock(Action). The lock is the whole reason this block was hard: mutating IChannel.IsEnabled and computing the outbound mask from it must happen in one critical section, or a status frame resyncing IsEnabled from the device (#409) can interleave between them and silently drop the just-requested channel from the mask. The collaborator therefore reaches the device's lock rather than taking one of its own. Both new members forward to members the device already had, so every command still passes through the device's own virtual Send — a subclass override (and every test double) still intercepts all of it.

No public API change. The three PWM constants and PwmFrequencyHz stay on DaqifiStreamingDevice with their docs; every moved method keeps its signature, its validation order (argument checks before the connected check), and its exception paramName.

One deletion. SendAdcEnableMask and SendDioEnableState are gone. They are private and have had no callers since #411, which replaced their call sites with the under-lock computation — git log -S confirms that commit removed the last use. Carrying dead code into a new file seemed worse than deleting it; it is called out here rather than buried.

DaqifiStreamingDevice: 2,189 → 1,756 lines.

Verification

The move is mechanical, and that is checked rather than asserted. A normalized statement multiset diff (comments/whitespace stripped, _host. and the const qualifications canonicalized) of every line the device lost against every line the collaborator gained leaves exactly three residues, all enumerated: the two dead methods above, MaxAdcBitmaskChannel becoming internal (the device's raw-command tracking decodes the same mask and now references it there), and _lastSentPwmFrequencyHz = null becoming the one-line ResetSentPwmFrequency(). Nothing else changed.

Test suite unchanged and green — the acceptance criterion this issue asks for. 2,529 passed / 2 skipped on net9 and net10, plus 23 in Daqifi.Mcp.Tests. Zero test edits: the existing coverage already pins the foreign-channel guards, the disconnected guards, the PWM range/duty/capability rules, the analog-out sequence, and — importantly — the #409 concurrent-resync regression test, which hammers a status resync against 500 EnableChannel calls and would fail or deadlock if the lock discipline had been broken by routing it through the seam. Release build: 0 warnings on both TFMs.

Bench-validated on the real Nq1 (fw 3.7.2, USB), because a mask that is computed correctly and never reaches the device looks identical to a green unit test:

Connected: Nq1 fw=3.7.2 sn=9090539562006014104
Channels: 16 analog, 16 digital
DisableAllChannels: ok
EnableChannels(0,1,2): ok
  streamed 78 frames, analog values/frame (mode) = 3  [expect 3]
DisableChannel(1): ok
  streamed 78 frames, analog values/frame (mode) = 2  [expect 2]
PwmFrequencyHz (session default) = 1000
SetPwmFrequency(2500) -> PwmFrequencyHz = 2500
SetPwmFrequency(2500) again (skip-if-unchanged): ok
SetDioDirection(ch 0, Input) -> Direction = Input
Foreign-channel guard: throws ArgumentException as expected
PWM range guard: throws ArgumentOutOfRangeException as expected
BENCH RESULT: PASS

The frame counts confirm the recomputed mask actually took effect on the device, not just in local state. Strictly non-destructive: nothing here drives a pin (PWM frequency is the shared timer with no channel enabled; DIO direction was set to Input, which is high-Z), writes NVM, or reboots.

Still open on #344 after this

Frame decode (the hot path, warrants its own PR), WifiModuleUpdater at 816 lines pending #271, and the scope question about DaqifiDevice now being the largest file in the repo.

Closes nothing — part of #344.

Not merging — opened for your review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Refactor device channel/DIO/PWM/analog-out control into ChannelControlOperations

✨ Enhancement 🕐 40+ Minutes

AI Description

• Extract channel enable/disable, DIO, PWM, and analog-out logic into an internal collaborator.
• Widen IDeviceOperationHost to expose channel snapshots and the device channels lock.
• Keep DaqifiStreamingDevice public API intact by forwarding operations through the collaborator.

Diagram

graph TD
  A["Client code"] --> B["DaqifiStreamingDevice"] --> C["ChannelControlOperations"] --> D["IDeviceOperationHost"] --> E["Send + SCPI producers"] --> F["Device firmware"]
  D --> G["Channels snapshot + lock"] --> C

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Partial-class split of DaqifiStreamingDevice

• ➕ Keeps all logic physically within one type (no seam widening).
• ➕ Can reduce indirection when navigating code.
• ➖ Does not reduce the device's responsibility count; still a monolith split across files.
• ➖ Harder to enforce the same collaborator pattern used by SD/network/diagnostics operations.

2. Nested private helper inside DaqifiStreamingDevice

• ➕ Avoids expanding IDeviceOperationHost and keeps access to private members direct.
• ➕ Can still isolate code while remaining non-public.
• ➖ Less reusable/consistent than the existing ‘operations collaborator’ architecture.
• ➖ Still tightly couples the helper to the device internals and can grow into another hidden monolith.

Recommendation: Proceed with the current collaborator-based extraction. It matches the existing internal architecture (network/SD/diagnostics collaborators), preserves the device’s Send interception path, and—crucially—reuses the device’s existing channels lock via the widened seam to prevent enable-mask races (#409). The alternatives reduce file size but don’t improve responsibility boundaries as cleanly.

Files changed (3) +514 / -454

Refactor (3) +514 / -454

DaqifiStreamingDevice.csDelegate channel/DIO/PWM/analog-out operations to ChannelControlOperations +21/-454

Delegate channel/DIO/PWM/analog-out operations to ChannelControlOperations

• Instantiates a new ChannelControlOperations collaborator and replaces in-class implementations of channel enable/disable, DIO control, PWM control, and analog output with forwarding methods. Routes PWM disconnect cache reset through the collaborator, updates ADC mask decoding to reference the collaborator’s shared constant, and expands explicit IDeviceOperationHost implementation to expose channel snapshot and channels-lock access.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

ChannelControlOperations.csAdd ChannelControlOperations collaborator for channel control and PWM bookkeeping +482/-0

Add ChannelControlOperations collaborator for channel control and PWM bookkeeping

• Introduces an internal sealed class that owns channel enable/disable (including ADC mask + global DIO enable derivation under the device channels lock), DIO direction/value, PWM enable/duty/frequency (including skip-if-unchanged cache + reset on disconnect), and analog output (DAC) writes. All outbound I/O flows through IDeviceOperationHost.Send and all channel-state mutation/mask derivation uses IDeviceOperationHost.WithChannelsLock.

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

IDeviceOperationHost.csWiden IDeviceOperationHost with channel snapshot and channels-lock APIs +11/-0

Widen IDeviceOperationHost with channel snapshot and channels-lock APIs

• Adds SnapshotChannels() and WithChannelsLock(Action) to allow collaborators to mutate IChannel.IsEnabled and derive outbound masks in a single critical section without taking their own lock. This preserves the device’s concurrency guarantees while keeping all I/O routed through the device’s virtual Send path.

src/Daqifi.Core/Device/Internal/IDeviceOperationHost.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 (Bugs 0, rule violations 0, requirement gaps 0, no unresolved inline threads) and CI build is green. Not merging — awaiting your review.