Repository navigation
feat(transmit): expose PTT Mic Override and document the v4.3 hardware-PTT default. Principle I. - #6445
Conversation
…e-PTT default. Principle I. (#6442) SmartSDR v4.3.16 (SMART-9340/12931) changed what a hardware PTT transmits. A footswitch, ACC or mic-jack PTT now sends the selected TX source, DAX and remote audio included, instead of always forcing the analog mic. The new radio-wide setting `ptt_mic_override` restores the old behavior (FlexLib v4.3.16.42034 Radio.cs:10310-10320, status Radio.cs:11151-11163). - Parse the `ptt_mic_override` transmit-status key into TransmitDelta and TransmitModel. Its presence is latched, so firmware before 4.3, which never reports the key, is told apart from a reported 0. - Add the typed TransmitControlRequest::Field::PttMicOverride. The Flex backend encodes it as `transmit set ptt_mic_override=<0|1>`; HL2 and Icom return Unsupported. The setter is not optimistic: the model changes only on the radio's echo. - Radio Setup → Transmit → Interlocks - TX REQ gets a "PTT Mic Override" toggle that follows the radio's value. Before 4.3 it is grayed out, with a tooltip saying why. - Add the setting to the bridge's `get transmit` snapshot and to the slice troubleshooting report, only once it has been reported. - Document the setting and the default flip in TX Controls, Radio Setup, Before You Transmit and WSJT-X Integration. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
The PR substantially implements #6442: status decoding and presence tracking, typed control dispatch, a radio-authoritative UI, conditional diagnostics, and documentation of the firmware default change. Request changes for two reproduced edge cases below. Reviewed head 6d59c776d against merge base 47aa4098d.
Scope
| File/group | Claimed in body? | Verdict |
|---|---|---|
TransmitDelta.h, TransmitModel.{h,cpp}, Flex status decoder |
Yes: parse, latch, reset | In scope |
ModelControlRequest.h, Flex encoder, HL2/Icom dispatch |
Yes: typed setter and unsupported families | In scope; sim/ANAN/RTL inherit Unsupported |
RadioSetupDialog.cpp |
Yes: toggle, echo-driven value, unavailable reason | In scope; dimming rather than hiding follows GUI canon |
AutomationServer.cpp, RadioModel.cpp, SliceTroubleshootingDialog.cpp |
Yes: conditional bridge/diagnostic field | In scope; additive bridge output explicitly disclosed |
| Four user documentation pages | Yes: default flip and operator guidance | In scope |
| Three test files | Yes: decoder, model latch/reset, encoder | In scope; socket-free injected seams |
Everything in the diff is explained by the issue and disclosed in the body. The single commit's author date matches this work. No unrelated restyling, client persistence, dependency, thread, default, CHANGELOG, or versioned-doc changes.
Blockers
-
Reject malformed override status instead of confirming “off” —
src/core/backends/flex/FlexBackend.cpp:1337.carry(..., optional<bool>)engages the field for every present string and maps anything except"1"to false. Injecting each ofgarbage,-1, and2through the production decoder produced:HOSTILE value=garbage engaged=1 value=0 HOSTILE value=-1 engaged=1 value=0 HOSTILE value=2 engaged=1 value=0All three added socket-free assertions requiring a disengaged delta failed. Applying these deltas will latch support and overwrite a previously true model value with false. FlexLib v4.3.16
Radio.cs:11151–11163explicitly rejects failed integer parses and values outside 0/1. Constitution I says “FlexLib C# source is the source of truth”; VII says “validates ranges, and fails closed on malformed input.” The new field does not follow either parser contract.Replace the added carrier with a validated parse (the existing namespace import exposes
parseInt):if (const std::optional<int> value = parseInt(kvs, "ptt_mic_override"); value && (*value == 0 || *value == 1)) { d.pttMicOverride = (*value == 1); }
Add decoder cases for malformed/out-of-range input, alongside valid 0/1 and absence. Confirm rejected input preserves both an unreported model and an existing true value.
-
Refresh the cached control when the session resets —
src/gui/RadioSetupDialog.cpp:3278.The new sync subscribes only to
stateChanged.TransmitModel::resetState()clears both fields at lines 52–53 but emits nostateChanged, so the open page retains its last value and availability after disconnect.Reproduced on the scratch GUI against DEMO-0001 with a temporary, explicitly enabled fixture that emits only
TransmitDelta{pttMicOverride=true}from the demo's initial-state path. No firmware peer or hardware was used. Before disconnect,get transmitreportedpttMicOverride: true, and the button was checked/enabled. After the real bridgedisconnectcompleted:get radio: connected=false get transmit: pttMicOverride absent dumpTree: {"accessibleName":"PTT Mic Override", "checked":true,"enabled":true,"text":"Enabled","visible":true}This contradicts the body's claim that the button always shows the radio/model value. Have reset notify this observer, or subscribe to the notification reset already emits. A small local correction next to the existing connection is:
connect(&tx, &TransmitModel::micStateChanged, pttOvrBtn, syncPttOverride);Add a socket-free UI lifecycle assertion for reported true → reset, requiring an unchecked, disabled control. The existing model test checks cleared members but misses the notification/UI consequence.
Nits — non-blocking
src/models/RadioModel.cpp:13000: brace the newif, per AGENTS.md's “Braces on all control flow.”src/models/TransmitModel.cpp:927: remove the Constitution citation from the code comment; AGENTS.md puts principle tags in commit subjects. The radio-wide/echo explanation is useful and can stay.
Verification and limits
- Built the named GUI and three focused targets in a scratch worktree using Qt 6.12, shared core and
-O2 -g1; all three original CTests passed. Independently reran the existing clean-head build's three binaries, also passing. No broad suite was run, and the body's 789/793 claim was not independently reproduced. - Mutation checks: removing the decoder line failed
aetherd_transmit_decode_test:61; breaking the presence latch failed the reported-zero assertion; forcing encoder output to zero failedsetPttMicOverride. Restored production code and reran all three successfully. The hostile-input assertions above were temporary review probes, not changes proposed in this review. - Drove the clean-head GUI on isolated offscreen DEMO-0001: verified conditional snapshot omission and the disabled button's tooltip/accessibility description. Drove the shared-core scratch GUI with the single demo status fixture for the disconnect finding. All review instances were closed, TX was disabled, and the scratch source modifications were restored. Real hardware PTT audio selection and live firmware set→echo convergence cannot be independently verified in ordinary demo mode; I did not connect to hardware. The author's live evidence remains reported evidence, not my reproduction.
- Checked FlexLib's setter, parser and semantics directly. Read unsupported sibling dispatch, no automatic setter/restoration paths, signal blocking, thread admission and teardown. No extra wire text above the seam or baseline changes: all four boundary gates pass on base and head with identical per-file findings (102 tracked engine warnings, 297 command-plane sites, 70 capability booleans); touchpoint manifest unchanged.
- No invocable automated code-review skill was available; performed the manual pass and an independent parallel audit. The accessibility bot's warning count does not identify a defect in the new button: it has both accessible name and description.
- CI is still running its platform builds; Static checks and Docs passed. Earliest workflow creation was
2026-10-13T22:04:37Z, while current main7d82707ewas committed at22:05:01Z. The pull-request runs test an older merge snapshot; strict up-to-date checks are not enforced, so their eventual green should retain that qualification.
Recommendation: Request changes. Correct parser validation and reset notification, then rerun the focused tests and the open-page disconnect scenario. The main feature path, scope and protocol command are otherwise consistent with #6442 and the reference.
… control on reset. Principle I. (#6442) Review of #6445: - Parse ptt_mic_override as FlexLib does (Radio.cs:11151-11163): only 0 or 1 is accepted. Garbage, -1, 2 and an empty value are dropped, never applied as a confirmed "off" or a reported key. - The Radio Setup control also listens to micStateChanged, the signal resetState() sends on disconnect, so an open page drops the last session's value and dims again. - Tests: decoder cases for malformed input; a dialog case for unreported → reported → click (typed request, no optimistic flip) → echo → reset. - Nits: brace the diagnostics if; drop the principle tag from a comment. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Both blockers and both nits are addressed in
Mutation checks:
🤖 Generated with Claude Code |
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
Ozy311
left a comment
Ozy311
left a comment
There was a problem hiding this comment.
Reviewed current head a63ef5bd1b10dfd45449093617c834143e51675f, including the incremental fixes since 6d59c776, against merge base 47aa4098d2b3182c8608d9bd7f4b6d842758d84a. The patch implements #6442 within its stated scope.
P3 — Name the PTT input correctly (docs/user/docs/tx-controls.md:153–154).
The new hardware-PTT instructions put a footswitch on the “RCA TX jack.” FlexRadio defines the RCA PTT connector as the footswitch input; TX/TX1/TX2/TX3 are outputs used to key external amplifiers or transverters. Following the new text would connect the footswitch to the wrong port and fail to key the radio. Replace “RCA TX jack” with “RCA PTT input.”
Reference: FlexRadio's input/output connector definitions.
Earlier findings addressed in the current revision.
The earlier review covered malformed status parsing and stale UI after reset. I independently reproduced the stale UI on the previous head; the current micStateChanged subscription addresses that path. The updated parser drops malformed/out-of-range values instead of engaging a false delta. Both now have focused regression coverage, which passed locally. I am not repeating these as outstanding findings.
Verification and limits.
- Rebuilt the current code locally on Mac with Qt 6.12/shared core. All three focused CTests passed:
transmit_model_test,model_control_routing_test, andaetherd_transmit_decode_test, including the new malformed-input cases. - Built
radio_setup_tx_timing_fields_testand ran specificallypttMicOverrideFollowsTheRadioAndResets: passed. This verifies unreported availability, reported zero, request without optimistic change, echoed true, and reset to disabled/unchecked using production widgets/models without a radio connection. - Engine-boundary strict check passed again on the updated head. On the original patch, all four boundary checks passed against merge base with identical tracked findings; bridge-docs registry check also passed. The follow-up introduces no new boundary sites or registry entries.
- Governance manifest passed at fetched main
7d82707e, with matching Constitution mirrors. The newly pushed commit has a verified/valid signature. - This is focused local validation, not a full test suite, release package, completed platform matrix, or live-radio test. Optional ASR/RADE/DSTAR/DFNR were disabled. No RF/TX or hardware audio-selection test was performed. The current-head CI run is still incomplete; earlier-head green checks do not establish current-head platform success.
Verdict: COMMENT. The reproduced reset defect and parser issue are addressed. The connector correction above is the remaining finding from this review; full platform CI and the required owner-review path still govern merge readiness.
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks for this — the protocol work is careful, the non-optimistic choice is right, and the "latch on presence, not on change" reasoning is the correct read of the pre-4.3 detection problem. One blocker: the new latch is cleared on disconnect but not on a family switch, which is the exact hazard teardownBackend() already documents for its sibling latch.
1. Issue fit
Solves #6442, with one gap. Every plan bullet maps to a hunk: parse (FlexBackend.cpp:1337-1340), latch (TransmitModel.cpp:210-216), typed control (ModelControlRequest.h:51, FlexModelControls.cpp:141-142), other backends Unsupported (Hl2Backend.cpp:3291-3292, IcomCivBackend.cpp:4523-4524), Radio Setup control, bridge snapshot, troubleshooting line, and all four doc pages. Principle VI holds: no key-on path is added and nothing sends the command on connect or restore.
The gap is the body's own claim — "Both appear only once the radio has reported the key, so a radio before 4.3 doesn't look like it has the override off". That is true across a disconnect and false across a family switch (Blocker 1).
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
ModelControlRequest.h |
Field::PttMicOverride + bool validity arm + label |
Yes | In scope. Appended before Count and at the end of the list, so no existing enumerator renumbers and m_controlIntentRevisions (sized on Field::Count, TransmitModel.h:551) still fits. |
TransmitDelta.h, FlexBackend.cpp |
optional + guarded decode | Yes | In scope |
FlexModelControls.cpp |
transmit set ptt_mic_override= |
Yes | In scope. New public wire surface, but it is FlexLib's own verb, not an AetherSDR invention — no maintainer call needed beyond the Principle I citation (see nit 2). |
Hl2Backend.cpp, IcomCivBackend.cpp |
new case → Unsupported |
Yes | In scope; these are the only other requestTransmitControl implementations, so no exhaustive-switch gap remains |
TransmitModel.{h,cpp} |
state, latch, setter, reset | Yes | In scope |
RadioSetupDialog.cpp |
the toggle | Yes | In scope. Lands at grid row 1 of the Interlocks group, which was free (row 0 only, :3224-3237); no widget collision. |
AutomationServer.cpp, RadioModel.cpp, SliceTroubleshootingDialog.cpp |
observability, both presence-gated | Yes | In scope. Key names match (mic["ptt_mic_override"] written and read). |
| 4 docs pages | the default flip | Yes | In scope. versioned_docs and CHANGELOG.md correctly untouched. |
| 4 tests | decode, model, routing, GUI | Yes | In scope |
Nothing in the diff is unexplained by #6442. No new config key, no persistence (correct — this is radio-owned state, Principle III), no removed guard, no default changed on our side, no deleted comment, no dead additions. The raw sendCommand("transmit set show_tx_in_waterfall=…") sibling at RadioSetupDialog.cpp:3360 is pre-existing and not this PR's to convert.
Socket tests: none added or removed. tests/radio_setup_tx_timing_fields_test.cpp:28,173 includes QHostAddress and builds QHostAddress::LocalHost, but it is an inert value — the file's own comment says "never dialled" and there is no bind/listen/connectToHost in any of the four touched tests.
3. Blockers
1. m_havePttMicOverride survives a family switch — the control stays live, and keeps the old Flex radio's value, on an HL2/Icom/demo. src/models/TransmitModel.cpp:52-53
The latch is cleared only in resetState():
m_pttMicOverride = false;
m_havePttMicOverride = false;and resetState() has exactly one caller, RadioModel::onDisconnected() (RadioModel.cpp:7930). A family switch never reaches it. RadioModel::connectToRadio() calls rebuildBackendForFamily() directly when the picked radio's family differs (RadioModel.cpp:3441-3444), which calls teardownBackend() — and teardownBackend() disconnects m_connection from this before destroying the backend specifically so status slots fail closed, so no onDisconnected() fires.
teardownBackend() already documents this exact class of bug for the latch yours is modelled on (RadioModel.cpp:1731-1735):
// A family switch starts a new session without onDisconnected(), so clear the
// power-provenance latches here or the old radio's drive publishes as the new
// radio's confirmed drive. resetPowerProvenance(), not resetState(): one caller
// is ~RadioModel(), and resetState() emits TX signals.
m_transmitModel.resetPowerProvenance();and resetPowerProvenance() (TransmitModel.h:89-93) clears m_haveTransmitStatus and m_haveMaxPowerLevel — the two existing presence latches. Yours is a third of the same shape and is not cleared there.
Failure scenario: connect to a FLEX-8600 on 4.3.16, let it report ptt_mic_override=1, then pick the Hermes-Lite 2 (or the demo radio) in the connection panel without disconnecting first. havePttMicOverride() is still true, so on the HL2: Radio Setup → Transmit shows PTT Mic Override enabled and reading Enabled; get transmit still returns pttMicOverride: true; the troubleshooting bundle reports a PTT mic override line for a radio that has none. Click it and the request routes to Hl2Backend::requestTransmitControl → ReceiveDispatch::Unsupported, which spends the session's one-shot unsupported notice on a control that should not have been clickable. Reasoned from code, not reproduced at runtime.
Fix — a non-emitting clear, safe from ~RadioModel(), alongside the existing one. In src/models/TransmitModel.h, next to resetPowerProvenance():
// Cleared on a family switch as well as a disconnect (#6442): the latch is
// radio-reported, so the old radio's report must not confirm the new one.
// Non-emitting, like resetPowerProvenance() and for the same caller
// (~RadioModel via teardownBackend); the next radio's first transmit status
// re-announces through stateChanged.
void resetPttMicOverrideReport() {
m_pttMicOverride = false;
m_havePttMicOverride = false;
}and in RadioModel::teardownBackend() (src/models/RadioModel.cpp:1735):
m_transmitModel.resetPowerProvenance();
m_transmitModel.resetPttMicOverrideReport();resetState() can then call the new method instead of repeating the two lines. The new test covers resetState() only; a case driving rebuildBackendForFamily("hl2") after a reported ptt_mic_override=1 and asserting the control goes back to disabled would pin this path — radio_setup_tx_timing_fields_test already builds the dialog against a bare RadioModel, and rebuildBackendForTest() exists, so this is a model/GUI-state assertion in a socket-free CTest, squarely within the test-layer boundary.
4. Nits (non-blocking)
- The mutation-check bullet describes code that no longer exists. The test plan says "deleting the
carry(kvs, "ptt_mic_override", …)line failsaetherd_transmit_decode_test:61", but after a63ef5b there is nocarrycall for this key — the decode is the guarded parse atsrc/core/backends/flex/FlexBackend.cpp:1339. The mutation evidence is worth restating against the line that shipped (deleting it, or swapping it forcarry, should both fail the test; the second is the more interesting mutation sincecarry's bool overload would accept"2"asfalse). - Principle I citations are unverifiable from here. There is no
reference/FlexLibtree in the PR checkout or onmain, so I could not confirmRadio.cs:10310-10320/11151-11163, theTX_PTT_MIC_OVERRIDE_DEFAULTname, or the claim atFlexBackend.cpp:1337-1338that "FlexLib rejects anything but 0/1". The live FLEX-8600 readings in the body are the stronger evidence for the wire grammar; a maintainer with the 4.3.16.42034 tree should spot-check the line numbers, especially as the in-repo reference has been 4.2.18 historically. - The strict 0/1 guard is the only one of its kind in this decoder.
src/core/backends/flex/FlexBackend.cpp:1334-1336—met_in_rx,mic_boostandmic_biasall go throughcarry(…, std::optional<bool>&), which maps anything that is not"1"tofalse(FlexKvCarry.h:44-48). Being stricter here is the better behavior and I would not ask you to loosen it; if it is worth keeping, acarryBool01()helper inFlexKvCarry.hwould let the rest of the family converge on it later instead of leaving one hand-rolled exception. - The branch base is ~2 commits behind
main..github/workflows/static-checks.ymlin the PR tree is still at the pre-#6439 word-splitting form ($FILESrather than"${FILES[@]}"), and thehl2_adc_pairing_testwork from #6441 is absent. Neither file is in your diff and CI tests the PR merged withmain, so nothing is broken — but a rebase onto47aa4098d+2 keeps local static-check runs honest. - No automation path to set it. The bridge gains the read but, per the body, the TX guard's name fallback classifies anything containing "ptt" as keying, so there is no way to exercise the write from the bridge even with
AETHER_AUTOMATION_ALLOW_TX=1. Disclosed and correctly left out of this PR; worth its own issue so the next TX-configuration toggle does not rediscover it.
5. What I tried to break (and could not)
- Signal recursion in the toggle.
RadioSetupDialog.cpp:3275-3278callssetPttMicOverride(on)thensyncPttOverride(), which callssetChecked()— but underQSignalBlocker(:3256), sotoggledcannot re-enter. Lifetimes are fine too: the lambda'spttOvrBtnis the connection's context object and a child of the dialog. - Stale GUI after disconnect. The comment at
:3281-3282is right —resetState()emits onlymicStateChanged()(TransmitModel.cpp:75), neverstateChanged(), and the PR wires both. Without the second connect the page would keep the last session's value; with it the test's reset assertions hold. - The latch firing on a reported 0.
if (d.pttMicOverride && !m_havePttMicOverride)is ahas_value()test,assign()then returns false because the value matches the default, andchanged = truestill reaches the singleif (changed) emit stateChanged();atTransmitModel.cpp:231— so thestateEdges == 1assertion intransmit_model_testis testing the real thing, and it would fail if the latch were keyed on change instead of presence. - Malformed and hostile status values.
parseInt(FlexKvCarry.h:21-28) requirescontains+toInt(&ok), and the extra(*v == 0 || *v == 1)drops"2"/"-1";""and"garbage"failok. All four are pinned inaetherd_transmit_decode_test, and that loop would fail if the guard were replaced bycarry. - Exhaustive-switch and ratchet breakage.
requestTransmitControlhas exactly three implementations (Flex, HL2, Icom) and all three are updated, so no-Werror=switchgap — andbuild,check-windowsandcheck-macosare all green on the heada63ef5b(they are cancelled on6d59c77only because the second push superseded it).tools/check_command_plane.pyexcludessrc/core/backends/, so the new encode site cannot move the 297 baseline.tools/gen_bridge_docs.pygenerates only the verb table, not snapshot keys, so no doc regeneration is owed forpttMicOverride. - Command injection into the Flex plane. The payload is
QString::number(int(std::get<bool>(request.value)))with the field pinned to the bool arm byTransmitControlRequest::valid(), so the wire text is literally0or1. The security-audit (CodeGuard) pass agrees and returned no findings. Every flaggedsrc/core/AutomationServer.cppline (299–2398, 6196, 6986, 7474, 8437, 9463, 11484, 11847, 12249-12250, 12406, 13040) sits outside this PR's only hunk there (1925-1928), and.github/workflows/static-checks.ymlis not in the diff at all — whole-file scan noise, dropped. - Could not check: runtime behavior of any kind (no build, no test run, no GUI here), the FlexLib line citations (no reference tree, nit 2), and the four touched tests' actual results — none of
aetherd_transmit_decode_test,transmit_model_test,model_control_routing_testorradio_setup_tx_timing_fields_testappear in anyctest -Rfilter inci.yml, so CI compiles them but does not run them. The body's local 789/793 is the only evidence they pass, and I cannot confirm it.
6. Recommendation
Request changes — for Blocker 1 only. Everything else here is solid work: the presence latch, the non-optimistic setter, the fail-closed decode and the four-page documentation of the default flip are all the right calls, and the GUI wiring handles the reset path that is easy to miss. The fix is the two-line clear in teardownBackend() above, ideally with a family-switch test case next to the existing resetState() one; after that I would recommend merge.
🤖 aethersdr-agent · cost: $11.0376 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (354 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: $11.1808 · model: claude-opus-5 |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
The PR substantially addresses #6442: it adds guarded decoding, a typed configuration request, radio-echoed model/UI state, presence-gated diagnostics, and user documentation. One reproduced lifecycle defect remains: a radio-family switch retains the previous Flex radio's override value and availability. Recommendation: request changes.
Scope
| File/group | Change | Claimed in title/body? | Verdict |
|---|---|---|---|
| TransmitDelta, ModelControlRequest, Flex backend | Decode and encode override | Yes | In scope |
| HL2/Icom backends | Explicit unsupported handling | Yes | In scope |
| TransmitModel | Value, presence latch, setter and reset | Yes | In scope; lifecycle blocker below |
| RadioSetupDialog | Radio-following toggle | Yes | In scope |
| AutomationServer, RadioModel diagnostics, SliceTroubleshootingDialog | Presence-gated reporting | Yes | In scope |
| Four user-doc pages | Firmware behavior and setting | Yes | In scope |
| Four tests | Decoder, model, routing and dialog coverage | Yes | In scope |
Everything in the PR diff is explained by #6442. No unrelated change, client persistence, client-side default change, or added TX-keying path was found. CHANGELOG and versioned_docs are correctly untouched.
Blocker
- [P2] Override state survives radio-family replacement — see the inline comment on TransmitModel.cpp. I independently reproduced the existing review's family-switch concern with the production model and dialog, injecting override=true and calling rebuildBackendForTest("sim"). The returned state was:
AFTER FAMILY SWITCH: reported= true value= true button enabled= true button checked= true
The PR's existing disconnect/reset case passed. Backend replacement therefore needs its own clear and UI notification, plus a socket-free regression case.
Non-blocking findings
- Correct the RCA connector name in TX Controls: PTT is the input; TX/TX1–3 are outputs (inline).
- Document the Multi-Flex condition on hardware override (inline).
- Add braces around the new decoder if (inline).
Verification and attacks
- Reviewed unchanged head a63ef5b; tested merge 4fadf73 against main 7d82707.
- Built the ARM64 desktop app with the local ARM64 toolchain and RADE enabled. Host/system processors and executable were ARM64; no RNNoise x86 sources were in build.ninja. Sandboxed iconutil failed initially; the native build completed.
- All four restored focused CTests passed: aetherd_transmit_decode_test, transmit_model_test, model_control_routing_test, radio_setup_tx_timing_fields_test. No broad suite was run. The four modified tests passed the socket-boundary preflight.
- Mutation checks: removing the micStateChanged subscription failed the dialog reset assertion; replacing the guarded decoder with carry produced four malformed-input failures. Both sources were restored and rebuilt before the final passing test run.
- Reported-zero detection, malformed-input rejection, typed encoding, non-optimistic request behavior and disconnect reset survived testing. Family replacement did not.
- Engine-boundary, touchpoint-manifest, capability-record and command-plane checks passed on current main and the merge result with identical findings. Bridge-doc registry check passed.
- Current-head hosted required checks are green; owner review is still required. Green per-PR CI does not establish execution of these four focused tests.
- Full-app demo validation was attempted with isolated settings, an explicit socket, offscreen mode and TX disabled, but the app did not reach bridge startup. A local QtKeychain/Qt version mismatch was observed; a scratch linkage correction did not resolve startup. No full-app demo success, hardware audio-selection behavior, or fresh FlexLib v4.3 source verification is claimed. The lifecycle failure above was reproduced in the running production model/dialog test, not inferred solely from code.
- No hardware connection or TX validation occurred. All review app instances were stopped. No invocable automated-review skill was available; existing bot findings were checked independently, with a second manual lifecycle/failure-path pass.
…itch. Principle I. (#6442) Review of #6445: - A family switch rebuilds the backend through teardownBackend() and never reaches resetState(), so the old Flex radio's reported override (value and presence) carried over to an HL2, Icom or the demo. teardownBackend() now clears it with a non-emitting resetPttMicOverrideReport(), beside resetPowerProvenance(), and resetState() uses the same helper. The open Radio Setup control also follows RadioModel::backendRebuilt. - Docs: a footswitch goes on the RCA PTT jack (TX1-3 are outputs). Note that under Multi-Flex FlexRadio applies the override when the client whose hardware PTT is pressed is the transmitting client. The tooltip and WSJT-X note drop "always". - Braces on the guarded parse. - Test: radio_setup_tx_timing_fields_test switches to the sim family after a reported override and requires the model unreported and the control unchecked and dimmed. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
|
Addressed in
🤖 Generated with Claude Code |
Ozy311
left a comment
Ozy311
left a comment
There was a problem hiding this comment.
Follow-up review of exact head c2416ffc46a58dbc08588d60e48c22d39ae02c20, compared with my previously reviewed a63ef5bd and the complete PR against merge base 47aa4098d2b3182c8608d9bd7f4b6d842758d84a.
No remaining actionable findings from this review. The PR implements #6442 within its stated scope. My connector-documentation finding is fixed, and the lifecycle/parser findings discussed in the earlier reviews are addressed in the current code.
Fixes verified
- Family replacement:
teardownBackend()now clears both the override value and its reported-presence latch through the non-emittingresetPttMicOverrideReport().rebuildBackendForFamily()emitsbackendRebuiltafter replacement, and the open control subscribes to it. This clears the old radio's value and availability without introducing destructor-time TX notifications. The new production-model/dialog family-switch assertions pass. - Disconnect:
resetState()uses the same clear helper; themicStateChangedsubscription refreshes the button. The reset assertions still pass. - Input and request behavior: guarded 0/1 decoding, rejection of malformed/out-of-range input, reported-zero presence detection, typed encoding, and waiting for the radio echo remain covered by passing tests.
- Documentation: TX Controls now correctly names the RCA PTT input, avoids assuming every model has a front-panel mic jack, and includes the Multi-Flex condition. The tooltip/WSJT-X text no longer promises “always.” I could read FlexRadio's current PTT Override guidance; it supports the added condition. The decoder conditional is now braced.
Verification
- Updated the isolated scratch snapshot to this exact head and verified all 20 PR-changed files match it. Rebuilt the four focused targets with local ARM64 Qt 6.12/shared core.
- 4/4 CTests passed:
model_control_routing_test,radio_setup_tx_timing_fields_test(the full target, including the new family-switch case),aetherd_transmit_decode_test, andtransmit_model_test. - Current-head strict engine-boundary, touchpoint-manifest, capability-record, and command-plane checks pass: 102 tracked engine warnings/zero blockers, manifest current, 70 capability booleans, 297 command-plane sites. Bridge-doc registry check passes.
- Refreshed trusted main to
709093e269766cca8f0dacda5ad55708cb490bb7. Governance manifest PASS; the governance-file hashes are unchanged from the earlier review and the Constitution mirrors match. Inspected intervening main changes in overlapping files: the hardware-PTT observation work is separate from this override configuration/reset path. Local tests cover the PR head, not a newly built merge with that main revision. - All three PR commits have verified/valid signatures. Current-head Static checks, docs, sanitizer configuration, and CodeQL are green. Required
build,check-windows, andcheck-macosremain in progress; their actual job steps were inspected and are not counted as passing.
Verdict: COMMENT, no new findings. The earlier defects are fixed in the reviewed code. Platform CI is incomplete, so this is not a merge-readiness approval; the required owner-review path and existing change-request state still need to be satisfied. No hardware connection, RF/TX, full suite, or release-package validation was performed. Local ASR/RADE/DSTAR/DFNR options remained disabled. No contributor-branch changes were made.
There was a problem hiding this comment.
Issue fit
Yes. #6442 asks for seven things and the diff does all seven: parse ptt_mic_override into TransmitDelta/TransmitModel with a presence latch (TransmitModel.h:189-190), a typed Field::PttMicOverride encoded only by Flex (FlexModelControls.cpp:141-142), Unsupported everywhere else, a Radio Setup control gated on reported rather than on value, the bridge snapshot key, the troubleshooting key, and the docs for the v4.3 default flip.
It also lands all three corrections the maintainer-review triage comment on #6442 called out in advance, which is the part I most expected to find missed:
- "A plain
boolcannot do the pre-4.3 detection" →m_havePttMicOverridelatches on presence, soassign()returning false for a reported0into afalsedefault still flips the latch (TransmitModel.cpp:210-215).transmit_model_test.cpppins exactly that case, including thestateEdges == 1edge. - "Do not copy the
micBiaswrite path" →setPttMicOverride(TransmitModel.cpp:920-928) deliberately does not pre-set the member, unlikesetMicBoost/setMicBiasat:898-918which are optimistic because#1045says the radio sends no echo. Correct: this key is a transmit-status key. - "The bridge snapshot should omit or null the field when unreported" →
AutomationServer.cpp:1925-1928omits it.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
docs/user/docs/tx-controls.md |
New ### Hardware PTT and the TX source section |
Yes (issue plan) | In scope |
docs/user/docs/{before-you-transmit,wsjt-x-integration}.md |
Pre-flight checklist line, WSJT-X caveat | Yes (issue plan names both) | In scope |
docs/user/docs/radio-setup.md |
One table cell for the Transmit page | Not named, but the companion edit | In scope |
ModelControlRequest.h, TransmitDelta.h |
New enumerator + delta field | Yes | In scope |
FlexBackend.cpp, FlexModelControls.cpp |
Status decode + wire encode | Yes | In scope |
Hl2Backend.cpp, IcomCivBackend.cpp |
Add the enumerator to the Unsupported arm |
Yes | In scope (required — these switches are exhaustive) |
TransmitModel.{h,cpp} |
Value, presence latch, setter, reset helper | Yes | In scope |
RadioModel.cpp |
teardownBackend() clear + diagnostics key |
Partly — teardown clear came from review of this PR | In scope |
SliceTroubleshootingDialog.cpp |
Render the diagnostics key | Yes | In scope |
RadioSetupDialog.cpp |
The toggle | Yes | In scope |
AutomationServer.cpp |
New bridge snapshot key pttMicOverride |
Yes | In scope — but see below |
| 4 test files | Encoder routing, decode, model latch, dialog | Yes | In scope |
Nothing in the diff is unexplained by the issue, and no CHANGELOG.md entry was added. Two rows deserve a maintainer's eye rather than mine:
- New public surface:
pttMicOverridein the bridge transmit snapshot andptt_mic_overridein the troubleshooting snapshot. Both are named in the issue plan and the triage comment endorsed the omit-when-unreported shape, but that triage isaethersdr-agent[bot], not a human ruling. Flagging per the new-surface rule, not as an objection — the design is right (absent ≠false), andsetPttMicOverridehas exactly two callers (RadioSetupDialog.cpp:3275and tests), so the bridge gains a reader and no writer. - Branch base. The branch is on
47aa4098d;mainis three commits ahead (dc7cdc1b5,7d82707eb,709093e26).git merge-treeis conflict-free, and the 95 lines709093e26changed inHl2Backend.cppdo not touch therequestTransmitControlswitch this PR edits. No action needed — noting it because it explains the next section.
Blockers
None.
Nits (all non-blocking)
-
src/core/backends/flex/FlexBackend.cpp:1337— the comment pins line numbers in a FlexLib copy nobody here can open:// FlexLib rejects anything but 0/1 (Radio.cs:11151-11163): a malformed valueThere is no
reference/FlexLibin my workspace at all, and the #6442 triage comment establishes the in-tree copy is 4.2.18, wheregrep ptt_mic_overridereturns nothing andRadio.cs:11151is_dockerWaveforms. The citation is presumably correct for 4.3.16.42034, but as written it reads as checkable and isn't. Suggest naming the version, or pointing at the primary evidence instead — the FLEX-8600 capture atsoftware_ver=4.3.16.42034in #6437:// FlexLib 4.3.16.42034 rejects anything but 0/1 (Radio.cs:11151-11163; the // in-tree reference is 4.2.18 and predates the key): a malformed value // must not confirm "off" or mark the key as reported.
-
tests/aetherd_transmit_decode_test.cpp:70-76—const TransmitDelta dinside thefor (const char* bad : …)loop shadows theconst TransmitDelta ddeclared at:58in the same block. I checked before reporting this:-WshadowatCMakeLists.txt:2252is scoped to the vendoredaether_crdvC target only, and there is no-Werroranywhere, so this costs nothing at build time. Readability only —dBadwould read better. -
src/gui/RadioSetupDialog.cpp:3241-3283— the issue plan says the control "shows only once the radio has reported the key, so firmware before 4.3 never sees it"; the PR instead always shows it and grays it with an explanatory tooltip. I think the PR's choice is the better one (a dimmed control with "Requires SmartSDR v4.3 firmware or later" teaches; an absent one doesn't), the docs say "grayed out" to match, andtx-controls.mdis consistent. Naming it only because it's a deliberate departure from the written plan. -
docs/user/docs/tx-controls.md:166-168— the Multi-Flex sentence ("the override applies when the client whose hardware PTT is pressed is also the transmitting client") is, by @ten9876's own note in this thread, sourced from @rfoust's summary of a helpdesk article that returns 403 to the author, and FlexLib's doc comment says only that the setting is radio-wide. I can't test it either. Worth one maintainer confirmation before it ships as documented behavior — the rest of the section is grounded in the live capture. -
src/models/RadioModel.cpp:3441-3447— a same-family Flex→Flex swap takes neither path that clears the latch:wantFamily != m_familyis false sorebuildBackendForFamily()(and itsteardownBackend()) is skipped, andresetState()only runs fromonDisconnected()at:7931. If connecting to a second Flex radio without an intervening disconnect is reachable from the UI — the RTL special-case just above at:3437exists precisely because same-family swaps happen — radio A's reported override survives into radio B's session, and a pre-4.3 radio B keeps an enabled control. This is a pre-existing shape shared withm_haveMaxPowerLeveland every other latch inresetState(), not something this PR introduces, so it isn't a blocker on this PR; I couldn't confirm from code whether the picker always disconnects first. -
src/gui/RadioSetupDialog.cpp:1376— the Transmit page's search keywords are"tx transmit rf power tune ptt band settings", so searching "ptt" finds the page but "mic override" or "override" does not. One word would fix it.
Security audit
Ran the required security-audit subagent against /tmp/aetherclaude/pr-6445 with the harness file list. Verdict: no findings. Credited to security-audit (CodeGuard):
FlexModelControls.cpp:142is the only new command sink and its operand isQString::number(int(std::get<bool>(...)))— literally0or1, to the radio's TCP command socket, never a shell.FlexBackend.cpp:1339is the only new parser of radio-controlled data and applies a positive allow-list before engaging the optional.- The new bridge key is read-only and reachable only over the
QLocalServeratAutomationServer.cpp:2304-2320, which setsQLocalServer::UserAccessOption— not one of the unauthenticated TCP listeners.
Every CodeGuard hit above is refuted, and I spot-checked rather than taking that on faith. The CMDI-BACKTICK/SEMI/AMP hits in AutomationServer.cpp (1755, 1759, 1785, 1789, 2398, …) land on comment prose and Qt string literals — e.g. :1757 is QJsonArray slotArr; // not "slots" — that's a Qt macro. SSRF-PRIV-192 at :6986 is a comment containing 192.168.50.255 inside the guard that rejects broadcast addresses; SSRF-LOCALHOST at :12249-12250 is a hardcoded ws://127.0.0.1:%1 with only the port substituted, which is not SSRF (no host control). All of them are pre-existing and none intersect this PR's single touched hunk at :1925-1928. static-checks.yml is not in this PR's diff at all — those hits are a whole-file scan of main's tip.
What I tried to break
- The presence latch against the thing it exists for.
if (d.pttMicOverride && !m_havePttMicOverride)testshas_value(), not the value, so a 4.3 radio reporting the0default into a model already atfalsestill latches and still emits — the exact case a change-keyed latch would miss.transmit_model_test.cppasserts it withstateEdges == 1, and that assertion fails if the latch is moved after theassign(). - Both clear paths, and whether either can fire from a destructor.
resetState()(disconnect) andteardownBackend()(family switch, and~RadioModel()at:2491) both call the non-emittingresetPttMicOverrideReport().backendRebuilt()is emitted from exactly one place —rebuildBackendForFamily()at:10097, after teardown — and never from the destructor path, so the refresh connection can't run against a dying model.setBackendForTest()tears down without emitting, but that is test-only. - The three refresh signals, for a hole between them.
stateChangedcarries the latch,micStateChangedcarriesresetState()(confirmed:resetState()emits it atTransmitModel.cpp:75, and notstateChanged),backendRebuiltcarries the family switch. I could not find a fourth way for the value or latch to move. The one gap I did find is the same-family swap, noted above as pre-existing. - Click reentrancy and a stale button.
QSignalBlockerinsyncPttOverridemeans the snap-backsetChecked(false)cannot re-entersetPttMicOverride(false). All three model/radio connections usepttOvrBtnas the context object so they die with the button; thetoggledconnection usesthisbut its sender is the button, so a destroyed button can't fire it. - Malformed wire input.
parseInt(FlexKvCarry.h:21-29) isok-guarded, and the PR adds a(*v == 0 || *v == 1)range check on top. This is deliberately stricter than the housecarry(…, std::optional<bool>&)overload at:44-48, which treats anything ≠"1"asfalse— soptt_mic_override=2is dropped here wheremic_boost=2would be applied as off. Correct for a tri-state (unreported / 0 / 1); the decode test pinsgarbage,-1,2and"". - Sibling implementations left behind.
requestTransmitControlhas exactly three overrides repo-wide (Flex, HL2, Icom) —IRadioBackend.h:144is the default — and all three are updated. ANAN, RTL and Sim don't override it.valid()andlabel()both gained the arm. The only other exhaustive consumer ofFieldism_controlIntentRevisions, declaredstd::array<quint64, static_cast<size_t>(Field::Count)>atTransmitModel.h:559, so the new enumerator sizes the array automatically — no out-of-bounds write fromcaptureControlIntent(PttMicOverride). - Static-analysis gates the diff could trip. The new wire text lives in
src/core/backends/, whichtools/check_command_plane.pyexcludes viaBACKENDS_PREFIX, so the shrink-only baseline of 41 forRadioSetupDialog.cppis untouched — the typed-control route is what keeps that green. The accessible name and description are set on the one new interactive widget. Static checks, Build docs site and the three CodeQL jobs are green onc2416ff; build, check-macos and check-windows were stillin_progresswhen I looked, so I am not calling the compile clean. - Test layer and coverage. All four are socket-free CTests at the right layer per AGENTS.md (wire encoding →
model_control_routing_test; parsing →aetherd_transmit_decode_test; model state →transmit_model_test; UI state machine →radio_setup_tx_timing_fields_test,QT_QPA_PLATFORM=offscreen,TIMEOUT 60). All four are registered intests/tests.cmake. I grepped both modified test files forQTcpServer|QUdpSocket|QTcpSocket|QLocalServer|connectToHost|listen(|bind(— no socket tests added or removed; the<QHostAddress>include in the dialog test is pre-existing and inert. None of the four are in.github/ci-test-gate.txt, so they run on full-suite-on-main and weekly sanitizers, not on this PR. - Would the new dialog test pass against the unfixed code? No.
pttMicOverrideFollowsTheRadioAndResetsappliespttMicOverride=true, callsrebuildBackendForTest("sim")and then requires!tx.havePttMicOverride()and the button unchecked and disabled — which is precisely the state @rfoust reproduced as wrong beforec2416ff. Deleting theresetPttMicOverrideReport()line fromteardownBackend()fails it at that assertion. - Doc-link integrity. The
#hardware-ptt-and-the-tx-sourceanchor used from three pages matches the### Hardware PTT and the TX sourceheading. I also checked whetherdocs/automation-bridge.mdenumerates transmit snapshot fields that would now be stale — it doesn't (nomicBias/amCarrier/metInRxrows), so no doc update is owed for the new key.
Everything above is reasoned from the head checkout at /tmp/aetherclaude/pr-6445; I have no build and no way to drive the GUI, so none of it was reproduced at runtime. What I could not test at all: the Multi-Flex claim (no access to the helpdesk article), the FlexLib 4.3.16 line citations (no reference copy in this workspace), and whether the radio actually echoes a set in transmit status — the #6437 capture proves the key is reported, not that a write is echoed, which is the single assumption the whole non-optimistic UI rests on.
Recommendation
Approve with nits, with one maintainer decision attached. The implementation is careful in the places that matter — presence-latched rather than value-latched, mirrored rather than optimistic, cleared on both teardown paths, stricter-than-house parsing — and the two review rounds already closed the real defect (@rfoust's family-switch carryover) with a test that fails without the fix. Every nit above is cosmetic or a request to confirm something neither the author nor I can read.
Next step: a maintainer signs off on the new bridge snapshot key pttMicOverride (shape is right; it's the public-surface precedent that needs a human), confirms or attributes the Multi-Flex sentence, and waits for build / check-macos / check-windows to finish. Nice work on the presence latch — it's the one part of this that was easy to get subtly wrong, and the test pins it at the exact value where a change-keyed latch would have silently failed.
🤖 aethersdr-agent · cost: $10.9030 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (470 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: $11.0442 · model: claude-opus-5 |
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
|
Landing validation at exact head
Fresh exact-head platform CI is required before approval/arming; older green runs are not reused. |
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
|
Current-main integration revalidated at
|
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
Accessibility check findingsThis PR touches
See Findings are warnings only — the build is not blocked. |
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Approved exact head 8a05e99 after rebuilding and validating its merge with current main ae699be.
- Author repair c2416ff fixes strict 0/1 decoding, disconnect notification and family-replacement state clearing. The non-emitting teardown clear and backendRebuilt UI refresh retire the reproduced blocking finding. Full production radio_setup_tx_timing_fields_test passes, including request-without-optimistic-state, echoed true, family replacement and reset. Deliberately removing the teardown clear failed its family-switch assertion; restoring it passed.
- Permissive decoder mutation produced four malformed-input failures; restored final decoder, transmit model and typed routing tests all pass. Final head: 4/4 focused offscreen CTests, complete ARM64 Qt 6.12 app build and user-doc production build passed.
- Cleanup 186c2cd versions the independently verified official FlexLib 4.3.16 citation, removes test-variable shadowing and adds “mic override” search terms. Final clean app identity 8a05e99 was driven only on isolated TX-disabled DEMO-0001. Search selected Transmit; the unreported field was omitted, the visible button was disabled/unchecked with an accessible reason, and the open page remained unavailable after disconnect. All five landing instances are stopped.
- FlexRadio official guidance confirms correct PTT input names and the Multi-Flex qualification. The conditional read-only bridge field is within the maintainer-authored issue/PR scope. Existing GUI canon requires unavailable controls to remain visible. No new maintainer decision, new TX permission, persistence/default change, dependency or architecture change was made.
- All four review threads have evidence-bearing author replies and remain resolved. The blocking review was dismissed with independent test/mutation evidence. Whole-file CodeGuard and nine accessibility warnings are pre-existing; base/head accessibility findings match exactly. Same-family connection cannot open a new Flex transport while connected; actual disconnect clears the model. No remaining actionable finding.
- Four boundary gates pass on updated base/head with identical findings: 102 tracked warnings, 70 capability booleans, 296 command sites. Touchpoint manifest, bridge registry, test registration, whitespace and Constitution mirror checks pass. Manual adversarial review performed; no invocable automated code-review pass was available.
Local optional ASR/RADE/DSTAR/DFNR/QtKeychain are disabled. No full local suite, release package, live radio or RF/TX evidence is claimed. The client’s enabled-state/echo path is covered by injected socket-free production model/widget tests; actual firmware set-to-echo and hardware source selection remain author-reported evidence. Main’s squash-commit CI is a separate mandatory post-merge check.
Fresh required PR checks were verified before posting this approval. The earliest head workflow was created at 2026-10-14T06:30:00Z (required CI/Static at 06:30:02Z), after current main ae699be committed at 06:27:57Z. No stale-green exemption was used.
|
Post-merge verification complete for squash
No live-radio or RF/TX test was performed during this landing pass. The isolated TX-disabled demo sessions are stopped. The independently rerun software evidence completes the pass under Principle XI. |
Fixes #6442. Implements the PTT Override item in FlexLib v4.3.16 tracker #6437.
SmartSDR v4.3 changed hardware PTT to transmit the selected TX source, including DAX and PC audio. AetherSDR now exposes the radio-wide PTT Mic Override setting so an operator can restore the last-used analog microphone for hardware PTT.
ptt_mic_overrideonly for valid 0/1 values. Track whether the radio has reported the field, distinguishing an unsupported firmware session from a reported false value.transmit set ptt_mic_override=<0|1>. Other families return Unsupported. This configuration write does not key TX and is never sent on connect or restore.Protocol authority: official FlexLib v4.3.16 source,
Radio.cs:10302-10320and11151-11163, independently checked during landing. FlexRadio's PTT Override guidance confirms the default and Multi-Flex condition.Validation
8a05e990770a1067535c69d14e958e30a6bbec31, clean working tree. Shared core; ASR, RADE, DSTAR, DFNR and QtKeychain disabled for this validation build.aetherd_transmit_decode_test,transmit_model_test,model_control_routing_test, and fullradio_setup_tx_timing_fields_test. These cover valid/invalid input, reported-zero presence, typed encoding, non-optimistic UI, disconnect reset and family replacement.DEMO-0001; searching “mic override” selected Transmit; the button was visible, disabled and unchecked with an accessible reason;get transmitomitted the field. The open page stayed unavailable after disconnect. All five landing demo instances were stopped.The bridge currently cannot click this non-keying PTT configuration control because its keying-name fallback refuses it. No automation TX-guard change or new bridge verb is included. No CHANGELOG or versioned documentation changes.
Generated with OpenAI Codex (GPT-6 Astra)