Skip to content

feat(radiocert): certify control readback, front-end interlock and calibrate SQL - #6205

Merged
ten9876 merged 3 commits into
aethersdr:mainfrom
jensenpat:feat/radiocert-icom-control-contracts
Oct 9, 2026
Merged

ten9876 merged 3 commits into
aethersdr:mainfrom
jensenpat:feat/radiocert-icom-control-contracts

Conversation

@jensenpat

@jensenpat jensenpat commented Oct 5, 2026 •

Copy link
Copy Markdown
Collaborator

Four IC-7300MK2 defects reached the operator in v26.10.1, and each had a backend test that agreed with the backend:

PR Defect Class
#6174 PROC NOR/DX snapped to DX+ on their own confirmation read a scaled control decoded in a different domain from the one it was written in
#6175 squelch "on at threshold 0" read back as Off and stuck a boundary value whose enable is derived from the level
#6183 ATT on dropped the preamp, but the button changed only at the 3 s poll a control the radio changes on its own, unreported
#6184 SQL line drawn on Flex's −160 + level, 7–10 dB from the MK2's gate a published scale nothing in the client can contradict

This PR adds checks for those classes to radiocert meters and radiocert persist. They're family-neutral and gated on published capabilities, so the next Icom gets them without any new code.

radiocert meters: three non-keying stages, before the keyed ones

Stage Method Concern when
control-domain Writes every published speech-processor level (PROC enabled for the sweep) and squelch level 0…100 through the operator's setters, and samples the model until the radio answers. Ranges of ten or fewer get every step; wider ranges get both ends, their neighbours and the quarters. The ends are held 6.5 s (two MK2 polls). The squelch sample carries the enable. a value is never read back as written, or is read back and then moves
front-end-interlock Five preamp/ATT transitions from both-off; times when the stage not written settles. Restores the front end. the radio moved the other stage and the pan showed it after 500 ms
squelch-scale Parks the dial 1.5 kHz below a steady carrier (sql=<MHz>), binary-searches the lowest level at which the operator's audio drops 20 dB, confirms it at ±1 level, and compares the published line there with the carrier's pan peak. Where Auto SQL is published, it repeats the search on band noise. the line is more than 6 dB from the carrier, or Auto's widest margin is below the noise gate
  • squelch-scale declines (INCONCLUSIVE) above S9, where a wrong scale passes too (measured below). It also declines when the carrier fades mid-search or there's no audio with squelch open.
  • With bridge TX permission off, every key in the run is refused (keyRefusals), and these three stages still run.
  • New bridge argument radiocert <phase> [freqMhz] [sql=<MHz>]. The persist snapshot now publishes transmit.speechProcLevelMaximum.
  • Pure helpers (domainProbeValues, readbackOf, settledAtMs, lowestClosedLevel, panPeakNear, median) live in RadioCertificationMath.h. radio_certification_math_test checks each one against the four bugs' own live numbers. For example, the pre-fix percent decode moves NOR and DX but not DX+, and a level-only squelch comparison certifies the on-at-0 defect.

tools/radiocert_persist_controls.py: the same classes across a restart

This is a new RX-only runner, separate like the multislice one. The existing persist runners refuse non-Flex radios by design; this one accepts Flex, HL2 and Icom.

  1. Range: seeds the processor level at an interior value of the published maximum (0..10 → 3/6, the MK2 bins; 0..2 → 1/2) through the real slider. It must hold through two polls, and come back from the connect-time read after a Quit/relaunch.
  2. Boundary: manual SQL at 0 must keep squelch: true and the applet's Manual through two polls.
  3. Restart: a distinctive manual SQL level must come back from the radio.
  • An Icom connects by --icom-host. The runner never handles credentials: the operator signs in at each launch.
  • Every mutation is pinned to family, model and session serial, including an Icom's endpoint-derived icom:<host> serial. A radio with no serial is refused, so a reconnect to another radio of the same model is never seeded or restored.
  • If Manual can't be re-entered after the boundary contract (the fix(icom): squelch on at threshold 0 reads back as on, so it can't stick off. Principle II. #6175 trap itself), that's recorded as a concern, not a stop.
  • 13 socket-free policy cases cover the runner, including the on-at-0 and COMP-snap fixtures and the same-model/different-serial and missing-serial cases.

Live proof (IC-7300MK2, RX only, isolated TX-off clients, 2026-10-05)

