feat(device): wire remaining NVM persistence SCPI commands (closes #207)

[tylerkron]

Summary

Exposes the firmware's remaining configuration-persistence NVM primitives as thin, typed wrappers, so Core consumers can persist/restore device settings without reimplementing SCPI. Not a generic serialization framework — each command maps 1:1 to a firmware primitive.

Closes #207.

SCPI wiring (ScpiMessageProducer)

Member	Command
LoadNetworkLan	SYSTem:COMMunicate:LAN:LOAD
FactoryResetNetworkLan	SYSTem:COMMunicate:LAN:FACRESET
SaveAdcCalibration	CONFigure:ADC:SAVEcal
LoadAdcCalibration	CONFigure:ADC:LOADcal
SaveVoltagePrecision	CONFigure:VOLTage:SAVE
LoadVoltagePrecision	CONFigure:VOLTage:LOAD

API surface

• INetworkConfigurable gains LoadNetworkConfigurationAsync() and FactoryResetNetworkAsync(), implemented on DaqifiStreamingDevice next to the existing UpdateNetworkConfigurationAsync (connected-guard + cancellation). These are thin wrappers over the NVM primitives — they repopulate/reset the device's runtime settings but do not themselves re-apply to the live interface (send an apply/reboot afterward); the local NetworkConfiguration snapshot is intentionally not refreshed. Matches the async shape the interface already establishes.
• SaveAdcCalibration / LoadAdcCalibration / SaveVoltagePrecision / LoadVoltagePrecision land on IStreamingDevice — device-global NVM ops, no new interface, matching the existing Reboot-style fire-and-forget command methods (per the ticket: "likely on the channel/calibration subsystem, not a new interface").

Tests

• Command-string round-trip per new producer member.
• Interface-level tests via a capturing transport: each method sends the correct command when connected and throws InvalidOperationException when disconnected, plus a cancellation test for the async network members.
• Full suite green (1543 passing).

Bench test

Ran all six commands against a real Nyquist (firmware 3.7.2, USB /dev/cu.usbmodem1101) and checked the SCPI error queue after each via DrainErrorQueueAsync:

ACCEPTED* LoadAdcCalibration     (CONFigure:ADC:LOADcal)   (other error: -200,"Execution error")
ACCEPTED  SaveAdcCalibration     (CONFigure:ADC:SAVEcal)
ACCEPTED  LoadVoltagePrecision   (CONFigure:VOLTage:LOAD)
ACCEPTED  SaveVoltagePrecision   (CONFigure:VOLTage:SAVE)
ACCEPTED  LoadNetworkLan         (SYSTem:COMMunicate:LAN:LOAD)
ACCEPTED  FactoryResetNetworkLan (SYSTem:COMMunicate:LAN:FACRESET)
RESULT: all 6 commands accepted by firmware (no -113).

Every command header was accepted — no -113 "Undefined header" — confirming the command strings match firmware. LoadAdcCalibration returned -200 "Execution error": the header is recognized, but the device had no stored calibration to load in that state — a device-state condition, not a command-string defect. (The bench harness issued LOAD before SAVE for both cal and voltage so any SAVE wrote back values just refreshed from NVM — a no-op persist.)

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Wire NVM persistence SCPI commands for LAN, ADC calibration, and voltage precision

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Add typed SCPI producer wrappers for LAN load/factory-reset and ADC/voltage NVM save/load.
• Expose new device APIs on IStreamingDevice and INetworkConfigurable with connected guards.
• Extend test suite to validate command strings, connection preconditions, and cancellation
 behavior.

Diagram

graph TD
  C["Core consumer"] --> I["IStreamingDevice / INetworkConfigurable"] --> D["DaqifiStreamingDevice"] --> P["ScpiMessageProducer"] --> T["Transport"] --> F["Device firmware / NVM"]
  subgraph Legend
    direction LR
    _api["API surface"] ~~~ _impl["Implementation"] ~~~ _fw["Firmware"]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Refresh local NetworkConfiguration after LOAD/FACRESET

• ➕ Reduces surprise: local snapshot matches device runtime settings after operation
• ➕ Avoids consumers needing an explicit readback step
• ➖ Requires defining/implementing a readback query surface (more firmware assumptions)
• ➖ Adds transport round-trips and more failure modes to otherwise thin wrappers

2. Introduce a dedicated persistence interface (e.g., INvmPersistence)

• ➕ Keeps IStreamingDevice smaller and separates device-global persistence concerns
• ➕ Allows grouping/policy (e.g., atomic save/load sequences) later
• ➖ More surface area and types for a small set of 1:1 primitives
• ➖ May conflict with existing pattern of device-global commands living on IStreamingDevice

Recommendation: The PR’s approach (thin 1:1 wrappers + connection/cancellation guards + focused tests) is the best fit for the stated goal and ticket constraints. Avoiding snapshot refresh and avoiding a new interface keeps semantics clear: these are firmware primitives, not a state-synchronizing configuration framework.

Files changed (8) +388 / -0

Enhancement (4) +214 / -0

ScpiMessageProducer.csAdd SCPI producers for ADC/voltage persistence and LAN load/factory reset +70/-0

Add SCPI producers for ADC/voltage persistence and LAN load/factory reset

• Defines new outbound SCPI messages: CONFigure:ADC:SAVEcal/LOADcal, CONFigure:VOLTage:SAVE/LOAD, and SYSTem:COMMunicate:LAN:LOAD/FACRESET with XML docs clarifying semantics (runtime vs applied state).

src/Daqifi.Core/Communication/Producers/ScpiMessageProducer.cs

DaqifiStreamingDevice.csImplement NVM persistence APIs with connection guards and cancellation checks +84/-0

Implement NVM persistence APIs with connection guards and cancellation checks

• Implements IStreamingDevice Save/Load ADC calibration and voltage precision as fire-and-forget SCPI sends guarded by IsConnected. Adds async LAN load/factory reset methods with cancellation pre-check and the same connected guard pattern as existing network operations.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

IStreamingDevice.csExpose ADC calibration and voltage precision NVM persistence on IStreamingDevice +34/-0

Expose ADC calibration and voltage precision NVM persistence on IStreamingDevice

• Adds four new public methods to persist/restore ADC calibration coefficients and voltage precision settings, documented as thin wrappers over the corresponding firmware SCPI primitives.

src/Daqifi.Core/Device/IStreamingDevice.cs

INetworkConfigurable.csAdd network NVM restore/reset methods to INetworkConfigurable +26/-0

Add network NVM restore/reset methods to INetworkConfigurable

• Extends the network configuration interface with LoadNetworkConfigurationAsync and FactoryResetNetworkAsync, documenting that these repopulate runtime settings but do not apply them to the live interface nor refresh the local snapshot.

src/Daqifi.Core/Device/Network/INetworkConfigurable.cs

Tests (4) +174 / -0

ScpiMessageProducerTests.csAdd SCPI command-string tests for new NVM persistence messages +48/-0

Add SCPI command-string tests for new NVM persistence messages

• Adds unit tests asserting the exact SCPI headers and message formatting for LAN load/factory reset and ADC/voltage save/load commands.

src/Daqifi.Core.Tests/Communication/Producers/ScpiMessageProducerTests.cs

DaqifiStreamingDeviceTests.csValidate new device-global NVM persistence methods via capturing transport +53/-0

Validate new device-global NVM persistence methods via capturing transport

• Introduces a parameterized test matrix for Save/Load ADC calibration and voltage precision. Verifies correct command is sent when connected and InvalidOperationException is thrown when disconnected.

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

NetworkConfigurableTests.csAdd tests for LoadNetworkConfigurationAsync and FactoryResetNetworkAsync +57/-0

Add tests for LoadNetworkConfigurationAsync and FactoryResetNetworkAsync

• Adds coverage for the two new network NVM methods: disconnected guard behavior, correct SCPI command emission when connected, and cancellation short-circuit (no command sent).

src/Daqifi.Core.Tests/Device/Network/NetworkConfigurableTests.cs

FirmwareUpdateServiceTests.csUpdate streaming-device fakes to satisfy new IStreamingDevice members +16/-0

Update streaming-device fakes to satisfy new IStreamingDevice members

• Extends multiple fake IStreamingDevice implementations with no-op NVM persistence methods to keep firmware update service tests compiling.

src/Daqifi.Core.Tests/Firmware/FirmwareUpdateServiceTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0) 📜 Skill insights (0)

