feat(device): confirming variants for the silent administration commands

[tylerkron]

What

The ADC-calibration and voltage-precision commands on IStreamingDevice are fire-and-forget: they send a SCPI primitive and parse no reply. A device that refuses the command is indistinguishable from one that carried it out.

This adds IConfirmingDeviceAdministration — a confirming ...Async twin for each of the nine commands that sends the same primitive and then reads the device's SCPI error queue, throwing DeviceCommandFailedException unless the device confirms it accepted the command.

Why

Bench evidence on a real Nyquist running fw 3.7.2 (posted on #344): on a unit whose user calibration bank was never written, CONFigure:ADC:LOADcal answers -200,"Execution error" — and LoadAdcCalibration() returns normally.

A control run sending the identical raw primitive produced the same -200 byte for byte, so the firmware is behaving as designed and Core's delegation is faithful. What was missing was any way for the caller to find out: "load the user calibration bank" was a silent no-op that reported success.

Design decisions

Throwing, not a result object. A result a caller can ignore reintroduces the original bug in a subtler form. DeviceCommandFailedException carries Command, ErrorCode and DeviceResponse, so a caller who wants to branch on the refusal can catch and read the device's own code.

A null ErrorCode means "unknown", not "fine". If no readable verdict comes back the command may or may not have been applied. That is not a success, so it also throws — but with a null code, because a refusal will be refused again while an unanswered query usually means the link needs attention first.

The queue is drained before the command is sent. The device's error queue is FIFO and can already hold entries from earlier commands or the connect sequence, so a single SYSTem:ERRor? afterwards could pop somebody else's failure. This is the same trap already documented on the SD listing's terminator, which is why that one is read as a liveness marker and never classified. Draining first is what makes the verdict attributable. Side effect, documented on the API: the drain discards what the queue held, so a caller who wants those entries should read them with DrainErrorQueueAsync first.

Additive throughout. The void commands are untouched, on the wire and in signature. The confirming ones live on a separate interface rather than as new IStreamingDevice members so existing implementers keep compiling — adding them to IStreamingDevice broke every test fake that implements it, which is a fair preview of what it would do downstream. This follows the capability-slice shape the device already uses (INetworkConfigurable, ISdCardOperations, ILanChipInfoProvider, IDeviceDiagnostics):

if (device is IConfirmingDeviceAdministration admin)
{
    await admin.LoadAdcCalibrationAsync(cancellationToken);
}

Confirmation costs text exchanges rather than a single write, so the void commands remain the right choice mid-stream or when a silent no-op is acceptable. Their docs now state the silence explicitly and point at the twin.

Tests

• Confirming path per operation: drain → command + SYSTem:ERRor? in one exchange, asserted in order (the drain happening before the exchange is the point, not just that it happens).
• The headline case: -200,"Execution error" surfaces as DeviceCommandFailedException with ErrorCode == -200 and the device's own line.
• Volunteered **ERROR: ... lines, unreadable verdicts (empty response, no queue reply), extra output ahead of a clean verdict.
• Guards: disconnected and pre-cancelled both throw having touched nothing — not even the drain.
• Argument validation throws before any device contact, ParamName preserved.
• TryParseSystemErrorReplyCode in ScpiResponseClassifier, including the overflow and wrong-form cases.
• The nine new entry points added to the existing DeviceNotConnectedExceptionTests guard theory.

Full suite green on net9.0 and net10.0 (2765 passed each). Five FirmwareUpdateServiceTests.UpdateWifiModuleAsync_* tests fail, but they fail identically on a clean checkout of origin/main at 7965b26 — verified in a separate worktree. Pre-existing and unrelated to this change.

Not bench-tested: the confirming path has no hardware evidence yet. The behavior it reacts to does — that is what motivated it.

🤖 Generated with Claude Code

[qodo-code-review]

PR Summary by Qodo

Add confirming async admin commands with SCPI error-queue verification

✨ Enhancement 🐞 Bug fix 🧪 Tests 🕐 40+ Minutes

AI Description

• Add confirming async variants for ADC calibration and voltage precision administration commands.
• Throw DeviceCommandFailedException when devices refuse commands or don’t confirm outcomes.
• Extend operation host with error-queue draining and add thorough unit test coverage.

Diagram

graph TD
  A["Caller code"] --> B[["IConfirmingDeviceAdministration"]] --> C["DaqifiStreamingDevice"] --> D["DeviceAdministrationOperations"] --> E[["IDeviceOperationHost"]] --> F{{"Device (SCPI)"}}
  D --> G["ScpiResponseClassifier"] --> H[["DeviceCommandFailedException"]]

  subgraph Legend
    direction LR
    _iface[["Interface"]] ~~~ _cls["Class/Module"] ~~~ _dev{{"External device"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Add confirming methods directly to IStreamingDevice

• ➕ Single interface for all device operations; fewer casts/pattern checks
• ➖ Breaking change for downstream implementers and test fakes
• ➖ Forces all implementations to carry confirmation semantics even if unsupported/undesired

2. Return a result object instead of throwing

• ➕ Avoids exceptions for expected refusals; can be more ergonomic for retry loops
• ➖ Easy for callers to ignore, reintroducing silent failures
• ➖ Still needs a strong convention to enforce checking the outcome

3. Always confirm (replace void commands)

• ➕ Eliminates the entire class of silent no-ops
• ➕ Simpler mental model: admin commands either succeed or throw
• ➖ Higher runtime cost (extra exchanges, pauses streaming consumer, takes locks)
• ➖ Potentially unsafe to run during streaming; loses the lightweight fire-and-forget option

Recommendation: Keep the current approach: a separate additive capability interface plus throwing DeviceCommandFailedException. It avoids downstream breakage (a historically sensitive point for this repo), forces callers to acknowledge failures, and preserves the existing low-cost void commands for mid-stream/low-risk usage.

Files changed (12) +853 / -21

Enhancement (6) +508 / -13

DaqifiStreamingDevice.csImplement confirming administration interface and host drain delegation +42/-1

Implement confirming administration interface and host drain delegation

• Makes DaqifiStreamingDevice implement IConfirmingDeviceAdministration and delegates each new async method to DeviceAdministrationOperations. Exposes DrainErrorQueueAsync through the IDeviceOperationHost explicit implementation.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

DeviceCommandFailedException.csIntroduce exception type for refused or unconfirmed device commands +80/-0

Introduce exception type for refused or unconfirmed device commands

• Adds DeviceCommandFailedException carrying Command, ErrorCode (nullable for unknown outcome), and raw DeviceResponse. Provides distinct constructors/messages for refusal vs unreadable verdict scenarios.

src/Daqifi.Core/Device/DeviceCommandFailedException.cs

IConfirmingDeviceAdministration.csAdd new confirming administration capability interface +175/-0

Add new confirming administration capability interface

• Defines confirming async counterparts for nine fire-and-forget administration commands, documenting costs, usage guidance (pattern-match for capability), and the pre-drain side effect on the SCPI error queue.

src/Daqifi.Core/Device/IConfirmingDeviceAdministration.cs

DeviceAdministrationOperations.csImplement confirming send+verify flow for administration commands +180/-12

Implement confirming send+verify flow for administration commands

• Adds confirming async variants that drain the error queue, send the command plus SYSTem:ERRor? in a single text exchange, and classify responses to throw on refusal or unknown verdict. Refactors argument validation into shared helpers and introduces confirmation-specific timeouts and drain caps.

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

IDeviceOperationHost.csExtend host contract with DrainErrorQueueAsync +9/-0

Extend host contract with DrainErrorQueueAsync

• Adds DrainErrorQueueAsync to IDeviceOperationHost to support attributing post-command SYSTem:ERRor? replies to the command being confirmed.

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

ScpiResponseClassifier.csAdd parser for numeric codes in SYSTem:ERRor? replies +22/-0

Add parser for numeric codes in SYSTem:ERRor? replies

• Introduces TryParseSystemErrorReplyCode to extract the integer code from standard SCPI error-queue reply lines, distinct from parsing volunteered **ERROR: lines.

src/Daqifi.Core/Device/ScpiResponseClassifier.cs

Tests (5) +307 / -4

DeviceNotConnectedExceptionTests.csAdd not-connected guard coverage for confirming admin methods +9/-0

Add not-connected guard coverage for confirming admin methods

• Extends the list of asynchronous guard sites to include the new confirming administration commands, ensuring they throw DeviceNotConnectedException when disconnected.

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

DeviceAdministrationOperationsTests.csAdd comprehensive tests for confirming administration variants +269/-4

Add comprehensive tests for confirming administration variants

• Introduces a full test matrix for confirming commands: correct drain/exchange ordering, refusal vs unknown verdict handling, volunteered error-line precedence, cancellation/disconnected behavior, and argument validation. Enhances the FakeHost to support text exchanges and error-queue draining while recording call order.

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

LiveSampleStreamTests.csUpdate test host stub for new DrainErrorQueueAsync contract +3/-0

Update test host stub for new DrainErrorQueueAsync contract

• Adds the DrainErrorQueueAsync member to the local IDeviceOperationHost test stub so it compiles with the updated interface.

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

StreamFrameDecoderTests.csUpdate test host stub for new DrainErrorQueueAsync contract +3/-0

Update test host stub for new DrainErrorQueueAsync contract

• Adds the DrainErrorQueueAsync member to the local IDeviceOperationHost test stub so it compiles with the updated interface.

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

ScpiResponseClassifierTests.csTest parsing of SYSTem:ERRor? numeric reply codes +23/-0

Test parsing of SYSTem:ERRor? numeric reply codes

• Adds unit tests covering successful parsing of signed/whitespace variants and rejection of non-reply formats and overflow cases for TryParseSystemErrorReplyCode.

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

Documentation (1) +38 / -4

IStreamingDevice.csDocument fire-and-forget semantics and point to confirming alternatives +38/-4

Document fire-and-forget semantics and point to confirming alternatives

• Adds a documentation block describing why the void admin commands can silently no-op and references the confirming interface methods for callers that need verified outcomes.

src/Daqifi.Core/Device/IStreamingDevice.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. Verdict lost to timeout ✓ Resolved 🐞 Bug ☼ Reliability

Description

SendConfirmedAsync uses ExecuteTextCommandAsync without overriding completionTimeoutMs, so the
default 250ms inactivity cutoff can end the exchange after an early echo and before the later
SYSTem:ERRor? reply arrives. This can throw DeviceCommandFailedException (unknown outcome) even
when the device ultimately accepted the command.

Code

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[R286-293]

+            var lines = await _host.ExecuteTextCommandAsync(
+                () =>
+                {
+                    _host.Send(command);
+                    _host.Send(ScpiMessageProducer.GetSystemError);
+                },
+                responseTimeoutMs: ConfirmationResponseTimeoutMs,
+                cancellationToken: cancellationToken).ConfigureAwait(false);

Relevance

●●● Strong

Timeout/echo handling has been actively tuned before; likely they’ll set completionTimeoutMs to
avoid premature exchange completion.

PR-#126

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The confirming exchange omits completionTimeoutMs, so it uses the default 250ms inactivity window.
ExecuteTextCommandAsync switches to that completion window immediately after the first received
line, which can be an echoed command, causing later verdict lines to be missed.

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[284-296]
src/Daqifi.Core/Device/DaqifiDevice.cs[2298-2337]

Agent prompt

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

### Issue description
Confirming admin commands may throw a false failure because the `ExecuteTextCommandAsync` exchange terminates after `completionTimeoutMs` of inactivity once *any* line has arrived. If the firmware echoes the command quickly, then performs a slow NVM write and only afterwards replies to `SYSTem:ERRor?`, the 250ms default completion timeout can expire before the verdict line arrives.

### Issue Context
- `SendConfirmedAsync` passes `responseTimeoutMs: ConfirmationResponseTimeoutMs` but **does not** pass `completionTimeoutMs`, so it defaults to 250ms.
- `ExecuteTextCommandAsync`’s collection loop switches to the short completion timeout immediately after the first received line (which could be an echo), potentially missing later lines.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[275-296]
- src/Daqifi.Core/Device/DaqifiDevice.cs[2298-2337]

### Implementation guidance
- In `SendConfirmedAsync`, pass an explicit `completionTimeoutMs` suitable for the worst-case gap between echo and verdict (at minimum, align it with `ConfirmationResponseTimeoutMs`, or introduce a dedicated `ConfirmationCompletionTimeoutMs`).
- Optionally (more robust), change the exchange termination strategy for confirming commands to wait until a `SYSTem:ERRor?` reply line is observed (or until the overall timeout/cancellation), rather than relying purely on inactivity after the first line.

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

---

2. Code-0 line diagnostics lost ✓ Resolved 🐞 Bug ◔ Observability ⭐ New

Description

In ThrowIfNotAccepted, an error-shaped line that parses to code 0 is ignored and, if no numeric
SYSTem:ERRor? reply is present, the later failure throws with DeviceResponse=null—discarding the
only device output. This makes debugging harder and can mislead callers into thinking the device was
silent when it actually responded.

Code

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[R333-336]

+                if (volunteeredCode != 0)
+                {
+                    throw new DeviceCommandFailedException(command, volunteeredCode, volunteeredError);
+                }

Relevance

●●● Strong

Repo favors retaining/surfacing raw device output for debugging rather than dropping responses;
similar observability changes were accepted.

PR-#266
PR-#185

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The confirming classifier explicitly falls through on volunteeredCode == 0, and if no
IsSystemErrorReplyLine is present it later throws using reply (which can be null), causing the
exception’s DeviceResponse to be null even though volunteeredError existed. The tests include a
scenario with only an error-shaped code-0 line, demonstrating this path is considered but not
asserting that the response is retained.

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[322-348]
src/Daqifi.Core/Device/DeviceCommandFailedException.cs[105-111]
src/Daqifi.Core.Tests/Device/Internal/DeviceAdministrationOperationsTests.cs[369-386]

Agent prompt

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

### Issue description
`ThrowIfNotAccepted` treats a SCPI error-shaped line whose parsed code is `0` as “not evidence of refusal” and falls through to rely on the numeric `SYSTem:ERRor?` reply. When that numeric reply is missing/unreadable, the method throws using `reply` (often `null`), which loses the previously-received device line (the code-0 error-shaped line).

### Issue Context
This behavior is in the new confirming-command classification logic. There is already a test case that simulates `"**ERROR: 0,\"No error\""` / `"ERROR: 0,\"No error\""` without a numeric queue reply, but it only asserts that `ErrorCode` is not reported as `0`, not that the raw response is preserved.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[333-341]

### Suggested change
When a code-0 error-shaped line is observed but no readable numeric `SYSTem:ERRor?` reply is found, include that code-0 line in the thrown `DeviceCommandFailedException` (e.g., pass it as `deviceResponse`) so diagnostics aren’t lost. This should still throw (unconfirmed outcome), but preserve the best available device evidence.

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

---

3. Misleading ErrorCode on parse ✓ Resolved 🐞 Bug ≡ Correctness

Description

ThrowIfNotAccepted ignores the boolean result of TryExtractErrorCode and always throws using the
integer-code exception overload, so an unparseable volunteered SCPI error line can surface with
ErrorCode == 0. This produces misleading diagnostics and can break callers that branch on
ErrorCode.

Code

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[R311-316]

+            var volunteeredError = lines.LastOrDefault(ScpiResponseClassifier.IsScpiErrorLine)?.Trim();
+            if (volunteeredError != null)
+            {
+                ScpiResponseClassifier.TryExtractErrorCode(volunteeredError, out var volunteeredCode);
+                throw new DeviceCommandFailedException(command, volunteeredCode, volunteeredError);
+            }

Relevance

●●● Strong

Deterministic correctness fix: don’t throw parsed code when parsing fails; similar
parsing/diagnostic improvements were accepted.

PR-#288
PR-#357

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new confirming logic throws using volunteeredCode even if extraction fails. The classifier
explicitly returns false and sets code = 0 on parse failure, making the thrown ErrorCode
misleading.

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[309-316]
src/Daqifi.Core/Device/ScpiResponseClassifier.cs[186-210]
src/Daqifi.Core/Device/ScpiResponseClassifier.cs[241-262]

Agent prompt

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

### Issue description
When a device volunteers a SCPI error line, `ThrowIfNotAccepted` calls `TryExtractErrorCode(...)` but ignores its return value and throws `DeviceCommandFailedException(command, volunteeredCode, volunteeredError)` regardless. On parse failure, `volunteeredCode` remains `0`, which is misleading because `0` conventionally indicates “no error”.

### Issue Context
- `IsScpiErrorLine` can match some `ERROR...` forms that still don’t yield a parseable integer code.
- `TryExtractErrorCode` returns `false` and sets `code = 0` when it can’t extract a valid integer.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[309-317]
- src/Daqifi.Core/Device/ScpiResponseClassifier.cs[49-54]
- src/Daqifi.Core/Device/ScpiResponseClassifier.cs[186-210]

### Implementation guidance
- Capture the boolean result:
 - If `TryExtractErrorCode(...)` returns `true`, throw the current integer-code overload.
 - If it returns `false`, throw the “unknown outcome” overload (`new DeviceCommandFailedException(command, volunteeredError)`) so `ErrorCode` is null and the raw device line is preserved.
- Add/extend a unit test covering a volunteered SCPI error line that matches `IsScpiErrorLine` but does not contain a parseable integer code, asserting that `ErrorCode` is null and `DeviceResponse` is preserved.

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

---

4. Misleading code-0 message ✓ Resolved 🐞 Bug ⚙ Maintainability ⭐ New

Description

DeviceCommandFailedException normalizes errorCode==0 to ErrorCode=null, but the message builder
treats it as an “unreadable code” case and claims the response carries no readable SCPI code. For
inputs like "**ERROR: 0,\"No error\"" this message is factually inaccurate and reduces trust in
exception diagnostics.

Code

src/Daqifi.Core/Device/DeviceCommandFailedException.cs[R92-96]

+        private static string BuildRefusalMessage(string command, int errorCode, string deviceResponse)
+            => errorCode == 0
+                ? BuildUnreadableVerdictMessage(command, deviceResponse)
+                : $"The device rejected '{command}' with SCPI error {errorCode} ({deviceResponse}). "
+                  + "The command did not take effect.";

Relevance

●●● Strong

Team frequently fixes misleading diagnostics/docs; improving message accuracy matches prior accepted
review feedback.

PR-#354
PR-#98

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The constructor’s message-building path for errorCode == 0 explicitly uses the “unreadable
verdict” message, which states there is no readable SCPI error code. The tests construct this
exception with a readable code-0 line, proving the misleading message path is reachable at the
public API boundary.

src/Daqifi.Core/Device/DeviceCommandFailedException.cs[92-118]
src/Daqifi.Core.Tests/Device/Internal/DeviceAdministrationOperationsTests.cs[388-403]

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 `DeviceCommandFailedException(string command, int errorCode, string deviceResponse)` constructor normalizes `errorCode == 0` to `ErrorCode == null`, but `BuildRefusalMessage` routes that case into `BuildUnreadableVerdictMessage`, which says the response “carries no readable SCPI error code.” That wording is incorrect when the provided response clearly contains a readable code `0` (meaning “No error”).

### Issue Context
Although production paths should not normally throw with code `0`, the PR deliberately supports this constructor behavior (and tests it) to keep the “never report 0” contract.

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceCommandFailedException.cs[92-97]

### Suggested change
Introduce a dedicated message for `errorCode == 0` that acknowledges the device reported SCPI code 0/“No error” but that this does not represent a refusal and is being normalized to `ErrorCode == null`. Optionally strengthen the existing unit test to assert the corrected wording (e.g., it should not claim the code is unreadable).

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

---

5. Zero ErrorCode allowed ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

DeviceCommandFailedException documents that ErrorCode is never 0 (reserved for SCPI "No error"), but
the public (string,int,string) constructor accepts and stores 0 without validation. This allows
constructing an exception instance whose state contradicts its documented contract and can confuse
consumers that interpret 0 as success/no-error.

Code

src/Daqifi.Core/Device/DeviceCommandFailedException.cs[R52-55]

+        /// when no code could be read — either because nothing came back or because what did carried no
+        /// readable code. Never <c>0</c>: that is the value SCPI reserves for "no error", so a refusal
+        /// is never reported under it. Pair with <see cref="DeviceResponse"/> to tell an unreadable
+        /// refusal from silence.

Relevance

●●● Strong

Team often adds fail-fast validation around SCPI “0/No error” semantics; invariant enforcement
likely welcomed.

PR-#185
PR-#419

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The XML docs explicitly promise that ErrorCode is never 0, but the constructor assigns the passed
value directly with no check, so callers can pass 0 and violate the documented invariant.

src/Daqifi.Core/Device/DeviceCommandFailedException.cs[50-57]
src/Daqifi.Core/Device/DeviceCommandFailedException.cs[71-78]

Agent prompt

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

### Issue description
`DeviceCommandFailedException.ErrorCode` is documented as never being `0`, but the public constructor that accepts an `int errorCode` does not enforce this invariant. This can lead to exception instances that contradict the documented contract.

### Issue Context
SCPI reserves `0` to mean "No error"; the docs explicitly say refusals are never reported under `0`.

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceCommandFailedException.cs[50-78]

### Implementation sketch
- Add a guard in `DeviceCommandFailedException(string command, int errorCode, string deviceResponse)`:
 - If `errorCode == 0`, throw `ArgumentOutOfRangeException(nameof(errorCode), errorCode, "ErrorCode must be non-zero; 0 means 'No error'.")`.
- Alternatively (if you want to allow 0), update the XML docs to remove/soften the "Never 0" guarantee; but enforcing the invariant is safer for a public 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 b736241

Results up to commit 332a9a1 ⚖️ Balanced

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

1. Verdict lost to timeout ✓ Resolved 🐞 Bug ☼ Reliability

Description

SendConfirmedAsync uses ExecuteTextCommandAsync without overriding completionTimeoutMs, so the
default 250ms inactivity cutoff can end the exchange after an early echo and before the later
SYSTem:ERRor? reply arrives. This can throw DeviceCommandFailedException (unknown outcome) even
when the device ultimately accepted the command.

Code

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[R286-293]

+            var lines = await _host.ExecuteTextCommandAsync(
+                () =>
+                {
+                    _host.Send(command);
+                    _host.Send(ScpiMessageProducer.GetSystemError);
+                },
+                responseTimeoutMs: ConfirmationResponseTimeoutMs,
+                cancellationToken: cancellationToken).ConfigureAwait(false);

Relevance

●●● Strong

Timeout/echo handling has been actively tuned before; likely they’ll set completionTimeoutMs to
avoid premature exchange completion.

PR-#126

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The confirming exchange omits completionTimeoutMs, so it uses the default 250ms inactivity window.
ExecuteTextCommandAsync switches to that completion window immediately after the first received
line, which can be an echoed command, causing later verdict lines to be missed.

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[284-296]
src/Daqifi.Core/Device/DaqifiDevice.cs[2298-2337]

Agent prompt

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

### Issue description
Confirming admin commands may throw a false failure because the `ExecuteTextCommandAsync` exchange terminates after `completionTimeoutMs` of inactivity once *any* line has arrived. If the firmware echoes the command quickly, then performs a slow NVM write and only afterwards replies to `SYSTem:ERRor?`, the 250ms default completion timeout can expire before the verdict line arrives.

### Issue Context
- `SendConfirmedAsync` passes `responseTimeoutMs: ConfirmationResponseTimeoutMs` but **does not** pass `completionTimeoutMs`, so it defaults to 250ms.
- `ExecuteTextCommandAsync`’s collection loop switches to the short completion timeout immediately after the first received line (which could be an echo), potentially missing later lines.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[275-296]
- src/Daqifi.Core/Device/DaqifiDevice.cs[2298-2337]

### Implementation guidance
- In `SendConfirmedAsync`, pass an explicit `completionTimeoutMs` suitable for the worst-case gap between echo and verdict (at minimum, align it with `ConfirmationResponseTimeoutMs`, or introduce a dedicated `ConfirmationCompletionTimeoutMs`).
- Optionally (more robust), change the exchange termination strategy for confirming commands to wait until a `SYSTem:ERRor?` reply line is observed (or until the overall timeout/cancellation), rather than relying purely on inactivity after the first line.

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

---

2. Misleading ErrorCode on parse ✓ Resolved 🐞 Bug ≡ Correctness

Description

ThrowIfNotAccepted ignores the boolean result of TryExtractErrorCode and always throws using the
integer-code exception overload, so an unparseable volunteered SCPI error line can surface with
ErrorCode == 0. This produces misleading diagnostics and can break callers that branch on
ErrorCode.

Code

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[R311-316]

+            var volunteeredError = lines.LastOrDefault(ScpiResponseClassifier.IsScpiErrorLine)?.Trim();
+            if (volunteeredError != null)
+            {
+                ScpiResponseClassifier.TryExtractErrorCode(volunteeredError, out var volunteeredCode);
+                throw new DeviceCommandFailedException(command, volunteeredCode, volunteeredError);
+            }

Relevance

●●● Strong

Deterministic correctness fix: don’t throw parsed code when parsing fails; similar
parsing/diagnostic improvements were accepted.

PR-#288
PR-#357

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new confirming logic throws using volunteeredCode even if extraction fails. The classifier
explicitly returns false and sets code = 0 on parse failure, making the thrown ErrorCode
misleading.

src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[309-316]
src/Daqifi.Core/Device/ScpiResponseClassifier.cs[186-210]
src/Daqifi.Core/Device/ScpiResponseClassifier.cs[241-262]

Agent prompt

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

### Issue description
When a device volunteers a SCPI error line, `ThrowIfNotAccepted` calls `TryExtractErrorCode(...)` but ignores its return value and throws `DeviceCommandFailedException(command, volunteeredCode, volunteeredError)` regardless. On parse failure, `volunteeredCode` remains `0`, which is misleading because `0` conventionally indicates “no error”.

### Issue Context
- `IsScpiErrorLine` can match some `ERROR...` forms that still don’t yield a parseable integer code.
- `TryExtractErrorCode` returns `false` and sets `code = 0` when it can’t extract a valid integer.

### Fix Focus Areas
- src/Daqifi.Core/Device/Internal/DeviceAdministrationOperations.cs[309-317]
- src/Daqifi.Core/Device/ScpiResponseClassifier.cs[49-54]
- src/Daqifi.Core/Device/ScpiResponseClassifier.cs[186-210]

### Implementation guidance
- Capture the boolean result:
 - If `TryExtractErrorCode(...)` returns `true`, throw the current integer-code overload.
 - If it returns `false`, throw the “unknown outcome” overload (`new DeviceCommandFailedException(command, volunteeredError)`) so `ErrorCode` is null and the raw device line is preserved.
- Add/extend a unit test covering a volunteered SCPI error line that matches `IsScpiErrorLine` but does not contain a parseable integer code, asserting that `ErrorCode` is null and `DeviceResponse` is preserved.

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

---

Results up to commit 134a8a2 ⚖️ Balanced

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

1. Zero ErrorCode allowed ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

DeviceCommandFailedException documents that ErrorCode is never 0 (reserved for SCPI "No error"), but
the public (string,int,string) constructor accepts and stores 0 without validation. This allows
constructing an exception instance whose state contradicts its documented contract and can confuse
consumers that interpret 0 as success/no-error.

Code

src/Daqifi.Core/Device/DeviceCommandFailedException.cs[R52-55]

+        /// when no code could be read — either because nothing came back or because what did carried no
+        /// readable code. Never <c>0</c>: that is the value SCPI reserves for "no error", so a refusal
+        /// is never reported under it. Pair with <see cref="DeviceResponse"/> to tell an unreadable
+        /// refusal from silence.

Relevance

●●● Strong

Team often adds fail-fast validation around SCPI “0/No error” semantics; invariant enforcement
likely welcomed.

PR-#185
PR-#419

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The XML docs explicitly promise that ErrorCode is never 0, but the constructor assigns the passed
value directly with no check, so callers can pass 0 and violate the documented invariant.

src/Daqifi.Core/Device/DeviceCommandFailedException.cs[50-57]
src/Daqifi.Core/Device/DeviceCommandFailedException.cs[71-78]

Agent prompt

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

### Issue description
`DeviceCommandFailedException.ErrorCode` is documented as never being `0`, but the public constructor that accepts an `int errorCode` does not enforce this invariant. This can lead to exception instances that contradict the documented contract.

### Issue Context
SCPI reserves `0` to mean "No error"; the docs explicitly say refusals are never reported under `0`.

### Fix Focus Areas
- src/Daqifi.Core/Device/DeviceCommandFailedException.cs[50-78]

### Implementation sketch
- Add a guard in `DeviceCommandFailedException(string command, int errorCode, string deviceResponse)`:
 - If `errorCode == 0`, throw `ArgumentOutOfRangeException(nameof(errorCode), errorCode, "ErrorCode must be non-zero; 0 means 'No error'.")`.
- Alternatively (if you want to allow 0), update the XML docs to remove/soften the "Never 0" guarantee; but enforcing the invariant is safer for a public API.

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

---

[tylerkron]

Addressed both findings in 134a8a2 (replies on the inline threads). Branch is rebased onto the updated base, and the full suite is now green on net9.0 and net10.0 — 2777 passed, 0 failed (the 5 UpdateWifiModuleAsync_* failures were pre-existing and are resolved by #456, which is now in this branch's history).

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 134a8a2

[tylerkron]

Round 3: finding 3 ("Zero ErrorCode allowed") addressed in b88828d — normalised at the constructor rather than guarded with ArgumentOutOfRangeException; reasoning on the inline thread. Findings 1 and 2 are marked resolved.

Note the review above was generated against 134a8a2; b89d981 and b88828d have landed since. Suite green on net9.0 and net10.0 — 2782 passed, 0 failed.

/agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 062043e

[tylerkron]

Bench evidence: confirming admin commands on real hardware — 8/8

Ran the confirming path against the bench Nyquist (fw 3.7.2, SN 9090539562006014104, USB CDC, /dev/cu.usbmodem1101). 8 passed, 0 failed. This closes the "not bench-tested" caveat in the PR description.

Non-destructive by construction. Only queries, the two RAM restores (CONFigure:ADC:LOADcal, CONFigure:VOLTage:LOAD), and one deliberately-bogus command to seed the error queue ever reached the device. Never sent: SAVEcal, SAVEFcal, LOADFcal, VOLTage:SAVE, USECal <n>, any device-name write, SYSTem:REboot. Active cal bank 0 and ch0 CalM 1 identical at entry and exit; error queue clean at exit.

Writing the user bank was avoided for a second reason: it would have destroyed the -200 reproduction that makes this unit useful for the headline test.

The motivating case, end to end

LoadAdcCalibrationAsync() — the call that used to return normally while the device refused it:

threw after 3117ms
  Command        = CONFigure:ADC:LOADcal
  ErrorCode      = -200
  DeviceResponse = **ERROR: -200, "Execution error"
  Message        = The device rejected 'CONFigure:ADC:LOADcal' with SCPI error -200
                   (**ERROR: -200, "Execution error"). The command did not take effect.

The false-positive check

LoadVoltagePrecisionAsync() — accepted by this unit — completed normally in 3112ms. Every false-failure mode in this feature fails in that direction, so this one carries as much weight as the test above.

Attribution: the pre-send drain

Seeded a stale -113 into the error queue, then issued a command this unit accepts. It reported success. Without the drain that stale entry would have been popped and blamed on the wrong command — the design decision a mock cannot validate. Negative control first: a bogus command really does land -113,"Undefined header", so the clean queues above mean something.

What the wire actually carries

A refusal produces both forms, and the classifier's two paths turn out to correspond to real firmware behaviour rather than to my assumptions:

CONFigure:ADC:LOADcal + SYSTem:ERRor?  ->  2 lines
  [0] **ERROR: -200, "Execution error"     <- volunteered, about the command
  [1] -200,"Execution error"               <- the bare queue reply

A success produces one line, 0,"No error". So this firmware answers SYSTem:ERRor? in the bare shape when clean and volunteers the **ERROR:-prefixed shape alongside it when not.

The measurement that was previously a guess

ConfirmationCompletionTimeoutMs = 1000 was reasoned by analogy to the SD listing. Measured:

• This firmware does not echo commands — the premise behind Qodo's finding does not hold on this transport.
• Every line arrives within ~20 ms of the first. 250 ms would have sufficed; the window is pure trailing latency here.
• Cost: ~3.1 s per confirming command (drain exchange ~1.06 s + confirming exchange ~2.06 s).

Kept at 1000 ms anyway, and I want to be explicit that this is a judgement call rather than something the bench endorsed. It is headroom for the two cases this pass could not cover: WiFi, whose gaps are the documented reason the SD listing raised its own window, and the NVM writers, which are destructive to a calibrated unit. Tightening it is a reasonable follow-up once either has evidence. Both the measurement and the reason for keeping the value are now recorded in the constant's docs (b736241), along with the ~3 s cost on the interface — it warned that confirmation was not free without ever saying how much.

_{Bench only — no device state changed. Harness was a throwaway console app built against this branch's Core.}