From 92a40b7009d15afcc18620db52df8756e7c99692 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 30 Jul 2026 13:35:44 -0600 Subject: [PATCH 1/4] fix(mcp): reject over-max sample rates instead of clamping (closes #410) Core's StreamingFrequency setter already throws on an out-of-range rate; Daqifi.Mcp.SetSampleRateAsync silently clamped instead, so the same input produced opposite outcomes depending on which layer a caller went through. Standardize on throw so a client can tell its request was refused rather than silently honored at a lower rate. Co-Authored-By: Claude Sonnet 5 --- src/Daqifi.Mcp/DaqifiAgent.cs | 15 ++++++++------- src/Daqifi.Mcp/Dtos.cs | 8 ++++---- src/Daqifi.Mcp/README.md | 2 +- src/Daqifi.Mcp/ServerOptions.cs | 5 +++-- src/Daqifi.Mcp/Tools/DaqifiTools.cs | 2 +- 5 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/Daqifi.Mcp/DaqifiAgent.cs b/src/Daqifi.Mcp/DaqifiAgent.cs index e96b2a89..def90c11 100644 --- a/src/Daqifi.Mcp/DaqifiAgent.cs +++ b/src/Daqifi.Mcp/DaqifiAgent.cs @@ -345,19 +345,20 @@ public async Task SetSampleRateAsync(string deviceId, int rate // The device's advertised hardware ceiling always applies (Core validates // StreamingFrequency against it too); --max-sample-rate-hz can only lower it. - // Guard against a non-positive cap so the applied rate is always a valid >= 1 value. + // Guard against a non-positive cap so the effective cap is always a valid >= 1 value. var hardwareMax = Math.Max(1, device.Metadata.Capabilities.MaxSamplingRate); var cap = _options.MaxSampleRateHz is { } max ? Math.Clamp(max, 1, hardwareMax) : hardwareMax; - var applied = Math.Min(rateHz, cap); - streaming.StreamingFrequency = applied; + if (rateHz > cap) + { + throw new InvalidOperationException( + $"Requested {rateHz} Hz exceeds the maximum {cap} Hz for this device."); + } - var note = applied != rateHz - ? $"Requested {rateHz} Hz clamped to {applied} Hz (maximum {cap} Hz)." - : null; - return new SampleRateResult(deviceId, rateHz, applied, applied != rateHz, note); + streaming.StreamingFrequency = rateHz; + return new SampleRateResult(deviceId, rateHz); } finally { diff --git a/src/Daqifi.Mcp/Dtos.cs b/src/Daqifi.Mcp/Dtos.cs index 9c05c001..1dad19f2 100644 --- a/src/Daqifi.Mcp/Dtos.cs +++ b/src/Daqifi.Mcp/Dtos.cs @@ -140,11 +140,11 @@ public static PwmResult From(string deviceId, IStreamingDevice device, IChannel } /// -/// Result of a sample-rate change. is true when -/// exceeded the effective ceiling (1000 Hz hardware limit, or a lower --max-sample-rate-hz), -/// in which case explains the adjustment. +/// Result of a sample-rate change. A request exceeding the effective ceiling (the device's +/// hardware limit, or a lower --max-sample-rate-hz) is rejected outright rather than +/// clamped — see . /// -public sealed record SampleRateResult(string DeviceId, int RequestedRateHz, int AppliedRateHz, bool Clamped, string? Note); +public sealed record SampleRateResult(string DeviceId, int RequestedRateHz); /// Result of starting SD-card logging. public sealed record StartLoggingResult( diff --git a/src/Daqifi.Mcp/README.md b/src/Daqifi.Mcp/README.md index c97f39cf..43703272 100644 --- a/src/Daqifi.Mcp/README.md +++ b/src/Daqifi.Mcp/README.md @@ -48,7 +48,7 @@ dotnet run --project src/Daqifi.Mcp ``` --read-only Expose discovery/introspection only; block configuration and logging. ---max-sample-rate-hz Clamp set_sample_rate to at most Hz. +--max-sample-rate-hz Reject set_sample_rate requests above Hz. -h, --help Show help. ``` diff --git a/src/Daqifi.Mcp/ServerOptions.cs b/src/Daqifi.Mcp/ServerOptions.cs index 3609b2cb..1423eacf 100644 --- a/src/Daqifi.Mcp/ServerOptions.cs +++ b/src/Daqifi.Mcp/ServerOptions.cs @@ -12,7 +12,8 @@ public sealed class ServerOptions public bool ReadOnly { get; init; } /// - /// Optional upper bound applied to set_sample_rate. Null means no clamp. + /// Optional upper bound enforced by set_sample_rate; requests above it are rejected. + /// Null means only the device's hardware ceiling applies. /// public int? MaxSampleRateHz { get; init; } @@ -29,7 +30,7 @@ public static ServerOptions Parse(string[] args) readOnly = true; break; case "--max-sample-rate-hz" when i + 1 < args.Length: - // Ignore non-positive values; a clamp of <= 0 would otherwise force an invalid rate. + // Ignore non-positive values; a cap of <= 0 would otherwise reject every rate. if (int.TryParse(args[++i], out var rate) && rate >= 1) { maxRate = rate; diff --git a/src/Daqifi.Mcp/Tools/DaqifiTools.cs b/src/Daqifi.Mcp/Tools/DaqifiTools.cs index c0874683..6d8f1c45 100644 --- a/src/Daqifi.Mcp/Tools/DaqifiTools.cs +++ b/src/Daqifi.Mcp/Tools/DaqifiTools.cs @@ -111,7 +111,7 @@ public static Task DisablePwm( => GuardAsync(() => agent.DisablePwmAsync(deviceId, channel)); [McpServerTool(Name = "set_sample_rate")] - [Description("Set the device sample (streaming) rate in Hz, applied to streaming and SD-card logging. Nyquist hardware supports 1–1000 Hz; requests above 1000 Hz (or above --max-sample-rate-hz) are clamped and reported in the result.")] + [Description("Set the device sample (streaming) rate in Hz, applied to streaming and SD-card logging. Nyquist hardware supports 1–1000 Hz; requests above 1000 Hz (or above --max-sample-rate-hz) are rejected — the call throws rather than silently applying a lower rate.")] public static Task SetSampleRate( DaqifiAgent agent, [Description("The device_id to configure.")] string deviceId, From c057326cfe8d5eba927db0d57836023a32755159 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 30 Jul 2026 13:42:46 -0600 Subject: [PATCH 2/4] fix(mcp): update stale --help text for --max-sample-rate-hz The README and tool description already said "reject"; ServerOptions.HelpText (what --help actually prints) still said "clamp". (Qodo review on #412) Co-Authored-By: Claude Sonnet 5 --- src/Daqifi.Mcp/ServerOptions.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Daqifi.Mcp/ServerOptions.cs b/src/Daqifi.Mcp/ServerOptions.cs index 1423eacf..2ff8aa01 100644 --- a/src/Daqifi.Mcp/ServerOptions.cs +++ b/src/Daqifi.Mcp/ServerOptions.cs @@ -54,7 +54,7 @@ public static ServerOptions Parse(string[] args) Options: --read-only Expose discovery/introspection only; block configuration and logging. - --max-sample-rate-hz Clamp set_sample_rate requests to at most Hz. + --max-sample-rate-hz Reject set_sample_rate requests above Hz. -h, --help Show this help and exit. """; } From fe0d1314b33a4c312e8bfed1df722e8eb1b4d784 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 30 Jul 2026 13:53:49 -0600 Subject: [PATCH 3/4] fix(mcp): validate set_sample_rate against the live per-channel cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The over-cap check landed in the previous commit validated against Capabilities.MaxSamplingRate, which is the sampling ISR's absolute hardware envelope, not what the device will actually accept for the channels enabled right now (CapabilityStreaming.CurrentMaximumRateHz). Confirmed on a real Nq1 (fw 3.7.2): ISR ceiling reports 22000 Hz while the real cap for 2 enabled channels was 6924 Hz and for 16 channels was 3518 Hz — a request in that gap (e.g. 5000-10000 Hz) sailed through the earlier check silently and would only have failed later, at stream start, via the firmware's SCPI -222. SetSampleRateAsync now prefers CurrentMaximumRateHz when the capability document has one, falling back to the board-derived ceiling otherwise. ConfigureAnalogChannelsAsync/ConfigureDigitalChannelsAsync re-read the capability document (best-effort) after changing the enabled set, so the cap set_sample_rate sees reflects the live configuration rather than whatever was true at connect time. Also corrected the set_sample_rate tool description and README line, which still claimed a flat 1-1000 Hz range. Co-Authored-By: Claude Sonnet 5 --- src/Daqifi.Mcp/DaqifiAgent.cs | 45 ++++++++++++++++++++++++----- src/Daqifi.Mcp/Dtos.cs | 6 ++-- src/Daqifi.Mcp/README.md | 2 +- src/Daqifi.Mcp/Tools/DaqifiTools.cs | 4 +-- 4 files changed, 44 insertions(+), 13 deletions(-) diff --git a/src/Daqifi.Mcp/DaqifiAgent.cs b/src/Daqifi.Mcp/DaqifiAgent.cs index def90c11..cf32ff4c 100644 --- a/src/Daqifi.Mcp/DaqifiAgent.cs +++ b/src/Daqifi.Mcp/DaqifiAgent.cs @@ -165,6 +165,11 @@ public async Task ConfigureAnalogChannelsAsync(string deviceId, streaming.EnableChannels(toEnable); } + // The device's authoritative rate cap (CapabilityStreaming.CurrentMaximumRateHz) is + // scoped to the channel set enabled when the document was read — refresh it now so + // set_sample_rate validates against the configuration that is actually live. + await RefreshCapabilityDocumentAsync(device).ConfigureAwait(false); + return new ConfigureResult(deviceId, EnabledAnalog(device), streaming.StreamingFrequency); } finally @@ -208,6 +213,10 @@ public async Task ConfigureDigitalChannelsAsync(string d streaming.EnableChannels(toEnable); } + // See ConfigureAnalogChannelsAsync: keeps CurrentMaximumRateHz current for + // set_sample_rate even though the rate model itself ignores digital channels. + await RefreshCapabilityDocumentAsync(device).ConfigureAwait(false); + return new ConfigureDigitalResult(deviceId, EnabledDigital(device)); } finally @@ -343,18 +352,21 @@ public async Task SetSampleRateAsync(string deviceId, int rate RequireControl(); var (device, streaming) = RequireStreaming(deviceId); - // The device's advertised hardware ceiling always applies (Core validates - // StreamingFrequency against it too); --max-sample-rate-hz can only lower it. - // Guard against a non-positive cap so the effective cap is always a valid >= 1 value. + // MaxSamplingRate is the absolute sampling-ISR ceiling, not what the device will + // actually accept for the channels enabled right now — that is + // CapabilityStreaming.CurrentMaximumRateHz, refreshed after every channel- + // configuration call (see ConfigureAnalogChannelsAsync/ConfigureDigitalChannelsAsync). + // Guard against a non-positive board-table value so the fallback is always >= 1; a + // reported CurrentMaximumRateHz of 0 is a real answer ("no channels enabled") and is + // deliberately not floored the same way. var hardwareMax = Math.Max(1, device.Metadata.Capabilities.MaxSamplingRate); - var cap = _options.MaxSampleRateHz is { } max - ? Math.Clamp(max, 1, hardwareMax) - : hardwareMax; + var deviceCap = device.Metadata.CapabilityDocument?.Streaming?.CurrentMaximumRateHz ?? hardwareMax; + var cap = _options.MaxSampleRateHz is { } max ? Math.Min(max, deviceCap) : deviceCap; if (rateHz > cap) { throw new InvalidOperationException( - $"Requested {rateHz} Hz exceeds the maximum {cap} Hz for this device."); + $"Requested {rateHz} Hz exceeds the maximum {cap} Hz for the currently enabled channels."); } streaming.StreamingFrequency = rateHz; @@ -473,6 +485,25 @@ private DaqifiDevice Require(string deviceId) return (device, streaming); } + // Best-effort: a device that doesn't support the capability document (or a query that + // fails/times out) just leaves CurrentMaximumRateHz stale or absent, and SetSampleRateAsync + // falls back to the board-derived ceiling. Not fatal to the channel-configuration call that + // triggered the refresh. + private static async Task RefreshCapabilityDocumentAsync(DaqifiDevice device) + { + try + { + await device.ReadCapabilityDocumentAsync().ConfigureAwait(false); + } + catch (OperationCanceledException) + { + throw; + } + catch (Exception) + { + } + } + private static ISdCardOperations RequireSdCard(DaqifiDevice device) { if (device is not ISdCardOperations sd) diff --git a/src/Daqifi.Mcp/Dtos.cs b/src/Daqifi.Mcp/Dtos.cs index 1dad19f2..6c7711a6 100644 --- a/src/Daqifi.Mcp/Dtos.cs +++ b/src/Daqifi.Mcp/Dtos.cs @@ -140,9 +140,9 @@ public static PwmResult From(string deviceId, IStreamingDevice device, IChannel } /// -/// Result of a sample-rate change. A request exceeding the effective ceiling (the device's -/// hardware limit, or a lower --max-sample-rate-hz) is rejected outright rather than -/// clamped — see . +/// Result of a sample-rate change. A request exceeding the effective ceiling (the device's cap +/// for its currently enabled channels, or a lower --max-sample-rate-hz) is rejected +/// outright rather than clamped — see . /// public sealed record SampleRateResult(string DeviceId, int RequestedRateHz); diff --git a/src/Daqifi.Mcp/README.md b/src/Daqifi.Mcp/README.md index 43703272..007d9f6d 100644 --- a/src/Daqifi.Mcp/README.md +++ b/src/Daqifi.Mcp/README.md @@ -23,7 +23,7 @@ The server speaks MCP over **stdio**, so the client launches it as a subprocess. | `set_digital_output` | Drive a digital channel high or low (switches it to output if needed). | | `set_pwm_output` | Start PWM on a capable channel: duty 1-100%, shared frequency 6-50000 Hz. | | `disable_pwm` | Stop PWM on a channel (pin is left high-impedance). | -| `set_sample_rate` | Set sample rate in Hz (Nyquist hardware supports up to 1000 Hz). | +| `set_sample_rate` | Set sample rate in Hz (ceiling depends on the enabled channel count; over-cap requests are rejected). | | `start_sd_logging` | Start on-device SD logging (**requires a USB/serial connection**). | | `stop_sd_logging` | Stop SD logging. | diff --git a/src/Daqifi.Mcp/Tools/DaqifiTools.cs b/src/Daqifi.Mcp/Tools/DaqifiTools.cs index 6d8f1c45..7f3ff2ac 100644 --- a/src/Daqifi.Mcp/Tools/DaqifiTools.cs +++ b/src/Daqifi.Mcp/Tools/DaqifiTools.cs @@ -111,11 +111,11 @@ public static Task DisablePwm( => GuardAsync(() => agent.DisablePwmAsync(deviceId, channel)); [McpServerTool(Name = "set_sample_rate")] - [Description("Set the device sample (streaming) rate in Hz, applied to streaming and SD-card logging. Nyquist hardware supports 1–1000 Hz; requests above 1000 Hz (or above --max-sample-rate-hz) are rejected — the call throws rather than silently applying a lower rate.")] + [Description("Set the device sample (streaming) rate in Hz, applied to streaming and SD-card logging. The achievable maximum depends on how many channels are currently enabled (configure channels first for an accurate ceiling) and is further capped by --max-sample-rate-hz if set; a request above the effective cap is rejected — the call throws rather than silently applying a lower rate.")] public static Task SetSampleRate( DaqifiAgent agent, [Description("The device_id to configure.")] string deviceId, - [Description("Sample rate in Hz (1–1000).")] int rateHz) + [Description("Sample rate in Hz. The ceiling varies with the enabled channel count; get_device_status or a prior configure_analog_channels call error message reports the current limit.")] int rateHz) => GuardAsync(() => agent.SetSampleRateAsync(deviceId, rateHz)); [McpServerTool(Name = "start_sd_logging")] From 9dbf62269085a916d9131c3709d5d02c9d9ac3f1 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Thu, 30 Jul 2026 14:48:03 -0600 Subject: [PATCH 4/4] fix(mcp): skip capability refresh while streaming, bound cap, log failures Three issues from Qodo review of fe0d131: 1. RefreshCapabilityDocumentAsync ran unconditionally after channel config, but ReadCapabilityDocumentAsync's own docs say not to call it while streaming (it pauses the protobuf consumer) -- SD logging sets IsStreaming too. Now skipped outright when streaming.IsStreaming; the cap just stays at its last-known value until the next quiescent refresh. Verified on real hardware: start_sd_logging -> configure_analog_channels mid-log no longer touches the capability document, and logging survives the reconfigure cleanly. 2. SetSampleRateAsync's deviceCap now bounds CurrentMaximumRateHz to hardwareMax, so a self-inconsistent capability document can't produce a cap above the ceiling StreamingFrequency itself enforces (which would otherwise let the check pass and then throw a different exception type one line down). 3. RefreshCapabilityDocumentAsync's catch-all was silent. DaqifiAgent now takes an optional ILogger (resolved via DI in Program.cs; defaults to NullLogger for the existing test constructor calls) and logs a warning when a refresh fails for a reason other than cancellation. Co-Authored-By: Claude Sonnet 5 --- src/Daqifi.Mcp/DaqifiAgent.cs | 36 ++++++++++++++++++++++++++++------- 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/src/Daqifi.Mcp/DaqifiAgent.cs b/src/Daqifi.Mcp/DaqifiAgent.cs index cf32ff4c..79a1bad6 100644 --- a/src/Daqifi.Mcp/DaqifiAgent.cs +++ b/src/Daqifi.Mcp/DaqifiAgent.cs @@ -3,6 +3,7 @@ using Daqifi.Core.Device; using Daqifi.Core.Device.Discovery; using Daqifi.Core.Device.SdCard; +using Microsoft.Extensions.Logging; namespace Daqifi.Mcp; @@ -24,11 +25,16 @@ namespace Daqifi.Mcp; public sealed class DaqifiAgent { private readonly ServerOptions _options; + private readonly ILogger _logger; private readonly ConcurrentDictionary _discovered = new(StringComparer.Ordinal); private readonly DaqifiDeviceRegistry _registry = new(); private readonly SemaphoreSlim _gate = new(1, 1); - public DaqifiAgent(ServerOptions options) => _options = options; + public DaqifiAgent(ServerOptions options, ILogger? logger = null) + { + _options = options; + _logger = logger ?? Microsoft.Extensions.Logging.Abstractions.NullLogger.Instance; + } // ---------------------------------------------------------------- discovery @@ -168,7 +174,7 @@ public async Task ConfigureAnalogChannelsAsync(string deviceId, // The device's authoritative rate cap (CapabilityStreaming.CurrentMaximumRateHz) is // scoped to the channel set enabled when the document was read — refresh it now so // set_sample_rate validates against the configuration that is actually live. - await RefreshCapabilityDocumentAsync(device).ConfigureAwait(false); + await RefreshCapabilityDocumentAsync(device, streaming).ConfigureAwait(false); return new ConfigureResult(deviceId, EnabledAnalog(device), streaming.StreamingFrequency); } @@ -215,7 +221,7 @@ public async Task ConfigureDigitalChannelsAsync(string d // See ConfigureAnalogChannelsAsync: keeps CurrentMaximumRateHz current for // set_sample_rate even though the rate model itself ignores digital channels. - await RefreshCapabilityDocumentAsync(device).ConfigureAwait(false); + await RefreshCapabilityDocumentAsync(device, streaming).ConfigureAwait(false); return new ConfigureDigitalResult(deviceId, EnabledDigital(device)); } @@ -360,7 +366,14 @@ public async Task SetSampleRateAsync(string deviceId, int rate // reported CurrentMaximumRateHz of 0 is a real answer ("no channels enabled") and is // deliberately not floored the same way. var hardwareMax = Math.Max(1, device.Metadata.Capabilities.MaxSamplingRate); - var deviceCap = device.Metadata.CapabilityDocument?.Streaming?.CurrentMaximumRateHz ?? hardwareMax; + + // Bound to hardwareMax defensively: CurrentMaximumRateHz and MaxSamplingRate come from + // two independently-parsed fields, so a self-inconsistent document (or a channel-set + // read that raced a board-table update) could otherwise report a "current" cap above + // the absolute ceiling StreamingFrequency itself enforces — which would let this check + // pass and then fail one line down with the wrong exception type. + var currentMax = device.Metadata.CapabilityDocument?.Streaming?.CurrentMaximumRateHz; + var deviceCap = currentMax.HasValue ? Math.Min(currentMax.Value, hardwareMax) : hardwareMax; var cap = _options.MaxSampleRateHz is { } max ? Math.Min(max, deviceCap) : deviceCap; if (rateHz > cap) @@ -488,9 +501,17 @@ private DaqifiDevice Require(string deviceId) // Best-effort: a device that doesn't support the capability document (or a query that // fails/times out) just leaves CurrentMaximumRateHz stale or absent, and SetSampleRateAsync // falls back to the board-derived ceiling. Not fatal to the channel-configuration call that - // triggered the refresh. - private static async Task RefreshCapabilityDocumentAsync(DaqifiDevice device) + // triggered the refresh. Skipped outright while streaming/logging: ReadCapabilityDocumentAsync + // runs a text-mode exchange that pauses the protobuf consumer, which Core documents as unsafe + // to call while streaming (SD logging sets IsStreaming too) — leaving the cap stale here is + // preferable to disrupting an active session. + private async Task RefreshCapabilityDocumentAsync(DaqifiDevice device, IStreamingDevice streaming) { + if (streaming.IsStreaming) + { + return; + } + try { await device.ReadCapabilityDocumentAsync().ConfigureAwait(false); @@ -499,8 +520,9 @@ private static async Task RefreshCapabilityDocumentAsync(DaqifiDevice device) { throw; } - catch (Exception) + catch (Exception ex) { + _logger.LogWarning(ex, "Capability-document refresh failed for '{DeviceId}'; set_sample_rate will keep using the last-known cap.", device.Metadata.SerialNumber); } }