feat(device): SetFriendlyNameAsync (part of #345)

[tylerkron]

Summary

Adds the device-level SetFriendlyNameAsync that #302 left to consumers — addressing item 2 of the #345 batch. Composes the producer commands already in Core into the validate → set → save → optimistic-update sequence desktop currently hand-rolls (its "no producer helper exists" comment is now stale and its copy can be deleted on the next Core bump).

Changes

• DaqifiStreamingDevice.SetFriendlyNameAsync(string name, CancellationToken):
  • Validates via ScpiMessageProducer.IsFriendlyNameValid (throws ArgumentException before sending anything).
  • Requires a connected device; observes cancellation before sending.
  • Sends SYSTem:DEVice:NAME "name" then SYSTem:DEVice:NAME:SAVE, then optimistically sets Metadata.FriendlyName (the device doesn't echo the name back synchronously).

Testing

• dotnet test — 1644 passed / 0 failed / 2 skipped (net9.0 + net10.0).
• 8 unit tests: null → ArgumentNullException; invalid/too-long/"/\ → ArgumentException with nothing sent; not-connected → InvalidOperationException; valid → sends both commands in order + updates metadata; max-length (31) accepted; pre-cancelled token → throws and sends nothing.
• Bench-validated on Nq1 (FW 3.7.2): read the device's current name and set it to itself (deliberately non-destructive — no identity change), confirming the command executes on real hardware and metadata updates.

Scope note

This closes acceptance-criterion 2 of #345 only. The other two items — PWM SetPwmFrequency skip-if-unchanged (+ removing the MCP _lastSentPwmFrequencyHz workaround) and consolidating SCPI error-code extraction into ScpiResponseClassifier — are intentionally left for separate focused PRs. Does not close #345.

Not merging — for review.

🤖 Generated with Claude Code

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

feat(device): Add DaqifiStreamingDevice.SetFriendlyNameAsync

✨ Enhancement 🧪 Tests 🕐 20-40 Minutes

AI Description

• Add device-level API to validate, set, and persist the firmware friendly name.
• Enforce preconditions (valid name, connected device, cancellation) before sending SCPI.
• Add unit coverage ensuring correct command sequence and optimistic metadata update.

Diagram

graph TD
  A["Consumer (Desktop/App)"] --> B["DaqifiStreamingDevice.SetFriendlyNameAsync"] --> C{"Preconditions OK?"}
  C -->|"no"| D["Throw exception"]
  C -->|"yes"| E["Send SCPI: set name"] --> F["Send SCPI: save name"] --> G["Update local metadata"]
  F --> H[("Device NVM")]
  T["Unit tests"] --> B

  subgraph Legend
    direction LR
    _proc["Process"] ~~~ _dec{"Decision"} ~~~ _db[("Persistence")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make it truly async with transport-level acknowledgement

• ➕ Could guarantee commands were transmitted/acknowledged before completing
• ➕ Can surface firmware/transport errors more deterministically
• ➖ May require broader transport/API changes (callbacks, response correlation)
• ➖ Firmware doesn’t echo name synchronously, so value confirmation still may require polling

2. Expose a single composed producer command (set+save)

• ➕ Keeps sequencing logic centralized in the message/producer layer
• ➕ Encourages reuse across all consumers
• ➖ Producer layer may become less granular and harder to reuse in other sequences
• ➖ Still needs device-level checks (IsConnected, cancellation) and metadata update elsewhere

Recommendation: The PR’s approach is a good fit for the current architecture: it centralizes a frequently reused, safety-checked SCPI sequence at the device layer and adds tests to prevent regressions (especially the “send nothing on invalid/cancelled” behavior). Consider the transport-acknowledged async alternative only if other device APIs start needing stronger delivery guarantees.

Files changed (2) +146 / -0

Enhancement (1) +49 / -0

DaqifiStreamingDevice.csAdd SetFriendlyNameAsync composing validate → set → save → optimistic metadata update +49/-0

Add SetFriendlyNameAsync composing validate → set → save → optimistic metadata update

• Adds SetFriendlyNameAsync(string, CancellationToken) to validate friendly-name constraints via ScpiMessageProducer, require connectivity, and observe cancellation before sending. Sends the SCPI set-name command followed by save-to-NVM, then optimistically updates Metadata.FriendlyName since firmware doesn’t echo the name back synchronously.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

Tests (1) +97 / -0

DaqifiStreamingDeviceFriendlyNameTests.csAdd unit tests for SetFriendlyNameAsync validation, ordering, and safety +97/-0

Add unit tests for SetFriendlyNameAsync validation, ordering, and safety

• Introduces a capturing test double for DaqifiStreamingDevice to record outbound SCPI strings. Verifies null/invalid names throw and send nothing, not-connected throws, valid names send set-then-save and update metadata, max-length is accepted, and pre-cancelled tokens abort without sending.

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

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 feat: own device friendly-name (SCPI setter, validation, DeviceMetadata.FriendlyName) 🎫 chore: small API polish batch (PWM frequency idempotence, SetFriendlyNameAsync, SCPI error-code parsing consolidation)

1. Async completion misleading ✓ Resolved 🐞 Bug ≡ Correctness

Description

SetFriendlyNameAsync returns Task.CompletedTask immediately after calling Send twice, but Send
commonly only enqueues to the background MessageProducer; awaiting SetFriendlyNameAsync therefore
does not guarantee the SCPI commands have been written to the transport yet. This can mislead
callers (and future maintainers) into assuming the name is already on-wire/persisted when the task
completes.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R695-726]

+        /// <returns>A task that completes once both commands have been sent.</returns>
+        /// <exception cref="ArgumentNullException">Thrown when <paramref name="name"/> is null.</exception>
+        /// <exception cref="ArgumentException">Thrown when <paramref name="name"/> fails validation.</exception>
+        /// <exception cref="InvalidOperationException">Thrown when the device is not connected.</exception>
+        /// <exception cref="OperationCanceledException">Thrown when the operation is cancelled.</exception>
+        public Task SetFriendlyNameAsync(string name, CancellationToken cancellationToken = default)
+        {
+            if (name is null)
+            {
+                throw new ArgumentNullException(nameof(name));
+            }
+
+            if (!ScpiMessageProducer.IsFriendlyNameValid(name))
+            {
+                throw new ArgumentException(
+                    $"Device name must be 1-{ScpiMessageProducer.MaxFriendlyNameLength} printable ASCII characters and cannot contain '\"' or '\\'.",
+                    nameof(name));
+            }
+
+            if (!IsConnected)
+            {
+                throw new InvalidOperationException("Device is not connected.");
+            }
+
+            cancellationToken.ThrowIfCancellationRequested();
+
+            Send(ScpiMessageProducer.SetDeviceName(name));
+            Send(ScpiMessageProducer.SaveDeviceName);
+            Metadata.FriendlyName = name;
+
+            return Task.CompletedTask;
+        }

Relevance

⭐ Low

Team routinely uses Send()+Task.CompletedTask for async wrappers; same pattern merged/accepted in PR
#329.

PR-#329

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The method returns a completed task right after two Send calls, but the standard Send path for SCPI
strings enqueues work to MessageProducer, which performs the actual stream writes asynchronously on
a background thread; therefore task completion does not correspond to transport write completion.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[679-726]
src/Daqifi.Core/Device/DaqifiDevice.cs[413-440]
src/Daqifi.Core/Communication/Producers/MessageProducer.cs[123-171]

Agent prompt

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

### Issue description
`DaqifiStreamingDevice.SetFriendlyNameAsync(...)` returns `Task.CompletedTask` after calling `Send(...)` twice, while the send pipeline for SCPI strings is typically queued and written by a background thread. The current XML docs say the returned task completes once commands have been “sent”, which reads like on-wire completion; in reality it is (at best) “queued for sending”.

### Issue Context
- `DaqifiDevice.Send(...)` routes string outbound messages into `_messageProducer.Send(...)` when available.
- `MessageProducer<T>.Send(...)` enqueues into a `ConcurrentQueue`, and the actual stream writes occur later in the background `ProcessMessages` loop.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[679-726]
- src/Daqifi.Core/Device/DaqifiDevice.cs[413-440]
- src/Daqifi.Core/Communication/Producers/MessageProducer.cs[123-171]

### Suggested fix
- Update the XML `<returns>`/remarks on `SetFriendlyNameAsync` to explicitly state the task completes once both commands are **queued/enqueued** for sending (not necessarily written/flushed, and not acknowledged by the device).
- If the API intent truly is “on-wire completion”, introduce an explicit mechanism to await queue drain/transport write completion (and document that this still does not imply device-side persistence/ack without a response).

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

---

[tylerkron]

Re: Async completion misleading — this is a deliberate, codebase-consistent design rather than a defect. SetFriendlyNameAsync follows the same fire-and-forget contract as the existing device commands LoadNetworkConfigurationAsync, FactoryResetNetworkAsync, and the SD-format path (all Send(...) + return Task.CompletedTask): these SCPI commands are not acknowledged by the firmware, so there is nothing to await on-device, and the async signature exists for cancellation + device-surface consistency.

I've made the semantics explicit in the doc (be74dbd): the returned task completes once the commands are enqueued to the outbound producer, not when the device has applied/persisted the name. Behavior unchanged.

[tylerkron]

/agentic_review

[qodo-code-review]

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

[tylerkron]

Latest Qodo pass clean (Bugs 0, updated to be74dbd), CI green, full suite 1644 pass, bench-validated on Nq1. Bench-verified, ready for review.