Repository navigation
Fix IC-705 preamp cycling above 74.8 MHz - #6679
Conversation
There was a problem hiding this comment.
1. Issue fit
Yes. #6497's primary root cause — a band-unaware three-position ladder driving a modulo cycle into an absorbing state — is fixed at the right seam, and the boundary is the reporter's own measurement rather than an invented constant (k9mq: "Preamp1 and Preamp2 is active up until 74.79999mhz. At 74.800mhz on up, there is only one preamp"). src/core/backends/icom/IcomModels.cpp:475 uses frequencyHz >= limit, and tests/icom_control_profile_test.cpp:265-273 pins both sides of the exact edge (74'799'999 → 3, 74'800'000 → 2).
Mapping the issue's acceptance list to hunks:
| #6497 expectation | Where |
|---|---|
| VHF/UHF offers only OFF / P.AMP1 | IcomModels.cpp:470-480 + IcomCivBackend.cpp:1564-1575 |
| HF and 6 m keep the full cycle | same, limit gate; icom_control_profile_test.cpp:265 |
| The control follows the radio's actual state | IcomCivBackend.cpp:1918-1926 readback |
Never send 16 02 02 above the boundary |
IcomCivBackend.cpp:4358-4360 setter bound |
| On 0xFA, don't re-send the invalid value | already correct pre-PR (triage established the repeats came from the GUI cycle, not a retry); removing the invalid step is the real fix |
| Check other Icom models | verified below — no sibling defect for preamp |
The one thread item not in the diff is the attenuator (nit 2). The issue body is preamp-only, so issue fit is unaffected.
Test layer is right: ladder contents and wire bytes are model-table/encoding concerns, so a socket-free CTest is the correct layer per AGENTS.md. No socket test added, modified, or removed — tests/icom_backend_seam_test.cpp has no QTcpSocket/QUdpSocket/bind/listen/connectToHost and uses an unstarted IcomSession as the transport stand-in. No fake radio peer was introduced, which is correct; I am not asking for one.
The added checks do discriminate. With preampLabelsFor ignoring frequency, icom_backend_seam_test.cpp:272 (info.last() == {"OFF","P.AMP1"}) and icom_control_profile_test.cpp:271 both fail — these are not assertions of the implementation back to itself.
2. Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
src/core/backends/icom/IcomModels.{h,cpp} |
ranged ladder + preampTwoStageUpperExclusiveHz on the IC-705 profile |
Yes | In scope. New field is an internal profile member, not public/protocol surface. |
src/core/backends/icom/IcomCivBackend.{h,cpp} |
publishPreampControls(), session reset, transition republish + readback, setter bound |
Yes | In scope. |
tests/icom_backend_seam_test.cpp |
new testIc705PreampFrequencyRanges, 3 access shims, deliver() address source |
Yes | In scope; shim change is required, see below. |
tests/icom_control_profile_test.cpp |
boundary / unknown-frequency / other-model checks | Yes | In scope. |
docs/user/docs/networked-icom.md |
IC-705 preamp bullet | Yes | In scope and accurate — "Changing ranges does not change the radio's preamp setting" matches the code, which only reads. |
docs/user/src/components/LogAnalyzer/rules.json |
12 cited RadioModel.cpp line numbers |
Yes ("generated log references are updated") | Explained by the body, but unrelated to the fix — see nit 1. |
Nothing in the diff is unexplained, and no guard or confirmation was removed. One deleted comment names a symptom, so per §3 here it is:
// The HF preamp ladder is published even on VHF/UHF (00/01 only there): the radio refuses P.AMP2 and reports what it did, and republishing per band would rewrite the control under the operator.
What now answers it: the republish fires only when the effective ladder changes (std::ranges::equal guard at IcomCivBackend.cpp:1917), not on every band change, and it is immediately followed by an authoritative 16 02 read so the rewritten control is then told the truth. That is the answer the triage comment proposed, and it is implemented. Residual edge is nit 3.
3. Blockers
None.
4. Nits (all non-blocking)
1. docs/user/src/components/LogAnalyzer/rules.json:12,24,97,114,119,136,170,196,213,230,240,477 — unrelated generated churn that no gate needs, and three open PRs are regenerating it.
RadioModel.cpp is byte-identical between main (6bb29bb) and this head — I diffed it — so these 12 edits only repair line-number drift that already exists on main. And the checker treats stale lines as a NOTE, not a failure, by design:
// The message still exists but moved: not a failure (edits elsewhere in a
// busy file such as RadioModel.cpp would otherwise redden Static checks on
// unrelated PRs). Report it, and let --update refresh the line number.On main I get NOTE: 12 cited line number(s) moved followed by OK: 707 checks passed for 45 rules. — the same 707/45 this PR reports, with or without the hunk. Meanwhile #6673 ("this commit refreshes the line numbers that moved in RadioModel.cpp") and #6666 carry the identical regeneration, and #6676 deliberately declined it. Whichever lands first makes the others' hunk stale and forces a rebase, the same reason CHANGELOG.md is kept out of ordinary PRs. Suggest dropping the file from this PR and letting one dedicated commit own it.
2. src/core/backends/icom/IcomCivBackend.cpp:1569-1572 — the attenuator sibling the issue thread asked for in this change is still band-unaware, and its own comment still names the symptom. The diff rewrote the preamp half of that comment block and left:
// 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.attenStepsFor(*m_model) still takes no frequency, so on 2 m/70 cm the IC-705's ATT button remains a dead control. The triage comment called this "the same missing band awareness [that] should be fixed in the same change"; the PR body discloses the deferral ("attenuator behavior ... unchanged") and the reporter never raised it, so this is maintainer visibility rather than a defect in what shipped. Worth a follow-up issue so the IcomModelProfile ranged-table shape gets reused rather than reinvented.
3. src/core/backends/icom/IcomCivBackend.cpp:1921 — the authoritative readback is queued behind the meter poll, on the one radio whose log already shows control-priority items waiting seconds.
queueRead(read, semanticKey(read), IcomCivScheduler::Priority::Control);Control is 4 in IcomCivScheduler.h:24-31, i.e. behind ActiveMeter (3) and Ptt (2), while the write-confirmation read at :4345 uses Operator (1). In the gap, SpectrumOverlayMenu::setPreampLabels (src/gui/SpectrumOverlayMenu.cpp:3301-3302) has already reset an out-of-range step to 0, so after tuning HF→VHF with P.AMP2 selected the button reads OFF while the radio is actually on P.AMP1 — and #6497's own log reports queueWaitMs of about 5–6 s for control priority items during load. Cosmetic and self-correcting (a click in that window sends 16 02 01, which converges), but Operator would match the sibling read and close it:
queueRead(read, semanticKey(read), IcomCivScheduler::Priority::Operator);4. src/core/backends/icom/IcomCivBackend.cpp:1440 — clearing the frequency also resets the S-meter's S9 reference, now on reconnect as well as first connect.
m_frequencyHz = 0; // A new session must establish its own receive frequency.s9ReferenceFor(m_frequencyHz) at :2889 is unguarded, and usesVhfSReference is hz >= 30'000'000 (IcomMeters.h:67-69), so 0 selects the HF reference (−73 dBm) instead of VHF (−93). Meter polls are ActiveMeter (3) while the startup frequency read is Maintenance (5, :1067), so meters will generally answer first: a 2 m/70 cm session reads ~20 dB hot until the first 03/00 reply. This window already existed on every first connect (the member's construction default is 0); the line extends it to reconnects, where the stale-but-correct value used to carry over. No code block — the trade is a judgment call between guarding s9ReferenceFor's caller, raising the startup read's priority, and tracking ladder knowledge separately from the frequency. Worth a sentence in the body either way, since it is a side effect the preamp fix doesn't need.
5. What I tried to break
- Sibling models on the shared ladder. Only two profiles use
kHfPreampLabels— IC-705 (IcomModels.cpp:569) and IC-7300MK2 (:656); the IC-9700 has its own two-positionkIc9700PreampLabels(:605), and 0x94/0x98/0x8E fall tokTunerOnlyProfile. The MK2 is HF/50 MHz hardware, so it cannot reach 74.8 MHz and needs no limit. No preamp sibling left broken. - The default argument widening the ladder elsewhere.
preampLabelsFor(model, hz = 0)defaults to the narrow ladder, which could have silently truncated other callers. Every production call site passesm_frequencyHz(:1566,:1915,:1917,:4359); the only defaulted caller istests/icom_family_test.cpp:874on the IC-9700, whoselimit == 0makes the branch inert. - Truncation direction.
labels.first(min(2, size))keeps indices 0/1, andsetPanPreampsends the index straight as the wire value — so the truncation preserves the register meaning of every surviving position. Truncating the head instead would have silently remapped OFF; it doesn't. - Shrinking the ladder under a selected P.AMP2, and the diagnostic scrub. GUI resets the step (
SpectrumOverlayMenu.cpp:3301);reassertPanPreampWireStepstill clamps 0..2 unconditionally (:4370-4375), so if the scrub fires before the readback lands it can emit16 02 02at VHF — but with the setter now bounded,m_preampStepcan only be 2 there if the radio itself reported 02, and the write's confirmation read corrects it either way. No new absorbing loop; I could not construct one. - Clearing
m_frequencyHzagainst its other consumers. Of the ten readers,:1859,:3960,:3993and:6049already guard== 0;bandRatedPowerWatts(.,0)returnsnulloptand the caller explicitly declines to borrow a rating. Only the S9 reference is unguarded — nit 4. - The test shim change (
frame.from = b.m_model->civAddress→b.m_session->civAddress()), which touches everydeliver()in the file. It is required, not incidental:onSessionConnectedsetsm_model = &unknownModel()(:1452), so the old expression would stamp the wrong source address on the reply that the new test delivers after reconnect, and the unexpected-responder guard at:1743would drop it. It is also behaviour-neutral for the existing tests —prepare()setsm_session->setCivAddress(b.m_model->civAddress)(:36) and no test in the file ever makes the two differ. - "Tune intent alone cannot change the ladder" (
icom_backend_seam_test.cpp:263). I checked this isn't vacuous:setSliceFrequencytakes Hz (:3969-4005), so144'300'000is a real in-range request, and the IC-705 has nobandsFortable so it reachessendUserCommand— which never writesm_frequencyHz. The assertion genuinely pins Principle II. - Includes and compile surface.
std::ranges::equal/std::minneed<algorithm>, present inIcomCivBackend.cpp,IcomModels.cppand both tests;.first()needs<span>, present. Consistent withcheck-windows(the-DENABLE_ASR=OFFlane) andcheck-macosboth green. - CodeGuard, verified not parroted. All 58 findings refute. The 26
CS-VLN-CMDI-BACKTICKhits are prose backticks inside//comments (the project's known lexical-comment class),CMDI-PIPE5930-6060 andCMDI-AMP5790 are C++||/&&in the scrub dispatch chain,CMDI-SEMI1889 is a C++17 if-with-initializer in thekReadIdcase, and therules.jsonhits (88/89/744/1003) are log-matching regex and prose at lines this PR does not touch. None intersect a changed hunk; nearest is 1968, below the frequency case ending at 1939. The frequency value fromdecodeFreqis used only as a comparison key against a constant — never as an index, length, or command text. Credit: security-audit (CodeGuard).
Honest limits. I ran headless: no build, no test execution, no GUI. Every finding above is reasoned from the code at 65c3658, not reproduced at runtime. The two things I did execute are the log-rule checker (707 checks / 45 rules, 12 stale-line NOTEs on main) and check_command_plane.py (125 raw Flex commands, baseline 125 — the PR's claim holds; the Icom backend is below the seam). I could not verify the 446.15 MHz hardware proof or the mutation runs; the preamp-proof.json and screenshots are consistent with the described bytes, but I take them on the author's word. I also cannot confirm the --update regeneration in nit 1 is byte-identical to what #6673 produces.
On "green": the frozen PR gate (ci.yml, #5405) runs ctest -R over only the ThumbDV four, cross_needle_meter_test, mac_nr_filter_test and asr_gpu_probe_test. Neither icom_control_profile_test nor icom_backend_seam_test ran in CI on this head — their pass is the author's local macOS run, and they first gate on full-suite.yml after merge to main. Separately, the icom_family_test failure the body discloses is pre-existing on main, not this PR: #6674's body records it failing at both 69a50077 and 6bb29bb7, and #6672 is the open fix. The body's disclosure of it is accurate and appropriately scoped.
6. Recommendation
Approve with nits. The fix sits at the correct seam, is bounded by the reporter's measured constant rather than an invented one, writes nothing on a range change, and strengthens rather than weakens Principle II — the backend now asks the radio instead of carrying a stale assumption. I tried the ladder-shrink race, the scrub reassert, the default-argument widening, the truncation direction, the sibling model profiles and the test's own discriminating power, and the change survived all of them.
Concrete next step: drop the rules.json hunk (nit 1) so this PR stops colliding with #6673/#6666, and consider the one-word priority change in nit 3. Nits 2 and 4 are follow-up issues, not rework here. Thanks for the precise boundary citation and for disclosing the icom_family_test state and the untested platforms rather than rounding the validation up.
🤖 aethersdr-agent · cost: $15.7741 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (58 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: $15.9681 · model: claude-opus-5 |
|
Addressed the review nits in signed commit 67cfca6:
Three focused tests, two discriminating mutation checks, macOS app/docs builds and independent source review passed. RX-only IC-705 validation confirmed P.AMP2 at 74.799 MHz and P.AMP1 at 74.800 MHz, with 2,819 replies and zero timeouts. Captured receive settings were restored and the owned app stopped. The cold-start meter guard is regression-tested; this lab run did not instrument the first displayed meter sample. Full validation details are in the updated PR body. No merge or auto-merge. |
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed exact head 67cfca650a2960e8e9b86078205687b4d30db036. No blockers or outstanding nits; approve.
Issue fit and scope
#6497 documents the rejected P.AMP2 loop on VHF/UHF, and the reporter confirms the 74.800 MHz boundary. The patch uses radio frequency readback, bounds operator requests, and preserves the existing confirmation and diagnostic paths.
| Files | Change | Claimed | Verdict |
|---|---|---|---|
| IcomModels.h/.cpp | Model-owned frequency limit and effective ladder | Yes | In scope |
| IcomCivBackend.h/.cpp | Republish on confirmed range change; bound setter; reset frequency per session; defer S-meter until frequency known | Yes | In scope; meter guard addresses the reset's calibration consequence |
| icom_control_profile_test.cpp, icom_backend_seam_test.cpp | Socket-free profile, wire-intent, scheduler and lifecycle regressions | Yes | In scope |
| networked-icom.md | Current control and reconnect behavior | Yes | In scope |
No GUI redesign, persisted radio state, dependency, default timer, TX permission, workflow or CHANGELOG change. No new above-seam coupling or ratchet exception. The attenuator follow-up is explicitly scoped separately as #6685.
What I tried to break
- Exact boundary and unknown frequency, tune intent versus radio readback, polled versus unsolicited replies, repeated changes within one range, and return to HF. The focused tests pass.
- Stale stage-2 requests and shrinking the published ladder: the setter bounds the request using the same profile, while a range change only publishes choices and requests a read; it sends no preamp write. Existing radio readback remains authoritative.
- Reconnect before frequency arrives: the changed handler withholds S-meter publication, keeps frequency-independent telemetry, and adopts the confirmed HF/VHF reference. Scheduler coverage verifies preamp confirmation before a queued meter after the in-flight frequency transaction.
- Sibling models and the diagnostic scrub: unrestricted profiles retain their ladders; the separate wire-state reassertion still preserves the radio-reported value. The test shim's source-address change is needed after identity reset and retains existing fixture behavior.
- Mutation: temporarily disabled the profile limit, lowered range-read priority, and disabled the unknown-frequency meter guard together.
icom_control_profile_testandicom_backend_seam_testfailed, including boundary/clamp and startup-meter assertions;icom_meters_teststayed green. Restored byte-identical head source, rebuilt and reran: 3/3 pass. This combined mutation is not claimed as independent isolation of each guard.
Validation and limits
Local macOS arm64 / Qt 6.12 test-target build and headless CTest: 3/3 passed (icom_control_profile_test, icom_backend_seam_test, icom_meters_test). Minimal test configuration disables ASR, RADE, D-STAR, DFNR and QtKeychain; no local app/package build is claimed.
Strict engine-boundary, touchpoint manifest, capability and command-plane checks pass and their complete findings match merge base 6bb29bb7 exactly. Test registration, generated references, whitespace and 707 Log Analyzer checks pass. Required hosted Linux/macOS/Windows/static checks and hosted Docs build pass on this head; focused Icom tests are local evidence rather than part of the frozen PR CI test selection. All contributor commits have valid signatures. No unresolved threads or blocking reviews.
The automated code-review skill is unavailable; this was a manual adversarial pass. Demo cannot exercise Icom CI-V/profile behavior, so no bridge or live-radio run was performed in this review. The author's RX-only IC-705 captures and boundary measurements are supporting evidence, not independently repeated hardware proof. No TX, other-model hardware, native rendering, full-suite or post-merge result is claimed.
On an IC-705 at 144 or 430 MHz, clicking the preamp button from P.AMP1 requested P.AMP2. The radio rejected that stage and readback restored P.AMP1, so another click repeated the invalid request instead of reaching OFF.
Fixes #6497.
The IC-705 model profile now limits its two-stage preamp ladder to frequencies below 74.800 MHz, matching the reporter's measured boundary. At and above that boundary, the existing control cycles OFF/P.AMP1. An unknown receive frequency also uses those shared positions until radio readback establishes the range.
Both polled and unsolicited CI-V frequency replies update the published choices when the effective ladder changes and request a preamp readback. Crossing a boundary never writes a preamp setting. The backend setter uses the same frequency-dependent bound, so a stale P.AMP2 request cannot send stage 2 in the upper range. New sessions discard the previous receive frequency. Other models, diagnostic wire-state reassertion, NAK confirmation behavior, and attenuator behavior are unchanged. The IC-705 user guide and generated log references are updated.
Validation:
icom_control_profile_test,icom_backend_seam_test), including 26 added checks covering the exact boundary, unknown frequency, other models, both frequency reply forms, tune intent versus readback, model lifecycle, stage readback and emitted/queued CI-V writes.icom_family_testwas attempted and fails its existing display-state/Flex-command assertion after twopan displayrefusals. This test's unchanged WNB/display path is outside the preamp change; no successful baseline run is claimed. It uses TEST-NET socket paths and was not rerun as part of the socket-free validation.npm ciandnpm run buildpass.16 02 02, receivedFA, and read back16 02 01. The fixed control requested16 02 00, receivedFB, and read back16 02 00; another click restored stage 1 with confirmed readback. Frequency, FM/FIL1 and attenuator OFF were preserved.Hardware evidence (446.15 MHz):
Allowlisted preamp requests and radio replies. Evidence commit:
f41a51cb53c4d7e27f1cbd14cf20ecd15d4880fe(signed).Linux and Windows builds/runtime were not run. No transmit validation is claimed.
Base:
6bb29bb7c7b1988d7f3300e41fc596f62902c65b. Head:65c3658e08f02d8a7762de090c07bfad2bd919ac(signed).Generated with OpenAI Codex.
Review follow-up (67cfca6)
Range-change preamp readback now uses Operator priority so it precedes queued meter reads. S-meter publication waits for confirmed receive frequency on reconnect, preventing a transient HF reference on VHF; frequency-independent telemetry remains available. Removed the unrelated RadioModel line-number churn from rules.json entirely. The separate attenuator band-awareness follow-up is #6685.
Validation: 3/3 focused socket-free CTests (backend seam, control profile, meters), independent mutations that fail each new regression, native macOS app build, test registration, 707 Log Analyzer checks, and clean docs install/build passed. Independent source review found no blockers.
RX-only IC-705 check of this source and binary SHA256
54e40611536d265968212bb78fe27cba974b54c800aa71721ae2fa5c782e868d: confirmed 74.799 MHz/P.AMP2 and 74.800 MHz/P.AMP1 readbacks; final scheduler 2,819 replies, zero timeouts. Restored 446.150 MHz/FM/FIL1 and matching captured receive settings, then disconnected and stopped the owned app. The first displayed startup meter sample was not instrumented in this lab run; that guard is covered by the production-handler regression. No TX or merge.