Context used

✅ Tickets: 🎫 feat: Wire remaining device NVM persistence SCPI commands (LAN load/factory-reset, ADC cal, voltage)

1. Cancellation can still send ✓ Resolved 🐞 Bug ☼ Reliability

Description

LoadNetworkConfigurationAsync and FactoryResetNetworkAsync only observe cancellation at method
entry; if cancellation is requested after the initial check but before the Send call, the
device-state-changing SCPI command can still be sent despite cancellation being requested.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R1006-1016]

+        public Task LoadNetworkConfigurationAsync(CancellationToken cancellationToken = default)
+        {
+            cancellationToken.ThrowIfCancellationRequested();
+
+            if (!IsConnected)
+            {
+                throw new InvalidOperationException("Device is not connected.");
+            }
+
+            Send(ScpiMessageProducer.LoadNetworkLan);
+            return Task.CompletedTask;

Relevance

⭐⭐⭐ High

PR #324 accepted adding cancellation guard immediately before state-changing LAN:APPLY Send; same
pattern applies here.

PR-#324

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The methods perform a single early cancellation check and then send a command; there is no second
cancellation observation immediately before Send(...). The interface docs indicate the token
should be observed for the operation, and similar cancellation-guard concerns have been accepted
recently for state-changing SCPI sends.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[999-1037]
src/Daqifi.Core/Device/Network/INetworkConfigurable.cs[45-69]
PR-#324

Agent prompt

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

### Issue description
`LoadNetworkConfigurationAsync` and `FactoryResetNetworkAsync` check `cancellationToken.ThrowIfCancellationRequested()` once, then proceed to `Send(...)`. A cancellation requested after the first check (even if rare) can still result in a state-changing command being sent.

### Issue Context
These methods are exposed as async APIs with cancellation tokens and are intended to be cancellable. While cancellation is cooperative and cannot be made fully atomic with a synchronous send, adding a second guard immediately before the `Send(...)` narrows the race window and aligns with prior cancellation-guard patterns in this codebase.

### Fix Focus Areas
- Add `cancellationToken.ThrowIfCancellationRequested();` immediately before each `Send(...)` in:
 - `LoadNetworkConfigurationAsync`
 - `FactoryResetNetworkAsync`
- (Optional) Add/extend a unit test that cancels from another thread right before invocation of `Send` (or refactor to enable deterministic testing via injectable send hook).

### Fix Focus Areas (code)
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1006-1017]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1026-1037]

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

