fix(device): an opt-in connect that leaves a running stream alone (closes #385) - #414
Conversation
A DAQiFi unit has one global acquisition, and InitializeAsync unconditionally sends SYSTem:StopStreamData (plus POWer:STATe, STReam:FORmat, and the USB STReam:INTerface routing step). A second session connecting to a device that is already streaming therefore ends the first session's acquisition, silently. Adds DeviceConnectionOptions.PreserveActiveStream (and the matching DaqifiDevice property, plus a DeviceConnectionOptions.Observing preset) which skips exactly the initialization commands that write global stream state. The session still sends SYSTem:ECHO -1 and the read-only identity/capability queries, so it reaches Ready with populated channels. Default behavior is unchanged. Also documents the hazard in the README and DEVICE_INTERFACES, and points multi-session consumers at DaqifiDeviceRegistry. Closes #385
|
/agentic_review |
PR Summary by QodoAdd opt-in observing connection to preserve an active device stream
AI Description
Diagram
High-Level Assessment
Files changed (8)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…storing it
Review caught a real race. The PreserveActiveStream decision was held in an
instance field between InitializeAsync capturing it and the derived hook
reading it, so a second InitializeAsync starting on the same device while the
first was still in flight overwrote it. The first initialization's USB routing
step then acted on the second one's decision — an observing session could route
the stream to itself, which is exactly the stream-stealing this change exists to
prevent.
Removes the field and passes the decision to OnDeviceInitializingAsync as a
parameter, so it lives on the stack and no concurrent operation can reach it.
Signature change to the protected hook; DaqifiStreamingDevice is the only
overrider.
Adds a regression test that overlaps two initializations on one device and
asserts each hook receives its own decision (fails as {false, false} against the
previous implementation), plus two tests pinning that the decision is read once
at the start and not re-read later.
No change to the SCPI sent on the wire for either path.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 04c2514 |
Re: finding 2, "Virtual hook API break" — declining, deliberatelyThe break is real and intended. I'm not taking the suggested fix, because it would reintroduce the exact defect this repo already decided against. The suggested fix trades a loud failure for a silent one. Keeping the old hook alive and having the new overload delegate to it means a downstream subclass overriding the old signature keeps getting invoked — but never receives This is settled precedent here, and #406 is the direct case. That PR hit the identical fork and its body records the outcome:
#406 then broke ten in-repo overrides on purpose, plus any downstream ones. This change breaks one ( For accuracy: I checked #329 and #388 as well and don't see anything in them supporting the "avoid breaking change was rejected" characterization — #406 is the precedent that actually carries this, and it carries it decisively. On the binary-compatibility claim. "Existing binaries may fail to load" is technically right: a downstream assembly compiled against the old Core throws Scope of the blast radius: Also worth noting Qodo rates this one No code change. Branch is unchanged at 04c2514, CI green (build pass, 1m35s), full suite green on net9.0 and net10.0 (2199 Core + 23 MCP). |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 04c2514 |
Review caught that an observing initialization could swallow a late cancellation. After channels populate, nothing on that path is guaranteed to observe the token: the capability read returns early on firmware that does not advertise the document, channel population can short-circuit when the status arrives synchronously, and the observing hook returns immediately because it has no work to do. A caller that cancelled in that window got a successfully completed task and a device reporting Ready. Enforces the invariant in InitializeAsync, immediately before the transition to Ready, rather than patching the one early return that was flagged. The same hole exists on the pre-existing non-USB early return and in any third-party override with no awaitable work; guarding the transition covers all of them. Cancellation here follows the path already documented for this method: state reverts to Connected, _isInitialized stays false, and the initialization can be retried. Adds a regression test that cancels at exactly that seam, covering both the observing and take-control paths. It fails without the guard with "No exception was thrown". No change to the SCPI sent on the wire.
|
Re: "Virtual hook API break" — this was answered in full here and it has been re-listed unchanged, with no new argument. The break is deliberate: PR #406 hit this exact fork, and the suggested delegating-overload shape is the one it tried, got flagged for, and reverted, because it leaves old overrides silently not receiving the new parameter. Position unchanged. Treating this as a settled disagreement rather than relitigating it each round. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 1049355 |
Resolves against #415 (connection-loss detection and the device ErrorOccurred surface) and #417 (SD->LAN restore inside the exchange lock, which added a finalizeAsync phase to ExecuteTextCommandAsync). One real conflict, in DaqifiDeviceInitializeTests: #417 wrapped the testable device's ExecuteTextCommandAsync body in try/finally to honor the new finalize phase and re-indented it, while this branch had inserted a MutateDuringInitialization hook between the prepare phase and setupAction. Kept both — the hook now sits inside the new try block, still after prepare and before setupAction, so the two overlapping-initialization tests still mutate state at the intended point. The two test doubles this branch added (OverlappingInitDevice and CancelDuringCapabilityReadDevice) also had to widen their ExecuteTextCommandAsync overrides for the finalizeAsync parameter, and now honor the finalize phase the way the other doubles do. That compile break is the seam from #406 working as designed. Verified nothing from main was dropped: the only deletions relative to origin/main are this branch's three intended OnDeviceInitializingAsync signature changes. #415's ErrorOccurred wiring, OnConsumerErrorOccurred subscription and the Connected->Lost transition, and #417's finalizeAsync phase are all intact, as are this branch's PreserveActiveStream command skipping and the pre-Ready cancellation guard. Full suite green on net9.0 and net10.0 (2246 Core + 23 MCP).
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 477dbc8 |
…nnect #414 (opt-in connect that leaves a running stream alone) landed while the previous merge was being resolved. One conflict, in the docs' Advanced snippet, where #414 added a note about InitializeAsync stopping a running stream to the same block this branch had switched to the async/cancellable calls. Kept both: the async calls carry the token, and #414's PreserveActiveStream guidance stays. DaqifiDevice.cs and DaqifiDeviceFactory.cs auto-merged and were reviewed by hand rather than trusted: #414 confined itself to InitializeAsync and a new PreserveActiveStream property, which is disjoint from the connect/disconnect/ dispose restructuring here. In the factory, #414's PreserveActiveStream object initializer still runs before the connect call, preserving its "never observed half-applied" invariant now that the call is ConnectAsync. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
closes #385
Not merging — for review.
Why
A DAQiFi box has one acquisition. One converter, one sample rate, one place the data goes. There is no such thing as "my stream" versus "your stream" — there is only the stream.
Connecting to a device tells it to stop streaming, as part of the standard setup handshake. That is the right thing to do when you are the only one using the box: it clears out anything a previously crashed session left running, so you start clean.
But if somebody else is already recording from that box, connecting ends their recording. Their data just stops. Nothing tells them, and nothing tells you. Plug in over USB while a script is logging over WiFi, open a second copy of the app, let a colleague peek at a unit you are logging from — same result every time.
What
Connecting can now be told to leave a running acquisition alone.
The observing session connects normally and is fully usable: it reads the device's serial number, firmware, and all 32 channels, and reports
Readylike any other connection. What it does not do is touch the acquisition — it will not stop it, will not change its format, and will not redirect it to itself. Since it leaves the stream configured for whoever owns it, an observer is not itself set up to record; a session that later wants to record has to take control, which necessarily stops the other one.Nothing existing changes. Connecting still takes control of the device by default, exactly as before. This is purely opt-in.
The hazard is also now written down where people will actually hit it — in the README connection section and in
docs/DEVICE_INTERFACES.md, with a table of which setup commands the observing path skips, and a pointer toDaqifiDeviceRegistryfor apps that want to stop the same box from being opened twice in the first place.How
InitializeAsyncsends five setup commands. Three of them write the device's global stream state (stop streaming, set power state, set stream format), and over USB a fourth points the stream at this connection. A newPreserveActiveStreamflag skips exactly those four. What is left is turning off command echo — a text-mode setting, unrelated to streaming — and the read-only queries that fetch the device's identity and capabilities.Two ways to set it:
DeviceConnectionOptions.PreserveActiveStream(or theDeviceConnectionOptions.Observingpreset) for the factory and the registry, orDaqifiDevice.PreserveActiveStreamwhen you build the connection by hand. It is read once, when initialization runs.Why this direction
The issue listed several options. This is the smallest one that actually closes the gap: no new types, no change to how anything behaves today, no attempt to guess whether a caller "intends to stream" — the caller simply says which kind of session it is opening. The alternatives either changed existing behavior for everyone or required inferring intent the caller already knows.
What this does not fix
This is a courtesy between two sessions using this library, not arbitration. It stops Core from clobbering a stream; it cannot stop anything else from doing so, and both sides have to opt in. A real cross-process fix has to come from the firmware — refusing or announcing a second controlling session while streaming. Worth a companion firmware issue if we want to go further; not filed here, since this PR does not depend on it.
One compile-time break
The protected hook
OnDeviceInitializingAsyncnow takes the preserve-stream decision as a parameter instead of reading it off a field. Anyone who subclassesDaqifiDeviceand overrides that hook has to add the parameter.DaqifiStreamingDeviceis the only overrider in this repo. It is a loud, one-line fix rather than a silent behavior change, and the library is 0.x — but it is a break, so it belongs in the notes.Testing
Unit: the default path is asserted to still send the full sequence; the opt-in path is asserted to omit every stream-touching command, skip the USB routing step, and still reach
Readywith populated channels. Full suite green on net9.0 and net10.0 (2196 Core + 23 MCP), Release build with zero warnings.Hardware, Nyquist 1 (HW 2.0.0, FW 3.7.2, SN 9090539562006014104) reachable over USB and WiFi simultaneously — the exact repro from the issue. USB streaming at 100 Hz, second session connecting over TCP mid-stream:
PreserveActiveStream* one bucket covering the 2.5 s connect plus the following second — 3.5 s × 79/s, i.e. not a single frame lost.
In both runs the second session itself connected fine (
Ready, 32 channels, correct serial). Also verified: an observing connect to an idle device still reachesReadywith 32 channels and correct firmware in 3.0 s (confirming that skipping the power-state command does not leave a usable session behind), and the device streamed normally at 200 Hz afterwards with WiFi still answering. Frame rates run at ~79% of the requested rate throughout — that is the known firmware clock offset on this unit, not an artifact of this change.🤖 Generated with Claude Code