feat(device): validate StreamingFrequency + return SD logging filename (closes #336, #337)

[tylerkron]

Summary

Two DaqifiStreamingDevice ergonomics fixes that each also delete duplicated logic from the in-repo MCP server.

#336 — validate StreamingFrequency against DeviceCapabilities.MaxSamplingRate

Previously an unvalidated auto-property; its value was sent to the device verbatim on StartStreaming(), so a bad rate (0, negative, 50 kHz) reached the hardware silently and every consumer re-declared the 1–1000 Hz limit.

• Setter validates against Metadata.Capabilities.MaxSamplingRate (the effective max comes from DeviceCapabilities, not a hardcoded constant) and throws ArgumentOutOfRangeException with the valid range — consistent with the PWM (feat: give PWM frequency and duty commandable defaults and clamp them on set #306) and channel-bounds (Add resolution/range/calibration bounds validation to AnalogChannel #300) guards.
• The ceiling is sanitized with Math.Max(1, …) so an invalid/uninitialized MaxSamplingRate (0/negative) can't produce an impossible 1..0 range (Qodo round 1).
• MCP server's duplicated HardwareMaxSampleRateHz = 1000 constant removed; it now reads the device's advertised max, and its --max-sample-rate-hz option still clamps below it.

#337 — expose the effective on-card filename (non-breaking)

Callers passing fileName: null never learned the log_{timestamp} name Core generated and had to duplicate the convention to report/download/delete the log.

• Added an additive Task<SdCardLoggingSession> StartSdCardLoggingSessionAsync(...) returning { FileName, Format }. The original Task StartSdCardLoggingAsync(...) signature is preserved (it shipped in v1.x) and now delegates to the new method — no public/ABI break (Qodo round 1).
• MCP server drops its duplicated filename generation (and the now-dead ExtensionFor helper) and uses the returned session.

Testing

• dotnet test — Core 1599 pass, MCP 23 pass, 0 fail. Boundary tests for StreamingFrequency (1, max, 0, max+1, lowered-cap, and invalid max≤0); tests asserting the returned SD name matches what was sent to the device (explicit + auto).
• Bench-tested on real hardware (Nyquist 1, FW 3.7.2): max read as 1000 Hz; 0/-1/1001/50000 rejected; invalid max sanitized to a 1..1 range; auto + explicit SD names returned via the session method and confirmed on the card.

Closes #336, closes #337.

[qodo-code-review]

PR Summary by Qodo

Validate StreamingFrequency against device capabilities and return SD log session info

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Validate StreamingFrequency using advertised DeviceCapabilities.MaxSamplingRate; reject invalid
 rates early.
• Make StartSdCardLoggingAsync return the effective SD filename + format via SdCardLoggingSession.
• Remove duplicated MCP server logic for sample-rate ceilings and SD filename generation; add
 coverage.

Diagram

graph TD
  A["Daqifi.Mcp DaqifiAgent"] -->|"set sample rate"| B["Core DaqifiStreamingDevice"] -->|"SCPI streaming"| C["Nyquist hardware"]
  B -->|"validate"| D["DeviceCapabilities.MaxSamplingRate"]
  A -->|"start SD log"| E["ISdCardOperations.StartSdCardLoggingAsync"] --> B
  B -->|"returns"| F["SdCardLoggingSession"] --> A

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Expose generated filename via separate query API

• ➕ Avoids a breaking signature change to StartSdCardLoggingAsync
• ➕ Lets callers fetch the name later if needed
• ➖ Adds extra state/queries and ordering concerns (must call after start)
• ➖ More surface area than a single return value; still needs format/name bundling

2. Return a tuple (string FileName, SdCardLogFormat Format)

• ➕ Minimal new types; quick to implement
• ➖ Less self-documenting and harder to evolve without breaking changes
• ➖ Public API tuples can be awkward across some consumers/serialization

3. Keep MCP-side filename generation

• ➕ No Core API change required
• ➖ Perpetuates duplicated conventions and drift risk between layers
• ➖ Callers outside MCP still can’t learn Core-generated names

Recommendation: Current approach (returning SdCardLoggingSession and validating StreamingFrequency against capabilities) is the best trade-off: it centralizes invariants/conventions in Core, removes duplication in MCP, and returns exactly the metadata consumers need. The only notable cost is the API signature change, which is justified by improved ergonomics and correctness.

Files changed (6) +179 / -28

Enhancement (3) +76 / -6

DaqifiStreamingDevice.csValidate StreamingFrequency and return SdCardLoggingSession from SD logging +34/-4

Validate StreamingFrequency and return SdCardLoggingSession from SD logging

• Replaces the StreamingFrequency auto-property with a guarded setter that enforces 1..Metadata.Capabilities.MaxSamplingRate and throws ArgumentOutOfRangeException with the valid range. Updates StartSdCardLoggingAsync to return SdCardLoggingSession containing the effective on-card filename and format.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs

ISdCardOperations.csChange StartSdCardLoggingAsync contract to return SdCardLoggingSession +6/-2

Change StartSdCardLoggingAsync contract to return SdCardLoggingSession

• Updates the interface signature and documentation so callers receive the effective filename and log format when starting SD logging, eliminating the need to replicate naming conventions externally.

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs

SdCardLoggingSession.csIntroduce SdCardLoggingSession return type for SD log startup +36/-0

Introduce SdCardLoggingSession return type for SD log startup

• Adds a small immutable DTO carrying FileName and Format for a started SD logging session. Designed to reflect the exact filename sent to the device, whether provided by the caller or auto-generated.

src/Daqifi.Core/Device/SdCard/SdCardLoggingSession.cs

Refactor (1) +10 / -22

DaqifiAgent.csUse capability-driven sample-rate ceiling and Core-returned SD log filename +10/-22

Use capability-driven sample-rate ceiling and Core-returned SD log filename

• Removes the hardcoded 1000 Hz constant and caps sample rate using device.Metadata.Capabilities.MaxSamplingRate (while still honoring the user cap option). Drops MCP-side SD log filename generation and uses the SdCardLoggingSession returned by Core to populate results.

src/Daqifi.Mcp/DaqifiAgent.cs

Tests (2) +93 / -0

DaqifiStreamingDeviceTests.csAdd StreamingFrequency range/behavior tests tied to capabilities +57/-0

Add StreamingFrequency range/behavior tests tied to capabilities

• Adds theory-based boundary tests ensuring in-range frequencies are accepted and out-of-range values throw ArgumentOutOfRangeException. Verifies failed sets do not mutate the previous value and that validation tracks Metadata.Capabilities.MaxSamplingRate rather than a hardcoded limit.

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

SdCardOperationsTests.csTest StartSdCardLoggingAsync returns effective filename and format +36/-0

Test StartSdCardLoggingAsync returns effective filename and format

• Adds assertions that a custom filename is returned verbatim and that an auto-generated log_*.{ext} name is returned exactly as sent to the device. Confirms the returned session includes the selected SdCardLogFormat.

src/Daqifi.Core.Tests/Device/SdCard/SdCardOperationsTests.cs

[qodo-code-review]

Code Review by Qodo

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

Context used

✅ Tickets: 🎫 Add resolution/range/calibration bounds validation to AnalogChannel 🎫 feat: StartSdCardLoggingAsync should return the effective on-card filename 🎫 feat: give PWM frequency and duty commandable defaults and clamp them on set +1 more

1. Breaking SD logging API ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

Changing ISdCardOperations.StartSdCardLoggingAsync from Task to Task<SdCardLoggingSession> is a
binary/source breaking change that will break downstream consumers and any external
implementations/mocks of ISdCardOperations. If compatibility is expected, this needs an additive
shim (new method name) or a major-version bump/release note strategy.

Code

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs[R112-118]

+        /// <returns>
+        /// A task that resolves to an <see cref="SdCardLoggingSession"/> carrying the effective on-card
+        /// file name (supplied or auto-generated) and the logging format, so callers can report, download,
+        /// or later delete the log without re-deriving Core's naming convention.
+        /// </returns>
        /// <exception cref="System.InvalidOperationException">Thrown when the device is not connected.</exception>
-        Task StartSdCardLoggingAsync(string? fileName = null, string? channelMask = null, SdCardLogFormat format = SdCardLogFormat.Protobuf, CancellationToken cancellationToken = default);
+        Task<SdCardLoggingSession> StartSdCardLoggingAsync(string? fileName = null, string? channelMask = null, SdCardLogFormat format = SdCardLogFormat.Protobuf, CancellationToken cancellationToken = default);

Relevance

⭐⭐⭐ High

Repo repeatedly accepts avoiding ABI breaks via shims/overloads for public APIs (PRs #321, #275,
#198).

PR-#321
PR-#275
PR-#198

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The interface method’s return type is changed to Task<SdCardLoggingSession> and the implementation
and MCP call sites were updated accordingly; this is a breaking contract change for any downstream
code compiled against the old interface signature.

src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs[92-118]
src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1342-1425]
src/Daqifi.Mcp/DaqifiAgent.cs[383-402]
PR-#321

Agent prompt

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

### Issue description
`ISdCardOperations.StartSdCardLoggingAsync(...)` changed return type from `Task` to `Task<SdCardLoggingSession>`. Return-type changes on public interfaces are breaking at both source and binary levels for downstream consumers and implementers.

### Issue Context
This repo has previously treated public surface-area ABI breaks as worth avoiding or shimming when possible.

### Fix Focus Areas
- src/Daqifi.Core/Device/SdCard/ISdCardOperations.cs[112-118]
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[1361-1425]
- src/Daqifi.Mcp/DaqifiAgent.cs[383-402]

### Suggested fix (compatibility-preserving)
- Re-introduce the original signature as a separate API (cannot overload by return type):
 - Keep `Task StartSdCardLoggingAsync(...)` **(old behavior)**
 - Add `Task<SdCardLoggingSession> StartSdCardLoggingSessionAsync(...)` (or similar) **(new behavior)**
- Implement the old method in `DaqifiStreamingDevice` by `await`ing the new method and discarding the session.
- Update in-repo callers (MCP) to use the new `...SessionAsync` method.

If you intentionally accept the breaking change, document it explicitly and ensure versioning/release notes reflect the interface break.

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

---

2. Invalid max sampling rate ✓ Resolved 🐞 Bug ≡ Correctness

Description

StreamingFrequency validation uses Metadata.Capabilities.MaxSamplingRate verbatim; if
MaxSamplingRate is 0/negative, the setter throws with an impossible range (e.g., 1..0) and prevents
configuring any valid streaming frequency. This is possible because MaxSamplingRate is publicly
settable and not validated anywhere in DeviceCapabilities.

Code

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[R106-113]

+                var maxSamplingRate = Metadata.Capabilities.MaxSamplingRate;
+                if (value < 1 || value > maxSamplingRate)
+                {
+                    throw new ArgumentOutOfRangeException(
+                        nameof(StreamingFrequency),
+                        value,
+                        $"Streaming frequency must be between 1 and {maxSamplingRate} Hz (the device's maximum sampling rate).");
+                }

Relevance

⭐⭐⭐ High

Team often sanitizes untrusted device values; similar “sanitize/guard invalid device-reported
fields” accepted (PR #328).

PR-#328
PR-#322

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new setter compares the requested value against MaxSamplingRate directly. Since MaxSamplingRate
is a mutable, unvalidated public property, it can be set to 0/negative, making the validation logic
reject all valid frequencies and emit an invalid range in the exception message; MCP already
contains a defensive clamp for the same field.

src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[101-117]
src/Daqifi.Core/Device/DeviceCapabilities.cs[53-72]
src/Daqifi.Mcp/DaqifiAgent.cs[346-369]

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.StreamingFrequency` validates against `Metadata.Capabilities.MaxSamplingRate` without ensuring that the advertised max is itself valid (>= 1). If `MaxSamplingRate` is ever set to 0 or a negative number, any attempt to set `StreamingFrequency` fails and the exception message reports a contradictory range.

### Issue Context
`DeviceCapabilities.MaxSamplingRate` is a public settable property with no bounds checks, so tests, simulators, or consumers can set it to invalid values.

### Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiStreamingDevice.cs[106-116]
- src/Daqifi.Core/Device/DeviceCapabilities.cs[54-72]

### Suggested fix
Pick one consistent approach:
1) **Fail fast on invalid capabilities**: if `MaxSamplingRate < 1`, throw `InvalidOperationException` (capabilities are invalid/uninitialized) with a clear message.
2) **Sanitize the ceiling**: compute `var maxSamplingRate = Math.Max(1, Metadata.Capabilities.MaxSamplingRate);` and validate against that, ensuring the exception message uses the sanitized value.

Optionally, enforce `MaxSamplingRate >= 1` in `DeviceCapabilities` (setter validation) to keep the invariant global.

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

---

[tylerkron]

agentic_review

[tylerkron]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7e608f4