Repository navigation
fix(streaming-rate): a profile with an out-of-range rate makes the device refuse to record (#248) - #255
Conversation
…Frequency will store WIP: 13 of 26 fail against origin/main. The Core_ cases pass unfixed and are the finding: Daqifi.Core 1.7.0's StreamingFrequency setter throws ArgumentOutOfRangeException outside 1..MaxSamplingRate, so an out-of-range value stored by the wrapper does not reach the hardware as a wrong rate -- it aborts InitializeStreaming / StartSdCardLogging. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…advertises AbstractStreamingDevice.StreamingFrequency was a bare [ObservableProperty] field, so 0, -1 and any rate above the device's advertised maximum were stored and then assigned onto the Core device at the top of InitializeStreaming and StartSdCardLogging. Core 1.7.0's setter validates 1..DeviceCapabilities .MaxSamplingRate and throws ArgumentOutOfRangeException outside it, so the consequence was not a wrong rate at the hardware: the device refused to start recording, with the cause several screens from where the rate was set. Guard at the setter, clamping to the device's own advertised range (sanitized the way Core sanitizes it) rather than a compiled-in 1000, so a board that describes itself as faster stays drivable at its real ceiling. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…a port Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoClamp streaming rates to device-advertised limits
AI Description
Diagram
High-Level Assessment
Files changed (2)
|
Code Review by Qodo
1. Capability refresh bypasses validation
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…ling moves Qodo review (High, correctness): the setter's guard held only at assignment time, but this wrapper outlives the capabilities it was clamped against. SyncFromCoreDevice re-hydrates on every ChannelsPopulated, and a reconnect builds a new Core device while keeping the wrapper and the rate the user already chose -- so a board that then describes a lower ceiling left a stale out-of-range rate stored, and the next start handed it to Core and was rejected there, which is the exact failure this PR removes. Re-run the setter from HydrateDeviceMetadata. Two tests: one pins the lowered-ceiling case (it fails with the new line disabled), one pins that a rate which still fits is left alone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 9d41358 |
…ead rate Qodo review round 2 (Moderate, correctness): the re-clamp read _streamingFrequency and then wrote it back through the setter. Hydration runs on Core's callback thread while the UI and profile apply write this property from theirs, so a rate written between the read and the write was overwritten by the stale one -- a lost update that discards a rate the user just chose. Assign the ceiling itself instead, and only when the stored rate exceeds it. Losing the race now costs at worst a rate the hardware could not have run at; the common case (a rate that still fits) writes nothing at all. Both existing re-hydration tests still hold. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Triage of the round-2 findings (reviewed against 1. "Capability refresh bypasses validation" (High) — already fixed; this finding is stale. 2. "Capability clamp leaves stale UI" (Medium) — the gap is real, but it predates this PR and is What makes it out of scope here is that this is not new. On Worth being explicit about the trade, because it is not "correct display" versus "stale display". 3. "Hydration can overwrite rate" (Moderate) — valid, and fixed in It now assigns the ceiling itself, and only when the stored rate exceeds it: var advertisedCeiling = Math.Max(1, Capabilities.MaxSamplingRate);
if (_streamingFrequency > advertisedCeiling)
{
StreamingFrequency = advertisedCeiling;
}No value read from the field is ever written back, so the interleaving cannot restore a stale rate: Suite 540/540; desktop and iOS both green; first-party warning set unchanged against my own |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 3765447 |
|
Round 3 ( "Capability clamp leaves stale UI" — Qodo has marked it ✓ Resolved. Agreed as scoped: the gap is "Capability refresh bypasses validation" (High) — rebutting on evidence, for the third round. Its
That has not been true since var advertisedCeiling = Math.Max(1, Capabilities.MaxSamplingRate);
if (_streamingFrequency > advertisedCeiling)
{
StreamingFrequency = advertisedCeiling;
}Three independent confirmations that the described failure cannot occur at this head:
If there is a different path that re-hydrates capabilities without going through State at |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 3765447 |
…ration Qodo review round 4 (Medium, correctness): HydrateDeviceMetadata installs the new capabilities at the top of the method and corrected the rate at the bottom, so acquisition starting in that window read the old rate against the new lower ceiling and Core rejected it -- the exact refusal this PR removes. Extract the correction into ClampRateToAdvertisedCeiling and call it immediately before each handoff to Core (InitializeStreaming, StartSdCardLogging) as well as from hydration. The handoff call is what closes the window; the hydration call is what keeps the stored rate, and anything displaying it, current. A residual interleaving remains and is documented on the method: Core reads its own copy of the ceiling inside its own setter, so this cannot be made atomic from here. It removes the systematic case, not every ordering. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 8db9f90 |
|
Round 5 ( Resolved by Qodo this round: "Startup can beat re-clamp" and "Capability clamp leaves stale The one item still listed, "Capability refresh bypasses validation", is the same carry-over from
and is pinned by four tests: the two re-hydration cases and the two handoff cases. I verified the I am treating this as disproven against the reviewed commit rather than pushing further changes for Final state: 30 new tests, all passing; full suite 542/542; desktop and iOS |
Shepherd verification — independent of the author's reportQodo, on the current head
CI on that exact SHA. Run Numbers re-derived from the final diff, not carried forward. Two body inaccuracies found and corrected in place (no commit, head unchanged): the The The gap no green covered: #255 against today's
|
| result | |
|---|---|
| Desktop | green, 191 warnings |
iOS Release/iossimulator-arm64 |
green, 192 warnings |
| Suite | 560/560, 0 failed |
packages.lock.json |
no drift |
The two warning totals are MSBuild all-project totals (~20 of them third_party OxyPlot), not
first-party-only, and they are identical to the totals reported for #255 alone — so #233 adds none.
The test count reconciles exactly: 512 (main) + 18 (#233) + 30 (#255) = 560, with no
interaction between the two PRs. The 18 is consistent with the 530 reported for #233 on this main.
#256 is not covered. Its head moved under me during this run (e2d49c2 at 20:51Z → 49be77c
at 21:00Z) while its builder is still working, so there was no stable tree to build against.
Qodo-clean, CI green — ready for review at 8db9f90a554d00ffa87f49fe1a201c7c7b23c074.
|
Correction to the bench section of this body — the code merged here is fine; two factual claims in Posting this because the description is now known to be wrong on a point that already cost a ticket, This body states:
and, in the
Both are false for this board. Re-measured on the same unit (Nq1, fw 3.7.2, USB, 1. The device does publish a capability document, with a rate model. Read directly with the two 2. 1000 is the board-table bootstrap value that is correct only before the document is applied. A What this does and does not change. The fix merged here is unaffected and still correct: the What it does change is the verdict on the Separately, |
closes #248
Not merging — for review.
What was wrong
Apply a saved profile whose sampling frequency is out of range, press START, and the device
refuses to record. Nothing in the app tells you the rate is why.
AbstractStreamingDevice.StreamingFrequencywas a bare[ObservableProperty]field, so it storedwhatever it was given —
0,-1,1001,int.MinValue. That value is not private state: bothpaths that start acquisition assign it straight onto the Core device as their first act
(
InitializeStreaming,StartSdCardLogging, each doingcoreDevice.StreamingFrequency = StreamingFrequency).The UI sliders all clamp to
Minimum="1" Maximum="1000", so dragging cannot cause this. A profilecan:
ProfilesPaneViewModel.ApplyProfileToDevicesassignspd.SamplingFrequencyhere unfiltered,and that number is
(int)d.Element("SamplingFrequency")out of an XML file on disk(
ProfileXmlStore.cs:218).What the ticket got right, and the part it did not
Right: the field has no setter validation, and 0 / -1 / 1001 are stored and pushed to Core.
Wrong, and it changes what this is: the ticket reasons about "a silently-wrong rate reaching the
hardware". Core 1.7.0's setter is validating. It throws
ArgumentOutOfRangeExceptionoutside1..DeviceCapabilities.MaxSamplingRate. So the bad value never reaches the hardware at all — itthrows inside
InitializeStreaming,LoggingFleet.Startcatches it as a device refusal, and theuser gets a device that will not start, several screens from wherever the rate was set. A refusal,
not a wrong number.
I did not take that from the ticket or from prose. The five
Core_*cases in the new test file (three methods)exercise the pinned
Daqifi.Corepackage directly and pass against unmodifiedmain— theyare the evidence for the paragraph above, and they stay in the suite so a future Core bump that
changes the contract fails here rather than on a bench.
The ticket's third claim — that the validating property "was dropped during the port" — does not
survive checking.
daqifi-desktop'sAbstractStreamingDevicecarries the identical unguarded[ObservableProperty] private int _streamingFrequency = 1;and the identical two comments. Theport copied it faithfully; upstream has the same hole.
The fix
One guard, at the one choke point every real device shares:
Four decisions worth arguing:
Where it dies: at the setter. That is the single place every write goes through — the shell's
SelectedStreamingFrequency, profile apply, and the mobile shell — and the class that hands thevalue to Core. Guarding here means
DaqifiViewModelandDevicesPaneViewModelboth read thecorrected value straight back on their next
get, so the RATE chip and the FREQUENCY sliderself-correct with no extra wiring.
Clamp, not throw. Throwing would move the failure to the write site, which sounds better until
you look at the write sites:
ApplyProfileToDevicesiterates a fleet, so a throw would abandon therest of the profile half-applied. Clamping also matches how Core itself handles the other
rate ceiling —
EnforceStreamingFrequencyCap()lowers the live rate rather than rejecting it — andhow the other first-party Core consumer already behaves: the MCP server clamps and reports
(
Requested 1001 Hz clamped to 1000 Hz). Nothing is hidden by the clamp: the stored value is whatthe UI displays, what
LoggingManagerrecords as the session'sSamplingFrequencyHz, and what islogged as a warning naming the requested rate, the range and the rate used.
The range is re-established when the ceiling moves, in two places, both raised in review and
both real. This wrapper outlives the capabilities it was clamped against:
SyncFromCoreDevicere-hydrates on every
ChannelsPopulated, and a reconnect builds a new Core device while keepingthe wrapper and the rate the user already chose, so a board that then describes a lower ceiling
would leave a stale out-of-range rate stored and fail at the next start — the exact failure this PR
exists to remove.
ClampRateToAdvertisedCeilingtherefore runs fromHydrateDeviceMetadata, so thestored rate follows a ceiling that has just moved, and immediately before each handoff to Core
(
InitializeStreaming,StartSdCardLogging), which is the only moment being out of range costs theuser anything — and which closes the window between capabilities being replaced at the top of
hydration and the rate being corrected further down it.
It assigns the ceiling itself, never a value re-read from the field: hydration runs on Core's
callback thread while the UI and profile apply write from theirs, and a read-then-write would let a
rate chosen in between be overwritten by the stale one. A rate that still fits writes nothing at
all. One residual interleaving is not closable from here and is documented on the method — Core
reads its own copy of the ceiling inside its own setter — so this removes the systematic case, a
stored rate that is permanently out of range, not every possible ordering of an unsynchronized
wrapper.
The ceiling is the device's own advertised maximum, not the sliders' 1000. Hard-coding 1000
would newly reject rates a faster board reports it can sustain, and Core validates against the
advertised figure, not against 1000 — so a compiled-in constant here could only ever disagree with
the layer below it.
Math.Max(1, ...)mirrors Core's own sanitization of that same property, whichis public and mutable and can be left at 0 by a malformed capability document.
Verification
Tests — 30 new cases. 28 of them compile against
origin/main, and 14 of those fail there(measured on a throwaway branch off
origin/mainwith the final test file, not carried forward froman earlier commit). Every one of the 14 is a stored value Core would reject: three below-1, three
above-ceiling, the lowered-ceiling re-hydration case, and the seven rows of
Whatever_the_wrapper_stores_is_a_rate_Core_will_accept, which stores through the wrapper and thendoes exactly what
InitializeStreamingdoes with the result — each row threwArgumentOutOfRangeExceptionunfixed. The 14 that pass unfixed are the fiveCore_*contract casesplus the four in-range rows and the default-value,
PropertyChanged, rate-still-fits,higher-advertised-ceiling and unusable-ceiling cases that pin what must not change. The remaining 2 are the handoff cases: they call
ClampRateToAdvertisedCeiling, which thisPR introduces, so they cannot be run against
mainat all — stated rather than folded into a ratio.All 30 pass with the fix; full suite 542/542.
There were no existing tests over this property to strengthen —
StreamingFrequencyhad nocoverage at all, which is why a value the layer below refuses could sit here through a port.
Bench (Nq1, fw 3.7.2, USB,
/dev/cu.usbmodem1101, non-destructive; rate restored to 100 Hz anddevice disconnected afterwards). Through Core against the real board:
1000 -> 1000accepted;1001 -> 1000and30000 -> 1000, both reported as clamped at "maximum 1000 Hz";0refusedbelow 1. With no analog channels enabled the cap came back as 1000, not 0 — per Core's
SampleRateCaprule that is the "device stated no cap and published no rate model" branch, whichreturns the board ceiling itself, so
MaxSamplingRateon this board is 1000 and the ticket'spremise that 1001 is out of range holds here.
The
22000comment — corrected reference, and why it is not fixed in this PR. The comment is atAbstractStreamingDevice.cs:505-506, not:430— the line moved as this branch grew, and theearlier reference in this body was stale. It says the bench Nq1 "left
MaxSamplingRateat the boardtable's 1000 Hz while the device itself reports 22000, and no capability document at all." Checked
against the pinned
Daqifi.Core1.7.0 source,22000is not derivable on this board:DeviceCapabilitiesdefaultsMaxSamplingRateto 1000 and itsFromDeviceTypetable returns 1000for
Nyquist1,Nyquist2andNyquist3, and the only assignment that raises it above that table isthe capability-document merge — which the same comment says this board does not produce. The bench
run above is the matching observation. The comment is wrong, but it is left alone here: it is
pre-existing code outside this diff; no code path reads
22000(the ceiling is always read fromCapabilities.MaxSamplingRateat runtime, so the wrong figure cannot produce a wrong ceiling); andcorrecting it honestly is not a one-line edit, because the false figure is the load-bearing example
justifying an existing second
HydrateDeviceMetadatacall — replacing it means re-establishing whatthat call actually changes on this board. Filed as #257 instead.
Build gate — desktop and iOS
Release/iossimulator-arm64both green. Warnings measured on myown merge-base in this worktree, both sides with
third_party/oxyplot-avalonia/objcleared:desktop 191 and iOS 192 MSBuild all-project totals on both heads, of which 171 / 172 are
first-party. Diffing the first-party warning sets gives zero added and zero removed — the only
delta is one pre-existing
CS8625in this file moving from line 1706 to 1785, which is the addedcode and comment above it. No
packages.lock.jsonchanged. Android is not buildable on this Mac (workload absent);CI covers it.
Net-additive, and what is out of scope
+359 / -4. 272 lines of that is the new test file and most of the rest is comment explaining why
the guard exists and what it deliberately does not close; the executable change is a property with
a ten-line setter replacing a two-line attributed field, a six-line private method, and three
one-line calls to it, and it deletes two now-false porting comments. No new type, no attribute, no range-config knob.
Not done, deliberately:
back a corrected number instead of
0 Hz— and staging it needs a connected board plus a corruptprofile on disk, which
AvaloniaCapturecannot arrange. Unverified in the UI; verified at theproperty and against Core.
in review and confirmed:
DaqifiViewModel's selected-device handler returns early on anything butFriendlyName, andDevicesPaneViewModel.FrequencyHzonly notifies from its own setter. This isnot new — on
maintodayApplyProfileToDeviceswritesdevice.StreamingFrequencydirectly andneither display follows it — and closing it means propagating device→view-model rate changes
through two more view models, which is a separate change. The trade here is still favourable: the
alternative to a stale chip is a device that records nothing.
DaqifiViewModel.cs:1787'sMath.Max(1, connectedDevice.StreamingFrequency)stays. It is nowredundant for
AbstractStreamingDevice, but it reads throughIStreamingDevice, andBootloaderSessionStreamingDeviceAdapterimplements that interface with an unguarded{ get; set; }. Removing it would be a different change with a different argument.BootloaderSessionStreamingDeviceAdapter.StreamingFrequencyis left unguarded. It exists fora bootloader session, which never streams, so its rate never reaches Core's validating setter.