---

2. Breaking interface expansion 🐞 Bug ⚙ Maintainability

Description

New abstract members were added to the public interfaces IStreamingDevice and INetworkConfigurable,
which is a source-breaking change for any downstream implementer and may also break already-compiled
implementers at runtime when loaded against the newer assembly.

Code

src/Daqifi.Core/Device/IStreamingDevice.cs[R152-185]

+
+        /// <summary>
+        /// Persists the device's current ADC calibration coefficients to NVM so they survive a reboot.
+        /// </summary>
+        /// <remarks>
+        /// A thin wrapper over the firmware NVM primitive (<c>CONFigure:ADC:SAVEcal</c>). Pair with
+        /// <see cref="LoadAdcCalibration"/> to restore them.
+        /// </remarks>
+        void SaveAdcCalibration();
+
+        /// <summary>
+        /// Restores the device's ADC calibration coefficients from NVM into its runtime.
+        /// </summary>
+        /// <remarks>
+        /// The inverse of <see cref="SaveAdcCalibration"/> (firmware primitive <c>CONFigure:ADC:LOADcal</c>).
+        /// </remarks>
+        void LoadAdcCalibration();
+
+        /// <summary>
+        /// Persists the device's current voltage precision setting to NVM so it survives a reboot.
+        /// </summary>
+        /// <remarks>
+        /// A thin wrapper over the firmware NVM primitive (<c>CONFigure:VOLTage:SAVE</c>). Pair with
+        /// <see cref="LoadVoltagePrecision"/> to restore it.
+        /// </remarks>
+        void SaveVoltagePrecision();
+
+        /// <summary>
+        /// Restores the device's voltage precision setting from NVM into its runtime.
+        /// </summary>
+        /// <remarks>
+        /// The inverse of <see cref="SaveVoltagePrecision"/> (firmware primitive <c>CONFigure:VOLTage:LOAD</c>).
+        /// </remarks>
+        void LoadVoltagePrecision();

Relevance

⭐ Low

Team rejected avoiding adding members to public interface (IDeviceInfo) as “binary-breaking”; likely
won’t change approach.

PR-#290
PR-#250

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new members are added directly onto public interfaces, and the project is packaged as
Daqifi.Core, so downstream implementers are impacted. This matches a previously accepted bug
pattern in this repo (avoid adding members to public interfaces).

src/Daqifi.Core/Device/IStreamingDevice.cs[6-186]
src/Daqifi.Core/Device/Network/INetworkConfigurable.cs[6-70]
src/Daqifi.Core/Daqifi.Core.csproj[3-18]
PR-#275

Agent prompt

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

### Issue description
The PR adds new members to existing **public interfaces** (`IStreamingDevice`, `INetworkConfigurable`). Any external type implementing these interfaces will fail to compile on upgrade (and may fail at runtime depending on how it’s consumed), making this a breaking change.

### Issue Context
`Daqifi.Core` is packaged for external consumption (NuGet package metadata in the csproj), so interface shape changes affect downstream consumers.

### Fix Focus Areas
- Prefer one of these patterns:
 - Introduce new derived interfaces (e.g., `IStreamingDeviceNvmPersistence : IStreamingDevice`, `INetworkConfigurableNvm : INetworkConfigurable`) and have `DaqifiStreamingDevice` implement them.
 - Or provide **default interface implementations** (net9+/C# supports this) that throw `NotSupportedException`, preserving source compatibility for implementers.
 - Or (if breaking changes are acceptable) ensure this is coordinated with a major-version bump.

- Update call sites/tests to depend on the new derived interfaces where needed.

### Fix Focus Areas (code)
- src/Daqifi.Core/Device/IStreamingDevice.cs[152-185]
- src/Daqifi.Core/Device/Network/INetworkConfigurable.cs[45-70]
- src/Daqifi.Core/Daqifi.Core.csproj[3-18]

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

---

[tylerkron]

Re Qodo's optional finding "Breaking interface expansion": keeping the new members directly on IStreamingDevice / INetworkConfigurable. Ticket #207 explicitly scopes extending INetworkConfigurable with these methods and putting the ADC/voltage ops on the device interface, and Daqifi.Core's sole production implementer (DaqifiStreamingDevice) is updated in the same PR. Core is pre-1.0 and evolves these interfaces additively (Qodo notes the team previously rejected avoiding interface additions, and rates this ⭐ Low). A throwing default-interface shim or split sub-interface would add surface area for no real consumer benefit here.