Skip to content

Commit c2a1322

Browse files
on8stclaude
andauthored
fix(seam): gate construction-time mic push on hostModulates — Principle II. (#5643)
## Summary `RadioModel::setupBackend()` hands a freshly built backend the mic level the model already holds, so a host modulator constructed at its own unity default cannot silently part from the slider. That push was gated on `!usesFlexCommandPlane()`. This replaces it with `backendCapabilities().hostModulates`. One line of behaviour, one file plus its family test. Nothing else in the diff. **It is the same change #5505 prepared and deliberately did not file** ("One thing split out, and NOT filed" — the gate was reverted there because, by the new localization rule, a wrong seam gate is capability-shaped work for its own PR rather than a drive-by). This is that PR. ## Why the gate was wrong Its own comment says what it is for: *"Same Flex gate as the seam: on a Flex the slider's `transmit set miclevel=` reaches the radio's own preamp and this must not double it."* The copy is the defect — **the two sites do not ask the same question.** The seam's gate is **de-duplication**. `TransmitModel::setMicLevel()` emits `transmit set miclevel=` through `commandReady` *beside* `micLevelCommandIssued`, so on a Flex the seam would issue a second copy of a command the wire text already carried. Excluding Flex there is correct. Nothing is in flight at construction. `setupBackend()` emits no wire text at all, so "did the Flex text path already carry this?" is not a question this site has. The question it has is **whether the fresh backend has a host modulator standing at its own default, waiting to be told where the operator left the slider** — which is exactly what `RadioCapabilities::hostModulates` answers, and what its declaration was written for: > `hostModulates` answers "does the HOST run the modulator" — true for the HL2, > false for an Icom, whose own firmware modulates. That declaration also carries the warning that conflating it with the flag beside it "cost a working transmitter". This reads the same flag for the question it was written to answer. **Flex is not the only family whose radio owns the modulator.** `IcomCivBackend` declares `hostModulates = false`, and `IcomCivBackend::setMicGain` is a live CI-V `14 0B` write to the radio's own MIC GAIN register — or, on an IC-9700 with LAN as the active modulation input, a SET `0114` LAN MOD write. So a gate written to keep a client-side slider away from a radio that owns its own modulator was handing it to one. ## Live bug, or correctness of layering? — **layering, with a real but bounded observable.** No frame reaches the radio from this site. This was checked before the code was written, because it decides how the PR is written. Three findings, in order: **1. `IcomCivBackend::setMicGain`'s own guards do NOT defend the radio in general.** The `phoneLevelFollowsNetworkInput` / `networkOnlyValue` branch — the one that refuses with *"ignoring Phone level change: LAN MOD readback is not established"* — is entered only when the model profile declares `phoneLevelFollowsNetworkInput`, and **only the IC-9700 does** (`IcomModels.cpp`: `kIc9700Profile`'s `ModulationProfile` sets it; the IC-705 and IC-7300MK2 profiles leave it at its `false` default). On every other Icom — and on an IC-9700 whose active MOD input is MIC, USB or ACC rather than LAN — the function falls straight through to an unguarded `sendUserCommand(cmdSetLevel(addr, level::kMicGain, …))`. Even inside the guarded branch the refusal lapses the moment the connect-time LAN readback lands, after which the client's number is written to SET `0114`. **2. What actually stops this particular push is somewhere else.** `IcomCivBackend::sendUserCommand` returns early on `!m_session || !m_connected`, and `setupBackend()` always runs against a backend `makeBackend()` constructed moments earlier, which has no session. **No CI-V frame leaves the host from this site, nothing goes on the air, and no radio-persisted register is written.** That is a guard two layers down from the gate, holding for a reason unrelated to mic gain — which is precisely what makes this a layering fix rather than a hazard report. **3. It is not, however, inert.** `setMicGain` writes its mirror *above* that guard: `m_micGainPercent = gainPercent` and `m_micGainReported = true` are on the lines before the send. `m_micGainReported` is cleared only in `onSessionDisconnected`, which has not run on a backend that was just built — so a false *"the radio reported this"* survives into the new session until the connect-time read of `14 0B` arrives, which on the lossy link this backend exists for is one datagram. In that window `IcomCivBackend::healthSnapshot` prints the client's number in its "MIC Gain" row as radio state, and `publishPhoneModulationLevel` will echo it back over `transmitChanged`. So: **a lying Radio Health readout, not a moved transmitter.** Stating it as a live on-air hazard would be overstating it, and stating it as cosmetic would be understating it. ## What an Icom operator will notice **Changed.** A backend rebuild no longer seeds the Icom's mic-gain mirror. Radio Health's "MIC Gain" row reads **not reported** from the family switch until the radio's own `14 0B` reply lands, instead of showing a number carried in from the previous radio. Nothing on the air moves, in either direction. **Unchanged, and deliberately so.** The Phone MIC slider is still a live control on an Icom. `micLevelCommandIssued` is operator intent and is untouched, so moving the slider still reaches `14 0B` (or SET `0114`) exactly as before — `IcomControls` registers `mic.gain` as `Wiring::Both` bound to `phoneMicSlider`, and that contract is preserved. This PR narrows the **construction-time** push only. ## The other families still take their own path Only **HL2** and **Icom** override `IRadioBackend::setMicGain` at all; every other family inherits the header's documented no-op. | Family | `hostModulates` | `setMicGain` | Before | After | |---|---|---|---|---| | HL2 | **true** | overridden (host DSP) | pushed | **pushed** — unchanged | | ANAN | **true** | inherited no-op | called, no-op | called, no-op — unchanged | | Flex | false | inherited no-op | excluded by the family gate | excluded by the capability | | **Icom** | false | **overridden (live CI-V)** | **pushed** | **not pushed** — the one change | | RTL | false | inherited no-op | called, no-op | not called — no-op either way | | sim | false | inherited no-op | called, no-op | not called — no-op either way | HL2's half is already pinned from the other side by `hl2_family_transition_test`, which asserts the rebuilt backend's `healthSnapshot()` reports the operator's 80 and that an untouched slider still lands on the modulator's unity 50. Both remain green — that is the regression gate on the positive direction. ## Why this is doubly worth doing `docs/HERMES.md` gained *"For coding agents — keep bring-up inside the family backend"* on 2026-09-12, written by @jensenpat (commit `f6f56458`, squashed to `1457d06d`). Under **Do not**: > Add `family == "hl2"` / `usesFlexCommandPlane()` branches above the seam (see #5554). and `src/models/RadioModel.*` is the **first path in its pre-PR grep**. This removes one such branch from that first file. It is his rule, not ours, and the replacement is the shape the same section asks for: *"declare a capability (`RadioCapabilities` + map + a consumer that already exists)"* — `hostModulates` already exists, is already mapped by every backend, and already has consumers. #5554 §2.1's own acceptance shape is the model here: a wrong seam gate, fixed on its own, with the other families named and proved. `RadioModel.cpp` goes from 15 `usesFlexCommandPlane()` occurrences to 14. ## Pre-PR grep — one hit, explained ```text src/models/RadioModel.* src/models/TransmitModel.* src/models/SliceModel.* src/gui/MainWindow*.cpp src/gui/*Applet* src/gui/SpectrumWidget.* src/gui/RadioSetupDialog.* ``` **One hit: `src/models/RadioModel.cpp`.** It is the point of the change, and it is the rule's own named exception rather than an unexplained one: this is not localizing a family into shared infrastructure, it is **removing** a family string test from shared infrastructure and replacing it with a capability the seam already publishes. The file gets strictly less family knowledge than it had. No shared GUI, no applet, no `AppSettings` key, no new capability field, no new verb. Complete file list: | File | Why | |---|---| | `src/models/RadioModel.cpp` | the gate, and the comment explaining why the two sites differ | | `tests/icom_family_test.cpp` | the family test for the family whose behaviour changes | ## The second site with the same gate — examined, and deliberately NOT changed `RadioModel.cpp`'s `monitorCommandIssued` connect calls `setTxMonitor` under the same `!usesFlexCommandPlane()`. **It is not the same defect, and narrowing it to `hostModulates` would break a working Icom control.** - It is **operator intent**, not a construction-time state push: `TransmitModel::setSbMonitor` / `setMonGainSb` emit it when the operator presses MON or moves MON GAIN. There is no freshly constructed object at its own default for it to seed, which is the entire premise of the site this PR changes. - Its gate is doing the **de-duplication** job correctly. Both emitters also emit `commandReady("transmit set mon=…")` / `("transmit set mon_gain_sb=…")`, the Flex wire form. Excluding Flex is what stops the doubled command. - `IcomCivBackend::setTxMonitor` is a deliberate, evidenced implementation of the radio's own `16 45` (enable) and `14 15` (level) registers. `IcomControls` registers both as `Wiring::Both`, bound to `txMonitorBtn` and `phoneMonitorSlider`. Gating it on `hostModulates` would leave an Icom operator a MON button and a MON level that display the radio's state and cannot write back — the dead-control shape this document's "hide what this radio cannot do" exception exists to prevent, inverted. The construct is still the forbidden one and should go. But the predicate it needs is *"the Flex text path already carries this verb"*, which is **not** `hostModulates` — and by the same rule that produced this PR, that is its own capability-shaped change. The same applies to the sibling `micLevelCommandIssued` and `txFilterCommandIssued` gates, which are left byte-identical here. ## Test plan `tests/icom_family_test.cpp` — the family test for the family whose behaviour changes, extended in place rather than given a new target. It drives the **production** family switch (`connectToRadio()` rebuilds the backend synchronously before any I/O, against unroutable TEST-NET-1), leaves the slider at 80 on the outgoing radio, swaps to Icom, and asserts **both directions**: 1. the rebuilt Icom backend was **not** handed that 80 — the mirror is still unreported, so nothing claims the radio answered; 2. the operator then moving the slider to 70 **does** reach it — proving the pin is not the vacuous "nothing ever calls `setMicGain` on an Icom", and that the control the operator actually uses is untouched. The mirror (`m_micGainReported` / `m_micGainPercent`) is the honest witness here rather than the wire: `setMicGain` writes it *before* `sendUserCommand` decides whether a frame can go out, so on a backend with no session it is the only place the difference is visible — and it is the same state `healthSnapshot()` publishes as the radio's own. It is read through the `IcomCivBackendTestAccess` friend struct this test already owns. Not asserted at the policy seam and not asserted back against the implementation: the test layer **can** construct a real Icom backend through `makeBackend()`, so this drives the real one. HL2's positive half needs nothing new — `hl2_family_transition_test` already pins it and stays green. ``` $ cmake --build build 3599 targets, exit 0, no warnings introduced (the 27 warnings in the log are pre-existing and none is in a file this PR touches) $ ctest --test-dir build 100% tests passed, 0 tests failed out of 426 Total Test time (real) = 288.72 sec The following tests did not run: 24 - crdv_quarantined_test (Skipped) 89 - app_settings_safety_explicit-profile-path-isolation (Skipped) 124 - weather_radar_texture_gl_test (Skipped) 243 - range_slider_a11y_test (Skipped) 244 - relay_bar_a11y_test (Skipped) ``` **Negative control.** The new pin was confirmed load-bearing rather than vacuous: with the `!usesFlexCommandPlane()` gate restored and only `icom_family_test` rebuilt, it fails with ``` FAIL: a rebuilt Icom backend is not handed the previous radio's mic level: the radio owns 14 0B and has not been asked yet ``` and passes again once the gate is put back. `vkamp_connection_test` passed in this run (10.81 s). It has been seen failing on this tree before — five isolated runs on another branch tonight gave two failures and three passes. Nondeterministic and unbisected is all that can honestly be said about it; it is unrelated to this diff either way (no mic, TX or Icom path). Local build: macOS/arm64, Qt 6.8.3, RelWithDebInfo, Ninja. RTL-SDR is disabled in this configuration (no `librtlsdr`), so the `rtl` row of the table above is read from `RtlSdrBackend.cpp` rather than exercised. ## Checklist - [x] Commits are signed (SSH) - [x] Local build passes — 3599 targets, exit 0 - [x] Existing tests pass — 426/426, 0 failures - [x] Code is clean-room - [x] No new flat-key `AppSettings` calls - [x] All meter UI uses `MeterSmoother` — no meter UI touched - [x] Documentation updated if user-visible behavior changed — the comment that asserted the old predicate is replaced in this diff with one stating why the two sites differ. No `CHANGELOG.md` entry, per AGENTS.md. - [x] Security-sensitive changes reference a GHSA — n/a - [ ] Behavior verified on a real radio — **not yet, and it needs an Icom.** I have an HL2, whose side of this is unchanged and pinned. The check that matters on an Icom: switch families with the MIC slider away from centre, connect, and confirm Radio Health's MIC Gain row reads "not reported" until the radio answers rather than showing the previous radio's number — and that moving the slider still changes the radio's mic gain. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_014TtnKQu6QGrSeBirGeAsAs --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 7f2411f commit c2a1322

2 files changed

Lines changed: 111 additions & 4 deletions

File tree

src/models/RadioModel.cpp

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1480,10 +1480,38 @@ void RadioModel::setupBackend(const QString& family)
14801480
// healthSnapshot() honestly.
14811481
//
14821482
// Free on the constructor's own call, where TransmitModel is at its 50 and
1483-
// 50 maps to the 1.0 the modulator already holds. Same Flex gate as the
1484-
// seam: on a Flex the slider's `transmit set miclevel=` reaches the radio's
1485-
// own preamp and this must not double it.
1486-
if (m_backend && !usesFlexCommandPlane())
1483+
// 50 maps to the 1.0 the modulator already holds.
1484+
//
1485+
// GATED ON hostModulates, NOT on the family predicate the operator-intent
1486+
// seam uses — the two sites ask different questions and the gate was copied
1487+
// between them.
1488+
//
1489+
// The seam's gate is DE-DUPLICATION: setMicLevel() emits
1490+
// `transmit set miclevel=` beside micLevelCommandIssued, so on a Flex the
1491+
// seam would issue a second copy of a command the wire text already carried.
1492+
// Nothing is in flight here. setupBackend() emits no wire text, so "did the
1493+
// Flex text path already carry this" is not a question this site has.
1494+
//
1495+
// The question this site has is whether the fresh backend HAS a host
1496+
// modulator standing at its own default, waiting to be told where the
1497+
// operator left the slider. That is precisely what hostModulates answers.
1498+
// A backend that does not host-modulate has no such object: it either has
1499+
// nothing to seed (Flex, RTL, the sim) or it owns the value INSIDE the radio
1500+
// and will report it on connect — Icom, where IRadioBackend::setMicGain is
1501+
// implemented as a live CI-V 14 0B write (or, on an IC-9700 with LAN as the
1502+
// modulation input, a SET 0114 LAN MOD write). Handing either one a
1503+
// client-held number at construction is a silent write of state the radio
1504+
// never asked for, and it marks the backend's own mirror as reported —
1505+
// healthSnapshot() then prints our number where the radio's belongs, before
1506+
// a single 14 0B reply has arrived. The operator's own slider move still
1507+
// reaches an Icom through micLevelCommandIssued; only this construction-time
1508+
// push stops.
1509+
//
1510+
// See the hostModulates declaration in RadioCapabilities.h, which carries
1511+
// the warning that conflating it with takesTxAudioOverSeam cost a working
1512+
// transmitter. This is that same flag, read for the question it was written
1513+
// to answer: does the HOST run the modulator.
1514+
if (m_backend && backendCapabilities().hostModulates)
14871515
m_backend->setMicGain(m_transmitModel.micLevel());
14881516
}
14891517

