Repository navigation
RTL: apply frequency-correction edits live with confirmed readback - #6003
Conversation
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
No fixes/closes #NNNN — the governing document is RFC #5468, whose retained-work list asks to "expose PPM correction in radio settings, apply it through the existing backend transaction, and persist only accepted correction under the appropriate device scope. Preserve requested RF frequencies and handle refusal/reconnect coherently." Measured against that, the diff delivers: the entry applies through the existing ppm.set transaction, never claims applied until readback confirms, the widget-level tests pin refusal, supersession, disconnect, capability loss and backend rebind, and the two backend tests pin that adoption moves only hardware.ppm + the capture generation while receivers, RF geometry and the viewport stay fixed. No value or sign is auto-selected, as the RFC requires.
What the RFC text does not ask for is the interaction change this PR is actually about — dropping the requirement to press Apply. That is a workflow change on a control that already worked, it costs a full capture restart per applied value (RtlCaptureTransaction::submit() mints a new revision unconditionally — there is no no-change early-out), and the only authority cited for it is the operator's subjective "confirmed the PPM adjust works great" recorded in #5468. The RFC's own decision line — "Jeremy, please approve or request changes to … the scoped PPM-setting addition" — is still open. I'm not calling that a blocker, but it is a maintainer call, not a review call. See "Needs maintainer decision" below.
Mechanically this is a stacked PR: 7911324a's parent is dea570a2 (#6002, draft), which sits on #5974 → #5973 → #5924. The five-file diff above is the increment; it cannot merge until that stack does.
Scope
| File / group | What it changes | Claimed by the title/RFC? | Verdict |
|---|---|---|---|
src/gui/RtlReceiverSettingsWidget.{h,cpp} |
Debounced auto-submit, RtlPpmSpinBox Return capture, apply->setAutoDefault(false), hideEvent discard, m_submittedPpm, help text |
Yes | In scope |
tests/rtl_receiver_settings_widget_test.cpp |
Dialog-shaped fixture, real key events, debounce/burst/hide/rebind/synchronous-completion coverage | Yes | In scope |
tests/rtl_capture_worker_test.cpp |
Adds a waterfallRowReady counter and asserts PPM adoption preserves the WFM receiver + RF geometry and rejects pre-adoption frames |
Adjacent — backend hardening, no backend source change here | In scope, tangential |
tests/rtl_device_controls_test.cpp |
seed() fixture Fm→Fmn, filters -8000/8000→-6500/7500, adds gain/pan/mute/squelch; narrows the view; adds four preservation assertions |
Explained by 7911324a's message, not by the PR title |
In scope, see nit |
No CHANGELOG.md entry (correct), no new protocol verb, config key or capability field, no settings-persistence surface, no removed guard, no deleted comment naming a symptom, no dead additions — RtlReceiverSettingsWidget is reached from RadioSetupDialog.cpp:906. No socket, QTcpServer/QUdpSocket/QLocalServer/bind/listen/connectToHost or fake-peer construct appears in any added or modified test; nothing to record under the socket-test disclosure.
CodeGuard's nine CG-PATH-001 hits are all in files this PR does not touch (MainWindow.cpp, wdsp_wbfm_test.cpp:2, two vendored third_party trees). Refuted as findings against this diff.
Blockers
None.
Needs maintainer decision
- Apply-on-edit replaces Apply-on-click. Arrow keys, the spin buttons, Enter and focus-out now each schedule a hardware transaction 200 ms later, and every accepted transaction restarts the capture — the help text says so ("reception briefly reacquires after a correction"). The Apply button survives, so nothing is lost; the question is whether an RX-path interruption should be reachable from an unmodified arrow-key press on a settings page. RFC #5468 scopes the control, not this interaction, and its decision on the scoped PPM addition is explicitly still requested. Worth an explicit ruling rather than arriving as part of an implementation PR.
Nits (non-blocking)
submitPpm()can dispatch a value-unchangedppm.set—RtlReceiverSettingsWidget.cpp:226. Inline comment; the backend already publishesrequestedPpm, which the test fake now mirrors but the widget never reads.- An explicit Enter can still be silently dropped —
RtlReceiverSettingsWidget.cpp:110. Enter is a commit gesture but it only starts the 200 ms timer; closing the page inside that window discards it. Inline. hideEventalso fires for window-system hides —RtlReceiverSettingsWidget.cpp:211. Inline.- Debounce-vs-
pumpFortiming margins in the new widget test —tests/rtl_receiver_settings_widget_test.cpp:166. Inline. tests/rtl_device_controls_test.cppmutates aseed()fixture that pre-existing assertions in the same file depend on. I read the assertions ahead of the change: they cover device writes, the restored-PPM snapshot anddisplayPeakon the display path, none of which theFm→Fmn, filter-width, oraudioMute/squelch changes touch, and the added comment explains why the narrowed view retains the DC bin. Clean as far as I can tell — flagging only so the fixture change is visible as a deliberate one.tests/rtl_capture_worker_test.cpp:197adds a second test consumer ofIRadioBackend::waterfallRowReady. Open PR #5978 proposes deleting that signal as "an outlet nobody connected". Purely a merge-order note — whichever lands second needs a touch-up.
What I tried to break
- Request-identity races. Submit
R1, edit again, let the debounce mintR2whileR1is in flight: theid != m_ppmRequestguards in both the result and error lambdas dropR1's late completion and the backend's "superseded" refusal, so no stale reason reachesm_errorand no stale value settles the entry. Held.m_bindingGeneration+ the capturedQPointersource close the same hole across a backend swap. - Reentrancy.
submit()assignspendingbeforeinvokeBackendExtension, so a synchronousextensionResultsees the correctm_ppmRequest, zeroes it, and the reentrantqueryStatus()→acceptStatus()runs underQSignalBlocker, so novalueChangedre-arms the timer. The test'ssynchronousPpmmode exercises exactly this. Held. m_ppmEditedclobbering. Walked confirm-while-newer-edit-pending, refuse-while-newer-edit-pending, refuse-then-Apply-retry, and reverting to the confirmed value while a different value is in flight. In each case the applied label kept the confirmed value and the entry kept the operator's newer number. Held.- Lifecycle. Disconnect, reconnect, capability loss and restore, backend replacement and widget destruction all stop the timer (
bindBackend()first line; theQTimeris parented tothis). Stalem_submittedPpmis only read whilem_ppmRequest != 0, whichbindBackend()clears. Held. - The Return override.
QAbstractSpinBox::keyPressEventdeliberatelyignore()s Enter soQDialog::keyPressEventcan click the autoDefault button —RadioSetupDialog.cpp:1100builds exactly the Close-onlyQDialogButtonBoxthe test mirrors, so without this override Enter would have closed the dialog. Thecheck(dialog.isVisible(), …)assertion is mutation-sensitive (key propagation to the parent happens inQApplication::notify, sosendEventreproduces it). Held. - Aggregate-init trap in the changed fixture.
{…, Mode::Fmn, 37, 68, true, true, 43}maps toaudioGain/audioPan/audioMute/squelchEnabled/squelchLevel, leavingwfmDeemphasisUsat its defaulted 75 — had it landed onwfmDeemphasisUs=43,submit()'s 50-or-75 validation would have rejected the whole fixture. Correct as written. - Could not check: anything requiring a build or a run. Both hand-checked failure modes below are reasoned from the code, not reproduced. Worth noting that CI on
7911324ahas only two completed checks — "Static checks" and "Sanitizer option configures". No compile job and noctestran, andrtl_receiver_settings_widget_testis not in anyctest -Rfilter inci.yml, so nothing in the green badges shows the new tests build, let alone pass.
Recommendation
Needs maintainer decision. The engineering is careful — the confirmed-readback discipline, request fencing and lifecycle handling all survived the attacks above, and the widget test is unusually honest about non-events. What needs a ruling is the interaction itself: auto-apply makes an RX-path capture restart reachable from a bare arrow-key press, and RFC #5468's decision on the scoped PPM addition is still outstanding with only a subjective operator report behind it. Next step: get that ruling on the auto-apply policy, fold in the requestedPpm comparison so a no-change submit cannot restart the capture, and get a real build+test run on this head before anyone reads the checks as green.
Thanks for the thorough test work here — the hidden-page, rebind and synchronous-completion cases are the ones that usually go untested, and they are all pinned.
🤖 aethersdr-agent · cost: $6.4046 · model: claude-opus-5
| if (m_ppmRequest && requested == m_submittedPpm) { renderStatus(); return; } | ||
| if (!m_ppmRequest && !m_confirmed.value(QStringLiteral("pending")).toBool() | ||
| && requested == m_confirmed.value(QStringLiteral("ppm")).toInt()) { | ||
| m_ppmEdited = false; | ||
| renderStatus(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This no-op guard only applies when the backend reports pending == false, so when something else is mid-transaction the widget submits a ppm.set whose value equals the applied value.
Reachable path: click Suppress IQ DC (m_dcRequest set, backend publishes pending: true), then nudge the ppm arrows up and back down. The debounce fires with m_ppmRequest == 0, pending == true, requested == confirmed → a ppm.set goes out. RtlSdrBackend::requestCapture() → RtlCaptureTransaction::submit() has no no-change early-out: it mints a new revision unconditionally, so that costs a full capture restart and the reacquire the help text warns about, for a value that did not change.
The information needed to avoid it is already on the wire — deviceSettingsStatus() publishes requestedPpm (RtlSdrBackend.cpp:2023), and this PR even adds that key to the test fake's accepted map, but the widget never reads it. Comparing against the backend's own latest-requested value would make the guard work while pending is true, and would drop the client-side m_submittedPpm shadow in favour of the radio-authoritative field (Principle II/III shape). Reasoned from code; I have no build here.
| connect(m_ppm, &QSpinBox::valueChanged, this, [this] { | ||
| if (!isVisible() || !m_haveState) { return; } | ||
| m_ppmEdited = true; | ||
| m_ppmTimer->start(); |
There was a problem hiding this comment.
Enter is an explicit commit gesture — the help text added above says the correction applies "after … a completed entry (Enter or focus change)" — but it only starts the 200 ms timer. Press Enter and close the page (or switch settings pages) inside that window and hideEvent() stops the timer and reverts the entry, so the committed number is silently discarded with no indication.
The debounce exists to coalesce arrow/spin bursts; Enter and focus-out are already single, deliberate, terminal gestures. Flushing them immediately (connect QAbstractSpinBox::editingFinished to submitPpm, or call it from the RtlPpmSpinBox Return branch) would keep the coalescing where it is needed and make the help text literally true.
| // Discard only unsent input. An accepted in-flight correction is not a | ||
| // preview and is still allowed to complete and persist after the page closes. | ||
| const QSignalBlocker block(m_ppm); | ||
| m_ppm->setValue(m_ppmRequest ? m_submittedPpm : m_confirmed.value(QStringLiteral("ppm")).toInt()); |
There was a problem hiding this comment.
hideEvent is being used as "the operator navigated away from this page", but it also fires for window-system-driven hides — hiding the application (macOS ⌘H) or platform-driven hides of the dialog window send hide events down to visible children. In that case an in-progress, not-yet-debounced edit is silently reverted to the applied value while the operator believes the dialog is merely behind something.
If you want to keep the discard scoped to a real page/dialog change, event->spontaneous() distinguishes them: programmatic setVisible(false) (stacked-page switch, QDialog::close) delivers a non-spontaneous hide, while a platform hide delivers a spontaneous one. I have not run this, so worth confirming on the platforms you can drive before changing anything.
| ppm->setValue(30); pumpFor(40); | ||
| ppm->setValue(31); pumpFor(40); | ||
| ppm->setValue(32); | ||
| check(source->controlRequests == beforeBurst, "rapid PPM edits do not dispatch an intermediate value"); |
There was a problem hiding this comment.
This asserts a non-dispatch across two pumpFor(40) gaps against a 200 ms debounce, so it depends on the process not being starved for 200 ms between the setValue calls. pumpFor busy-loops with processEvents(AllEvents, 10) + msleep(1), which is fine on an idle machine, but the full suite runs ctest in parallel and a 200 ms scheduling stall on a loaded runner turns this into a red test on correct code.
The several pumpFor(260) waits have only a 60 ms margin over the same 200 ms interval, in the other direction.
A test-only seam for the interval (or driving the timer explicitly) would make the burst-coalescing assertion deterministic rather than margin-dependent. Non-blocking — flagging it as a flake risk, not a defect in the feature.
|
Cisco CodeGuard — static analysis of this PR (9 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: $6.5366 · model: claude-opus-5 |
|
Antares Detector — candidate vulnerable file(s)
The backend parses and validates PPM correction values from QJsonDocument; if the backend implementation copies or manipulates those values into buffers (or otherwise uses untrusted input) without bounds checks, it can lead to out-of-bounds writes or use-after-free. Localized by Cisco Foundation AI Antares-1B running locally in the AetherClaude sandbox, seeded by the Cartographer security map. Advisory only — please verify before acting. 🤖 aethersdr-agent · cost: $6.5366 · model: claude-opus-5 |
|
Thanks for the careful write-up on this one — the failure is a stale generated file, not a problem with your code. What failedOne step, in Static checks → aetherd touchpoint manifest ( "Sanitizer option configures" passed. The other run on this SHA ( FixOne line in -| `core/backends/RadioCapabilities.h` | 10 | universal — Backend-neutral capability descriptor …
+| `core/backends/RadioCapabilities.h` | 11 | universal — Backend-neutral capability descriptor …I ran that at Why it drifted — worth a look, because it will come backNot a missed regeneration on your part. The manifest row is a count of UI includers (
A note on bisecting this if you try it yourself: the signal is non-monotonic, so The upstream branches need it tooI checked the stacked heads, and regenerating only here will re-conflict on the next merge down:
Regenerating in #5973 and #5974 as well should settle it for the whole stack. On the inline review commentsThe four bot comments on Nothing here needs a rebuild — just the regenerated manifest. Nice work on the PPM fencing. 🤖 aethersdr-agent · cost: $7.2366 · model: claude-opus-5 |
b6f87a9 to
2602312
Compare
9c50425 to
5e39790
Compare
Review moved to #6082
During separation of Digital FM into RFC #6079, branch ancestry was changed before PR bases were retargeted. GitHub automatically recognized this head inside the Digital branch and marked this PR merged there, then deleted the branch. This was not a merge into main. I restored the branch and created #6082 against the correct analog base. That replacement is Ready for review; original review discussion and historical evidence remain here.
Current analog order: #5924 → #5973 → #5974 → #6082 → #6083. Draft Digital #6002 follows #6083 and is no longer an analog dependency. The previous full integration remains preserved at
codex/rtl-digital-evaluation-preserved-20261001.Prior PR description, retained as historical context
Summary
RTL frequency correction applies completed arrow/keyboard edits after a 200 ms debounce, allowing visual PPM adjustment while receiving. Apply submits immediately; Enter commits the field without activating the parent dialog's Close button. Requested values stay separate from confirmed hardware readback and accepted per-device persistence.
Newer edits survive older completion/refusal. Rejected values are not automatically retried. Closing or hiding the page cancels unsent edits; accepted in-flight correction may finish. Disconnect, capability loss and backend replacement cancel deferred work without replaying it into another session. No new queue, thread, direct USB path, automatic calibration value or synthetic spectrum shift is introduced.
Current head:
9c50425e533d5a3c417a8945d4419e36df53a9ff, stacked on #6002. Reconciled withmainat68cdf661. The latere4eb2ffeupdate changes audio-pipeline documentation only. This requests human review and preserves the maintainer decisions below; it does not authorize a merge or close RFC #5468. The optional decoder remains a separate draft dependency; this PR cannot land independently of that base as currently stacked.Validation
The native Apple Silicon/Qt 6.12 downstream aggregate builds with the optional decoder OFF. Its real-keyboard/widget regression and injected capture-worker/model tests pass, including stale responses, refusal, coalescing, synchronous completion, accepted-state geometry and PPM preservation. The worker test follows upstream's single spectrum outlet, while model tests retain actual waterfall-row coverage. The earlier exact
7911324aNobara checkpoint passed 63 selected tests and the automatic-apply mutation check; that historical evidence is not relabeled as a new exact-head run.The operator explicitly confirmed live PPM adjustment, and reports days of stability on the later delivered
7601593fbuild. This does not measure absolute calibration or reacquisition timing on the reconciled head.Maintainer decision
Apply-on-edit versus Apply-on-click remains an explicit UX decision under RFC #5468. Existing nonblocking review notes about no-change requests during another pending transaction, Enter followed immediately by hide, and window-system hides remain disclosed; this readiness request does not mark them resolved. No blocking defect was reported in the existing PPM review. Principles II, V/XIV and XI govern readback, persistence and verification.
73, Ozy K6OZY · Codex