feat(mcp): an agent can read a measurement, not just configure one - #524
Conversation
The MCP server had fifteen tools and no way to answer "what is the voltage on AI0?" — an agent could discover, connect, configure channels, drive DIO/PWM and start an SD recording, but the data only ever came back afterwards as a file. Two tools close that: read_channel_values takes a spot reading of every enabled channel, and capture_samples returns a bounded block of live data as timestamp-aligned rows. Both attach to Core's existing live-sample stream, start the device's stream only if nothing is streaming yet (and stop it again afterwards), and are refused while the device is recording to its SD card — a card recording routes the data away from this machine, so a capture would wait out its window and return nothing. Core gains ILiveSampleSource, a capability interface over the live stream it already had, so a consumer holding a device can read data without naming DaqifiStreamingDevice itself. Deliberately additive rather than new members on IStreamingDevice, which is public and implementable outside the library; ISdCardOperations is the same shape. closes #498
|
/agentic_review |
PR Summary by Qodofeat(mcp): add live measurement tools (read values + capture samples)
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…out of time The final flush closes the row that was still being filled when the window ended, and that row alone can bring the count up to the budget — so a capture that ended on time could report rowLimitReached and send a caller back for a continuation that does not exist. The sink now latches whether the budget filled while the capture was still running, which is the question being asked.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit f233681 |
|
Qodo-clean, CI green — ready for review. 2 rounds on head Production code changed during the round, so the bench was re-run on the real Nyquist afterwards: still 24/24, with the 10 s capture reporting Not merging — this is for your review. |
1 participant
What was wrong
The MCP server shipped fifteen tools and not one of them could answer "what is the voltage on AI0?". An agent could find a device, connect to it, enable channels, set the sample rate, drive DIO and PWM, and start an SD recording — and then had to wait for the recording to finish and download a file to see a single number. Reading data is the whole point of a DAQ, and it was the one thing the agent could not do.
How it was fixed
Two tools, both reading the live-sample stream Core already had:
read_channel_values— the latest value on every enabled channel, with the timestamp it was sampled at. It returns as soon as every channel has reported (about 100 ms on the bench), not when its timeout expires, and a channel that said nothing comes backnullrather than0.capture_samples— a bounded block of data as rows: one row per sample tick, one column per channel (AI0,DIO3). It ends on whichever budget runs out first, the duration or the row count, and reports what it actually got: the rate achieved, the rate the device's own clock claims, and how many samples were dropped.Both start the device's stream only if nothing is streaming yet and stop it again afterwards — a session the caller already had running is read and left alone — and both are refused while the device is recording to its SD card, because a card recording routes the data away from this machine and a capture would just wait out its window and return nothing.
Three things a reviewer may want to push back on:
ILiveSampleSourceinstead of putting the members onIStreamingDevice. The issue asked for the promotion ontoIStreamingDevice; that interface is public and implementable outside the library, so adding abstract members to it breaks every external implementer. A capability interface gets the same benefit — a typed consumer reads live data without namingDaqifiStreamingDevice— and is additive.ISdCardOperationsis exactly this shape already.configure_*landing mid-capture would change the channel set the columns are aligned to, and the rows would silently stop meaning what they say.measuredRateHzis this machine's clock,deviceClockRateHzis the device's own timestamps. Either alone can only say "slower than requested"; the two disagreeing is what identifies a device clock that is not keeping real time — which is exactly what this bench unit does (789-796 Hz measured against a device clock insisting on 1000 Hz, firmware #716).Verification
Tests: +37 (Core 3135 → 3137, MCP 86 → 123). Full suite green on net9.0 + net10.0, 0 warnings. The MCP tests cover the grouping rules that shape the data — including a channel reporting twice under one timestamp starting a new row rather than overwriting, which is what firmware 3.7.2 does at high rates — plus the drain's stop conditions and the
--read-onlyrule.Bench (real Nyquist, firmware 3.7.2, USB, non-destructive — streaming and SCPI only): 24/24 checks. The issue's criterion, 4 channels at 1 kHz for 10 s, returned 7863 rows / 31,452 samples with 0 dropped and 0 rows missing from the sequence the device's own clock accounts for. A mixed analog+digital capture came back with columns
AI0..AI3, DIO0, DIO1in that order, and both tools left the device not streaming.Two things the bench changed in the code: the minimum budgets are now 500 ms (read) and 250 ms (capture), because a device that is not streaming yet sends nothing for the first 85-110 ms and a 100 ms budget reported a healthy device as silent; and
measuredRateHzis timed from the first sample rather than from the call, so that start-up wait is not charged to the device as a lower rate.closes #498
Not merging — this is for your review.