tests/icom_family_test.cpp

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
// written, tested and green while nothing in the application could construct it.
1111

1212
#include "models/RadioModel.h"
13+
#include "models/TransmitModel.h"
1314
#include "core/RadioDiscovery.h"
1415
#include "core/backends/icom/IcomCivBackend.h"
1516
#include "core/backends/icom/IcomControls.h"
@@ -49,6 +50,21 @@ struct IcomCivBackendTestAccess {
4950
backend.m_connected = true;
5051
backend.onCivFrame(frame, backend.m_sessionGeneration);
5152
}
53+
54+
// The mic-gain mirror, which setMicGain() writes BEFORE sendUserCommand()
55+
// decides whether a frame can go out. It is therefore the honest witness to
56+
// "did this backend take the value at all" on a backend that has no session
57+
// yet: the CI-V write is dropped by the not-connected guard, but the mirror
58+
// is already dirty, and healthSnapshot() publishes it as the radio's own.
59+
static bool micGainReported(const IcomCivBackend& backend)
60+
{
61+
return backend.m_micGainReported;
62+
}
63+
64+
static int micGainPercent(const IcomCivBackend& backend)
65+
{
66+
return backend.m_micGainPercent;
67+
}
5268
};
5369

5470
} // namespace AetherSDR::icom
@@ -806,6 +822,69 @@ int main(int argc, char** argv)
806822
"and neither is a different sub-command");
807823
}
808824

825+
// ── The construction-time mic push stops at a radio that owns its mic gain ──
826+
//
827+
// RadioModel::setupBackend() hands a freshly built backend the mic level
828+
// the model already holds, so a host modulator constructed at its own unity
829+
// default cannot silently part from the slider (pinned from the other side
830+
// by hl2_family_transition_test). That push is gated on
831+
// caps.hostModulates, and this is the family it exists to exclude.
832+
//
833+
// On an Icom the mic level is not a client-side DSP gain at all: it is the
834+
// radio's own 14 0B MIC GAIN register — or, on an IC-9700 with LAN as the
835+
// modulation input, SET 0114 — read back on connect and mirrored into the
836+
// same slider (IcomControls: "mic.gain", Wiring::Both). A backend rebuild
837+
// is not the operator asking for anything, so pushing a number carried in
838+
// from the previous radio is a silent write of state this radio never asked
839+
// for. The wire itself is defended by sendUserCommand()'s not-connected
840+
// guard, but the mirror is written before that guard is reached, and
841+
// healthSnapshot() then prints our number where the radio's belongs.
842+
//
843+
// Break it by restoring the !usesFlexCommandPlane() gate at the end of
844+
// setupBackend() and the first check below fails with the mirror reporting
845+
// 80 on a radio that has answered nothing.
846+
{
847+
// Away and back, because connectToRadio() rebuilds only on a family
848+
// CHANGE — a same-family reconnect keeps the backend it already has.
849+
model.connectToRadio(infoFor(QStringLiteral("flex"),
850+
QStringLiteral("1234-5678-9012-3456")));
851+
// Where the operator left the slider on the PREVIOUS radio. The model is
852+
// never reset across a family switch, which is what makes this a value
853+
// that can travel. Set while Flex is live, where the seam is silent and
854+
// `transmit set miclevel=` is the wire form.
855+
model.transmitModel().setMicLevel(80);
856+
model.connectToRadio(infoFor(QStringLiteral("icom")));
857+
auto* rebuilt = dynamic_cast<icom::IcomCivBackend*>(model.backend());
858+
check(rebuilt != nullptr, "the family switch produced a fresh Icom backend");
859+
if (rebuilt) {
860+
check(!icom::IcomCivBackendTestAccess::micGainReported(*rebuilt),
861+
"a rebuilt Icom backend is not handed the previous radio's mic "
862+
"level: the radio owns 14 0B and has not been asked yet");
863+
864+
// AND THE OPERATOR'S OWN HAND STILL REACHES IT. This narrows the
865+
// construction push only; micLevelCommandIssued is operator intent
866+
// and stays wired, so the Phone MIC slider remains a live control on
867+
// an Icom rather than a readout that cannot write back.
868+
// WHY THIS LANDS IN THE PHYSICAL-MIC BRANCH, since the answer is not
869+
// local: IcomCivBackend's constructor seeds m_model to
870+
// unknownModel(), whose civAddress matches no case in profileFor(),
871+
// so modulationProfileFor() is nullopt and setMicGain() never takes
872+
// its LAN-MOD early return. An unidentified Icom has no modulation
873+
// profile, so this is the 14 0B path.
874+
//
875+
// It matters for the NEXT reader rather than for today: if
876+
// unknownModel() ever gained a profile with
877+
// phoneLevelFollowsNetworkInput, this would fail on the
878+
// m_networkModLevelPercent < 0 refusal — a reason that has nothing
879+
// to do with the gate this section pins (PR #5643 review).
880+
model.transmitModel().setMicLevel(70);
881+
check(icom::IcomCivBackendTestAccess::micGainReported(*rebuilt)
882+
&& icom::IcomCivBackendTestAccess::micGainPercent(*rebuilt)
883+
== 70,
884+
"the operator moving the slider still reaches the Icom mic gain");
885+
}
886+
}
887+
809888
if (g_failures == 0)
810889
std::printf("icom_family_test: all checks passed\n");
811890
return g_failures == 0 ? 0 : 1;

0 commit comments

Comments
 (0)