Repository navigation
fix(rate): the FREQUENCY slider stopped at 1000 Hz on a board advertising 22000 (#270) - #273
Conversation
…sing 22000 (#270) Both Devices views wrote `Maximum="1000"` into the markup. That 1000 is Core's board-table bootstrap value, correct only until the device describes itself: the bench Nq1 (fw 3.7.2) publishes a schema-2 capability document stating sample_rate_range_hz.max = 22000, and InitializeAsync raises MaxSamplingRate to it during connect. The wrapper's rate guard (#255) and Core's own setter both validate against that figure, so the slider was the only thing in the chain that could not ask for a rate the hardware supports. The ceiling is now read from the device. AbstractStreamingDevice grows MaxStreamingFrequency, which is the `Math.Max(1, Capabilities.MaxSamplingRate)` expression the rate guard and the re-clamp each already computed separately, so the concept has one home; DevicesPaneViewModel exposes it as MaxFrequencyHz and both views bind their slider's Maximum to that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/agentic_review |
PR Summary by QodoUse device-advertised maximum for frequency sliders
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all |
|
Ready for review at Still uncovered: Not merging. |
Not merging — for review. closes #270
The Devices pane's FREQUENCY slider wrote
Maximum="1000"into the markup. That 1000 is Core'sboard-table bootstrap value, correct only until a device describes itself. The bench Nq1
(fw 3.7.2) publishes a schema-2 capability document stating
sample_rate_range_hz.max = 22000, andInitializeAsyncraisesMaxSamplingRateto it during connect. The wrapper's rate guard (#255) andCore's own setter both validate against that figure — so the slider was the only thing in the chain
that could not ask for a rate the hardware supports.
The ceiling now comes from the device.
What the user saw, on the attached board
Both frames are the real Devices drawer rendered by the parity-audit harness with the real Nq1
connected over USB (
/dev/cu.usbmodem1101), driven through the pane's own view model: open thedrawer, ask for 5000 Hz.
Before — the wrapper stored 5000 Hz (the device advertises 22000, so its guard accepted it), but the
slider was already at its own maximum and coerced the value back to 1000. The number on screen
was not the number the user asked for, and not the one the device would have run at:
After — 5000, with the thumb a fifth of the way along a 1..22000 track:
Harness output from those two runs, same board, same session:
Non-destructive: connect, read the capability document, set the wrapper's rate, disconnect. The rate
is only written to hardware by
InitializeStreaming/StartSdCardLogging, neither of which ran, sonothing on the board was changed and there was nothing to restore. The evidence mode was a temporary
edit to
AvaloniaCapture/Program.cs, reverted — it is not in this diff.Where the ceiling lives now
AbstractStreamingDevice.MaxStreamingFrequencyis theMath.Max(1, Capabilities.MaxSamplingRate)expression that the rate setter's guard and
ClampRateToAdvertisedCeiling()each already computedseparately; both now read it, so the concept has one home instead of two copies.
DevicesPaneViewModel.MaxFrequencyHzexposes it for the selected device, and both device views bindtheir slider's
Maximumto that.SelectedTileis a sufficient trigger for the ceiling moving:ConnectionManagerpublishes a deviceonly once
Connect()has returned, andConnect()hydrates the capability document before itreturns, so no tile ever exists while the ceiling is still the bootstrap value — and a reconnect
rebuilds the tile.
MaxFrequencyHzis announced beforeFrequencyHz(the generator raises[NotifyPropertyChangedFor]in declaration order) so the slider never sees an incoming rate againstan outgoing ceiling.
One extra line in
FrequencyHz's setter: a write carrying the rate the device already holds isdropped. A
Sliderwrites itsValueback when a fallingMaximumcoerces it, and pushing thatecho through the shell would answer a change nobody made with "Cannot change sampling frequency
while logging."
Why the bound is the absolute ceiling and not
current_max_rate_hz#270 asks this to be settled explicitly, so: the absolute advertised ceiling, and the current cap
is not an option at this layer.
current_max_rate_hz(7746 on this board) is not static. Core 1.7.0 documentsCapabilityStreaming.CurrentMaximumRateHzas "the device's authoritative cap for the channel setenabled right now … It changes whenever the enabled set changes, so it is only as fresh as the last
document read." This app reads the capability document exactly once, in
Connect(). It neverre-reads it. Binding the slider to that figure would pin the control to a cap computed for whatever
channels happened to be enabled at connect — optimistic once the user enables more, needlessly
restrictive once they enable fewer.
SampleRateCap.ComputeForDevicecan also legitimately return0("nothing enabled"), and a slider whoseMaximumis 0 sits below its ownMinimumof 1.The honest statement about the hazard #270 raises: the slider's maximum is not, and cannot be, the
enforcement point for the current cap. It is unenforced at 1000 too — a board with sixteen analog
inputs enabled can have a current cap well below 1000, and today's slider offers 1000 anyway. What
this change does is let a user ask for something the firmware answers with SCPI
-222(
RateValidation: "error") where before they could only ask for something smaller that thefirmware might refuse for the same reason. Enforcement belongs beside
ClampRateToAdvertisedCeiling()at the Core handoff, and needs a fresh
ReadCapabilityDocumentAsyncwhen the enabled set changes —a device round trip on a channel toggle that nothing in this app does yet. Filed separately as #272
rather than smuggled in here.
Every
1000in the rate pathChanged — the two sliders that set a connected device's live rate:
View/Prototype/DevicesPanePrototype.axamlMaximum="1000"Maximum="{Binding MaxFrequencyHz}"Views/Mobile/DevicesMobileView.axamlMaximum="1000"Maximum="{Binding MaxFrequencyHz}"Deliberately left — the four Profiles sliders:
View/ProfilesPane.axamlSamplingFrequency(aProfileDevice)View/ProfilesPane.axamlNewProfileFrequencyViews/Mobile/ProfilesMobileView.axamlSamplingFrequencyViews/Mobile/ProfilesMobileView.axamlNewProfileFrequencyA
ProfileDeviceis an[ObservableProperty]-backedObservableObject(Models/ProfileModels.cs:27)deserialized from XML with no device behind it: a profile can name a board that is not connected, and
one profile can name several boards with different ceilings, so there is no device to read a bound
from. Making it device-derived means
ProfilesPaneViewModel— which today observes neitherConnectionManager.ConnectedDevicesnor implementsIDisposable— grows a subscription and adisposal contract at every construction site. That is machinery this change does not need.
Deferring it is safe for stored data: applying a profile assigns its saved rate through the
guarded setter (
ProfilesPaneViewModel.cs:390), which clamps to the real board's ceiling, so aprofile can never drive a device at a rate it would refuse. It is not free for editing, and
this PR widens that gap rather than creating it —
SaveCurrentSettings(:539) copies the livedevice rate into a profile unfiltered, so a profile can now hold 5000 while the edit drawer's slider
still stops at 1000. Measured headless: opening the drawer does not rewrite the stored value
(the numeric readout is
Mode=OneWayand still reads 5000), but the thumb saturates at the right ofthe track, and any drag drops the profile to ≤1000 — which drawer-close persists. Filed as #274,
alongside #272, rather than smuggled in here.
Also left, and not caps:
ProfilesPaneViewModel.cs:60and:224setNewProfileFrequency = 1000asthe default rate a new profile starts at.
AbstractStreamingDevice.DEFAULT_PWM_FREQUENCY_HZis thePWM output frequency, not a sampling rate. No new
1000is introduced anywhere.Tests
Daqifi.Avalonia.Tests/ViewModels/RateSliderCeilingTests.cs, following the patternFirmwareUploadAffordanceTestsestablished: neither device view declares anx:DataType, so theirbindings resolve by reflection at runtime and a missing or misspelled member fails silently while
both heads build green. Each view fact therefore asserts the pair a reflection binding needs — the
binding is in the markup, and the member it names exists and is readable on the view model.
Run against
origin/main's markup with the test file in place, 4 of 13 fail — both views forThe_devices_pane_takes_its_rate_ceiling_from_the_device(Maximum="{Binding MaxFrequencyHz}"notfound) and both for
Neither_devices_view_writes_a_rate_ceiling_of_its_own. The remaining 9 aredevice-level facts about
MaxStreamingFrequency; they cannot be run againstmainat all, becausethe member they name does not exist there. What they pin is that the bound the slider offers and the
bound the setter saturates at cannot drift apart, and that a zero or negative advertised ceiling is
floored to 1 rather than producing a slider bound beneath its own minimum.
Build and verification
net10.0, iOSRelease/iossimulator-arm64: both green. Android is not buildable on thisMac (workload absent) — CI covers it.
-t:Rebuild) against merge-base8438046: 171 first-party +20 vendored OxyPlot = 191 on both. Identical sets; the only textual difference is a line-number
shift of a pre-existing
CS8625inAbstractStreamingDevice.cscaused by the added lines.packages.lock.jsonunchanged (iOS restored without-r, built with it).DAQIFI_DATA_DIR): 25/25 byte-identical to the committedmacos-arm64baseline. No screen moves — the FREQUENCY slider lives inside the device drawer(
IsVisible="{Binding IsSettingsOpen}"), which no captured screen opens. No re-record needed.🤖 Generated with Claude Code