Repository navigation
fix(plot): the live sample counter under-reported by 5x — count what the device delivers - #153
Conversation
…nder tick saw The live plot's readout was incremented inside PollActiveSamples, which runs on the 50 ms render timer and can only ever observe the LATEST value per channel per tick. That capped it at 20 samples per channel per second regardless of the streaming rate, so at 16 channels / 100 Hz it reported about a fifth of what the hardware delivered — on a data-acquisition screen, under a label that says "samples". Count at the source instead: IChannel.OnChannelUpdated fires once per channel per parsed streaming frame, on the Core receive thread, and is the same signal the logging path already consumes. The handler is a single Interlocked.Increment; TotalSamples reads back with Interlocked.Read. Subscriptions are keyed by channel REFERENCE, not AbstractChannel.Equals (which compares by Name), so a re-synced channel list cannot be mistaken for one already subscribed. The render tick re-arms idempotently against the current DataChannels, the same self-healing the poll already relied on, and every path that ends a stream — StopStream, HandleStreamLost, HandleConnectionLost, Dispose — detaches. Moving the count off the poll also moves the silent-stream watchdog's reference point. It compared the total against a value read at the top of the SAME poll, which used to work only because the poll body did the incrementing. With samples arriving asynchronously that asks whether one landed during the few microseconds the body takes — almost never — and the bench watched it tear a healthy 100 Hz connection down after 8-20 s. It now compares against the total at the end of the PREVIOUS poll, which is what "no data for 8 seconds" was always meant to mean. The plot itself is unchanged: it still draws one point per channel per tick, so a transient shorter than 50 ms is still invisible. That half shares the same root cause but its fix is min/max decimation, which changes the render point budget and belongs with the rest of the render-path work in #122. The header now reads "N samples acquired". store/listing-copy.md records that screenshots 01 and 02 show the old label and old magnitude and need regenerating on the bench A16 before the next submission. closes #120 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoCount live samples at the device update source
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Qodo (PR #153, High) found that restarting a stream with a narrower selection left the counter under the truth again. Reproduced on the bench: stream 16 channels, stop, deselect 8, restart — the device stayed at 16 active analog inputs while the shell said "Streaming 8 channel(s)", and TotalSamples reported 50.00% of what was delivered. The cause is not in the counter. StopStreaming clears each channel's sample but not its activation, and AddChannel ORs into an enable mask built from whatever is already active, so nothing ever turned a deselected channel off. It kept being acquired and logged, with no trace on the plot to show for it — the counter is just where it became visible. StreamToggle now removes active analog inputs that are not in the selection before enabling the ones that are, so the device's active set matches what the user picked. Counting is also driven off IsActive rather than off the selection or off which channels got a series. Those three agree after the change, and this keeps the readout true to its label if they ever stop agreeing: whatever the device streams is what "samples acquired" has to count. Also gates the render tick's re-arm behind an armed flag. HandleStreamLost and HandleConnectionLost can be reached from a device property-change raised on the Core thread; without the gate a poll already inside its channel loop could re-attach a subscription just after teardown detached it, and that one would keep counting for a stream that is over. Bench, 16ch @ 100 Hz, restart with 8 selected: before: 16 active, 25,280 delivered, 12,640 counted (50.00%) after : 8 active, 12,640 delivered, 12,640 counted (100.00%) A 60 s straight run is still exact (76,144 / 76,144) and the silent-stream watchdog still trips at exactly 160 polls when the count is frozen, and never while it advances. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit be1dabb |
…under one try Qodo (PR #153, High): the new subscription loop enumerated the live DataChannels list, which Core's SyncChannelsFromCore replaces in place with Clear() + AddRange() from its own thread. A re-sync landing mid-enumeration throws InvalidOperationException, and the loop sat OUTSIDE the try that cleans up a failed start — so the throw would escape the command with sample counting still armed. The reconciliation loop above it already snapshotted; this one did not. Snapshots it, and moves channel reconciliation, series setup, subscription and InitializeStreaming into a single try so any failure in the arming sequence lands on the existing StopStream cleanup, which disarms counting and detaches every subscription. A re-sync between snapshot and subscribe can still leave a subscription on a replaced instance. That is harmless here and noted at the site: the device is not streaming yet, and the render tick's idempotent re-arm repairs it within 50 ms. Re-verified on the bench: 30 s straight run 38,048 delivered / 38,048 counted (100.00%, shortfall 0); restart with 8 of 16 selected reconciles the device to 8 active and counts 12,632 of 12,632 (100.00%); 60 s soak clean; watchdog still trips at exactly 160 polls on a frozen count and never over 500 advancing ones. Desktop 297 and iOS Release 1253 warnings, both unchanged from origin/main built --no-incremental the same way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo (PR #153, Medium): detaching a handler cannot cancel an OnChannelUpdated invocation that has already read the channel's delegate field. A callback from the previous stream can therefore still run after StopCountingSamples, and if the user restarts quickly enough it lands after the total has been reset — crediting the old stream's sample to the new one. The boolean armed flag could not tell the two apart. Counting now runs on a generation. BeginCountingSamples takes a fresh generation and builds the handler closed over it; the handler drops its sample unless the generation is still current; StopCountingSamples advances the generation BEFORE detaching, so every callback the detach cannot reach becomes a no-op. The delegate is retained so the same instance is the one unsubscribed. The cost on the receive thread is one volatile read before the interlocked add. Re-verified: 30 s straight run 38,048 / 38,048 (100.00%); restart with 8 of 16 selected — the exact case this protects, since the counter resets while the old stream's frames are still draining — reconciles to 8 active and counts 12,632 of 12,632 (100.00%); 60 s soak clean; watchdog trips at exactly 160 polls frozen and never over 500 advancing. Reflecting the event's backing delegate still shows exactly one handler per channel from the view-model. Desktop 297 / iOS Release 1253 warnings, unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 459f77e |
|
/agentic_review |
|
No code changes since the last review — review skipped |
|
Qodo-clean, CI green, re-benched on current |
Thanks for the update. The reported merged-tree validation addresses all three previously raised items:
With the 100% counter/restart results, watchdog checks, live watcher teardown validation, and unchanged warning baselines, there are currently no active Qodo findings. |
Not merging — for review.
What was wrong
The live plot's headline figure was wrong by about 5x, and it was wrong in the direction that matters: it under-reported.
Streaming 16 channels at 100 Hz, the Nyquist delivers ~1,600 samples/s. The screen said ~320. The figure was incremented inside
PollActiveSamples, which runs on the 50 ms render timer and can only ever see the latest value on each channel when the tick happens — at most one sample per channel per tick, i.e. a hard ceiling of 20/channel/s no matter what the device is doing. At the top selectable rate of 1000 Hz it would have been 1 in 50.Recorded data was never affected — logging runs off the per-frame device message, not off this poll — but the number a user reads while acquiring could not be trusted, on an app whose entire job is acquisition.
The two symptoms in #120 are one root cause, not two
The issue reports two things: the counter under-reports, and the plot cannot show transients. Both fall out of the same design — the live view samples the stream at render rate instead of consuming it. They do not, however, have the same fix:
So the plot still draws one point per channel per tick and a spike shorter than 50 ms is still invisible. That is unchanged by this PR and stays with #122; nothing is lost when #120 closes.
A second defect fell out of this, and it is arguably worse
Qodo pushed on the restart case and it turned out deselecting a channel never actually turned it off.
StopStreamingclears each channel's sample but not its activation, andAddChannelORs into an enable mask built from whatever is already active. So: stream AI0–AI15, stop, deselect half, start again — the device stayed at 16 active channels while the status line read "Streaming 8 channel(s)". Those eight were still being acquired and still being logged to CSV, with no trace on the plot to account for them. Reproduced on the board and fixed here (StreamTogglereconciles the device's active analog-input set to the selection before enabling anything); numbers below.How it is fixed
IChannel.OnChannelUpdatedfires once per channel per parsed streaming frame, on the Core receive thread — the same signalLoggingManageralready subscribes to, which is why the CSV was always complete.TotalSamplesis now counted there, with anInterlocked.Incrementand nothing else on that thread.Three details that are load-bearing rather than stylistic:
IsActive, not off the selection and not off which channels got a series. Those three agree after the reconciliation above; driving offIsActivekeeps the readout true to its label if they ever stop agreeing, because whatever the device streams is what "samples acquired" has to count.AbstractChannel.Equalscompares byName, so aHashSet<IChannel>with the default comparer would treat a replacement instance forAI0as one already subscribed and silently stop counting it. The set usesReferenceEqualityComparer.DataChannels. That is the same self-healing the poll already relied on for instance churn, now extended to the subscription. Steady state cost is one set lookup per channel per tick.OnChannelUpdatedinvocation that has already read the channel's delegate field, so a callback from the previous stream could otherwise land after the total was reset for the next one. The handler is built per stream, closed over its generation, and teardown advances the generation before detaching.try, and everyDataChannelsenumeration in it is snapshotted — Core replaces that list in place from its own thread. Any failure while arming lands on the sameStopStreamcleanup, which disarms counting and detaches.Every path that ends a stream (
StopStream,HandleStreamLost,HandleConnectionLost,Dispose) detaches the subscriptions.The header text changes from "N points plotted" (#121's honest workaround for the bad number) back to "N samples acquired", which the figure now actually is.
Verification
Everything below was re-measured on the tree merged with current
main. The branch's first round of evidence was gathered against base7b54e63(#148);mainhas since taken #150, #151 and #152.origin/mainis merged into this branch as87ba1eb, and every number here comes from that merged tree.There is no test project in this repo and no
dotnet teststep in CI — that is the honest state of the guarding tests here, and it is why the evidence below is a bench harness rather than a unit test. The harness drives the realMobileShellViewModel(via its ownAdoptConnectedDevice/StreamToggleCommand/PollActiveSamples, at the same 50 ms cadenceMobileShellView's render timer uses) against the bench Nyquist over USB serial, and takes ground truth from an independent place: a subclass overriding theprotected virtual DispatchDeviceMessagecounts parsed streaming frames, andframes x active channelsis the number of samples the device actually delivered. A separate harness-ownedOnChannelUpdatedcounter cross-checks that; the two agreed exactly in every run.Interaction with #150 (
Services/DeviceWatcher)#150 routed device removal through a new
IDeviceWatcherseam that raisesDeviceRemoved; this PR rewrites the view-model that callsConnectionManager.RegisterConnectedDevice/UnregisterConnectedDevice/ActiveChannelCountand that changed teardown ordering. Both now drive teardown, so they were checked against each other rather than assumed independent.On the shipped heads they cannot both be live.
MobileShellViewModelis constructed only fromMobileMainView, whichApp.OnFrameworkInitializationCompletedbuilds only underISingleViewApplicationLifetime— the Android and iOS heads.DeviceWatcherFactory.Create()returnsNoOpDeviceWatcheron exactly those heads (IsWindows/IsMacOS/IsLinuxare all false on Android and iOS), and the desktop heads, where the watcher is real, never construct this view-model. #150 also never touchedRegisterConnectedDevice,UnregisterConnectedDeviceorActiveChannelCount.The bench is nonetheless a stricter case than production, and was run that way on purpose. It drives
MobileShellViewModelon macOS, where #150'sSerialPortPollingDeviceWatcheris the live backend — which the harness now prints, so the run is on record as having exercised the new seam:Double teardown, measured.
ConnectionManager.Disconnectis the terminal action of #150's removal path; calling it on a device the shell holds makes the device's ownIsConnectednotification run this PR'sHandleConnectionLost, which unregisters the same device again. Both halves, one disconnect, on the UI thread as in the real app:One successful removal, two change notifications:
HandleConnectionLostremoves the device, andConnectionManager.Disconnect's ownRemovethen returns false but still raises. SoUnregisterConnectedDevicedoes its work exactly once, a repeat call is inert, and neither ordering strands an entry or throws.ActiveChannelCountagrees with the reconciled set. Streaming 16 of 16 it reads 16; after stop → deselect 8 → restart it reads 8, matching the device's active analog inputs afterStreamToggle's reconciliation.16 channels @ 100 Hz, 30 s, merged tree
origin/main(was, on the old base)OnChannelUpdatedevents (cross-check)The
origin/maincolumn is the pre-fix measurement from the first round (three runs, 79.3 / 79.3 / 79.2 frames/s and 25.19% every time);origin/mainhas no fix in it to re-measure. The merged-tree column is fresh. The same run also replays the OLD counting rule alongside the new one — 9,584 in the same 30 s window, exactly 599 polls x 16 channels, so the contrast is not an artefact of comparing two sessions.Restart with a narrower selection — the second defect, on the merged tree
Stream 16 @ 100 Hz, stop, deselect 8, restart.
OnChannelUpdated eventsis the harness's own counter, attached to every analog input regardless of selection, so it measures what the device delivered rather than what the app counted:Before the fix the same sequence left 16 channels active on the device, delivered 25,280 samples against a status line reading "Streaming 8 channel(s)", and put 12,640 on screen — 50.00%. The events row halving is the direct evidence that the deselected channels stopped being delivered, not merely stopped being counted.
The silent-stream watchdog, re-checked on the merged tree
This is the check that caught a real regression in the first round: the first version of this change looked perfect on a green build and tore healthy streams down after exactly 8 s (160 polls x 50 ms), because the watchdog's "did anything arrive?" comparison was against a value read microseconds earlier in the same poll. #150's teardown changes made it the most likely thing to have broken here. It has not:
Trips at exactly its threshold, with the right user-facing message, and its teardown unregisters cleanly from
ConnectionManager— and it does not fire on healthy data.No subscription leak
Reflecting the field-like event's backing delegate at the end of a run, on every active channel:
Exactly one handler from the view-model per channel after 600 re-arming polls, and the tracking set stays at 16.
Builds
Both sides built
--no-incrementalin the same worktree, against currentmain(ffd91ac), not the branch's original base:iossimulator-arm64: 1248 → 1248 warnings.mainat7b54e63; fix(macos): give USB hotplug detection a working macOS/Linux backend #150–feat(macos): ship a real .app bundle instead of a bare Mach-O #152 moved the baseline, mostly by removing theSystem.Managementusage fromConnectionManager. Both sides re-measured here.)packages.lock.jsonunchanged by restore and by the-rbuild.Not verified
store/screenshot-01/02still show both the old label and the old (5x low) magnitude. Regenerating them needs the bench A16;store/listing-copy.mdrecords what has to change so it is not discovered at submission time.closes #120