Repository navigation
Conversation
The waterfall rate was stored and restored for any scope with hasRadioIdentity(), which is true for the anonymous scope RadioModel gives a connected RTL dongle that reports no serial. That scope's row is the family-wide one, so the rate one serial-less dongle left behind was applied to the next serial-less dongle. FFT averaging, in the same document, already refused that scope. Both now ask one predicate, ClientDisplaySettings::keepsFor(): an exact radio id. An anonymous radio's rate lasts for the session, is never written to the family-wide row and never read from it. A family-wide row an older build wrote is left in place, unread. Radios with an identity are unchanged. FFT FPS and the dBm range used the wider predicate too and now ask the same one. No backend that can be anonymous owns either today (RTL declares neither), so that part changes no behaviour. The store's `shapedLocally` parameter is `clientOwns`, as on the averaging pair (the rename aethersdr#6402 deferred). Refs aethersdr#6185. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed against the head checkout (5e79f7ac0). The implementation is clean and the body is unusually honest — nearly every finding I went looking for was already disclosed in it. The substance of this review is therefore not the code but the ruling the code is waiting on.
1. Issue fit
Yes, for the part it claims. #6185 asked for the capability shape (declared owner vs "not a Flex"); that half is already on main (panWaterfallRateShaping / clientPersistsWaterfallRate). This PR takes decision (b) from the triage comment on #6185 — hasRadioIdentity() vs radioId().isEmpty() — and implements the averaging predicate, plus the shapedLocally → clientOwns rename #6402 deferred. Every requirement maps to a hunk: the predicate (ClientDisplaySettings.h:45-48), the four store gates, the four scheduling sites, the rename, the docs.
One correction of the triage, and the author is right. The triage said a serial-less dongle's rate "becomes every rtl dongle's default." It does not. I grepped for every reader of the ClientDisplay feature name — there are none outside ClientDisplaySettings.h, and the store reads exclusively through scope.featureExact(); the only four callers of the fallback-chain reader scope.feature( are Hl2HardwareOptions.cpp:55, Hl2FreqCal.cpp:70, AnanDroopCalibrator.cpp:91 and RadioStateMemory.cpp:70, all different features. So no dongle with a serial could ever read that row. The triage overstated the harm; the body's "Limits, first" has it correctly.
That matters for the verdict: with the cross-dongle harm not existing, what is left is a consistency choice that removes working persistence for the serial-less case, with no reported user defect behind it. See §6.
2. Scope
Diffed against the merge base 91ddeb87f: exactly the 8 files listed, 117 insertions.
| File / group | Change | Claimed? | Verdict |
|---|---|---|---|
src/core/ClientDisplaySettings.h |
keepsFor() + 5 gate swaps + param rename |
Yes | In scope |
src/gui/MainWindow_Wiring.cpp |
4 scheduling-site gate swaps | Yes | In scope |
tests/client_display_settings_test.cpp |
+63, anonymous waterfall-rate behaviour | Yes | In scope |
tests/client_display_pan_settings_test.cpp |
+23, anonymous FPS/dBm; source-text literals updated | Yes | In scope |
docs/HERMES.md, radio-capabilities-map.md, rtl-m1-runtime.md |
invariant paragraphs | Yes | In scope |
docs/user/docs/rtl-sdr.md |
user-visible sentence | Yes | In scope |
Everything in the diff is explained by the issue. Two widenings beyond the title, both volunteered in the body and both inert today: FFT FPS and the dBm range now use the same helper, and RTL — the only family that can produce an anonymous scope (RadioModel.h:1341-1344) — declares panFrameRateShaping = nullopt (RtlSdrBackend.cpp:178) and clientPersistsDbmRange = false (:176), so clientOwns is already false on both paths there. No new public surface, no removed guard, no CHANGELOG.md edit. CI on 5e79f7ac0: 7 checks, all success (one release job correctly skipped).
3. Blockers
None.
4. Nits (all non-blocking)
-
src/core/ClientDisplaySettings.h:127and:145— the two sentences the PR adds overstate the invariant by one function. The header comment says:// Every table here is kept per exact radio. An anonymous scope addressesand
docs/HERMES.mdsays "Every table in the document is kept per exact radio, through the one predicateClientDisplaySettings::keepsFor()." ButshowTxInWaterfall()/saveShowTxInWaterfall()in the same file still gate onscope.hasRadioIdentity(), which is true for an anonymous scope. The body discloses this and the reasoning holds —txWaterfallClientFlagis declared only byHl2Backend.cpp:2133(RTL is explicitlynulloptatRtlSdrBackend.cpp:206), and an HL2 scope is never anonymous — so it is inert, not a live bleed. The issue is only that a future reader of the header, or of HERMES, will not know the exception exists; "table" is doing load-bearing work that the surrounding prose does not acknowledge. Either route the pair throughkeepsFor(behaviour-identical today) or name the exception in the comment. Independently surfaced by security-audit (CodeGuard) as the one invariant gap in the diff. -
Mutation coverage is honestly short of the diff. The table covers the waterfall-rate save and restore and a blanket "helper always refuses", and the body says the new FPS and dBm-range checks have no mutant of their own — so reverting
readEntry/writeEntryindividually is not shown to fail anything. Given those paths are inert for every real backend, this is worth naming, not worth withholding a merge for. -
docs/user/docs/rtl-sdr.md:81— the new sentence states the new behaviour but not the one-time consequence for an existing user: a serial-less dongle that already saved a WtrFall Rate loses it once on upgrade, and the orphaned row stays in the store forever. The paragraph directly below is already an "Upgrading from an older version" note for the USB-position → serial change, so the precedent for disclosing exactly this is two lines away.
5. What I tried to break
- Save/restore asymmetry — the dangerous shape would be a value readable but not writable (or the reverse), stranding or resurrecting state. All four restore sites (
MainWindow_Wiring.cpp:3259,:3362,:3370,:3379) route through the store accessors, and all four writers (:3189,:3212,:3244,:3280) through the save functions; every one of the eight now passes the same predicate. Symmetric. The scheduling-site gates are belt-and-braces on top of the store's. - Sibling call sites left behind — grepped
hasRadioIdentity()acrosssrc/. Of the remaining users, only the two inClientDisplaySettings.htouch this document (the nit above);BandStackSettings.cpp:385,400,PskReporterMapDialog.cpp:1678,1726,RtlSdrBackend.cpp:1345and theRxApplet.cppsquelch sites are different features with their own policies. - The anonymous-scope producer —
RadioModel.h:1341-1344creates one for exactly one case: familyrtl, empty serial, and connected. So a disconnected model yields a non-anonymous empty-ID scope, which both the old and the new predicate refuse. No change there, and no other family is affected by any hunk in this PR. - The pre-existing family row — the test asserts it is byte-identical (
featureExact(feature) == older) andFeatureReadStatus::Presentafter an anonymous save attempt. It is read back from the store, not from the in-memory model. - Do the new tests discriminate? Walked the mutation by hand: restore
hasRadioIdentity()insaveWaterfallRateand the anonymous save writes the family row, sofamilyRow(&status).isEmpty() && status == Missingfails; restore it inwaterfallRate()and the existing-family-row check returns 41 instead of nothing. They are not asserting the implementation back to itself. The exception is theclient_display_pan_settings_testsource-text literals, which are string matches on the wiring and prove only that the text was updated in step — the body says so, and no test constructsMainWindow. - Could
keepsForadmit an invalid scope? It does not checkisValid(), but a non-emptyradioIdwith an empty family reachesfeatureExact, which returnsFeatureReadStatus::Unavailable, and every writer refuses onUnavailable(:174-178,:286-290). No write path to an invalid row. - CodeGuard leads — all refuted. The three
ClientDisplaySettings.hCS-VLN-CMDI-BACKTICKhits are Markdown backticks in doc comments (:14is "the schema-1 `ClientDisplay` document"); 15 of the 17 flagged files do not intersect this diff at all, which is the whole-file-scan behaviour, so nothing inTciServer.cpp,TciProtocol.cpporrules.jsonis this PR's doing. The audit discharged those categories lexically (no shell invocation, no outbound sink, no credential literal); I am reporting that it ran and what it found, not that those files were reviewed as part of this change. - No socket-owning test is added or removed by this PR.
- What I could not check: nothing was built or run — every finding above is reasoned from the head checkout's source, not reproduced. The
MainWindowwiring has no behavioural coverage in any lane, as the body states, and the serial-less-dongle path has not been exercised on hardware by anyone.
6. Recommendation
Needs maintainer decision — on ruling (b), exactly as the author asks.
The code is a correct, well-tested implementation of the averaging-predicate direction, and the scope is clean. But the choice itself is not a defect fix, and it is worth putting the alternative in front of you plainly. RadioSettingsScope.h:27-33 documents anonymousRadio() as "explicit provenance for a connected radio whose identity policy chooses the family row" — hasRadioIdentity() returning true for it is the designed semantics, not an oversight. And RtlSdrBackend.cpp:1345 gates the dongle's own operating state on hasRadioIdentity(), with the user-doc line immediately above this PR's addition saying serial-less dongles "share one family-wide entry". So after this change a serial-less dongle persists frequency, mode, passband, sample rate and RF gain family-wide while keeping WtrFall Rate for the session only. That is defensible, but it is a divergence, and the opposite ruling — flipping fftAverage/saveFftAverage to hasRadioIdentity() and revisiting the pinned expectation at client_display_settings_test.cpp:58 — would unify in the other direction and keep persistence instead of removing it. Either way the two shipped predicates stop disagreeing.
Concrete next step: rule on (b). If the averaging predicate is the policy, this is ready to merge once the three nits are addressed (the header/HERMES exception being the only one I would ask for before merge). If hasRadioIdentity() is to stay for the waterfall rate, close it as the author offered — and the rename and the keepsFor helper would still be worth keeping in a smaller PR, since they are good independent of the ruling.
Thanks for building it as a judgeable diff and for the "Limits, first" section — the disclosure of the showTxInWaterfall exception, the inert FPS/dBm widening and the orphaned row is what made this review short.
🤖 aethersdr-agent · cost: $12.5061 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (327 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $12.6629 · model: claude-opus-5 |
The header and HERMES said every ClientDisplay table goes through keepsFor(); showTxInWaterfall still gates on hasRadioIdentity(). Name it. The RTL-SDR page now says what an upgrade does to a serial-less dongle's saved WtrFall Rate. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks for the review. Two of the three nits are addressed in
Not addressed: the FPS and dBm-range checks still have no mutant of their own, as the body says. Full ctest at The ruling on (b) is yours. If 73, ON8ST |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
The implementation matches the remaining identity-policy proposal in #6185; the capability-ownership work already landed in #6402. I found no PR-specific correctness blocker. Recommendation: needs maintainer decision on whether serial-less RTL dongles retain family-wide waterfall-rate persistence or keep display settings for the session only. The issue thread contains no maintainer ruling on that choice, and this PR explicitly asks for one. The decision is anchored inline at ClientDisplaySettings.h:49.
Scope
| File/group | Change | Claimed in body? | Verdict |
|---|---|---|---|
src/core/ClientDisplaySettings.h |
Shared exact-radio predicate, store gates, parameter rename | Yes | In scope |
src/gui/MainWindow_Wiring.cpp |
Four scheduling guards | Yes | In scope |
tests/client_display_settings_test.cpp, tests/client_display_pan_settings_test.cpp |
Anonymous-scope refusals, identified-radio controls, source-text pins | Yes | In scope |
docs/HERMES.md, docs/architecture/radio-capabilities-map.md, docs/rtl-m1-runtime.md, docs/user/docs/rtl-sdr.md |
Policy, behavior, exception, upgrade consequences | Yes | In scope |
Everything in the diff is explained by the issue and the disclosed follow-up scope. The FPS/dBm widening is inert for today's RTL backend: it declares neither client ownership. No unrelated changes, new settings keys, default-value changes, or changelog edits.
Blockers
None found in the implementation. The identity-policy ruling remains a maintainer decision, rather than a code defect to correct. GOVERNANCE.md states: “The Project Maintainer is the sole authority on visual design and UX direction.” This choice removes existing persistence for serial-less dongles; it should be accepted explicitly rather than inferred from the earlier triage bot's recommendation.
Nits / coverage boundaries — non-blocking
The MainWindow scheduling sites remain source-text checks (inline at MainWindow_Wiring.cpp:3183). The actual serial-less RTL lifecycle was not exercised on hardware, and the demo cannot reach an anonymous RTL scope. I am not requesting a synthetic firmware peer.
The header/HERMES exception and RTL upgrade note raised in the earlier review are addressed at this head. I also closed the earlier mutation-evidence gap for the shared FPS/dBm paths below.
Verification and attacks
Reviewed head 553f2a0dbe3220fda50330a97428103b9fc82863. Built the merge result with fetched main ac2a6c729b5d578d6d95fab9db9c97269344b12c; the constructed, uncommitted validation tree was 1daa22e0c4b581ac80c94d211f364acaef93dcb5 (GitHub merge ref edab28707 initially used main 86f592a9).
- Full macOS ARM64 application build passed using the dedicated ARM64 toolchain/build directory, with RADE enabled. Host/system processors and executable were ARM64, with no RNNoise x86 sources in the graph.
ctest --test-dir build-codex-arm64 -R '^client_display_(pan_)?settings_test$' --no-tests=error --output-on-failure: 2/2 passed against the final validation tree. The changed targets were inspected first; neither opens/binds/connects sockets or runs a synthetic peer.- Independently reversed four store guards in scratch header copies. All mutants exited 1: waterfall save produced 3 failed checks; waterfall restore 1; shared FPS/dBm write 1; shared FPS/dBm read 1. The unmodified controls passed. These tests discriminate the intended behavior.
- Settings, engine-boundary, capability-record, test-registration, and touchpoint-manifest checks passed. Tested refusals, existing family rows, identified-radio/pan isolation, invalid values, newer schemas, and preservation of adjacent document fields.
- Required hosted CI is green on the unchanged PR head, and all three commits have verified signatures. CI is a
pull_requestworkflow; I did not run the broad local suite. No separate model-invocable automated reviewer was available; I performed a second lifecycle/failure-path pass. The reported CodeGuard backtick hits inClientDisplaySettings.hare comments, with no shell sink; the other cited source paths are outside this diff.
The isolated, offscreen demo (DEMO-0001, model AetherSDR Demo) exercised the actual Display controls with TX pinned off and an explicit review socket. Rates 1, 100 and 63 read back correctly; setting 63 twice also held. Example bridge output:
{"model":"display","ok":true,"pans":[{"objectName":"","panIndex":0,"wfLineDuration":63}],"property":"wfLineDuration"}Runtime limitation: after the normal Quit action, SQLite held (sim, DEMO-0001, ClientDisplay, schema 1, {"waterfallRates":{"0":63}}), but a new demo process settled at wfLineDuration: 100. A baseline comparison reproduced 100 from the same saved value using clean current-main sources for both affected translation units, relinked with unchanged objects. I therefore do not attribute that discrepancy to this PR and do not claim successful end-to-end restart persistence. Normal close handling also reached unrelated RTL-discovery teardown and waited there; only review-owned processes were terminated for cleanup. No live radio was connected and no TX action was performed.
Concrete next step: @ten9876, rule on the identity policy. If session-only anonymous display settings are intended, the implementation and focused regression coverage support that direction. If family-wide waterfall-rate persistence should remain, retain it and split out the helper/rename as the author offered.
| // only the HL2 declares it, whose scope is never anonymous. | ||
| static bool keepsFor(const RadioSettingsScope& scope) | ||
| { | ||
| return !scope.radioId().isEmpty(); |
There was a problem hiding this comment.
Needs maintainer decision — anonymous-radio persistence. This predicate deliberately removes existing waterfall-rate persistence for a connected serial-less RTL dongle. The previous family row was read only by other anonymous scopes: the identified-radio readers use featureExact(), so dongles with serials never inherited it. RadioSettingsScope::anonymousRadio() explicitly represents a connected radio whose identity policy chooses the family row; its hasRadioIdentity() result was intentional. The issue thread has no maintainer ruling on switching this feature to session-only storage. @ten9876, please choose the identity policy before landing. I found no implementation blocker in the chosen direction; the store tests and independent guard mutations support it.
| const RadioSettingsScope scope = m_radioModel.settingsScope(); | ||
| const bool clientOwns = clientPersistsWaterfallRate(); | ||
| if (!clientOwns || !scope.hasRadioIdentity() || panIndex < 0) { | ||
| if (!clientOwns || !ClientDisplaySettings::keepsFor(scope) || panIndex < 0) { |
There was a problem hiding this comment.
Non-blocking coverage boundary. The store refusals are behaviorally covered and mutation-checked, but this scheduling guard and the sibling MainWindow sites remain source-text checks. The demo exercised the identified-radio controls; it cannot exercise the anonymous RTL scope. The real serial-less dongle lifecycle therefore remains unverified. No synthetic firmware peer is requested; keep that limit explicit in the validation claims.
|
On your restart note (demo back at 100 with 63 saved): the HL2 does not show it. Setup: a real HL2 in receive-only mode, an isolated profile, and an app built from 73, ON8ST |
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Not accepted: this change removes existing persistence through shared display/UI policy to accommodate an RTL-specific identity limitation. The operator has explicitly rejected that approach and requested that this PR be closed.
The original capability-ownership work from #6185 already landed in #6402. The remaining issue is an inconsistency: a connected serial-less RTL dongle has an intentionally anonymous settings scope; waterfall rate currently accepts that scope and persists to the family-wide RTL row, while FFT averaging requires an exact radio identity. This PR resolves that inconsistency by removing waterfall-rate persistence. That is not the accepted direction.
Scope
| Files | Change | Assessment |
|---|---|---|
src/core/ClientDisplaySettings.h |
Introduces an exact-identity predicate for display tables and changes waterfall/FPS/dBm store guards | Shared persistence-policy change; rejected approach |
src/gui/MainWindow_Wiring.cpp |
Applies that predicate at four shared scheduling sites | Carries the RTL identity limitation into shared UI persistence policy |
| Two client-display settings tests | Assert anonymous-scope persistence refusal and pin wiring text | Cover the proposed policy, but cannot establish that removing persistence is acceptable |
| Four documentation files | Describe session-only anonymous display settings and upgrade consequences | Disclosed, consistent with the proposal |
Blocker
src/core/ClientDisplaySettings.h:47,src/core/ClientDisplaySettings.h:105,src/core/ClientDisplaySettings.h:168,src/gui/MainWindow_Wiring.cpp:3183— preserve persistence; address identity at its owner.keepsFor()requires a nonemptyradioId(). Applying it in the store and MainWindow prevents a serial-less RTL dongle from saving or restoring its waterfall rate, including a rate it already persisted before upgrading. Removing working persistence through shared UI/display guards is not an acceptable solution to missing RTL device identity. The replacement must preserve existing persistence and address RTL identity within its discovery/backend ownership, without adding RadioModel seam coupling, changing core UI behavior, or affecting Flex/Icom behavior. A USB-port-based fallback is only a possible separate design choice; none is approved by this closure.
The earlier claim that the anonymous row becomes every RTL dongle's default was overstated: these readers use featureExact(), so dongles with serial numbers do not inherit it. This PR therefore removes working behavior without fixing that claimed cross-device leak.
Verification and limits
Reviewed head 553f2a0dbe3220fda50330a97428103b9fc82863. The changed guards differ only for anonymous scopes; source inspection found that RadioModel currently produces those only for connected serial-less RTL dongles. No current Flex/Icom behavior regression was found, and no RadioModel, backend interface, Flex/Icom backend, control, or layout is changed. The blocker is the chosen shared persistence policy and its loss of RTL functionality, not a claim that all radios presently lose persistence.
Built client_display_settings_test and client_display_pan_settings_test successfully on macOS ARM64 with Qt 6.12 using -j22; working tree clean. Tests not run. Head engine-boundary, capability-record, command-plane, touchpoint-manifest, test-registration, and QSettings checks passed (engine-boundary retains tracked legacy warnings). The changed tests were inspected and add no socket-owning behavior. Hosted required checks and Docs CI are green on this head; all three commits have verified signatures. No app/bridge or live-hardware validation was performed, and no runtime or RF claim is made. GitHub reports the PR mergeable, so no rebase was needed. No separate automated reviewer was invoked.
Closing as not accepted at the operator's request. No merge or code changes.
Fixes #6185
Ruling (b) from the triage on #6185, built so it can be judged on a diff, plus the parameter rename #6402 deferred. No ruling was given on (b); if
hasRadioIdentity()is to stay for the waterfall rate, close this.Limits, first
rtldongle's default". The store reads withfeatureExact, so the family-widertlrow a serial-less dongle wrote was only ever read back by serial-less dongles. A dongle with a serial never saw it. What changes: a serial-less dongle's WtrFall Rate now lasts for the session, as its FFT averaging already does.hasRadioIdentity()too. They now use the same helper. No behaviour changes for them today: the only backend that can be anonymous (RTL-SDR) declares neither. Say so if you want that split out.showTxInWaterfall/saveShowTxInWaterfallstill gate onhasRadioIdentity(). Only the HL2 declares that flag, and an HL2 scope is never anonymous.rtlrow is left in place. It is never read and never rewritten by an anonymous scope, and it is not deleted.MainWindowsave and restore sites are covered by source-text checks only. No test constructsMainWindow.Change
ClientDisplaySettings::keepsFor(scope): one predicate, "this scope names an exact radio", used by FFT averaging, the waterfall rate, FFT FPS and the dBm range, in the store and at the four scheduling sites inMainWindow_Wiring.cpp.waterfallRate()/saveWaterfallRate():shapedLocallyis nowclientOwns, matching the averaging pair.docs/user/docs/rtl-sdr.md(user-visible),docs/HERMES.md,docs/architecture/radio-capabilities-map.md,docs/rtl-m1-runtime.md.Unchanged for every radio with an identity: HL2, ANAN, Icom, a dongle with a serial, the demo radio (serial
DEMO-0001). Flex stays radio-owned.Tests
client_display_settings_testandclient_display_pan_settings_test, isolated settings profile.Mutation check, scratch copies, at
74fa45a6:client_display_settings_testclient_display_pan_settings_testhasRadioIdentity()againhasRadioIdentity()againThe new FPS and dBm-range anonymous checks have no mutant of their own.
Full ctest at
5e79f7ac(74fa45a6merged withmain91ddeb8;-j8): 784 of 785 passed;vkamp_connection_testfailed and passed on the serial rerun (its known race, #5254). At that commitcheck_test_registration,check_qsettings,check_engine_boundary,check_capability_records(all--strict) andgen_touchpoint_manifest.py --checkpass.check_a11yonMainWindow_Wiring.cppat74fa45a6: 0 findings.73, ON8ST
🤖 Generated with Claude Code