I built three binaries from this branch: main as-is (#6183/#6184 merged, #6174/#6175 open), main with #6183/#6184 reverted, and main with #6174/#6175 merged. Every meters run had keyRefusals: 3, so nothing transmitted.

Check Main #6183/#6184 reverted #6174/#6175 merged
control-domain PROC NOR and DX read back, then moved to DX+ 200 ms later same all 11 COMP steps held
control-domain squelch on-at-0 never read back on same 0…100 held, on-at-0 included
front-end-interlock 4 coupled changes shown in 100–200 ms 3 of 4 only at the poll: 2700 / 1300 / 1800 ms (one early at 300 ms) 4 in 100–200 ms
squelch-scale 1120 kHz behind 20 dB ATT (−108…−112 dBm) gate 45, line −3.8 dB from carrier gate 41 confirmed, line +7.5 dB carrier faded to S0: declined
persist range DX written; model and slider at DX+ from sample 1 — COMP 3 held, and came back after restart
persist boundary (manual SQL 0) squelch: false, applet Off, all 14 samples — held on
persist restart (SQL 26, COMP 3) — — ESTABLISHED; cleanup restored COMP 9 / SQL Off

Two other squelch-scale results shaped the stage:

On the confirmation rule: night-to-dawn medium-wave carriers faded by up to 25 dB within one search. Requiring an exact repeat at the gate level declined steady carriers too, because at the gate the carrier sits on the threshold and chatters. So the confirmation is a one-level bracket. The radio was left as found: 1.340 MHz AM, SQL manual 12, PROC on at COMP 9, P.AMP1 / ATT off.

Tests (headless, QT_QPA_PLATFORM=offscreen)

radio_certification_math_test, radiocert_persist_policy (65 cases), bridge_docs_check, automation_tx_watchdog_test, automation_persist_diagnostics_test, automation_boundary_core_test, icom_control_profile_test, squelch_level_scale_test: all pass. The check_* static scripts and gen_bridge_docs.py --check pass. The full suite is left to CI.

The front-end interlock stage never writes through, or reads from, a radio or pan that went away during an event-loop wait. automation_tx_watchdog_test's interlockSurvivesRadioTeardown destroys the radio mid-stage and checks for an INCONCLUSIVE result; without the guards it segfaults.

Docs

  • CERTIFICATION.md §1.41–1.43 (one lesson per class) and a new-radio checklist step.
  • radio-certification.md: Controls rows and the measured table above.
  • automation-bridge.md: the stages, sql=, and the controls runner.

Not in this PR

  • No screenshots: there's no UI change; the evidence is the report JSON above.
  • The persist boundary contract isn't asserted across a restart. A radio with no squelch enable reads threshold 0 as Off after reconnect, which is fix(icom): squelch on at threshold 0 reads back as on, so it can't stick off. Principle II. #6175's documented behaviour.
  • Bridge verbs that would make this cheaper, left for follow-ups:
    • a pan frontend preamp|att <step> verb, plus preampStep/attenuatorStep in get pan. The proof had to open the ANT panel and click its buttons to choose a carrier's front end.
    • a backend-neutral squelch-gate readback (Icom 15 01), so squelch-scale could use the radio's own gate as a second instrument beside the audio.

🤖 Generated with Claude Code

…e SQL line, and persist the Icom control contracts. Principle VIII.

radiocert meters gains three non-keying stages, run before the keyed ones:

- control-domain: writes every published value of the speech processor level
  and the squelch level (both ends held through two periodic polls) and
  requires the model to read back where it was written. Catches aethersdr#6174's COMP
  snap and aethersdr#6175's on-at-0 squelch.
- front-end-interlock: steps the pan's preamp and attenuator and times when
  the stage NOT written shows what the radio did to it. Catches aethersdr#6183's
  interlock that waited for the 3 s poll.
- squelch-scale: binary-searches the level at which a steady carrier's audio
  stops and compares the published SQL line there with the carrier's pan peak;
  checks Auto SQL's margin against the band-noise gate. Catches aethersdr#6184's
  Flex-scaled line. Declines above S9, where a wrong scale passes too.

`radiocert ... sql=<MHz>` picks the carrier. The persist snapshot publishes
speechProcLevelMaximum.

tools/radiocert_persist_controls.py is an RX-only, family-neutral (Flex, HL2,
Icom) persist runner for the same classes across a restart: a mid-range
processor level, manual SQL at 0, and a distinctive SQL level.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jensenpat
jensenpat marked this pull request as ready for review October 5, 2026 12:43
@jensenpat
jensenpat requested a review from a team as a code owner October 5, 2026 12:43
@jensenpat jensenpat changed the title feat(radiocert): certify control readback, front-end interlock and the SQL line, and persist the Icom control contracts. Principle VIII. feat(radiocert): certify control readback, front-end interlock and calibrate SQL Oct 5, 2026

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue fit

No fixes/closes issue — this is a tooling PR against its own stated intent, and GOVERNANCE.md's RFC requirement doesn't bite (new diagnostic stages inside an existing verb, no architectural seam). It does what it says: three non-keying stages that reproduce the four named defect classes, family-neutral and capability-gated, plus a persist runner for the restart half. I checked each defect class against the code and the mapping holds — the squelch sample genuinely carries the enable (RadioCertification.cpp:685, -1 while off), which is the one detail that makes #6175 catchable rather than certifiable, and domainProbeValues(2) == {0,1,2} is what makes #6174's low end reachable. The meters phase really is ungated (AutomationServer.cpp:8827-8836), so the body's "TX permission off, these three stages still run" claim is true, not a convenient framing.

Blockers below are in the new argument parser and in two stages' preconditions/lifetimes, not in the measurement idea.

Scope

File / group What it changes Claimed in body? Verdict
src/core/RadioCertification.{h,cpp} three new non-keying stages + Options::squelchCarrierMhz yes In scope
src/core/RadioCertificationMath.h 6 pure helpers (domainProbeValues, readbackOf, settledAtMs, lowestClosedLevel, panPeakNear, median) yes In scope
src/core/AutomationServer.cpp sql=<MHz> token + transmit.speechProcLevelMaximum yes New public bridge surface — maintainer nod (disclosed, documented, verb help and automation-bridge.md kept in sync; gen_bridge_docs.py --check should pass)
tests/radio_certification_math_test.cpp helper assertions against the four bugs' live numbers yes In scope
tools/radiocert_persist_controls.py new RX-only runner (Flex/HL2/Icom) yes In scope; referenced from automation-bridge.md, imports the existing radiocert_persist helpers, not a dead addition
tools/test_radiocert_persist.py policy cases for the runner yes In scope; registered as radiocert_persist_policy at tests/tests.cmake:4576
docs/CERTIFICATION.md, docs/radio-certification.md, docs/automation-bridge.md §1.41–1.43, Controls rows, stage table, runner yes In scope

Everything in the diff is explained. No CHANGELOG.md entry (correct). I read the - lines: the only removal is the four-line freqArg parse, replaced in kind — and that replacement is Blocker 1. No sibling radiocert arg parser exists to leave behind (doRadioCert is the only consumer of parseActionValue for this verb).

Socket-test record: none added or removed. tools/test_radiocert_persist.py patches radiocert_persist.subprocess.Popen (line 322) and binds nothing; the new helpers are Qt-free pure math. The radiocert meters stages themselves are the bridge/real-firmware layer AGENTS.md routes positive convergence to, which is the honest home for them — I am not asking for a fake Icom peer.

CI: all five checks green on 13dc9e70, but neither radio_certification_math_test nor radiocert_persist_policy appears in any ctest -R filter in .github/workflows/, so none of the new assertions gate this merge. They run in full-suite after merge.

Blockers

1. src/core/AutomationServer.cpp:8851-8855 — the rewritten parser accepts nan as a frequency, which main rejected

The polarity flip loses a guard. main had:

    const double mhz = freqArg.trimmed().toDouble(&okF);
    if (okF && mhz > 0.0)
        opts.frequencyMhz = mhz;

NaN > 0.0 is false, so nan fell through and opts.frequencyMhz kept its 14.200 default. The new form inverts the test:

        const double mhz = token.toDouble(&okF);
        if (!okF || mhz <= 0.0)
            return err(QStringLiteral("radiocert: expected [freqMhz] [sql=<MHz>], got '%1'")
                           .arg(token));
        opts.frequencyMhz = mhz;

NaN <= 0.0 is also false, so neither branch catches it and opts.frequencyMhz = NaN is assigned. QString::toDouble() parses "nan" with ok == true — this file says so itself, in the comment on the helper that exists for exactly this trap (AutomationServer.cpp:1306-1308): "NaN/inf are refused by name before any range check: QString::toDouble() accepts them and NaN passes every comparison."

Failure scenario: radiocert rx nan from a bridge-authenticated caller. o.frequencyMhz reaches slice->setFrequency(NaN) (RadioCertification.cpp:1418, 1428, 1781); SliceModel::setFrequency has no finite guard (SliceModel.cpp:138-140). Worse, the stage that exists to guarantee the dial reports success: std::fabs(readbackMhz - o.frequencyMhz) > 1e-6 (RadioCertification.cpp:1795) is false for NaN, so the run records no concern and every later stage measures an unknown frequency while the report says the dial was verified. This needs no TX permission — tune/rx/meters all consume frequencyMhz.

The parser also drops the kMinTunableMhz/kMaxTunableMhz bounds (0.001 .. 105000) that tune, targettune and pan center all enforce, so radiocert rx 1e300 is accepted here and refused there.

Route both tokens through the existing helper:

    // [freqMhz] [sql=<MHz>]: the keyed stages' dial, and the steady carrier
    // stage-squelch-scale measures the radio's gate on.
    static const QRegularExpression certArgSep(QStringLiteral("\\s+"));
    for (const QString& token : freqArg.trimmed().split(certArgSep, Qt::SkipEmptyParts)) {
        double mhz = 0.0;
        if (token.startsWith(QLatin1String("sql="), Qt::CaseInsensitive)) {
            if (auto refusal = refuseUntunableMhz(QStringLiteral("radiocert sql="),
                                                  token.mid(4), mhz))
                return *refusal;
            opts.squelchCarrierMhz = mhz;
            continue;
        }
        if (auto refusal = refuseUntunableMhz(QStringLiteral("radiocert [freqMhz]"),
                                              token, mhz))
            return *refusal;
        opts.frequencyMhz = mhz;
    }

2. src/core/AutomationServer.cpp:8844-8848 — sql=inf becomes the dial target

Same hole, and here the inf case is new surface rather than a regression:

            const double sqlMhz = token.mid(4).toDouble(&okF);
            if (!okF || sqlMhz <= 0.0)

sql=nan is neutralised by luck two files away — o.squelchCarrierMhz > 0.0 at RadioCertification.cpp:883 is false for NaN, so it falls back to referenceCarrierMhz. sql=inf passes that test, so carrierMhz = inf, dialMhz = inf, and slice->setFrequency(inf) at RadioCertification.cpp:970. Depending on an accidental > 0.0 comparison in another file is not a boundary guard. Fixed by the same snippet above.

Blockers 1 and 2 were found by the security-audit (CodeGuard) pass and confirmed against the diff and the sibling helper.

3. src/core/RadioCertification.cpp:678 — "Auto SQL must be off" is a comment with no guard, no check, and nothing in the report

    // defect. Auto SQL must be off: its loop would write over these probes.
    if (SliceModel* slice = m_radio->slice(0)) {

Nothing in stageControlDomain reads, disables, or reports Auto SQL state — and nothing here can, because the state lives in the GUI: SpectrumWidget::m_autoSquelchEnabled (SpectrumWidget.h:1641). While it is on, updateAutoSquelchFromBins emits autoSquelchLevelSuggested on every pan frame (SpectrumWidget.cpp:4613-4640), and the wiring writes it straight to the slice:

            s->setSquelch(true, level);

(MainWindow_Wiring.cpp:4509, on activeSlice())

Failure scenario: an operator with Auto SQL on slice 0 runs radiocert meters. Every squelch probe is overwritten within one pan frame, so readbackOf reports agreedAt >= 0, departedAt >= 0 for most levels and the stage emits "squelch 50 read back as written, then moved to 37 after 300 ms" — a confident fabricated defect report, in the one stage whose whole purpose is to be trustworthy about readback. stageSquelchScale's binary search (:943) is corrupted the same way, and its scale->autoSquelch test at :1030 checks the capability record, not whether the operator has Auto engaged.

This is the §1.1 shape the PR is about, one level up: the stage's precondition and the stage's evidence disagree, and only the comment knows. Two honest options — either surface it (a bridge-readable Auto-SQL flag, reported in measured and turning the stage INCONCLUSIVE when set), or detect it behaviourally: a level that lands on neither the written value nor the prior value, repeatedly, across every probe, is Auto writing, not a domain mismatch. Saying "declined: Auto SQL appears to be writing this slice" is worth more than a wrong gate level.

4. src/core/RadioCertification.cpp:679-688, 850, 943, 963-970 — raw SliceModel*/PanadapterModel* dereferenced across spin(), and stageControlDomain has no restore guard at all

spin() is QEventLoop::exec() (RadioCertification.cpp:113-118), which runs deleteLater. Slices and pans are destroyed that way on disconnect and radio swap (RadioModel.cpp:1520-1528, 7030-7045, 11927-11936). The PR's own stageFrontEndInterlock knows this — const QPointer<PanadapterModel> watched(pan) at :733, re-checked every loop iteration — and so does the pre-existing stageControlEffect, whose comment at :475-477 reads "spin() has run event loops since, where a link drop can reset m_backend and leave the old pointer dangling."

The two new stages don't follow it:

  • stageControlDomain:679-688 captures slice raw into both sweep lambdas and dereferences it after each spin(kControlSampleMs) (:616-617). The squelch sweep alone spans ~19 s of nested event loops.
  • stageSquelchScale takes QPointer<SliceModel> watchedSlice at :890 for the restore guard only. The body still uses raw slice: :963 setMode, :966 setFrequency, and :943 slice->setSquelch(true, level) inside closedAt, which is called 8–16 times across spin(900) + audioDb(700) — a 30–60 s window. pan->preampStep() at :985 is likewise raw (readPan correctly guards its own QPointer p).

Separately, stageControlDomain has no qScopeGuard. If the radio drops between the sweep and the restore at :665-667 / :686-688, the operator is left with PROC force-enabled and the slice squelched at whatever probe value was last written — on a meters run that continues into keyed stages. stageSquelchScale got this right (:891-900); stageControlDomain should get the same treatment, and both should early-return on a null QPointer rather than report a measurement taken on a destroyed model.

Nits (non-blocking)

  • src/core/RadioCertification.cpp:959 — confirmed = closedAt(std::min(gate + 1, 100)) && (gate == 1 || !closedAt(gate - 1));. At gate == 100 this re-probes level 100, i.e. a repeat at the gate — precisely the "the gate chatters: closed, then open 6 dB down, at the same level" case the comment two lines above says the bracket exists to avoid. Either declare level 100 unconfirmable or fall back to the gate - 1 half alone there.
  • src/core/RadioCertification.cpp:1043-1045 — the noise branch runs findGate(noiseOpenDb) with no equivalent of the carrier branch's openDb < -90.0 silence guard (:1001). A near-silent band-noise floor yields a bogus noiseGateAboveFloorDb and can emit the "Auto leaves the noise open" concern from nothing.
  • src/core/RadioCertification.cpp:814-816 — restored is watched && …, so a mid-stage disconnect reports "could not restore the front end to preamp N / attenuator M", attributing a link drop to a restore failure.
  • tests/radio_certification_math_test.cpp:247 — check(legacyMisses >= 2, "Flex's scale misses the MK2 gate at low levels") over 8 measured points. Six of the eight could regress to agreement and this still passes. The two 2026-10-05 stage measurements right below it are the sharp assertions; this one is nearly free.
  • src/core/RadioCertification.cpp:588-595 cites #6171/#6172/#6178/#6180 (issues) while record() at :701 points the reader at docs/CERTIFICATION.md 1.41, which cites #6174/#6175/#6183/#6184 (PRs). The test comments pair them; the stage comments and the doc don't. One cross-reference each way would save the next reader the hunt.
  • Run cost: the three stages add roughly 1.5–2 min of nested event loops to every radiocert meters and radiocert all (PROC sweep ~24 s, squelch sweep ~19 s, interlock ~22 s, squelch-scale 30–60 s, doubled where Auto SQL is published). Fine for a bring-up diagnostic, but worth a sentence in automation-bridge.md so nobody reads a 2-minute meters as a hang.

What I tried to break

  • The body's central claim that these stages run with TX off. Read the gate at AutomationServer.cpp:8827-8836: keys is Tx || All, meters is excluded by name and by comment, and the parse happens after. The claim holds, and the keyRefusals: 3 in the live table is consistent with it.
  • Whether the new tests would pass against the unfixed code. They would not, and I checked the mechanism rather than the labels: the {step, step, readback, readback} fixture at :159 reproduces TransmitModel's optimistic write, and departures == {0, 1} is exactly "NOR and DX move, DX+ holds". readbackOf({0,0}, 0).held() at :188 is the deliberate negative control showing a level-only comparison certifies #6175. These are fixtures, not assertions of the implementation back to itself.
  • panPeakNear out of bounds. Target far below the pan: both toBin results negative, first clamps to 0, last stays negative, loop doesn't execute → NaN. Far above: first > last → NaN. A single bin's worth of overlap at the low edge returns bin 0, which is marginal but not a read past the end. No memory-safety issue; the isfinite(peakDb) INCONCLUSIVE path at :999 catches the off-pan case.
  • settledAtMs on an oscillating coupled control (1,0,1,0 → 0): returns the last transition, not the first touch. Correct for "when did it reach the value it held".
  • The front-end restore ordering. attWas > 0 → preamp first then ATT; else → ATT first then preamp. On an interlocking radio both-on is unreachable, and on a non-interlocking one both writes land. The 5-transition sequence from both-off yields 4 coupled changes, which matches the live table's count — the proof and the code agree.
  • Scope-guard-in-destructor. stageSquelchScale's guard calls spin(800); it fires after record(), at normal scope exit, with no exception path (house style is no exceptions). Fine.
  • Dead additions. radiocert_persist_controls.py imports the real radiocert_persist helpers, reads speechProcLevelMaximum from the field this PR actually adds to transmitSnapshot, and is referenced from automation-bridge.md; test_radiocert_persist.py is registered. Nothing orphaned.
  • Could not check: no build, no test run, no radio. Every finding above is reasoned from the head checkout at /tmp/aetherclaude/pr-6205, not reproduced at runtime. Specifically unverified: whether QString::toDouble("nan") returns ok == true on this Qt build — I'm relying on this file's own comment at :1306-1308, written by whoever added refuseUntunableMhz for that reason, plus the polarity flip being visible in the diff either way. And the entire live-proof table is an operator observation I have no way to re-measure; it reads consistently with the code, which is as far as I can take it.

Recommendation

Request changes — on Blockers 1 and 2, which are a one-snippet fix using a helper already in the file, and on 3 and 4, which are about the stage's own trustworthiness rather than its arithmetic. The measurement design here is the strongest thing in the PR: four defect classes turned into capability-gated probes, with negative controls in the test and INCONCLUSIVE paths where the measurement genuinely can't decide. That makes Blocker 3 the one worth arguing about rather than patching quietly — a stage that can fabricate a readback defect on any station with Auto SQL on undercuts exactly the credibility it's buying. Next step: route both tokens through refuseUntunableMhz, add the QPointer + scope guard to stageControlDomain and the raw derefs in stageSquelchScale, and decide whether Auto SQL gets surfaced through the bridge or inferred behaviourally. The new-public-surface row (sql=, speechProcLevelMaximum) is disclosed and documented but still wants a maintainer's yes.

Thanks for the three-binary A/B — reverting #6183/#6184 to show the stage goes red against the unfixed code is the part that makes this reviewable at all.


🤖 aethersdr-agent · cost: $15.9552 · model: claude-opus-5

…ages under Auto SQL, guard stage lifetimes. Principle VIII.

Review follow-up for aethersdr#6205.

- `freqMhz` and `sql=` go through refuseUntunableMhz: QString::toDouble()
  accepts nan/inf, and `radiocert rx nan` tuned to NaN while the control-plane
  stage, comparing against NaN, reported the dial verified. Arguments are now
  validated before the audio-engine and already-running checks.
- The verb uses parseActionRest: parseActionValue kept one token, so the line
  form `radiocert meters 14.2 sql=1.12` silently dropped `sql=`.
- Options::autoSquelchEngaged, read by the bridge from the RX applet's SQL
  button. Both squelch stages decline while Auto SQL is engaged, whose
  per-frame writes would read as a fabricated decode defect.
- stageControlDomain: QPointer slice, alive checks after every wait, and scope
  guards that restore PROC and squelch on every exit. stageSquelchScale uses
  guarded slice/pan throughout and reports a mid-search loss as inconclusive.
- Gate 100 confirms from below only; the noise search has the carrier
  branch's silence guard; a lost pan is not reported as a failed restore.
- Sharper legacy-scale fixture assertion; issue/PR cross-references; run cost
  documented.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@jensenpat

Copy link
Copy Markdown
Collaborator Author

Review follow-up in 0d866dc6:

  • Blockers 1–2: freqMhz and sql= now go through refuseUntunableMhz (finite, 0.001–105000 MHz), and arguments are validated before the audio-engine and already-running checks. automation_persist_diagnostics_test now refuses nan, inf, 1e300, -14.2, sql=inf, sql=nan, sql= and stray tokens, and accepts a finite 14.2 sql=1.12. That test also caught a second bug: the verb used parseActionValue, which dropped sql= after a frequency in the line form. It now uses parseActionRest.
  • Blocker 3: Options::autoSquelchEngaged, read by the bridge from the RX applet's SQL button (the RxApplet/Squelch mode target invoke already resolves). Both squelch stages decline while it reads AUTO, re-check after the sweep, and report when the state isn't observable.
  • Blocker 4: both stages now use QPointer slice/pan with alive checks after every wait. stageControlDomain has scope guards restoring PROC and squelch on every exit. A mid-run loss is reported as INCONCLUSIVE.
  • Nits: gate 100 is confirmed from below only; the noise search has the silence guard; a lost pan isn't reported as a failed restore; the legacy-scale fixture asserts the exact missed points; issue/PR cross-references are added; the run cost is documented.

Targeted tests pass headless (8 ctest targets) and the static checks pass.

🤖 Addressed by Claude Code

@ten9876 ten9876 self-assigned this Oct 8, 2026

@ten9876 ten9876 left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR adds diagnostics for the COMP/SQL domain, front-end coupling and squelch-axis defects, plus a receive-only persistence runner. There is no closing issue; I reviewed against the stated contracts and the referenced #6174/#6175/#6183/#6184 defect shapes. The finite-input and Auto SQL follow-up fixes are present, but the new lifetime protection is incomplete and the Icom runner does not enforce its claimed identity pin. Reviewed 0d866dc6d9c9db3a93fc7733bafdab9ba154760c: request changes.

File/group Claimed change Scope verdict
RadioCertification.cpp/.h, math helpers Domain, interlock and SQL-scale stages; guarded sampling Direct implementation; lifetime blocker below
AutomationServer.cpp sql= carrier, validation, Auto SQL callback and published COMP maximum Disclosed additive public bridge surface; maintainer decision
Two C++ tests Parser refusals and measurement/policy helpers In scope
radiocert_persist_controls.py, Python tests Isolated RX-only control/restart contracts and restoration In scope; identity blocker below
Certification and bridge documents Contracts, usage and honest measurement limits In scope

No unrelated dependency, settings migration, theme, changelog or test-gate expansion. The new diagnostic parameter/response fields and runner are disclosed. Jeremy (@ten9876) should accept the additive bridge surface as part of this diagnostic feature; that scope ruling does not replace the code fixes below.

Blockers

  1. src/core/RadioCertification.cpp:787–814,858 — radio teardown crashes the front-end interlock stage. watched is a QPointer, but write() dereferences the QPointer m_radio without testing it; restoration invokes that closure after event-loop waits. The sampling loop also checks watched before spin(), then dereferences it after the wait without rechecking.

    I compiled a socket-free fixture against the exact production stage: create a real RadioModel/PanadapterModel, publish stepped preamp/ATT labels, invoke the stage through its existing test friend, and destroy the RadioModel at 2350 ms during the first sample wait. No transport was started and no TX was requested. GDB reproduced:

    destroy radio during first interlock sample
    Thread 1 received signal SIGSEGV
    #0 RadioModel::setPanAttenuatorFor — RadioModel.cpp:6629
    #1 operator() — RadioCertification.cpp:790
    #2 RadioCertification::stageFrontEndInterlock — RadioCertification.cpp:858
    

    Thus the observed crash is the restoration call through the cleared radio pointer; the unchecked post-wait pan read is an additional lifetime hole in the same stage. The previous stages' guards do not protect this one.

    These two local replacements were compiled against the same fixture and returned safely instead of crashing (source restored afterwards):

     auto write = [&](bool preamp, int step) {
         if (!m_radio || !watched) {
             return;
         }
         if (preamp) {
             m_radio->setPanPreampFor(panId, step);
         } else {
             m_radio->setPanAttenuatorFor(panId, step);
         }
     };

    Immediately after spin(kControlSampleMs) in the inner loop:

             if (!m_radio || !watched) {
                 break;
             }

    Add an admissible teardown regression at this existing injection seam and preserve the INCONCLUSIVE/not-restored outcome. The fixture tests client lifetime, not third-party firmware; no synthetic radio peer is needed.

  2. tools/radiocert_persist_controls.py:166–174 — the Icom path omits the session identity from every subsequent mutation check. wait_ready() pins serial only when self.serial was supplied. With --icom-host, the identity is only family/model. A disconnect/reconnect or operator selection of another same-model Icom is therefore admitted for both seeding and cleanup. Manual Icom sessions do have a stable endpoint-derived serial (ConnectionPanel.cpp:2910: icom:<resolved-address>[:port]), and persist publishes it.

    A socket-free policy probe exercised the actual ControlsRun.wait_ready() and action() methods using the same data-fixture shape as the PR tests:

    {"initialSerial":"icom:192.0.2.1","acceptedSerial":"icom:192.0.2.2",
     "pinnedIdentity":{"family":"icom","model":"IC-7300MK2"},
     "mutationDispatched":true}

    This establishes the policy failure, not an actual connection to a second radio. It contradicts the helper's “family/model/serial” contract and can apply the first station's seeded/restored controls to another station. Pin the observed stable serial for Icom as well; refuse missing identity rather than silently omitting it. Replace the current if self.serial: block with:

                     serial = self.serial or radio.get('serial')
                     if not isinstance(serial, str) or not serial:
                         raise StopRun('radio has no stable session identity')
                     self.identity['serial'] = serial

    Add a same-model/different-serial policy case; the existing changed-model case does not cover this failure.

Non-blocking notes: src/core/RadioCertification.cpp:605 / docs/CERTIFICATION.md:831: an optimistic value with no contradicting readback can still appear held. The documentation correctly acknowledges this; keep it separate from independent firmware confirmation and the restart contract. Actual carrier/gate calibration and the author's live/reverted-firmware results were not independently repeated.

Verification and attacks: Exact-head automation_persist_diagnostics_test and radio_certification_math_test passed 2/2; the 63 socket-free Python policy tests passed. Disabling readback-departure detection made the COMP-departure and SQL periodic-poll assertions fail. The additional production-stage teardown crashed; the local guard replacement made that same fixture return safely. Both production source and original test were restored and rebuilt. No broad suite, synthetic firmware peer or modified socket-owning target was run.

Built and drove the exact-head app as isolated offscreen pr-6205-review, explicit socket, TX disabled, only DEMO-0001. Queried persist metadata and tested invalid carrier/overflow arguments before and after sim disconnect:

{"ok":false,"error":"radiocert sql= requires a positive finite frequency in MHz"}

The demo stayed transmitting:false; the owned process exited via Quit. Demo lacks the Icom coupled front-end pair, so it cannot reproduce that positive firmware path. The crash proof is the socket-free production-stage fixture, not a claimed GUI/hardware reproduction. Linux static-core build; no sanitizer or local Windows/macOS run.

Head/base engine, command-plane, capability, registration, settings, timeout and frozen-gate checks passed without per-file growth; manifests are current. Hosted required checks passed on an older main merge. All reviewed commits are GitHub-verified. Separate automated-review skill unavailable; earlier bot findings were checked and fixed finite-input/Auto SQL issues were not repeated.

Recommendation: Request changes. Guard stage lifetimes and pin Icom session identity, then rerun the focused tests with the two new policy/lifetime cases. Positive hardware convergence remains a separate certification task.

Final CI recheck: required checks still pass and the reviewed head is unchanged. Current main is 14ef3e78841009d73ddf2e7de22b9a5a83d5ee0a (2026-10-08 08:23:34 UTC), newer than this head's earliest pull-request run creation. With non-strict branch protection, the green checks do not prove current-main integration.

@ten9876 ten9876 assigned ten9876 and unassigned ten9876 Oct 8, 2026
…Icom runner pins its session serial. Principle VIII.

- stageFrontEndInterlock never writes through, or reads from, a radio or
  pan that went away during an event-loop wait: the restore closure and
  each post-wait sample check both pointers first. A radio torn down
  mid-stage now ends it INCONCLUSIVE (front end not restored) instead of
  segfaulting. Pinned by interlockSurvivesRadioTeardown in
  automation_tx_watchdog_test.
- radiocert_persist_controls pins the session serial on every path,
  including the endpoint-derived icom:<host> serial, and refuses a radio
  with none (resetting the partial identity, so a retry cannot slip past
  the pin). A reconnect to another same-model Icom is refused before any
  seed or restore.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@ten9876

ten9876 commented Oct 9, 2026

Copy link
Copy Markdown
Collaborator

Landing pass at 439db9bf0. Both blockers from the last review are fixed, each with a regression test that fails without its fix:

  • Interlock teardown: a radio torn down mid-stage now ends the stage INCONCLUSIVE. interlockSurvivesRadioTeardown segfaults (exit 139) without the guards.
  • Icom session-serial pin: two new policy cases, same-model/different-serial and missing serial, fail on the original code.

The maintainer accepted the additive bridge surface (radiocert meters sql=<MHz> and the published speechProcLevelMaximum). The earlier bot review's finite-input and Auto SQL findings were already fixed in 0d866dc6. The positive live firmware path (Icom coupled front end, squelch calibration) is unchanged by this pass; that evidence stays in the body as recorded.

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving at 439db9bf0.

Fixed in this pass (439db9bf0):

  1. Interlock-stage teardown crash. The restore closure and the post-wait samples check the radio and pan first. interlockSurvivesRadioTeardown passes with the guards and segfaults (exit 139) without them.
  2. Icom runner session identity. The serial is pinned on every path, a radio without one is refused, and a partial identity is reset. Two new policy cases fail on the original code; one also fails on the review's suggested fix without the reset.

Maintainer decision: the additive bridge surface (sql=, speechProcLevelMaximum) is accepted.

Verified:

  • automation_tx_watchdog_test, automation_persist_diagnostics_test and radio_certification_math_test (3/3) on Linux/Qt 6.12.
  • 65/65 Python policy tests.
  • Engine-boundary and command-plane findings identical to the merge base; capability records at 70; manifest clean.
  • Required CI green on this head.

Not verified: the positive live Icom firmware paths, which only hardware can show.

@ten9876
ten9876 merged commit df4d4fe into aethersdr:main Oct 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants