Skip to content

feat(icom): IC-705 bring-up — controls, UX, CI-V stall, audio transport, and transmit-meter staleness - #4799

Open
jensenpat wants to merge 2 commits into
mainfrom
feat/icom-bringup-controls-ux
Open

feat(icom): IC-705 bring-up — controls, UX, CI-V stall, audio transport, and transmit-meter staleness#4799
jensenpat wants to merge 2 commits into
mainfrom
feat/icom-bringup-controls-ux

Conversation

@jensenpat

@jensenpat jensenpat commented Aug 6, 2026

Copy link
Copy Markdown
Collaborator

Icom IC-705 network bring-up: the controls and UX round, the CI-V stall, the
audio transport, and one cross-cutting meter fix that turned out not to be an
Icom bug at all.

Certified against a live IC-705 over WiFi throughout. Where something is not
proven, this description says so rather than leaving it implied.

Controls and UX

  • RF Gain, preamp and attenuator now say what they mean. The gain slider had
    been driving the preamp for weeks, and the readout claimed "0, 1, 2 dB" for
    what are really radio-side steps. Gain reads %; preamp and attenuator get
    their own labelled rows and advance through every step rather than sticking
    at the first.
  • Manual notch wired to the radio's own notch (MN button, position
    control), replacing the Flex-only DSP buttons (ANF/NRL/ANFL/ANFT) that an
    Icom cannot do and which are now hidden on this family.
  • Filter buttons follow the mode and read narrow→wide: SSB 1.8/2.4/3.0k,
    CW 250/500/1.2k, AM & SAM 3/6/9k, FM 7/10/15k. They previously published one
    ladder for every mode, so three buttons reached one filter in AM and in CW.
  • Connect dialog: typing an IP no longer flips the radio type, Enter in the
    password field connects, and the field/label margins are fixed. The password
    is now staged and only saved after the connect succeeds — it used to be
    written before the attempt, so a failed connect destroyed the stored one.
  • Pan drag retunes smoothly past the zoomed margin instead of snapping back
    to centre; zoom alone leaves the VFO where it is.
  • Status bar shows the manufacturer above the model.
  • AF gain, TX monitor, VOX, RIT/XIT and the ATU are wired, and the radio's
    own values are adopted at connect rather than pushed from our defaults.

Tooling: the control registry and the linkage scrub

A half-wired control is indistinguishable from a working one by inspection —
that is how the RF-gain/preamp swap survived so long. IcomControls.h/.cpp is
now a declarative registry of every CI-V message this backend names, carrying
its wiring state (Both / SendOnly / DecodeOnly / Declared), raw and neutral
ranges, seam verb and UI target.

The automation bridge gained controls <map|meters|scrub>. The scrub drives
every settable control at its current value and watches the wire, so
"we believe this is wired" becomes "this reached the radio and it answered".
Nothing on the radio moves; PTT, ATU and power-off are never scrubbed.

It reports three outcomes, and the third is a real state rather than a failure:
LINKED, BROKEN, NOT-TESTED (a control that could not be re-asserted
without changing the operator's setting).

The 60–90 s CI-V stall

The session token was renewed at 45 s against a 60 s expiry with a single
fire-and-forget datagram, and m_authOk was set once and never cleared. On a
lossy link, losing that one packet silently expired the session while the
transport stayed healthy — the app reported connected, and no CI-V ever flowed
again.

Now: 20 s cadence, a 2.5 s ack grace with up to 4 retries, and a 50 s dead-session
failure. Proven stable across 250 s and four renewal windows. The backend
also warns once per stall naming the last command sent, so this fails loudly
next time.

Audio transport — mirroring kappanhang

Frame size is now derived from duration rather than hardcoded, txbuffer moved
200 → 300 ms, and the tracked pkt0 idles were dropped from the audio stream
(they competed with ~100 audio packets/sec for airtime and sequence space on a
link already showing 14.7% retries).

Which change fixed it is not established, and the design doc says so. The
frame derivation is a no-op at 48 kHz — it still computes 1920 — so it cannot
be responsible; it is insurance for a future rate change. That leaves the
txbuffer and the idles, changed together. Each is a one-line revert.

The low-bandwidth signalling path is left dead, as before, deliberately.
Verified along the way that Opus does not work against a real IC-705
wfview force-downgrades any codec ≥ 0x40 to LPCM16 unless it is talking to its
own server.

Meters: staleness is announced on the model's own clock

Forward power and SWR held their last in-transmission reading forever — value
and tooltip — with the radio back on receive.

The cause was not in the Icom backend. Every liveness gate in MeterModel
(swrValid, the freshness windows) was evaluated only on the path where a
packet arrives — correct arithmetic resting on an unstated assumption that
something keeps feeding it:

  • Streaming backends (Flex, HL2) keep landing packets while receiving, the
    gates re-run, and readings fall to rest for free.
  • Polling backends (Icom) rightly stop asking for TX-only meters at unkey.
    No packet, no emit, every gate frozen at its last verdict.

That is why swrValid did not save the SWR gauge despite being wired
correctly: it never got the chance to become false. MeterModel now watches
its own clock and announces the crossing once.

What it deliberately does not do is equally load-bearing and is asserted by
test: the stored values and every timestamp are untouched, so "MeterModel keeps
LAST-KNOWN values" stays true and "how old is this" still answers honestly. An
earlier attempt published zeros from the Icom backend on the unkey edge — the
wrong layer, and worse than the bug, because it refreshes the very timestamps
the gates read and would have turned "stale, invalid" into "live, reading zero".

Co-emit ordering with directionalPowerMetersChanged is preserved (#4243) and
the absent-SWR contract is followed exactly (#4536).

This likely also fixes HL2's stuck SWR, which gates on a noise floor and
stops publishing below it — the same producer-goes-quiet shape. Not confirmed
on HL2 hardware.

What is certified, and how

Phase State Evidence
Connection proved 30 controls adopted at connect; 20 assertions in icom_backend_test
RX proved FT8 decodes. Machine-verified: needs a genuinely-48 kHz rate and 15 s of unbroken continuity
TX proved Voice legible on an unrelated Kenwood TH-D75; FT8 spots on PSK Reporter confirm tone spacing and keying-edge timing
Meters partial All TX meters certified by effect against live voice; the staleness fix above is bench-tested, not yet re-flown

Both ends were on WiFi for the FT8 runs — two wireless hops, no wired leg, on a
link measuring 21.7 / 118.2 / 307.4 ms with 102.8 ms of jitter. That is the
adversarial case, not the easy one.

Transmit testing was into a 10 W dummy load on 7.200 MHz with the operator's
authorisation.

Not in this PR, deliberately

  • Zoom and band-change panadapter testing — never completed; it kept getting
    bumped by the stall work.
  • MOX was not separately verified (PTT was).
  • hl2_tx_loopback_test fails — 5 sideband/DSP assertions. Verified by
    stashing that it fails identically on a clean tree; unrelated to this work and
    worth its own issue.

Tests

Full suite 264/265, the single failure being the pre-existing
hl2_tx_loopback_test above. New coverage: the connect-time state pull, the
CI-V stall detector, pan drag/zoom/dead-zone, mode-dependent filter ladders, and
the transmit-meter stale announcement.

Why this PR touches Flex, HL2 and Sim

Reviewers will notice edits under backends/flex/, backends/hl2/ and
backends/sim/. No Flex or HL2 behaviour changes here. Both are mechanical
consequences of seam changes, and both are the kind that must be done in the
same commit or not at all.

1. setPanCenter gained a PanCenterIntent parameter.

The Icom needs to tell a DRAG from a ZOOM, because on that radio the scope
window is slaved to the VFO: a drag has to retune, a zoom must not. Nothing in
the old signature carried that distinction — a zoom arrives with a centre
attached exactly like a drag does.

It is a pure virtual on IRadioBackend, so every override had to move with it.
This is deliberately NOT a defaulted extra argument: with a default, an override
left at the old signature silently becomes an unrelated overload, stops
overriding, and the base no-op runs instead — a backend that quietly ignores
every pan-centre request, with nothing failing to build. Changing the signature
makes the compiler find all four implementations.

Flex and HL2 ignore the new parameter, and say so in one line each: their
panadapter window is genuinely independent of any slice, so a drag and a zoom
mean the same thing there. Behaviour is byte-for-byte what it was.

2. RadioCapabilities gained three fields.

manufacturer (for the status-bar row), hasLmsNoiseFilters and
hasManualNotch. The last two exist because hasRadioSideDsp was doing two
jobs: an Icom has radio-side DSP but not FlexRadio's WDSP LMS/FFT filters, so
that one flag was lighting up NRL/ANFL/ANFT buttons which reached no register
on the radio at all.

Each backend states all three explicitly rather than letting them default,
because the struct's own documented rule is that a backend which omits a
capability silently declares it absent
— and a silent absence is precisely
the failure mode this PR spent its time chasing elsewhere. So Flex declares
hasLmsNoiseFilters = true / hasManualNotch = false (a Flex notches with
TNFs, which are pinned to absolute frequencies and are a different instrument),
and HL2 declares both false with a note that they are moot while its
hasRadioSideDsp is false, since the host runs every filter it has.

3. MeterModel is shared, and that one IS a behaviour change.

Covered in its own section above. It is in the shared model rather than the
Icom backend on purpose — the bug is a property of the reactive design, not of
Icom — and it is the only cross-cutting change here that a Flex or HL2 operator
could observe. Full meter-model test suite passes, including every pre-existing
SWR and staleness test, which is the regression check on the Flex path. It
should additionally fix HL2's stuck SWR; not confirmed on HL2 hardware.

Post-review fix

A review pass over this branch found one real bug in it, now fixed in
fa129e6d: the control scrub's noise-reduction branch cleared its dedupe
sentinel before reading it, so the scrub asked for NR off rather than
re-asserting it. An operator with noise reduction on who ran controls scrub
documented as leaving the radio untouched — had it silently switched off, and
the scrub then reported the row LINKED because the intent did reach the wire.
The three sibling branches already had the correct shape.

@jensenpat
jensenpat marked this pull request as ready for review August 6, 2026 18:54
@jensenpat
jensenpat requested review from a team as code owners August 6, 2026 18:54

@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.

Very strong bring-up work, and the write-up does the thing I most want from a PR this size: it separates what was proven on hardware from what was reasoned about. The MeterModel diagnosis in particular is correct at the right layer — "every liveness gate was evaluated only on the arrival path" is a real property of the reactive design, not an Icom quirk, and putting the fix in the shared model with the stored values and timestamps left untouched is the right call. The control registry plus controls scrub is a genuinely good idea; the fact that the first thing it caught was its own NR bug (fa129e6d) is a point in its favour, not against it.

The Flex/HL2/Sim churn is justified and I checked each one is a signature move with behaviour byte-for-byte intact. Three things I'd like addressed before merge, all in the Icom layer.

Would like fixed before merge

  • IcomSession::onTokenRenew — once the 4 retries are spent, m_renewUnacked stays latched, so no further renewal is ever attempted and the session coasts into the 50 s teardown even if the link recovers at 31 s. This partly defeats the fix's own purpose.
  • The preamp and attenuator ladders are published as IC-705 values for every Icom model, and setPanAttenuator hardcodes 20 dB — an IC-7610 has a multi-step attenuator, and this is the same "claim the radio never made" shape the PR spent its time removing. powerCurveFor() already models the honest pattern.
  • scrubDrive still reads an unknown enable sentinel (-1) as "off" for anf/notch (and nr/nb), so on a session where the connect-time read was lost the scrub switches the operator's filter off — fa129e6d one step out.

Polish

  • MeterModel::checkTxMeterStaleness will also zero the gauges mid-transmission on a streaming backend if meter packets gap past 2 s.
  • setPanCenter does take a defaulted argument, contrary to the PR description's rationale — and the default is Range, the silent-no-retune value.
  • kAudioFrameBytes is derived from a compile-time rate while StreamRequest::sampleRateHz is a runtime field, so the static_assert cannot catch the failure its own comment describes.

Non-blocking notes

  • controls scrub writes TX-plane registers but isn't TX-gated, unlike civ send.
  • A staged Icom password commits on the next successful connect of any family.
  • CodeGuard's five CG-PATH-001 hits on MainWindow.cpp (8082/8194/8313-8315) are all outside this diff — they land in the pre-existing FFTW-wisdom dialog and the macOS open -n relaunch built from QCoreApplication::applicationFilePath(). Not attacker-influenced, not touched here; dropping them.

hl2_tx_loopback_test failing identically on a clean tree matches what I'd expect — please do open that issue so it stops shadowing other work.


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

Comment on lines +415 to +426
if (m_renewUnacked) {
if (m_renewRetries < kTokenRenewMaxRetries) {
++m_renewRetries;
qCWarning(lcIcom) << "token renewal unacknowledged after"
<< kTokenAckGraceMs << "ms — resend" << m_renewRetries
<< "of" << kTokenRenewMaxRetries;
m_control->sendTracked(buildAuth(m_control->localSessionId(),
m_control->remoteSessionId(),
m_innerSeq++, m_authId, AuthKind::Renew));
}
return;
}

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.

The latch never releases. Once m_renewRetries reaches kTokenRenewMaxRetries this branch returns having done nothing, and m_renewUnacked stays true forever — so the normal-cadence branch below can never run again either. The session is then guaranteed to be torn down at kTokenDeadMs, even though the radio's token is good until 60 s.

Concretely: renewal goes out at t=20 s, WiFi drops for 10 s, the four resends at 22.5/25/27.5/30 s are all lost. At t=31 s the link is healthy again and a single fresh renewal would save the session — but nothing is sent between t=30 s and the fail() at t=50 s, and the operator gets a full disconnect/teardown for a 10 s outage that was already over.

Releasing the latch after the cap makes the cadence branch below start a fresh renewal (with a fresh retry budget), which is effectively continuous retry until the dead-session deadline:

Suggested change
if (m_renewUnacked) {
if (m_renewRetries < kTokenRenewMaxRetries) {
++m_renewRetries;
qCWarning(lcIcom) << "token renewal unacknowledged after"
<< kTokenAckGraceMs << "ms — resend" << m_renewRetries
<< "of" << kTokenRenewMaxRetries;
m_control->sendTracked(buildAuth(m_control->localSessionId(),
m_control->remoteSessionId(),
m_innerSeq++, m_authId, AuthKind::Renew));
}
return;
}
if (m_renewUnacked) {
if (m_renewRetries < kTokenRenewMaxRetries) {
++m_renewRetries;
qCWarning(lcIcom) << "token renewal unacknowledged after"
<< kTokenAckGraceMs << "ms — resend" << m_renewRetries
<< "of" << kTokenRenewMaxRetries;
m_control->sendTracked(buildAuth(m_control->localSessionId(),
m_control->remoteSessionId(),
m_innerSeq++, m_authId, AuthKind::Renew));
return;
}
// Retries exhausted for THIS renewal — but the token is alive until
// kTokenDeadMs, so release the latch and let the cadence below start a
// fresh one rather than coasting into the teardown.
m_renewUnacked = false;
m_renewRetries = 0;
}

Comment on lines +400 to +407
emit panPreampInfoChanged(panId(), {QStringLiteral("OFF"),
QStringLiteral("P.AMP1"),
QStringLiteral("P.AMP2")});
// ONE step, and naming it in dB is honest here where it was not for the
// preamp: the guide gives this attenuator an actual figure. HF and 50 MHz
// only — on higher bands the radio ignores the request and reports OFF.
emit panAttenuatorInfoChanged(panId(), {QStringLiteral("OFF"),
QStringLiteral("20 dB")});

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.

These are IC-705 ladders published to every Icom. panPreampInfoChanged / panAttenuatorInfoChanged are emitted unconditionally from the connect path, but the front-end stages are per-model: an IC-7610 has a multi-step attenuator (not one 20 dB position), and the IC-9700's preamp ladder differs from the HF ladder here. On those radios the operator gets a labelled "20 dB" button that misdescribes what the register does — which is exactly the class of defect this PR's registry exists to make visible.

The codebase already has the honest pattern one file over: powerCurveFor() returns empty for a model it has no curve for, and publishMeterDefs() then falls back to Percent rather than fabricating watts. Same shape works here — put the ladders in IcomModel (or a preampLabelsFor(model) / attenStepsFor(model) helper keyed on civAddress, IC-705 populated and others empty), and emit nothing when the model has no entry, so the control simply doesn't appear rather than appearing wrong. setPanAttenuator's hardcoded wanted ? 20 : 0 needs the same treatment.

Given #4786/#4791 are actively driving an IC-9700 through this backend, this will be hit.

Comment on lines +1878 to +1889
if (id == QLatin1String("anf")) {
const bool on = m_anfEnableSent == 1;
m_anfEnableSent = -1;
setSliceAutoNotch(slice, on);
return true;
}
if (id == QLatin1String("notch") || id == QLatin1String("notch.pos")) {
const bool on = m_mnEnableSent == 1;
m_mnEnableSent = -1;
setSliceManualNotch(slice, on, m_notchPosPercent);
return true;
}

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.

Unknown is being read as off. fa129e6d fixed the read-after-clear ordering, but the -1 (never established) state still collapses to on = false, so the scrub sends the filter OFF. The sentinels are populated by the connect-time adoption at lines 690–708 — and on the very link this PR is about, that read is a single datagram that can be lost. An operator with ANF or MN on, whose connect-time read didn't come back, runs controls scrub (documented as leaving the radio untouched) and has it switched off, reported LINKED.

NOT-TESTED is the honest outcome — the same third state the function's own header argues for:

Suggested change
if (id == QLatin1String("anf")) {
const bool on = m_anfEnableSent == 1;
m_anfEnableSent = -1;
setSliceAutoNotch(slice, on);
return true;
}
if (id == QLatin1String("notch") || id == QLatin1String("notch.pos")) {
const bool on = m_mnEnableSent == 1;
m_mnEnableSent = -1;
setSliceManualNotch(slice, on, m_notchPosPercent);
return true;
}
if (id == QLatin1String("anf")) {
// UNKNOWN IS NOT OFF. If the connect-time read never landed we have no
// current value to re-assert, and treating -1 as off would switch the
// operator's notch off — fa129e6d one step out. NOT-TESTED is honest.
if (m_anfEnableSent < 0)
return false;
const bool on = m_anfEnableSent == 1;
m_anfEnableSent = -1;
setSliceAutoNotch(slice, on);
return true;
}
if (id == QLatin1String("notch") || id == QLatin1String("notch.pos")) {
if (m_mnEnableSent < 0)
return false;
const bool on = m_mnEnableSent == 1;
m_mnEnableSent = -1;
setSliceManualNotch(slice, on, m_notchPosPercent);
return true;
}

The nr and nb branches just above need the same guard on the bare-nr/nb path (their .level paths are already covered by the != 1 check, which happens to reject -1 too).

Comment thread src/models/MeterModel.cpp
Comment on lines +124 to +126
// means no forward power.
emit txMetersChanged(0.0f, 0.0f, false);
emit directionalPowerMetersChanged(0.0f, 0.0f, 0.0f, false, false);

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.

This fires on any 2 s gap in transmit-meter packets, not only at unkey — and for a streaming backend a gap mid-transmission is reachable. On a lossy Flex or HL2 link, a >2 s stall during a long transmission now drops the forward-power gauge to 0 W and invalidates SWR while the operator is still keyed, recovering on the next packet. Previously it held the last reading, which for that case was the better answer.

The model has no TX-state input today, so I wouldn't block on it, but it's worth either gating the announcement on "not currently transmitting" (a setTransmitting(bool) seam the RadioModel already knows) or widening the window for streaming backends — otherwise the fix trades a stuck reading at rest for a wrong reading under load, and the second one is on-air.

Comment thread src/core/backends/IRadioBackend.h Outdated
Comment on lines +140 to +141
virtual void setPanCenter(const QString& panId, double hz,
PanCenterIntent intent = PanCenterIntent::Range) = 0;

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.

The PR description says this is "deliberately NOT a defaulted extra argument" because a default would let an override silently stop overriding — but the declaration does carry = PanCenterIntent::Range. In practice the pure-virtual-plus-override combination is what caught all four implementations, not the absence of a default.

Worth reconciling one way or the other, because the default has its own edge: a new call site that forgets the intent gets Range, which on the Icom path means the drag is refused and re-asserted — the exact bug this parameter was added to fix, arriving silently. Drag would at least fail loudly (an unwanted retune) rather than quietly, or dropping the default matches what the description claims.

Comment on lines +441 to +442
inline constexpr std::size_t kAudioFrameBytes =
static_cast<std::size_t>(kAudioRateHz) * kAudioSampleBytes * kAudioFrameMs / 1000; // 1920

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.

The derivation reads from kAudioRateHz, a compile-time constant, while the rate that actually goes out on the wire is StreamRequest::sampleRateHz — a runtime field. So the static_assert below cannot catch the failure its own comment describes: someone setting sampleRateHz = 16000 (which RadioModel's low-bandwidth comment says is the eventual plan) still gets 1920-byte frames, still gets 60 ms of audio per frame, and still gets the silent zero-power transmit. The build stays green.

A runtime check at the point the request is built — reject or clamp when sampleRateHz != kAudioRateHz — is what would actually hold the invariant. Cheap to add now, and it's the guard the low-bandwidth work will need anyway.

Comment on lines +6759 to +6761
return err(QStringLiteral("controls requires an action (map|meters|scrub)"));

// Same synchronous-extension contract doCiv documents: a backend that does

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.

civ send is gated on m_txAllowed a few dozen lines down, with the reasoning that raw CI-V can key the transmitter. controls scrub isn't gated, and it does write to the radio — including the transmit plane (tx.power, mic.gain, monitor, vox, comp) and setSliceFrequency.

I think the exclusion list plus re-asserting current values genuinely makes this safe, so I'm not asking for a gate. But it's worth saying so explicitly here rather than leaving the asymmetry with doCiv unexplained — the next person to read these two side by side will assume one of them is an oversight. (map and meters are read-only and clearly fine either way.)

Comment thread src/gui/ConnectionPanel.cpp Outdated
Comment on lines +1220 to +1224
if (connected && !m_pendingIcomPassword.isEmpty()) {
IcomCredentials::save(m_pendingIcomPassword);
if (!m_pendingIcomHost.isEmpty())
IcomSettings::setLastHost(m_pendingIcomHost);
}

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.

The commit isn't scoped to the family that was actually connected. Stage an Icom password, have the attempt fail in a way that doesn't drive a setConnected(false) edge (the panel is already disconnected, so a state-change-only caller wouldn't), then connect successfully to a Flex — and this writes the unproven Icom password to the keychain and moves IcomSettings::setLastHost to the Icom host.

Not damaging, but it's the failure mode the staging was added to prevent, so worth closing: check the connected family before committing, and clear the staged pair on the connect attempt's failure signal rather than relying on the disconnected edge.

@aethersdr-agent

Copy link
Copy Markdown
Contributor

Cisco CodeGuard — static analysis of this PR (5 finding(s))

  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-4799/src/gui/MainWindow.cpp:8082
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-4799/src/gui/MainWindow.cpp:8194
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-4799/src/gui/MainWindow.cpp:8313
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-4799/src/gui/MainWindow.cpp:8314
  • [MEDIUM] CG-PATH-001 — Potential path traversal in src/gui/MainWindow.cpp /tmp/aetherclaude/pr-4799/src/gui/MainWindow.cpp:8315

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.6648 · model: claude-opus-5

@nigelfenton

Copy link
Copy Markdown
Contributor

Opened #4815 for the hl2_tx_loopback_test request in the review above.

Worth flagging what I found, because it changes the shape of the ask: I could not reproduce the failure, and the reason is itself the bug.

The test targets a hardcoded 192.168.1.12, with no environment override. On this machine (a 10.0.0.0/24 LAN) it exits 0 on every run, having tested nothing — the skip path returns 0 with a stderr message, which is indistinguishable from a pass to CI and to anyone reading a terminal.

So it has three behaviours and nothing outside distinguishes the second from the third:

Result
Nothing answers SKIPPED, exit 0 — reads as a pass
A real HL2 simulator answers genuinely runs
Something else answers at that address runs against the wrong device and fails

The presence probe is looser than the hardcoded IP alone implies — simPresent() returns s.waitForReadyRead(1500), so any datagram arriving on that socket counts. It never checks the sender address and never parses the payload as a Metis discovery reply.

That is more consequential here than in a normal test: past that gate it deliberately qunsetenv("AETHER_AUTOMATION")s to open the TX gate and asserts canTransmit. It is a transmit test pointed at whatever happened to answer.

Which is very likely why it fails on your tree and passes on mine — different subnets, different neighbours.

Suggested fixes are in #4815; the cheapest and most valuable is making the skip visible as a skip, since a silent exit-0 is exactly what let it shadow this review.

Not touching it in this PR — it is pre-existing and unrelated to the Icom work here.

jensenpat added a commit that referenced this pull request Aug 7, 2026
Seven findings from the review on #4799. Two of them are mine one step short,
and one contradicts a claim I made in the PR description.

1. THE STALE WATCH FIRED MID-TRANSMISSION (the blocker).

The window is 2 s, and a 2 s gap in meter packets is reachable during a
transmission on a lossy streaming link. As written, the watch would zero the
forward-power gauge and invalidate SWR with the operator still keyed —
trading a stuck reading at rest for a wrong reading under load, and only the
second one is on the air. MeterModel now holds while transmitting, driven by
TransmitModel::transmittingChanged (which covers VOX, break-in and a
footswitch, not just an explicit MOX). The freeze this exists for happens at
UNKEY, which is exactly where the guard stops applying, so nothing is lost.

A backend that never reports transmit state gets the un-gated behaviour, which
is the correct default for a polling backend.

2. THE TOKEN-RENEWAL LATCH NEVER RELEASED.

Once retries were exhausted m_renewUnacked stayed true forever, so the cadence
branch could never run again and the session coasted into a guaranteed
teardown. A 10 s outage at t=20 s lost the four resends and then sat silent
from t=30 s until fail() at t=50 s — a full disconnect for an outage that had
been over for twenty seconds. The latch is now released after the cap, so a
fresh renewal starts with a fresh budget: effectively continuous retry up to
the dead-session deadline.

3. UNKNOWN WAS STILL BEING READ AS OFF IN THE SCRUB.

fa129e6 fixed the read-after-clear, but -1 (never established) still collapsed
to false, so the scrub SENT "off". On this backend's own lossy link the
connect-time read is one datagram; lose it and an operator with NR, ANF or the
notch running has it switched off by a check documented to leave the radio
untouched — reported LINKED, because the intent did reach the wire. All four
branches now return NOT-TESTED for unknown, which is the state the scrub
already has for exactly this.

4. IC-705 FRONT-END LADDERS WERE PUBLISHED TO EVERY ICOM.

The preamp and attenuator labels were IC-705 literals emitted unconditionally,
so an IC-7610 (multi-step attenuator) or IC-9700 (different preamp ladder) got
a control that misdescribed its own register — the defect class this PR's
registry exists to surface, reintroduced by the fix for it, and #4786/#4791 are
driving an IC-9700 through this backend now. Moved to preampLabelsFor() /
attenStepsFor() in IcomModels, following powerCurveFor's rule exactly: no
verified table means publish nothing, so the operator gets no button rather
than a lying one. setPanAttenuator now takes the wire dB from that table
instead of a hardcoded 20, and the decode maps dB back through it.

5. THE static_assert COULD NOT CATCH WHAT ITS COMMENT CLAIMED.

It reads kAudioRateHz, a compile-time constant, while the rate that reaches the
wire is the runtime StreamRequest::sampleRateHz. Setting that to 16000 still
produced 1920-byte frames, still 60 ms per frame, and still the silent
zero-power transmit — with the build green. There is now a runtime clamp where
the request is actually built.

6. THE CREDENTIAL COMMIT WAS NOT SCOPED TO THE CONNECTED FAMILY.

Stage an Icom password, have the attempt fail without driving a disconnected
edge, then connect to a Flex, and the unproven password was written over a
working keychain entry — the failure the staging was added to prevent, one step
removed. The commit now requires the manual family to still be Icom, and every
other connect path clears the staged pair, so it cannot outlive its own attempt.

7. THE INTENT PARAMETER DID CARRY A DEFAULT.

The PR description said it was "deliberately NOT a defaulted extra argument".
That was simply wrong — it read `= PanCenterIntent::Range`. The reviewer is
also right about the hazard: a caller that omitted it silently got Range, which
on the Icom means the drag is refused and re-asserted, the exact bug the
parameter was added to fix. The default is gone, which is what the description
claimed and what the one call site already did.

Also documented why `controls scrub` is not TX-gated where `civ send` is — it
drives named seam verbs rather than raw frames, and ptt/tuner/power are
excluded from the walk — since the asymmetry otherwise reads as an oversight.

The CodeGuard path-traversal findings are on MainWindow.cpp:8082-8315, outside
every hunk in this PR (they stop at ~6749); the scanner scans changed files
rather than diffs.

Tests 264/265, the one failure being hl2_tx_loopback_test, which #4815 now
explains: it targets a hardcoded 192.168.1.12 and its presence probe accepts any
datagram without checking sender or payload. This LAN is 192.168.1.0/24 and
something does answer at .12, so it runs a transmit test against whatever that
is. Pre-existing and unrelated; not touched here.
jensenpat pushed a commit that referenced this pull request Aug 7, 2026
…ver the seam (#4812)

**Live on `main`.** The built-in AX.25 modem cannot transmit at all on a
backend that declares `takesTxAudioOverSeam` — Icom today. PTT never
keys, no audio is sent, and nothing logs an error.

Found operating packet on an IC-9700 this evening.

## What happens

The modem gates its transmit path on `hostModulates`, which asks **who
runs the modulator**. The question it needs to ask is **whether transmit
audio needs a DAX stream** — and those came apart when
`takesTxAudioOverSeam` was added.

On an Icom `hostModulates` is `false`, and correctly so: the *radio*
modulates. So:

1. The modem takes the DAX branch and asks for a stream a networked Icom
has never had
2. `RadioModel::ensureDaxTxStream()` returns **true** for a seam backend
— *"there IS a route for transmit audio, it just isn't DAX"*
3. So the modem's failure path never fires either. `m_txPendingStream`
stays set, waiting on a stream that cannot arrive, until the wait times
out

Every layer behaves correctly by its own lights, which is why it is
silent. From the operator's side the modem simply does nothing.

## Why it looks familiar

The call site already documents this outage for the HL2 (2026-07-31:
*"PTT never keyed, 181 audio chunks never sent"*). This is the same
failure reached from the opposite direction — the HL2 was excluded
because it **host-modulates**, and a seam backend needs excluding
because its audio **does not go through DAX at all**.

## The change

`hostModulatesTx()` → `txAudioBypassesDax()`, returning `hostModulates
|| takesTxAudioOverSeam`.

Renamed because the name no longer described what it answers, and both
of its callers are asking the DAX question rather than the modulator
one. It is private to this dialog — nothing else reads it.

## Verified on hardware — IC-9700 over LAN, 2026-08-06

| | Before | After |
|---|---|---|
| DAX stream requests | 1 per frame, never satisfied | **0** |
| PTT | never keyed | **keys** |
| AX.25 T1 retry ladder | never started | **runs 8/8** — frames going
out and being timed |
| TX meters (FWDPWR / SWR / ALC) | stale | **LIVE at 0.9 s** |

Confirmed **modulating**, not a bare carrier — a beacon and a connect
attempt to a live station both went out with audio.

The T1 timeouts are expected here: the station did not answer. What
matters is that the ladder runs at all, which it could not do before.

## Scope

Affects any backend declaring `takesTxAudioOverSeam`, so Icom today and
the same shape for anything added later. No change for Flex or the sim,
which declare it false, or for the HL2, which was already excluded by
`hostModulates`.

Related: #4799 introduces `takesTxAudioOverSeam` and wires it into
`ensureDaxTxStream`; this caller was left on the old flag. The
capability and the Icom's declaration are already on `main`, so this
stands alone and does not depend on that PR.
…rt, meters

Certified against a live IC-705 over WiFi throughout. Where something is not
proven, this says so rather than leaving it implied.

CONTROLS AND UX

RF gain, preamp and attenuator now say what they mean. The gain slider had been
driving the preamp, labelling three radio-side steps "0/1/2 dB" — none of which
is a decibel of anything. Gain reads %; preamp and attenuator get their own
labelled rows and advance through every step instead of sticking at the first.

Manual notch wired to the radio's own (16 48 enable, 14 0D position, 16 57
width), replacing the Flex-only WDSP buttons (ANF/NRL/ANFL/ANFT) that reach no
register on this radio and are now hidden for the family.

Filter buttons follow the mode and read narrow->wide: SSB 1.8/2.4/3.0k, CW
250/500/1.2k, AM and SAM 3/6/9k, FM 7/10/15k. One fixed ladder was published for
every mode, so three buttons reached one filter in AM and one in CW.

Connect dialog: typing an IP no longer flips the radio type, Enter connects from
the credential fields, and the field margins are fixed. The password is staged
and saved only AFTER the connect succeeds — it used to be written before the
attempt, so a failed connect destroyed a working keychain entry permanently.

Pan drag retunes past the zoomed margin instead of snapping back; zoom alone
leaves the VFO alone. Status bar shows the manufacturer above the model. AF
gain, TX monitor, VOX, RIT/XIT and the ATU are wired, and the radio's own values
are adopted at connect rather than pushed from our defaults.

TOOLING: THE CONTROL REGISTRY AND LINKAGE SCRUB

A half-wired control is indistinguishable from a working one by inspection —
that is how the RF-gain/preamp swap survived. IcomControls.h/.cpp is a
declarative registry of every CI-V message this backend names, with its wiring
state, raw and neutral ranges, seam verb and UI target. The bridge gains
`controls <map|meters|scrub>`; the scrub drives every settable control at its
CURRENT value and watches the wire, reporting LINKED / BROKEN / NOT-TESTED.
Nothing on the radio moves, and ptt/tuner/power are never scrubbed.

THE 60-90 s CI-V STALL

The token was renewed at 45 s against a 60 s expiry with one fire-and-forget
datagram, and m_authOk was set once and never cleared. Losing that packet
silently expired the session while the transport stayed healthy: connected,
statistics climbing, CI-V dead. Now 20 s cadence, 2.5 s ack grace, up to 4
retries, 50 s dead-session failure. Stable across 250 s and four renewals.

AUDIO TRANSPORT — MIRRORING KAPPANHANG

Frame size derived from duration rather than hardcoded, txbuffer 200 -> 300 ms,
and the tracked pkt0 idles dropped from the audio stream. WHICH change fixed it
is not established and the design doc says so: the frame derivation is a no-op
at 48 kHz, so it cannot be responsible. The low-bandwidth path stays dead;
verified that Opus does not work against real Icom hardware.

METERS: STALENESS ANNOUNCED ON THE MODEL'S OWN CLOCK

Forward power and SWR held their last in-transmission reading forever. The cause
was NOT in the Icom backend: every liveness gate in MeterModel was evaluated
only when a packet arrived — correct arithmetic resting on an unstated
assumption that something keeps feeding it. Streaming backends keep landing
packets and fall to rest for free; a polling backend rightly stops asking for
TX-only meters at unkey, so every gate freezes at its last verdict. MeterModel
now watches its own clock and announces the crossing once, without touching the
stored values or timestamps.

WHY THIS TOUCHES FLEX, HL2 AND SIM

No Flex or HL2 behaviour changes. setPanCenter gained a PanCenterIntent
parameter — deliberately NOT defaulted, because a default would let an override
left at the old signature silently become an overload, stop overriding, and run
the base no-op. RadioCapabilities gained manufacturer, hasLmsNoiseFilters and
hasManualNotch, stated explicitly by every backend because the struct's rule is
that an omitted capability silently declares itself absent. MeterModel is the
one cross-cutting change a Flex or HL2 operator could observe.

CERTIFIED

Connection and RX proved (FT8 decodes — machine-verified, needs a genuinely
48 kHz rate and 15 s of unbroken continuity). TX proved: voice legible on an
unrelated Kenwood TH-D75, and FT8 spots on PSK Reporter confirming tone spacing
and keying-edge timing. Both ends on WiFi, two wireless hops, on a link
measuring 21.7/118.2/307.4 ms with 102.8 ms of jitter. Transmit testing into a
10 W dummy load on 7.200 MHz with the operator's authorisation.

Meters partial: all TX meters certified by effect against live voice; the
staleness fix is bench-tested, not yet re-flown.

NOT INCLUDED, DELIBERATELY

Zoom and band-change panadapter testing was never completed. MOX was not
separately verified. hl2_tx_loopback_test fails — 5 sideband/DSP assertions,
verified by stashing to fail identically on a clean tree, unrelated to this work
and worth its own issue.

Tests 264/265, the one failure being that pre-existing hl2_tx_loopback_test.
Seven findings from the review on #4799. Two of them are mine one step short,
and one contradicts a claim I made in the PR description.

1. THE STALE WATCH FIRED MID-TRANSMISSION (the blocker).

The window is 2 s, and a 2 s gap in meter packets is reachable during a
transmission on a lossy streaming link. As written, the watch would zero the
forward-power gauge and invalidate SWR with the operator still keyed —
trading a stuck reading at rest for a wrong reading under load, and only the
second one is on the air. MeterModel now holds while transmitting, driven by
TransmitModel::transmittingChanged (which covers VOX, break-in and a
footswitch, not just an explicit MOX). The freeze this exists for happens at
UNKEY, which is exactly where the guard stops applying, so nothing is lost.

A backend that never reports transmit state gets the un-gated behaviour, which
is the correct default for a polling backend.

2. THE TOKEN-RENEWAL LATCH NEVER RELEASED.

Once retries were exhausted m_renewUnacked stayed true forever, so the cadence
branch could never run again and the session coasted into a guaranteed
teardown. A 10 s outage at t=20 s lost the four resends and then sat silent
from t=30 s until fail() at t=50 s — a full disconnect for an outage that had
been over for twenty seconds. The latch is now released after the cap, so a
fresh renewal starts with a fresh budget: effectively continuous retry up to
the dead-session deadline.

3. UNKNOWN WAS STILL BEING READ AS OFF IN THE SCRUB.

fa129e6 fixed the read-after-clear, but -1 (never established) still collapsed
to false, so the scrub SENT "off". On this backend's own lossy link the
connect-time read is one datagram; lose it and an operator with NR, ANF or the
notch running has it switched off by a check documented to leave the radio
untouched — reported LINKED, because the intent did reach the wire. All four
branches now return NOT-TESTED for unknown, which is the state the scrub
already has for exactly this.

4. IC-705 FRONT-END LADDERS WERE PUBLISHED TO EVERY ICOM.

The preamp and attenuator labels were IC-705 literals emitted unconditionally,
so an IC-7610 (multi-step attenuator) or IC-9700 (different preamp ladder) got
a control that misdescribed its own register — the defect class this PR's
registry exists to surface, reintroduced by the fix for it, and #4786/#4791 are
driving an IC-9700 through this backend now. Moved to preampLabelsFor() /
attenStepsFor() in IcomModels, following powerCurveFor's rule exactly: no
verified table means publish nothing, so the operator gets no button rather
than a lying one. setPanAttenuator now takes the wire dB from that table
instead of a hardcoded 20, and the decode maps dB back through it.

5. THE static_assert COULD NOT CATCH WHAT ITS COMMENT CLAIMED.

It reads kAudioRateHz, a compile-time constant, while the rate that reaches the
wire is the runtime StreamRequest::sampleRateHz. Setting that to 16000 still
produced 1920-byte frames, still 60 ms per frame, and still the silent
zero-power transmit — with the build green. There is now a runtime clamp where
the request is actually built.

6. THE CREDENTIAL COMMIT WAS NOT SCOPED TO THE CONNECTED FAMILY.

Stage an Icom password, have the attempt fail without driving a disconnected
edge, then connect to a Flex, and the unproven password was written over a
working keychain entry — the failure the staging was added to prevent, one step
removed. The commit now requires the manual family to still be Icom, and every
other connect path clears the staged pair, so it cannot outlive its own attempt.

7. THE INTENT PARAMETER DID CARRY A DEFAULT.

The PR description said it was "deliberately NOT a defaulted extra argument".
That was simply wrong — it read `= PanCenterIntent::Range`. The reviewer is
also right about the hazard: a caller that omitted it silently got Range, which
on the Icom means the drag is refused and re-asserted, the exact bug the
parameter was added to fix. The default is gone, which is what the description
claimed and what the one call site already did.

Also documented why `controls scrub` is not TX-gated where `civ send` is — it
drives named seam verbs rather than raw frames, and ptt/tuner/power are
excluded from the walk — since the asymmetry otherwise reads as an oversight.

The CodeGuard path-traversal findings are on MainWindow.cpp:8082-8315, outside
every hunk in this PR (they stop at ~6749); the scanner scans changed files
rather than diffs.

Tests 264/265, the one failure being hl2_tx_loopback_test, which #4815 now
explains: it targets a hardcoded 192.168.1.12 and its presence probe accepts any
datagram without checking sender or payload. This LAN is 192.168.1.0/24 and
something does answer at .12, so it runs a transmit test against whatever that
is. Pre-existing and unrelated; not touched here.
@jensenpat
jensenpat force-pushed the feat/icom-bringup-controls-ux branch from c250248 to dfe4a62 Compare August 8, 2026 00:09
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