feat(agc): AGC-T noise-floor calibration tool (engine + panel)#3350
Conversation
Adds a guided tool to calibrate a slice's AGC-T value against the receiver noise floor, so operators can find the right value per band without guesswork. Why - The AGC-T knob is a 0-100 value whose dBm mapping the radio firmware never exposes (v4.2.18), so the "perfect" value can only be found empirically. - The RF noise floor does not move when AGC-T moves; the only signal that bends at the AGC knee is the post-AGC audio level. The tool observes that. - Mode matters: with AGC on, AGC-T is the `agc_threshold` knee (set just above the noise floor); with AGC off, it is `agc_off_level`, a fixed gain with no knee, so we solve for a comfortable noise level instead. What - core/AgcTCalibrator: headless QObject engine. Auto-sweep and manual recording share one engine. Knee detection (max-distance-from-chord on audio-RMS-vs- threshold) for AGC-on; target-level solve for AGC-off. Quiet-spot guard from S-meter vs measured noise floor. Restores the original value on Stop. The radio stays authoritative -- values are only ever set via SliceModel setters. - gui/AgcCalibrationDialog: PersistentDialog (frameless + geometry-persist per project convention) with a live AgcCurveWidget plot (audio noise vs AGC-T), Auto-Sweep-first plus Apply/Stop, AGC-off target control, and a quiet-spot status line. - Entry point: right-click the AGC-T slider in RxApplet (calibrateAgcTRequested -> MainWindow::showAgcCalibrationDialog), with a tooltip discovery hint. MainWindow supplies the per-slice noise floor via spectrumForSlice(). Design decisions and the full rationale are documented in docs/agc-t-calibration-design.md (surface choice, auto-vs-manual, entry point, per-band recall, and the two flagged-tension mitigations). Deferred to follow-up PRs to keep this reviewable: per-band suggest chip and an ambient noise-floor line on the panadapter. Not yet exercised on-air (no radio in the build env); sweep timing and knee detection are verified by construction. Builds clean (Qt6/C++20, RelWithDebInfo). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Thanks for this — really nicely scoped and documented PR. The design doc captures the load-bearing distinction (knee-find vs target-solve, audio-domain observable, no exposed dBm mapping) clearly, and the engine/panel split keeps the surface reviewable. A few notes:
Functional — levelChanged tap is ambiguous
AudioEngine::levelChanged(float rms) is emitted from multiple points in the pipeline: raw pre-NR plus after each NR stage (src/core/AudioEngine.cpp:1628-1673). Your design doc flagged this and prescribed either selecting the raw tap or freezing NR/AF for the sweep — but the implementation does neither. With NR active, the dialog will accumulate samples from different taps in the same EMA, and NR-reduced noise after the AGC stage will mask the very knee you're trying to find.
Cheapest mitigations:
- Add a one-line warning to
m_hintLabelwhen the slice has NR on ("Disable NR for accurate calibration"), and/or - Add a precondition in
startAutoSweep()that refuses while NR is engaged, mirroring the quiet-spot guard.
Either keeps the engine honest without the larger refactor of adding a raw-tap output to AudioEngine.
Minor — raw SliceModel* instead of QPointer<SliceModel>
Both AgcTCalibrator::m_slice and AgcCalibrationDialog::m_slice hold raw pointers. Other dialogs that retain a slice (FlexControlDialog.h:73, Ax25HfPacketDecodeDialog.h:84) use QPointer<SliceModel> because slices can be torn down (radio disconnect, slice remove). If the slice is destroyed while the dialog is open, the polling timer's m_noiseFloorFn(m_slice) call and the engine's m_slice->agcMode() checks will dereference a dangling pointer.
Swap both to QPointer<SliceModel> and null-check before each use.
Cosmetic — unfilled template placeholder in PR description
The heading ## Radio-authoritative ${PR_DIFF} protocol notes looks like an unsubstituted template variable leaked through. Worth tidying before merge — the content underneath reads fine, just the heading.
Everything else looks solid: convention adherence is good (PersistentDialog, showOrRaisePersistent, AppSettings via PersistentDialog geometry, no QSettings), the radio-authoritative pattern is correctly followed (setters only, no applyStatus override), cross-thread levelChanged is queued by default (the comment at the connect site is correct), and stop() properly restores m_originalValue. The max-distance-from-chord knee detector is a sensible choice for the L-shaped audio-RMS curve, and the quiet-spot S-meter-vs-floor guard is a nice touch. The right-click + tooltip mitigation for discoverability is exactly what the design doc promised.
Out-of-scope file check: the .github/workflows/macos-dmg.yml delta showing up in git diff main..pr-3350 is just the PR being behind main on the #3349 CI hotfix — your commit doesn't actually touch that file. A rebase will clean it up.
Nice work overall.
🤖 aethersdr-agent · cost: $18.8716 · model: claude-opus-4-7
Two of the three @aethersdr-agent review items on PR aethersdr#3350 — the third (unfilled \${PR_DIFF} placeholder) is in the PR description, edited separately. 1. NR-tap ambiguity (functional). AudioEngine::levelChanged is emitted from every NR branch in feedAudioData() — raw, then again after each of NR2/RN2/NR4/DFNR/MNR/BNR — so with NR active the calibrator was sampling post-NR audio and the AGC knee the tool is supposed to find was getting crushed before it could be detected. The design doc flagged this and prescribed either selecting the raw tap or freezing NR/AF for the sweep; the original implementation did neither. Approach: keep the post-NR tap (raw-tap refactor is out of scope), add a nrSuppressesCalibration() helper covering all 6 client NR stages plus the radio-side SliceModel::nrOn(), and use it in two places: * updateModeUi() — swaps the hint label to a "Disable NR for accurate calibration" warning when NR is active. * onStartStop() — refuses to start auto-sweep with a QMessageBox explaining which NR stages to disable. Mirrors the existing quiet-spot guard pattern. 2. QPointer<SliceModel> instead of raw SliceModel*. Slices can be torn down (radio disconnect, slice remove) while the dialog is open; the polling timer's m_noiseFloorFn(m_slice) and the engine's m_slice-> agcMode() checks would dereference a dangling pointer. Swapped both AgcTCalibrator::m_slice and AgcCalibrationDialog::m_slice to QPointer. Existing null guards (if (!m_slice)) now correctly catch the post-destruction case for free. Local build clean. Principle XI.
|
@jensenpat — pushed a fix-up commit (a347656) addressing the two code items from @aethersdr-agent's review. Intentionally on top of your branch so the original feature attribution stays clean. 1. NR-tap ambiguity — added `nrSuppressesCalibration()` covering all 6 client NR stages (NR2/RN2/NR4/DFNR/MNR/BNR) plus radio-side `SliceModel::nrOn()`. Two integration points:
Kept the post-NR `levelChanged` tap — adding a raw-tap output to `AudioEngine` is the deeper fix the bot also flagged as out of scope. Guard + warning is the cheaper-and-correct mitigation. 2. QPointer — swapped both `AgcTCalibrator::m_slice` and `AgcCalibrationDialog::m_slice` from raw to QPointer. Existing `if (!m_slice)` null guards now correctly catch the post-destruction-of-slice case for free. The third bot item (unfilled `${PR_DIFF}` placeholder in the description) — looks like it's already resolved in the current PR body. CI rerunning on the new commit. Happy to revert if you'd rather take a swing yourself. Re: scope — @ten9876 still has the underlying feature-scope decision (does AetherSDR want an AGC-T calibration tool). My fix-up here just makes the engineering items NF0T-bot-clean while that decision is pending. |
ten9876
left a comment
There was a problem hiding this comment.
Maintainer approval for both feature scope and engineering items.
Feature-scope decision (per AGENTS.md autonomous boundaries): Yes, AetherSDR wants the AGC-T noise-floor calibration tool. The radio's lack of an exposed dBm→AGC-T mapping makes empirical calibration the only practical path, and the SmartSDR procedure jensenpat is automating is a well-established workflow operators already do by hand. The design doc captures the load-bearing trade-offs correctly (knee-find vs target-solve, post-AGC audio as the only observable that bends, mode-aware behavior), and the implementation honors the radio-authoritative-settings policy throughout.
Engineering items: Both code items from @aethersdr-agent's review are addressed in the fix-up commit (a347656) — NR guard preventing the post-NR-tap masking the knee, and QPointer protecting against mid-calibration slice destruction.
Deferred items (per-band suggest chip, ambient noise-floor line on the panadapter) are appropriately scoped out for follow-up PRs.
Approving as Tier 2 (@aethersdr/infrastructure) for CMakeLists.txt + docs/, and Tier 3 (@aethersdr/reviewers) for src/.
Principle XI.
Summary
Adds a guided tool to calibrate a slice's AGC-T value against the receiver
noise floor, so operators can dial in the right value per band instead of
guessing with the bare 0–100 slider.
The classic SmartSDR procedure — tune to a clear spot, lower AGC-T until the
band noise just begins to drop, then stop — is now assisted with a live graph
and an optional one-click auto-sweep.
Why this approach
reports what dBm a given AGC-T value corresponds to, so the "perfect" value
can only be found empirically — by varying the knob and watching the
receiver respond.
pre-AGC FFT. The only observable that bends at the AGC knee is the
post-AGC audio level (
AudioEngine::levelChanged). That's what the toolplots and what knee detection runs on. (This is also why a "line on the pan"
alone can't visualize AGC-T — see the design doc.)
agc_thresholdis the knee; we find theelbow on the audio-RMS-vs-threshold curve.
agc_off_levelis a fixed gain with no knee; we insteadsolve for the value that places band-noise audio at a comfortable target.
What's included
src/core/AgcTCalibrator.{h,cpp}QObject. Auto-sweep + manual recording on one engine. Knee detection (max-distance-from-chord) for AGC-on; target-level solve for AGC-off. Quiet-spot guard (S-meter vs measured noise floor). Restores original value on Stop.src/gui/AgcCalibrationDialog.{h,cpp}PersistentDialog(frameless + geometry-persist per convention). LiveAgcCurveWidgetplot (audio noise vs AGC-T), Auto-Sweep-first + Apply/Stop, AGC-off target control, quiet-spot status line.src/gui/RxApplet.{h,cpp}calibrateAgcTRequested(sliceId); tooltip advertises it (discoverability mitigation).src/gui/MainWindow.{h,cpp}showAgcCalibrationDialog(sliceId), supplies per-slice noise floor viaspectrumForSlice().CMakeLists.txtdocs/agc-t-calibration-design.mdRadio-authoritative & protocol notes
SliceModelsetters (
setAgcThreshold/setAgcOffLevel) — the engine acts like a userturning the knob, then the normal status echo flows back. No
applyStatusoverride, no client-side persistence of the radio value in this PR.
Decisions (from the design decision pass)
Auto-sweep-first (consistent across skill levels), right-click entry point,
panel + optional ambient pan line as the surface, suggest-never-auto-apply for
per-band recall. Two tradeoffs against the "all skill levels" bias were flagged
and mitigated (tooltip discovery hint; non-modal dismissible suggestion). See
the design doc for the full table and rationale.
Deferred (follow-up PRs, to keep this reviewable)
Testing
RelWithDebInfo, Ninja, all warnings-as-usual(no new warnings in the added files). App bundle links successfully.
💻 Generated with Claude Code (Opus 4.8) with architecture by @jensenpat