chore(api): move the per-configuration sample-rate cap into Core - #513
chore(api): move the per-configuration sample-rate cap into Core#513tylerkron wants to merge 3 commits into
Conversation
#481) The rule for how fast a device can actually stream with the channels it has enabled lived only in the MCP server, so every other consumer of Daqifi.Core — the desktop application first among them — could command a rate the configuration cannot deliver and get a session that silently produces nothing. Core now owns it: SampleRateCap decides the ceiling, IStreamingDevice exposes MaximumStreamingFrequencyHz and EnforceStreamingFrequencyCap(), and the MCP server keeps only its own --max-sample-rate-hz clamp. The device's reported cap stays authoritative; the published rate model — which had no production caller — is the fallback when the device states none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoMove per-configuration sample-rate cap logic into Core
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… enabled analog inputs Qodo round 1: with the device stating no cap, the rate-model fallback answered a healthy 18,333 Hz for an empty configuration, because the model's formula keeps a finite per-tick overhead term at zero channels. Every "cap is 0, so nothing is enabled" check downstream would read that as a live configuration. The device's own answer for that case is 0 — measured on the bench NQ1 with nothing enabled, and again with digital pins only — so the fallback now says 0 too. A device with no capability document at all is unchanged: it has said nothing about how its channel set affects the rate, so it still reports the board ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 66eade3 |
…why digital-only reads zero Qodo round 2. The board-ceiling fallback was documented as "no capability document at all", but it also covers a document that carries neither a current cap nor a rate model — the case an existing test already pins. Reworded on both the interface member and ComputeForDevice. The digital-only semantics were implicit in the code and are now stated: digital pins are captured on the analog sample tick rather than driving one of their own, which is why the model counts only analog inputs and why the device itself answers 0 for a digital-only selection. Documentation only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Qodo-clean, CI green — ready for review. Round 3 on head Three rounds. Round 1 found one real bug — with the device stating no cap, the rate-model fallback answered 18,333 Hz for an empty configuration, because the model's per-tick overhead term stays finite at zero channels. Fixed in Round 2 found a real doc mismatch — the board-ceiling fallback was documented more narrowly than it behaves — fixed in Re-verified after the round-1 production fix: full suite green on net9.0 (3,004 Core + 86 MCP) and net10.0 (3,004), 0 warnings in Debug and Release, and the bench table in the description re-run end to end on Not merging — this is for your review. |
|
Code review by qodo was updated up to the latest commit 0218b60 |
Re-check complete: I see no remaining actionable issues on head |
1 participant
What was wrong
How fast a DAQiFi will actually stream depends on how many channels you have enabled — a Nyquist that manages 7,746 Hz with one analog channel manages 3,518 Hz with sixteen. Core never told anyone that.
StreamingFrequencyonly checked the board's absolute 22,000 Hz ceiling, so a perfectly ordinary "set the rate, then enable more channels" sequence left an impossible rate live, and the library reported it back as if it were fine. The firmware does not clamp: it rejects the start outright and streams nothing, so the first sign of trouble is a recording session that comes back empty.The rule for computing that ceiling did exist — but only inside the MCP server, where it had been added for #447. Every other consumer of the library, the desktop application first among them, silently went without it.
How it was fixed
Core owns the rule now.
SampleRateCapdecides the ceiling, andIStreamingDeviceexposes it asMaximumStreamingFrequencyHztogether withEnforceStreamingFrequencyCap(), which lowers a live rate that no longer fits and tells you what it was before. The MCP server keeps only the part that was genuinely its own — the operator's--max-sample-rate-hzclamp — and gets the device half from Core, so its behaviour is unchanged.What a reviewer may want to push back on:
current_max_rate_hzstays authoritative; the published rate model — which until now had no production caller anywhere in Core — is only the fallback for a document that states no cap. The bench numbers below are the argument: the model sits above the device's answer in every configuration measured (15,714 vs 7,746 for one channel; 5,000 vs 3,518 for sixteen), exactly as its own docs warn, because it accounts for channel count and type but not for the transport. Preferring it, or taking the lower of the two, would either over-permit or contradict the documented contract.StreamingFrequencyreject against the per-configuration cap would break the reasonable ordering of setting a rate before enabling channels, and would be a behaviour break for existing callers. Enforcement is therefore something you ask for.IStreamingDevice. Both have default implementations, so existing implementers keep compiling — the same pattern the interface already uses forStartStreamingAsync.Daqifi.Mcp.SampleRateCapCalculatoris gone, its tests ported down to Core with the logic.Daqifi.Mcpships as a dotnet tool, not a consumable library, so nothing depends on that type. This also makes the MCP README's "all device/protocol logic lives inDaqifi.Core" true again, which it had stopped being.Verification
Tests — 27 new in Core, 4 left in MCP for the server clamp. Proven to catch regressions by mutating the implementation and re-running: inverting the source precedence fails 5, counting disabled channels fails 4, dropping the hardware-maximum floor fails 2, dropping the write-back in the enforcement path fails 2, counting digital channels fails 1, looking a channel up by id without checking its kind fails 1, and treating a negative reported cap as real fails 1. Full suite green on net9.0 (3,002 Core + 86 MCP) and net10.0 (3,002), 0 failures, 0 warnings in Debug and Release.
Bench (non-destructive), Nq1 fw 3.7.2 on
/dev/cu.usbmodem1101. Core's cap was compared against the device's own answer across five channel selections, re-reading the capability document each time:current_max_rate_hzCore agrees with the device exactly in every case, which is what says the MCP server's numbers have not moved. The 7,746 and 3,518 figures are the same ones #447 measured. The right-hand column is the fallback that would have been used had the device stated nothing — consistently optimistic, which is the evidence for the precedence choice above.
The #447 trap itself was then reproduced end to end on hardware: rate set to 7,746 Hz with one channel enabled, all sixteen enabled, cap drops to 3,518 Hz with 7,746 Hz still live,
EnforceStreamingFrequencyCap()lowers it to 3,518 and reports 7,746, and a second call reports no change. Channels were restored to none enabled afterwards. Discovery, capability reads and channel enables only — no streaming, no SD, no reboot, no firmware, no LAN writes.closes #481
Not merging — this is for your review.