perf(device): give the init SCPI exchange a reply to finish on instead of waiting out its timeout - #700
Conversation
…d of waiting out its timeout The four commands the init sequence sends produce no reply unless something goes wrong, so the text exchange never had anything to complete on and sat out its whole 1000ms response timeout on every single connect. Appending SYSTem:ERRor? gives it a reply to finish on: the wait loop flips into its 250ms completion window as soon as that lands. This is the same terminator trick SdCardOperations already uses to bound the SD directory listing (#396). Measured on a bench Nq1 (fw 3.7.2, USB/serial), 5 interleaved A/B pairs of connect + init + 1s stream: origin/main mean 5.620s median 5.426s this branch mean 5.082s median 4.900s delta 0.538s mean, 0.526s median; all 5 pairs favour the branch The settle delay before the query is not optional -- the firmware has to have acted on SetProtobufStreamFormat and queued any complaint before we ask, or a failed command would answer "0, No error". The reply is also read rather than discarded. The error-queue form carries a bare code (-200,"Execution error") with no ERROR token, so IsScpiErrorLine does not and should not match it; the init retry check now falls back to parsing that code. A setup command that failed silently -- recorded in the queue rather than volunteered -- was previously never noticed at all. An observe session (PreserveActiveStream) deliberately forgoes the terminator and keeps the old timing: reading the error queue pops the entry it returns, and taking one that belongs to the session actually driving the device is not worth 0.5s on the rarer path (#385). Refs #667 -- item 1 landed already in #694, item 3 is a stated won't-do pending the #485 single-reader rework, and item 4 remains open. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoComplete initialization SCPI exchange with an error-queue reply
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…aking the 250ms default Qodo review, PR #700. The two existing users of the SYSTem:ERRor? terminator -- the SD directory listing and the confirming-administration exchange -- both raise completionTimeoutMs to 1000ms, with a test pinning it, because on a device that echoes it is an echo line that starts the inactivity clock and a verdict trailing behind it would be missed. Taking the 250ms default here was an unstated divergence from that convention. Now stated explicitly at 500ms, with the reasoning recorded. It does not follow the siblings to 1000ms because the consequence differs: losing the verdict there means an incomplete listing or failing a command the device accepted, while here it means only that the queue is not consulted -- which is where initialization stood before the terminator existed, and the volunteered **ERROR: form is still read either way. 1000ms would also erase the saving entirely, since the exchange ends one inactivity window after the last line. Bench Nq1, 13 interleaved A/B pairs at this setting: origin/main mean 5.657s this branch mean 5.091s saving mean 0.566s, median 0.544s, 12/13 pairs favour the branch The extra headroom over 250ms costs nothing measurable -- device variance dominates it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Re bug 2, "Settle delay precedes write" (marked Weak) — declining this one, for the reason the finding itself cites.
The commands also share one ordered producer queue, so And the cited precedent argues the other way: #591 explicitly rejected requiring an outbound-queue drain before an exchange boundary. Bug 1 is accepted and fixed in 4d412c6 — see the inline thread. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 4d412c6 |
1 participant
What was wrong
Every connect to a device spent most of a second doing nothing.
The initialization sequence sends four SCPI setup commands, none of which the device answers unless
something goes wrong. So the text exchange had nothing to complete on and simply sat out its entire
1000 ms response timeout, every time, on a device that had finished its work almost immediately.
How it was fixed
Ask the device a question at the end, so there is an answer to finish on. Appending
SYSTem:ERRor?lets the wait loop leave its long first-response window as soon as the reply lands,instead of waiting out silence. This is not a new idea here —
SdCardOperationsalready uses exactlythis terminator to bound the SD directory listing (#396).
Three things a reviewer should push on:
The settle delay before the query is deliberate, and it costs 100 ms of the saving. It is what
makes the reply mean anything: the firmware has to have acted on
SetProtobufStreamFormatandqueued any complaint about it before we ask, or a command that failed would answer
0,"No error".The reply is read, not discarded. The error-queue form carries a bare code
(
-200,"Execution error") with noERRORtoken, soIsScpiErrorLinedoes not — and should not —match it. The init retry check now also parses that code, which means a setup command that failed
silently, recording an error rather than volunteering one, is caught for the first time. Before
this, only the volunteered form was ever seen.
The completion window is 500 ms, and that is a deliberate divergence. The two other users of this
terminator both raise their window to 1000 ms, because a verdict that trails an echo by more than the
window would be lost. This path does not follow them, because the consequence differs: losing the
verdict there means an incomplete listing or failing a command the device accepted, whereas here it
means only that the error queue is not consulted — exactly where initialization stood before the
terminator existed, with the volunteered
**ERROR:form still read either way. 1000 ms would alsoerase the saving outright, since the exchange ends one full inactivity window after the last line.
The value is stated explicitly and pinned by a test rather than left to the 250 ms default.
An observe session (
PreserveActiveStream) deliberately skips the terminator and keeps the oldtiming. Reading the error queue pops the entry it returns, and taking one that belongs to the
session actually driving the device isn't worth half a second on the rarer path (#385).
Verified
Bench Nq1, fw 3.7.2, over USB/serial. Thirteen interleaved A/B pairs of connect + init + 1 s
stream, alternating between a CLI built from
origin/mainand one built from this branch —interleaved because #667 records a previous timing claim that turned out to be a wash when measured
properly, and because this device's own run-to-run spread is comparable to the effect size:
origin/main12 of 13 pairs favour the branch; the one that does not is −0.187 s, within the device's noise.
Streaming and SD listing re-verified healthy on the branch afterwards.
Full suite green on net9.0 and net10.0 — 4,122 in
Daqifi.Core.Tests, 217 inDaqifi.Mcp.Tests,0 failures, Release build warning-clean. Seven new tests; the two covering error-queue detection were
confirmed to fail without the fix.
Scope
Refs #667 rather than closing it. That ticket has four items and this is item 2:
Thread.Sleep(100)) already landed in perf(device): stop the init SCPI sequence blocking a thread-pool thread for 300ms #694 — the code isawait Task.Delaytoday and cites perf(device): the rest of the SCPI exchange overhead — init Thread.Sleep, the no-reply 1000ms wait, and the consumer swap #667 in its comment.
re-decides regression assertions encoding fix(consumers): connect can fail spuriously with "a previous consumer thread has not yet exited" #383, investigate: per-device operation serialization for concurrent consumers #342 and api: a timed-out SD LIST is indistinguishable from an empty SD card #396.
for it: with a sentinel to stop on, the exchange would not need to guess an inactivity window at
all, and the 500 ms compromise above would become unnecessary.
🤖 Generated with Claude Code