refactor(device): extract device administration into a collaborator (part of #344)

[tylerkron]

Part of #344. Not merging — opened for review.

Problem

DaqifiStreamingDevice is still 1,385 lines against #344's ~800 target. #422 extracted four of the five blocks the issue names — SD card, diagnostics, network config, LAN chip info — and #432/#435 took channel control and the frame decode path. The device-administration block named in the issue body was left behind: reboot, the six ADC-calibration bank commands, the two voltage-precision commands, the two per-channel calibration setters, and the friendly-name write.

They belong together and they belong out: every one is a guarded fire-and-forget SCPI command with no reply to parse, and none of them touches the channel collection, the streaming session, or any device state.

Change

New internal DeviceAdministrationOperations (Device/Internal/). DaqifiStreamingDevice drops to 1,271 lines.

IDeviceOperationHost gains exactly two members, both forwarding to device members that already existed:

• Metadata — the device's own object, not a copy. The friendly-name write updates it optimistically because the firmware never echoes the new name back.
• Disconnect() — reboot has to tear the local link down after the device drops its own. Routed through the device so the full disconnect path (lifecycle lock, message pumps, status event) runs exactly as it does for a caller-issued Disconnect.

Public API is unchanged. All eleven members stay on the device as one-line delegations, so every command still passes through the device's own virtual Send and any subclass override of it.

Why this block and not a bigger one

DaqifiDevice is still the largest file at 4,193 lines, but its remaining blocks are the reconnect loop (entangled with _sessionEpoch, ConnectAsync and the protected virtual snapshot hooks), the text-exchange core (consumer swap plus two locks plus disposal state) and the lifecycle/deferral gates. Each is a place where a compile-clean move can deadlock. The discipline that has worked on this issue — pure translation, no lock, no event, no device state — is what picked this block, not the line count.

Verification

The move is verified mechanically, not by eye. A normalized statement multiset diff of what the device lost against what the collaborator gained leaves zero residue on the device side: every removed statement appears in the collaborator. The collaborator-only residues are class/constructor scaffolding plus three signature lines git treated as unchanged context because they are textually identical either side of the move.

No existing test changed. DaqifiStreamingDeviceTests (command text), DaqifiStreamingDeviceFriendlyNameTests and DeviceNotConnectedExceptionTests (all ten disconnected-guard sites) still drive the same behavior through the device — that is the extraction evidence, so they are untouched and deliberately not duplicated.

+16 new cases test the collaborator directly and cover only what a direct test can see: the ordering and the total set of calls back into the host. The fake host throws on every seam member outside this block's remit, so a future change that grabs the channels lock or stops a stream fails loudly instead of passing quietly.

The two ordering assertions were mutation-verified rather than trusted green:

• swapping Reboot to disconnect-then-send fails Reboot_SendsTheRebootCommandBeforeTearingTheConnectionDown — disconnecting first closes the transport the reboot command still has to travel over;
• hoisting the metadata write above the two sends fails SetFriendlyNameAsync_WhenTheSaveSendFails_LeavesMetadataUnchanged.

Full suite green on net9 + net10 (2,583 passed, 2 skipped each) plus Daqifi.Mcp.Tests (23). Release solution build: 0 warnings, both TFMs.

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

Run as a true A/B against origin/main — the only thing that actually proves a refactor changed nothing on hardware. The same harness was built twice, once against this branch's Core and once against a throwaway origin/main worktree, and both were run on the same board: byte-for-byte identical output.

Only RAM-load commands were issued (CONFigure:ADC:LOADcal, CONFigure:VOLTage:LOAD — NVM read, RAM write). No NVM write, no bank selection, no friendly-name write, no reboot.

• AI0 mean 0.0024 V before and after the reload (delta 0.0001 V) — nothing perturbed.
• Argument guards fired against the live device: SetAdcCalibrationSlope(-1) and UseAdcCalibration(2) both threw ArgumentOutOfRangeException, the connection stayed up, and the SCPI error queue stayed clean — so nothing reached the wire.
• SaveAdcCalibration and Reboot both refused a disconnected device, so neither an NVM write nor a reboot was ever issued.

Observation, not a defect and not filed: this bench unit answers CONFigure:ADC:LOADcal with -200,"Execution error". -200 rather than -113 means the firmware recognized the header and failed to execute it — consistent with the unit having no saved user ADC-calibration bank. CONFigure:VOLTage:LOAD is accepted cleanly on the same unit, and the identical -200 appears on origin/main, so it is device state, not something this change introduced.

Scope

Touches no file that PR #434 touches.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Refactor: move device-administration commands into DeviceAdministrationOperations

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Extract reboot/calibration/friendly-name SCPI commands into a dedicated internal collaborator.
• Keep public IStreamingDevice API unchanged via one-line delegations on DaqifiStreamingDevice.
• Add focused unit tests validating host-call ordering and restricted host surface usage.

Diagram

graph TD
  Caller["App / Unit tests"] --> Dev["DaqifiStreamingDevice"] --> Admin["DeviceAdministrationOperations"] --> Host["IDeviceOperationHost"] --> Device["DAQiFi device"]
  Admin --> Scpi["ScpiMessageProducer"]
  Admin --> Meta[("DeviceMetadata")]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep methods on DaqifiStreamingDevice (region-only refactor)

• ➕ Zero seam expansion (no IDeviceOperationHost changes).
• ➕ Avoids an additional allocation/field for the collaborator.
• ➖ Does not reduce file size/complexity meaningfully (main motivation is chore: decompose the two god-classes — DaqifiStreamingDevice (2.1k lines) and FirmwareUpdateService (2.5k lines) #344 line-count pressure).
• ➖ Harder to unit-test call ordering/host interaction boundaries directly.

2. Generalize a base 'FireAndForgetScpiOperations' helper

• ➕ Could reduce repeated connection-guard boilerplate across multiple operation blocks.
• ➕ Creates a consistent pattern for future extractions.
• ➖ Adds an abstraction layer with unclear immediate payoff.
• ➖ Risk of over-generalizing and obscuring command-specific constraints (e.g., reboot ordering, optimistic metadata write).

Recommendation: Proceed with the collaborator extraction as implemented. It achieves the #344 goal (shrinking DaqifiStreamingDevice) while preserving the public API and routing all commands through the device’s virtual Send. The small, explicit IDeviceOperationHost expansion (Metadata + Disconnect) is justified by concrete behavioral needs (optimistic name update and reboot teardown) and is guarded by targeted tests.

Files changed (6) +495 / -133

Refactor (3) +232 / -133

DaqifiStreamingDevice.csDelegate device-administration methods to DeviceAdministrationOperations +19/-133

Delegate device-administration methods to DeviceAdministrationOperations

• Creates and stores a new DeviceAdministrationOperations collaborator during initialization, replacing in-class implementations of reboot/calibration/voltage precision/friendly-name operations with one-line delegations. Implements new IDeviceOperationHost members by forwarding Metadata and Disconnect to existing device members.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

DeviceAdministrationOperations.csIntroduce internal collaborator for reboot/calibration/voltage/friendly-name commands +196/-0

Introduce internal collaborator for reboot/calibration/voltage/friendly-name commands

• Adds an internal operations class containing the extracted fire-and-forget SCPI commands with argument validation, connection guards, and host-mediated send/disconnect/metadata writes. Keeps behavior aligned with prior DaqifiStreamingDevice implementations while centralizing this cohesive command block.

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

IDeviceOperationHost.csExpand host seam with Metadata and Disconnect for admin operations +17/-0

Expand host seam with Metadata and Disconnect for admin operations

• Adds Metadata (device-owned object) and Disconnect() to support optimistic friendly-name updates and reboot teardown through the full device disconnect path. Documents rationale and expected semantics in XML comments.

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

Tests (2) +249 / -0

DeviceAdministrationOperationsTests.csAdd direct unit coverage for DeviceAdministrationOperations host interactions +247/-0

Add direct unit coverage for DeviceAdministrationOperations host interactions

• Introduces tests asserting reboot send-then-disconnect ordering, that single-command operations perform exactly one send, and that friendly-name updates metadata only after both sends succeed. Uses a strict fake host that throws on out-of-scope seam members to detect accidental coupling.

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

StreamFrameDecoderTests.csUpdate test host stub for new IDeviceOperationHost members +2/-0

Update test host stub for new IDeviceOperationHost members

• Extends the existing fake IDeviceOperationHost implementation with Metadata and Disconnect members (throwing NotSupportedException) to satisfy the expanded interface contract.

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

Documentation (1) +14 / -0

SESSION_LOG.mdLog PR #436 device-administration extraction and verification notes +14/-0

Log PR #436 device-administration extraction and verification notes

• Adds an entry documenting why device-administration was selected for extraction, what moved, and how the change was mechanically/bench verified. Records test and bench A/B observations for traceability.

SESSION_LOG.md

[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. Shared log merge conflicts ✗ Dismissed 🐞 Bug ⚙ Maintainability

Description

This PR appends another block to SESSION_LOG.md, which commonly causes add/add conflicts with other
in-flight branches that also append to the same file and can block merges until manual resolution.
The new entry itself documents this happening repeatedly and calls for a structural change, but the
PR still perpetuates the pattern.

Code

SESSION_LOG.md[R100-103]

+- **This is a self-inflicted, recurring process defect and it should be named as such.** Every fire appends its entry to the end of this file *on its PR branch*, so the moment any loop PR merges, **every other open loop PR goes `CONFLICTING`** — and the repo ruleset requires branches be up to date, so each one then needs a manual resolve before the user can merge. It has now cost two separate fires (the 23:37 `gh pr update-branch` fire on #434, and this one). It will recur: whichever of #434/#436 the user merges first, the other conflicts on this file again, because both branches now carry log entries the other lacks. **The fix is structural — stop appending a shared journal file on feature branches** (one file per fire under a directory, or keep the journal off the PR branches entirely). Flagged rather than changed unilaterally, since it is the loop's own convention.
+- Resolutions were **chronological, not "take one side"**, and both entry blocks were kept in full. #436's own entry (fire ended 22:32) sorts *before* main's three #437-era entries; #434's own entry (fire ended 23:37, after the last #437 note at 23:28) sorts *after* them. Verified lossless mechanically rather than by eye: for each merged file, **every non-blank line of both parents is still present** (0 missing against each parent, both PRs).
+- Verified each merged tree is the clean union: merged-vs-branch shows exactly main's `LifecycleGate` delta and nothing else; merged-vs-main shows exactly that PR's own files and nothing else. No source file was touched by either resolve.
+- Tests, on trees that had **never been built** (this is the point of re-running rather than trusting the pre-merge greens): **#436 merged — 2,608 passed, 2 skipped** on net9 + net10; **#434 merged — 2,606 passed, 2 skipped** on net9 + net10. Plus `Daqifi.Mcp.Tests` 23 on each. Release solution build **0 warnings** both TFMs, both trees.

Relevance

●● Moderate

Structural/process change to logging convention; no clear precedent they’ll refactor away from
SESSION_LOG.md appends in this PR.

PR-#437

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new log block states that multiple PRs became CONFLICTING and that SESSION_LOG.md was the
only conflicting path, attributing it to the practice of appending this shared file on each PR
branch and recommending a structural fix.

SESSION_LOG.md[96-106]

Agent prompt

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

### Issue description
`SESSION_LOG.md` is being appended on feature branches. When multiple PR branches do this concurrently, updating/merging against `main` frequently creates avoidable merge conflicts that block mergeability and require manual resolution + re-validation.

### Issue Context
The newly added log entry explicitly states that `SESSION_LOG.md` was the only conflicting path across multiple PRs and that this is recurring.

### Fix Focus Areas
- SESSION_LOG.md[96-107]

### Suggested fix
- Stop appending to a single shared `SESSION_LOG.md` from feature branches.
- Use one file per fire under a directory (e.g., `session_log/2026-08-05-fire-...md`) or keep the journal updates on `main` only (post-merge), so parallel PRs don’t contend on the same file.

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

[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.

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 18e9670

[tylerkron]

Ready for review (supersedes the earlier note, which pinned cb7acda).

This PR was CONFLICTING and therefore not mergeable as it stood — #437 merged at 23:40, and the repo ruleset requires branches to be up to date, so you would have been blocked here.

The conflict was diagnosed before anything was touched: git merge-tree --write-tree showed the only conflicting path is SESSION_LOG.md, an append collision. Zero source overlap — this PR touches Device/DaqifiStreamingDevice.cs + Device/Internal/*, #437 landed Device/DaqifiDevice.cs + Device/Internal/LifecycleGate.cs.

Resolved chronologically, keeping both entry blocks in full (this PR's entry is from the 22:32 fire, so it sorts before main's #437-era entries). Verified lossless mechanically rather than by eye: every non-blank line of both parents is still present — 0 missing against each. Merged-vs-branch shows exactly main's LifecycleGate delta and nothing else; merged-vs-main shows exactly this PR's own files and nothing else.

Current head 662e256:

• CI build green on net9 + net10.
• Full suite re-run locally on the merged tree, which had never been built before this: 2,608 passed, 2 skipped on each TFM, plus Daqifi.Mcp.Tests 23. Release solution build 0 warnings both TFMs.
• Bench re-validated on the real Nq1 over USB, non-destructive (connect → status → stream → disconnect only; no NVM write, no reboot, no SD, no calibration write). This mattered here: the device-administration extraction had never run alongside main's LifecycleGate on hardware. Example CLI built against the merged core: 3 cycles, all exit 0, each reporting analogIn=16 digital=16 fw=3.7.2 with a stable serial, 30 CSV rows at 20 Hz x 2 s — identical run to run, no wedge across repeated open/close of the port.
• Qodo re-reviewed the new head and raised one item, "Shared log merge conflicts" — the journal-append pattern itself. Accepted as a real defect (it is what blocked both PRs today) but declined in this PR: removing the entry reduces the conflict surface by zero, since the branch already carried a SESSION_LOG.md delta, and doing the restructure here would turn fix(firmware): stop reflashing supported WiFi modules when the release lookup fails (part of #269) #434's trivial content conflict into a delete/modify one. Replied in full and resolved. Worth your attention as a scheduling call — the fix needs a window when no open PR has a SESSION_LOG.md delta.

mergeable: MERGEABLE, state BLOCKED — merge-ready pending your approval. Not merging — awaiting your review.