Repository navigation
fix(device): give health telemetry a way to refresh, and stop the docs claiming it is live - #558
Conversation
…s claiming it is live
device.Metadata.Health is documented as live telemetry. It is a snapshot
taken once at connect that never changes again for the life of the
connection. A UI built on it shows "Battery: 87%" an hour later on a device
that has since run flat.
Two independent causes, both confirmed on a bench Nq1 running 3.7.2:
Nothing ever asks again. The only GetDeviceInfo sends are inside
WaitForChannelsPopulatedAsync, which stops the moment channels populate
during InitializeAsync, and there was no public API to ask afterwards.
The firmware sends no periodic status frames on a connected link. Each
stream frame is built from four tags and none of them is health, so a
connected device is silent unless asked. Measured: 1,587 inbound
messages over 65 s, exactly ONE carrying health -- the one the harness
requested.
ADDS RefreshDeviceStatusAsync(TimeSpan?, CancellationToken): sends
GetDeviceInfo, waits for the next status message, and returns once it has
been applied. Additive and non-breaking. It subscribes before sending so a
fast reply cannot slip through the gap, the same ordering
WaitForChannelsPopulatedAsync already relies on, and it unsubscribes in a
finally so a caller polling in a loop does not accumulate a handler per
call. Default timeout 5 s -- a device that is answering at all replies in
milliseconds, so this is a hang detector rather than a prediction.
Cadence stays with the caller deliberately. A self-poll inside Core would
contend for the operation lock against streaming and SD work, and how often
"current" needs to be is a decision only the application can make.
DOCS CORRECTED. DeviceHealth claimed its values update "including the
periodic ones emitted during streaming". There are none; the original
ticket #335 hedged this as "if present" and the hedge was dropped when the
doc was written. The summary now says status messages are not periodic and
points at the refresh.
BoardTemperatureCelsius now says what it is: no shipping firmware populates
it. The encoder's temp_status case is empty, so the field is never
assigned, stays 0, and proto3 omits it -- checked at v3.5.0, v3.6.0,
v3.7.0, v3.7.1 and v3.7.2, the whole range Core supports. Against real
hardware it cannot be anything but null. Kept because the wire field exists
and a future firmware may fill it.
Four tests: the refresh sends SYSInfoPB and applies the reply; a silent
device times out rather than hanging; not-connected throws; and a caller
refreshing repeatedly leaves nothing subscribed behind. All four fail
without the new method -- it does not exist to be called.
Baseline unchanged: the same 4 pre-existing LinuxUsbPortDescriptorProvider
failures, passing 3538 -> 3542.
Fixes #535
|
/improve |
|
/agentic_review |
PR Summary by QodoAdd on-demand refresh for device health telemetry
AI Description
Diagram
High-Level Assessment
Files changed (3)
|
PR Code Suggestions ✨Warning
|
|||||||||
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Two findings from review of the #535 fix. Both are about the same thing: what a caller is actually promised. CONCURRENT REFRESHES COULD COMPLETE EACH OTHER. Two calls both subscribed to the same multicast StatusMessageReceived event, so ONE incoming frame -- which answers only one of the two outstanding requests -- completed both TaskCompletionSources. The loser returned success off a reply that was never its own. Nobody got wrong DATA from this: both callers saw a genuinely fresh reading, since the reply updates the shared Metadata either way. What was wrong is the promise. A method whose entire purpose is "ask the device now and tell me when it answered" should not report an answer to a question it did not ask, and a caller retrying after its own short timeout while the first attempt is still in flight is not an exotic usage. A SemaphoreSlim now serializes them, so each call sends its own request and completes on its own reply. Both reviewers raised this independently -- Qodo as a suggestion, the audit as an unsound refutation of the single-refresh analysis -- which is a fair signal that the reasoning "nobody gets wrong data" was answering the wrong question. THE TIMEOUT COVERS MORE THAN THE ROUND TRIP. Send is fire-and-forget and OperationSerializer.TryDeferSend parks it while an exclusive operation holds the device -- an SD download can hold it for many minutes. So a short timeout taken during one elapses before the device is ever asked, and the caller gets a TimeoutException that reads like a silent device. Left as-is behaviourally: the caller's deadline bounding total time-to- answer is the defensible reading, and it fails loudly rather than silently. But it was undocumented, which is the part that would have cost someone an afternoon. The parameter now says the deadline covers waiting behind another refresh and waiting for the send to leave the queue, and names the SD-download case specifically. One deadline now covers the whole operation rather than only the reply wait. The concurrency test is mutation-proved: widening the semaphore to unlimited makes it fail. It asserts the queued caller has not sent yet, is not completed by the first reply, and then completes on its own.
|
/agentic_review |
Audit round 1:
|
| var statusTcs = new TaskCompletionSource<bool>( | ||
| TaskCreationOptions.RunContinuationsAsynchronously); | ||
|
|
||
| void OnStatus(DaqifiOutMessage _) => statusTcs.TrySetResult(true); |
There was a problem hiding this comment.
1. Late reply satisfies next refresh 🐞 Bug ≡ Correctness
After a refresh times out, the gate is released while its transmitted request may still produce a delayed response; the next refresh's unfiltered handler accepts that response and returns before its own reply arrives. The caller can therefore observe stale metadata despite the new serialization claiming each call waits for its own request.
Agent Prompt
## Issue description
A status response can arrive after its refresh timed out. Because the timeout releases the semaphore and the next call completes on any status event, that delayed response can incorrectly satisfy the next refresh.
## Issue Context
The protocol event currently has no request correlation. Ensure an outstanding response from a timed-out request cannot be consumed as the response to a later refresh; use protocol correlation if available, otherwise quarantine/drain the outstanding response or reset the request channel before allowing another refresh. Add a test where the first call times out, its response arrives during the second call, and the second call must continue waiting for its own response.
## Fix Focus Areas
- src/Daqifi.Core/Device/DaqifiDevice.cs[3371-3404]
- src/Daqifi.Core.Tests/Device/DaqifiDeviceTests.cs[199-233]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit c52c684 |
…alling a delivered reply a timeout Two more from review, both of which would have told a caller the refresh FAILED while the status it asked for was already sitting in Metadata -- the most confusing failure this API could produce. A THROWING SUBSCRIBER COULD BLOCK THE REFRESH. .NET stops invoking a multicast delegate at the first exception, and RaiseClassifiedEvent wraps the WHOLE invocation list in one try/catch. That isolates the device from a faulting subscriber, which is what it was written for, but it does not isolate subscribers from each other. So a consumer that subscribed to StatusMessageReceived earlier and throws would prevent the refresh from ever seeing its own reply. The waiter no longer rides the public event. It is a private field invoked directly from OnStatusMessageReceived, before any consumer subscriber and outside the multicast list, so no third party can delay or prevent it. Its own faults are contained there for the same reason the classified event contains a subscriber's. One refresh runs at a time, so a single field is enough. The wider problem -- that RaiseClassifiedEvent does not isolate subscribers from one another for ANY classified event -- is real but bigger than this PR, and filed separately. A REPLY AT THE DEADLINE BECAME A TIMEOUT. If the status landed in the same instant CancelAfter fired, WaitAsync could throw cancellation even though the reply had arrived and been applied, and the catch translated that unconditionally into TimeoutException. The wait now checks whether the task actually completed before reporting failure. Both are mutation-proved. Routing the waiter back through the public event makes the throwing-subscriber test fail (it times out, 2 s); the test otherwise passes. Baseline unchanged: the same 4 pre-existing LinuxUsbPortDescriptorProvider failures, passing 3543 -> 3544.
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit cb1c32d |
A status frame carries nothing saying which request it answers. So if one refresh times out and its reply arrives late, the NEXT refresh can be completed by that older frame rather than its own. Documented rather than worked around, because there is nothing to work around it WITH. The obvious heuristic -- skip one frame after a timeout -- makes the commoner case worse: if the earlier reply never arrives at all, the next refresh would discard its own reply and time out too. MsgTimeStamp is not a usable correlation key either; nothing in the device layer reads it for status, and the device-side counter wraps about every 102 seconds. What the caller actually gets is worth stating plainly, because it is much less alarming than "wrong answer": a genuine device reading taken before their call, bounded by the previous refresh's timeout, with their own reply landing moments later and updating Metadata.Health again. This is the third finding arising from the same underlying fact, so the remark names that root cause once -- no request/response correlation -- rather than describing another symptom. The concurrent-refresh case had the same shape for a different reason and IS prevented outright, by serializing.
Round 2 audit: PASS /
|
Audit round 3: PASS, clean, 0 findings. Merging.Three adversarial rounds and three Qodo passes. Every finding across both was real; none were declined on the merits except the one the protocol makes unfixable, and that is documented rather than waved off. Final state of the two open counters
What each round actually caught
Three of those would have told a caller the refresh failed while the status it asked for was already applied — the most confusing thing this API could do. Each fix is mutation-proved; one of my mutation attempts was itself invalid (it did not compile, so it proved nothing) and only surfaced because the run printed no pass/fail line. Six tests, all of which fail without the code they cover. Two things filed rather than folded in
Baseline unchanged throughout: the same 4 pre-existing |
Fixes #535.
device.Metadata.Healthis documented as live telemetry. It is a snapshot taken once at connect that never changes again for the life of the connection. Build a UI on it and it shows "Battery: 87%" an hour later on a device that has since run flat.Two independent causes, both confirmed on hardware
1. Nothing ever asks again. The only
GetDeviceInfosends are insideWaitForChannelsPopulatedAsync, which stops the moment channels populate duringInitializeAsync. There was no public API to ask afterwards.2. The firmware sends no periodic status frames on a connected link. Each stream frame is built from four tags and none of them is health, so a connected device is silent unless asked. From the issue's bench run (Nq1, fw 3.7.2):
GetDeviceInfo1,587 inbound messages; exactly one carried health, and only because it was requested.
The fix
RefreshDeviceStatusAsync(TimeSpan?, CancellationToken)— sendsGetDeviceInfo, waits for the next status message, returns once it has been applied. Additive, non-breaking.It subscribes before sending, so a fast reply cannot slip through the gap between
Send()and the subscription — the same orderingWaitForChannelsPopulatedAsyncalready relies on. It unsubscribes in afinally, so a caller polling in a loop does not accumulate one handler per call for the life of the connection. Default timeout 5 s: a device that is answering at all replies in milliseconds, so this is a hang detector, not a prediction.Cadence stays with the caller, deliberately. The issue flagged this and it is right — a self-poll inside Core would contend for the operation lock against streaming and SD work, and how often "current" needs to be is a decision only the application can make.
Docs corrected — both claims were wrong
DeviceHealthsaid its values update "including the periodic ones emitted during streaming". There are none. Worth noting the original ticket #335 hedged this correctly as "if present"; the hedge was dropped when the doc was written.BoardTemperatureCelsiusnow says what it is: no shipping firmware populates it. The encoder'stemp_statuscase is empty, so the field is never assigned, stays 0, and proto3 omits it — checked at v3.5.0, v3.6.0, v3.7.0, v3.7.1 and v3.7.2, i.e. the entire range Core supports. Against real hardware it cannot be anything butnull. Kept, because the wire field exists and a future firmware may fill it.Which option this took, and why
The issue offered docs-only or docs-plus-refresh and left it open. The acceptance criteria settle it — they ask for "a supported way to refresh health telemetry" and "a test covering the refresh path", which docs-only cannot satisfy. It is also the smaller surprise: the alternative documents
Send(ScpiMessageProducer.GetDeviceInfo)as the refresh path, which works but leaves every consumer to reinvent the wait, the timeout and the unsubscribe.Test plan
CompletesAndAppliesTheReply_WhenAStatusArrivesSYSInfoPB, andMetadata.Healthcarries the reply on returnWhenTheDeviceStaysSilent_ThrowsTimeoutExceptionrather than a hang — the realistic failure, since a lost request produces silence, not an error frameWhenNotConnected_ThrowsDeviceNotConnectedExceptionUnsubscribesAfterCompletingAll four fail without the new method — it does not exist to be called. The device double now captures
Sendrather than transmitting (it has no transport), which also lets the first test assert which command went out rather than merely that the call did not throw.Baseline-checked:
mainfails 4 tests on this station before any change (Device.Discovery.LinuxUsbPortDescriptorProviderTests, a WSL/Windows runtime artefact). After: the same 4, passing 3538 → 3542.No hardware needed; the bench evidence is recorded in the issue.
🤖 Generated with Claude Code