fix(icom): Fix Icom RX filter preset identity, skirt mapping, and recall - #5363
Conversation
jensenpat
changed the title
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
There is no linked issue — the body describes behaviour "observed on an IC-7300MK2" rather than citing a report. That matters here because the PR does two different things under one title: (a) it fixes a genuine defect (FIL identity was being inferred from a sorted width list, so a customised slot got renamed/reordered/mis-highlighted — that root cause is real and the fix is well-shaped), and (b) it introduces a new recall semantic ("clicking FILn resets its customised width and centres both PBTs") that is not a defect fix and has no authority behind it. Part (a) is solid. Part (b) needs a maintainer ruling, and as written it deletes state the radio persists.
The validation is honest and detailed, but it covers USB/LSB/AM/CW only. FM, DV and WFM are neither tested nor exercised, and that is exactly where the diff breaks (Blocker 1).
2. Scope
| File / group | What it changes | Claimed by the body? | Verdict |
|---|---|---|---|
RadioCapabilities.h (RxFilterPreset/RxFilterControl, hasCompleteRxFilterPresets) |
New capability struct + helper | Yes | In scope. New capability surface — caps-map row is present, good. |
IRadioBackend.h (setSliceFilterPreset default no-op) |
New backend seam | Yes | In scope; default no-op preserves Flex/HL2/ANAN/Sim. |
CivCodec.{h,cpp} (filterPresetsForMode, filterPresetRecallPlan) |
Preset identity + wire recall plan | Yes | In scope; identity half is the actual fix. |
IcomCivBackend.{h,cpp} |
Publish rxFilterControl; split slot-pick out of setSliceFilter |
Yes | In scope, but see Blockers 1 & 2. |
RxApplet, VfoWidget, AppletPanel, MainWindow* |
Feed the capability to both filter surfaces; label/tooltip/a11y | Yes | In scope. a11y names added — nice. |
FilterPassbandMath.h + FilterPassbandWidget |
Mode-aware Hz/px and clamps | Yes | In scope. Header is referenced and the test target is registered in tests.cmake:4030. |
AutomationServer.cpp (slice filterpreset, snapshot fields) |
New public protocol verb + 2 new snapshot fields | Yes | Needs maintainer decision — third parties bind to bridge verbs and they outlive the fix. Reasonable addition, but it is protocol surface arriving inside a fix PR. |
docs/automation-bridge.md civ ... incident row |
Syncs the doc table to the help string already on main (AutomationServer.cpp:3459) |
Partly ("document the existing diagnostic") | Not a blocker — unrelated doc drift, but it is one line and bridge_docs_check likely requires it. |
docs/architecture/radio-capabilities-map.md |
New caps row | Yes | Required by AGENTS.md. In scope. |
No CHANGELOG.md entry — correct.
3. Blockers
1. FIL buttons are now completely dead in FM / DV / WFM — a removed guard whose symptom recurs
setSliceFilter previously had this branch, which this PR deletes:
"FM, DV and WFM have no settable width at all, so the slot IS the only filter control the radio offers there and every request has to be a slot pick."
The replacement comment at IcomCivBackend.cpp:3783 asserts "Their FIL selection travels through setSliceFilterPreset()". Reading the head checkout, it does not:
widthClassFor()returnsWidthClass::FixedforFM/NFM/DFM/WFM/DV/DSTAR(CivCodec.cpp:555).filterWidthCodeFor()returnsnulloptimmediately forFixed(CivCodec.cpp:591).filterPresetRecallPlan()therefore hitsif (!widthCode) return std::nullopt;(CivCodec.cpp:534) and discards the whole plan, including the already-built slot-select command.setSliceFilterPreset()thenreturns on!plan— no CI-V frame is sent at all, silently, with noqWarning.
FM's ladder is {15000, 10000, 7000} (distinct), so filterPresetsForMode publishes three FIL buttons in FM — all three inert. WFM collapses to one button, also inert. Before this PR, clicking a filter button in FM sent cmdSetVfoMode(addr, m_mode, m_dataMode, filter) and worked.
Fix: emit the plan when only the width write is unavailable, rather than failing the whole plan — build the select command, and append the width/PBT writes only when filterWidthCodeFor yields a code. (Reasoned from code; I cannot run the Icom backend here.)
There is also no test for it — checkRecall in tests/icom_civ_test.cpp:504-515 covers USB/LSB/AM/CW only. An FM case asserting a one-command plan would have caught this and is exactly the socket-free codec layer AGENTS.md points at.
2. Every FIL click overwrites the slot's radio-persisted width (Constitution III)
setSliceFilterPreset always sends the mode's client-side factory 1A 03 width plus centred 14 07/14 08 after the slot select (IcomCivBackend.cpp:3855, and the body states this is deliberate: "The sequence is issued even when the requested FIL slot is already selected.").
On an Icom the width and Twin-PBT position stored inside FIL1/FIL2/FIL3 are state the radio persists and recalls. Constitution III:
"If the radio can persist and recall a setting, that setting is saved to and recalled from the radio — never duplicated in client-side config. … the deciding test is simply whether the radio can save and restore the value."
So clicking through FIL1→FIL2→FIL3 now destroys all three of the operator's customised filters, replacing them with a table hardcoded in ladderFor(). That is the client writing its remembered value back over the radio's, which II forbids and III owns. It also directly reverses the comment the first commit in this same branch preserves and the second deletes:
"the slots are the operator's own three presets and clicking through them must not redefine them."
Commit 1 and commit 2 hold opposite positions on the same question. I don't think this is smuggled — the body is upfront about it — but "the filter button should reset the slot" is a preference/design decision, not a defect fix, it has no issue behind it, and it conflicts with canon. Per GOVERNANCE.md this needs its own issue and a maintainer ruling.
Suggested split: land the identity fix (commit 1 + the capability plumbing + the skirt math) as-is; take the recall semantics to a separate issue. If recall is wanted, a plausible middle ground is to recall only when the requested slot is already selected (an explicit "reset this preset" gesture), leaving a plain slot change non-destructive.
4. Nits (non-blocking)
IcomCivBackend.cpp:444—widthStepHz = limits.minHz == 200 ? 200 : 50;discardslimits.stepHz, whichfilterWidthLimitsForalready authors (SSB/RTTY = 100). Above 600 Hz the SSB table is a 100 Hz grid (filterWidthHzFromCode,CivCodec.cpp:582), so a 50 Hz snap produces requests the radio quantises straight back.= limits.stepHzis the answer the codec already has. (Not a regression — the old constant was an unconditional 50.)filterForWidthHz()now has no production caller — its only one was the deleted slot-pick branch. It survives inCivCodec.h:470and in tests. Worth deleting or explicitly keeping with a note.MainWindow::applyCapabilitiesToUicallssetRadioFilterControland thensetRadioFilterWidths; the second stomps them_radioFilterWidthsthe first just wrote, and when presets are complete only the list's size is ever read. Harmless today, but the two setters are load-bearing on ordering for a value nothing consumes. Worth a comment or collapsing to one setter.- Radio widths now beat the operator's saved
FilterPresets_<mode>(RxApplet.cpp:2822,VfoWidget.cpp:5443). The body discloses this, but the right-click "Set Custom Edges…" path still writes settings that are now unreadable while an Icom is connected — the menu item should probably be disabled there. - CodeGuard's five
CG-PATH-001hits are all inMainWindow.cppat lines 8872–9105, far from this diff'sapplyCapabilitiesToUihunks (~7194/7318). Pre-existing, not introduced here — dropping them.
5. What I tried to break (and failed)
- Stale/mismatched preset metadata during a mode or connect transition.
hasCompleteRxFilterPresetsgating bothpresets.at(i)indexing sites (RxApplet.cpp:3038ff,VfoWidget.cpp:5569ff) holds: I walked USB↔WFM↔FM transitions where the preset count changes 3↔1 and the size check rejects the mismatched pair before any.at(). No out-of-range path found. - Disconnect.
RxFilterControl{}→setWidthRange(0,0,0)falls back to 50/6000/50, andsetRadioFilterWidths({})clears the width list, so the operator's own presets come back. Reversible as claimed. - The stated recall geometry table. I checked all 12 entries against
passbandCentreHz+passbandFromWidthAndPbtby hand; USB FIL1 = 0..3000 etc. all follow, and they agree with the readback path (not withpassbandForModeAndFilter, but the readback path is what the UI actually sees, so that is consistent). - The automation verb.
slice filterpresetrejects non-FILjunk (toIntfails), rejects undeclared ids viafind_ifon the advertised list, andRadioModel::selectRadioFilterPresetre-validates independently. Empty-slice-list case returns a proper error rather than dereferencing. Clean. - Test honesty. I checked whether the new tests would pass against unfixed code:
filterPresetsForModewith a custom width andfilterPresetRecallPlan's four-command assertion are both new surface, so they cannot pass on main — they are real tests, not tautologies.filter_passband_math_testis registered attests/tests.cmake:4030and linksQt6::Core, so it is not a dead target. - Socket tests: none added or removed by this PR. Both new tests are socket-free CTest at the codec/math layer, which is the correct boundary for wire encoding and presentation math.
- Could not check: anything requiring a build, a GUI, or a radio. Every finding above is reasoned from the head checkout at
/tmp/aetherclaude/pr-5363, not reproduced at runtime. I also could not exercise the live IC-7300MK2 sweep the body reports; I have no way to confirm or refute the 12/12 result, though its scope (USB/LSB/AM/CW) is what makes Blocker 1 invisible to it.
6. Recommendation
Needs maintainer decision, with one hard blocker to fix first.
Blocker 1 is a straightforward regression with a small fix and a missing FM test case — that one is unambiguous. Blocker 2 is a genuine design question the maintainer should rule on, not something I should decide: the identity fix in this PR is good work and clearly correct, but the recall semantics bolted onto it overwrite radio-persisted state and contradict the comment the same branch's first commit preserves.
Concrete next step: fix the FM/DV/WFM plan and add the codec test case; then either split the recall commit into its own issue/PR, or get an explicit ruling that a FIL click may redefine the slot before merging it.
Thanks for the unusually thorough write-up — the wire-format derivation and the mutation check on the outer-PBT command are the kind of evidence that makes a review like this possible.
🤖 aethersdr-agent · cost: $4.5656 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (5 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: $5.1570 · model: claude-opus-5 |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
No linked issue; reviewed against the title's three claims (preset identity, skirt mapping, recall). The Icom-decomposition core is genuinely good: two Flex-style passband edges reduced to one width + one Twin-PBT shift, reversibly, with FilterPassbandMath extracted and unit-tested, and the preset-recall-is-not-slot-select insight (re-assert factory width + centred PBT because the radio remembers a mutable shape per FIL slot) is correct and well-commented. The skirt/PBT and SSB/CW/AM/RTTY recall paths look right.
But one mode class the feature explicitly enumerates — the fixed-width modes — comes out with dead filter buttons, and that's the blocker.
Scope
25 files, all explained by the three claims. Preflight: no sockets, no fake peers; two new pure-math/codec tests. CI green.
Blockers
1. FM/WFM/DV filter-slot buttons now send nothing (inline). filterPresetRecallPlan pushes the slot-select command first (cmdSetVfoMode/cmdSetMode), then computes the 1A 03 width via filterWidthCodeFor and returns nullopt when there is no settable width — which is exactly the fixed-width modes (widthClassFor(FM) == Fixed), whose own comment says FM has three real fixed slots at 15/10/7 kHz. setSliceFilterPreset early-returns on !plan, so the slot-select command that was already built is discarded with it. Clicking FIL1/FIL2/FIL3 in FM/WFM/DV issues no CI-V at all. The pre-refactor setSliceFilter sent the mode/slot command in precisely the maxHz <= 0 case; the new path dropped it. The tests cover only USB/LSB/AM/CW, so the gap is uncovered. Fix: when widthCode is absent, return the plan with just the select command (and no 1A 03 / PBT writes) rather than discarding it — the fixed slots need no width write.
Nits (non-blocking)
widthStepHzre-derived instead of read (inline):c.rxFilterControl.widthStepHz = limits.minHz == 200 ? 200 : 50ignores the authoritativeFilterWidthLimits.coarseStepHz(SSB/CW/RTTY = 100, AM = 200) thatfilterWidthLimitsForalready returns. RTTY and SSB get a 50 Hz gesture grid the radio doesn't use, so radio readback silently re-quantizes every drag — contradicting the PR's "snap to the radio-advertised step". ReadcoarseStepHz.- Per-slice snapshot uses the radio-global control (inline):
doGet("slices")passes the sameradio->radioFilterControl()(single-active-slice, radio-global) into every slice's snapshot, so on a main+sub in different modes both slices report the active slice'sfilterPreset/filterPresetId. Automation reads the sub slice's filter wrong. Resolve per-slice or document the field as active-slice-only. - Double rebuild of
m_radioFilterWidths(MainWindow.cpp:7197, and the VfoWidget twin at:7321):setRadioFilterControlsets it from presets in radio order and rebuilds, thensetRadioFilterWidths(sorted)overwrites and rebuilds again — final list is sorted whilem_radioFilterControl.presetsstay radio-ordered, andrebuildFilterButtonsindexes both at the samei. Benign only because the widthwis unused in the stable-preset branch; a future consumer readingeffectiveFilterWidths()[i]againstpreset[i]gets a mismatch, plus wasted work on every capability update.
What was verified vs read
- Verified by me in the PR head: the FM-dead-button path end to end (
filterPresetRecallPlan'snulloptreturn under!widthCode,widthClassFor(FM)==Fixed,setSliceFilterPreset's!planearly return dropping the already-built select command);coarseStepHzexisting as an ignored field; the radio-global control fed to everysliceSnapshot; the doublem_radioFilterWidthswrite. - Read, not re-run: the passband math and the SSB/CW/AM recall correctness (the new
filter_passband_math_testandicom_civ_testcover them and CI is green); the Twin-PBT decomposition reasoning. - Not run: no hardware, no bridge session — blocker 1 is a static control-flow gap reproducible from the code; the fix needs a fixed-width-mode recall test row (FM FIL2 → expect a select command, no 1A 03) that would have caught it.
Keep fixed-mode FIL selection plans without writing programmable width or PBT. Preserve unrestricted width and legacy drag scaling/rounding when filter capabilities are empty, including after disconnect. Add socket-free codec and production QWidget event coverage. Removing both fixes caused 32 codec checks and four widget checks to fail; restored fixes pass the focused tests.
|
Thanks for this — the filter-slot-as-identity model is the right shape, and the live IC-7300MK2 sweep behind it is a lot of work. The CI failure is a one-line bookkeeping miss, not anything wrong with your logic. What failedOn Every other step in that job passed (engine/UI dependency direction, test registration, per-PR test gate, colour ratchet, a11y). The step is: # .github/workflows/static-checks.yml:161
- name: aetherd touchpoint manifest
run: python tools/gen_touchpoint_manifest.py --checkWhy
Your PR adds
so that header's UI-includer count goes 2 → 5, and the committed row still says 2. The fixpython tools/gen_touchpoint_manifest.py
git add docs/architecture/aetherd-touchpoints.mdI ran this against your exact head in a throwaway worktree; the whole diff is one line, and -| `core/backends/RadioCapabilities.h` | 2 | universal — Backend-neutral capability descriptor … | unconverted |
+| `core/backends/RadioCapabilities.h` | 5 | universal — Backend-neutral capability descriptor … | unconverted |No tag or status change is needed: Worth noting the sibling static check, Engine/UI dependency direction, passed — so nothing about the direction of these includes is objectionable. On the reviewer commentsThe bot's blockers on Not a maintainer ruling on the third comment (the unconditional Nothing here is CI infrastructure flakiness; regenerate the manifest and the Static checks job should go green. 🤖 aethersdr-agent · cost: $4.3398 · model: claude-opus-5 |
rfoust
left a comment
rfoust
left a comment
There was a problem hiding this comment.
No linked issue; reviewed against the stated preset identity, mode-aware skirt mapping, and explicit recall behavior. The fixed-width FIL regression and the shared widget's empty-capability regression are repaired. Recall semantics follow the maintainer's ruling in review 5083885393. Approving the repaired head at the operator's explicit direction; I authored the follow-up repairs.
| Scope | Claimed purpose | Verdict |
|---|---|---|
| Capabilities, backend seam, RadioModel, Icom codec/backend | Stable FIL identity and explicit recall | In scope |
| RxApplet, VfoWidget, AppletPanel, MainWindow wiring | Shared radio-authoritative preset presentation | In scope |
| FilterPassbandMath and production widget | Mode-aware gestures with legacy fallback | In scope; fallback regression repaired |
| Automation server and bridge/capability docs | Preset selection and snapshot surface | Disclosed and reviewed |
| Codec/math/widget tests and registration | Regression coverage | In scope; socket-free |
| Generated touchpoint manifest | Capability include inventory | Required generated refresh |
Blockers: none remaining in the reviewed tree. All seven existing threads have a reply explaining the repair or non-blocking disposition.
Non-blocking limitations: final edge snapping can move an off-grid untouched CW edge; the 50 Hz fine gesture grid is still quantized more coarsely by the codec above 500 Hz. Radio-global snapshot metadata is valid for today's single-slice Icom backend but needs revisiting before multi-slice Icom support. Existing redundant width-list rebuilds are unchanged.
Verification: merged current main a4227e6 into the PR. Full macOS ARM64 application build passed with RADE enabled, verified ARM64 host/system/executable and no RNNoise x86 sources. icom_civ_test, filter_passband_widget_test, filter_passband_math_test and bridge_docs_check passed (4/4). Strict engine-boundary, test-registration, frozen-gate, generated-manifest and whitespace checks passed. Final head 2ab0cc4 adds only the generated manifest row after that code build; hosted CI is still running.
Attacks survived: all advertised fixed-mode slots using both command variants retain select-only plans with no width/PBT writes; invalid IDs are rejected. Production QWidget mouse-event tests preserve legacy 10 kHz shift/widening, vertical scale and minimum width, enforce advertised AM/USB maximums, and restore legacy behavior when capabilities clear. Removing the repairs while retaining the tests caused 32 codec failures and four widget failures; restoring the repairs returned the tests to green.
No new hardware sweep was performed. The isolated offscreen full app failed QRhi initialization before the bridge became available, so runtime evidence is the production widget event test, not a full-app demo claim. The review instances were stopped; the operator's running app was untouched. Merge only after required CI succeeds.
The sole blocking finding in this review (fixed-width FM/WFM/DV FIL selection discarded by the recall planner) is repaired in b77b84e. The planner now retains a select-only command, keeps widthHz=0 and preserves display edges. Added socket-free coverage exercises every advertised fixed-mode slot through both command variants; reverting the repair causes 32 failing checks. All inline threads have verified repair or explicitly non-blocking dispositions. Reviewed and approved final head 2ab0cc4 at the operator's explicit request to fix, push, approve and merge. Dismissing this addressed request so ordinary required CI and approval gates can control auto-merge.
3 participants
Summary
Fix the Icom RX filter controls so FIL1/FIL2/FIL3 remain stable radio-owned preset identities, skirt dragging follows the connected radio's mode-specific capabilities, and clicking any FIL button—including the already-selected button—recalls that preset's default width with Twin PBT centered.
This addresses the filter applet and VFO behavior observed on an IC-7300MK2 across USB, LSB, AM, and CW.
User-visible behavior fixed
FIL1,FIL2, andFIL3after their widths are customized.AppSettingsfilter presets during mode changes or reconnect presentation rebuilds.Root causes
The UI previously represented radio filter presets only as a sorted list of widths. That loses an important part of the Icom contract: FIL1/FIL2/FIL3 are stable slot identities, while the width stored in each slot is mutable. Once a slot was customized and
1A 03read it back, sorting and nearest-width matching could rename, reorder, or highlight the wrong button.Preset selection and passband editing also shared
setSliceFilter(low, high). The backend tried to infer a button click by comparing the requested width with the factory ladder. A drag that happened to land on a ladder width was indistinguishable from a FIL selection, and a customized preset could no longer be selected reliably by identity.Finally,
FilterPassbandWidgetused fixed pixel-to-Hz constants. Those constants could not simultaneously describe the IC-7300MK2's 3.6 kHz SSB/CW range and 10 kHz AM range, causing the panadapter/app skirt gestures to over- or under-drive the radio depending on mode.Implementation
Stable preset identity
RxFilterPreset/RxFilterControlcapability data containing stable ID, label, mutable width, selected ID, and mode-specific width limits.IRadioBackend::setSliceFilterPreset()seam and validate advertised preset IDs inRadioModelbefore dispatch.Correct skirt mapping
FilterPassbandMathhelpers for mode-aware Hz-per-pixel scaling and width constraints.Explicit recall semantics
26 00, or model fallback06);1A 03);14 07, value 128);14 08, value 128).Automation coverage
slice filterpreset FIL1|FIL2|FIL3so automation selects a preset by stable identity rather than synthesizing a width.filterPresetIdandfilterPresetto slice snapshots.Expected recall geometry
Validation
Live IC-7300MK2 automation-bridge proof
Ran a customize-then-click-the-same-visible-button sweep for all 12 combinations of USB, LSB, AM, and CW with FIL1/FIL2/FIL3.
For every case:
Receive filter FILnbutton;Result: 12/12 passed. The authenticated CI-V scheduler recorded 14,353 replies, 0 timeouts, 0 late replies, 0 stale replies, and 0 unmatched frames in the final verification snapshot.
The bridge was launched with transmit disabled (
txAllowed=false). The radio remained unkeyed. After testing it was restored to 14.2435 MHz, DIGU, FIL1, -300..3300 Hz, ANT1, verifiedtransmitting=false, and disconnected cleanly.Hardware proof is specifically from the IC-7300MK2. The implementation is in the shared profile-driven Icom backend used by IC-705, IC-7300MK2, and IC-9700; socket-free codec coverage exercises the shared path, but this PR does not claim live IC-705 or IC-9700 hardware validation.
Automated tests
icom_civ_test: stable preset identity plus the exact four-command recall plan and expected geometry for USB/LSB/AM/CW x FIL1/FIL2/FIL3.filter_passband_math_test: mode-aware gesture scaling, minimum/maximum clamps, untouched-edge anchoring, and stale-capability rejection.icom_civ_testicom_civ_scheduler_testicom_meters_testicom_family_testrf_gain_presentation_testicom_power_derivation_testphone_cw_mic_gain_authority_testfilter_passband_math_testbridge_docs_checktools/check_test_registration.py --strict: passed.tools/check_engine_boundary.py --strict: passed with only tracked legacy warnings.git diff --check: passed.AetherSDR.appbuild: passed using-j22.Scope and compatibility
rxFilterControl, so their existing width-based buttons and persistence remain unchanged.Generated with OpenAI Codex (Daybreak Blue)
Review follow-up: fixed-width selection and legacy gesture compatibility
icom_civ_testnow covers all advertised fixed-mode slots through both mode-command variants, the DATA flag, and invalid IDs.filter_passband_widget_testinjects Qt mouse events into the production widget. It covers 10 kHz AM shift and widening, legacy vertical scaling/minimum width, advertised AM/USB maximums, and clearing capabilities.Known non-blocking boundary: final edge rounding can still move an off-grid untouched edge (for example the 125 Hz edge of CW FIL3); this predates these repairs. Icom's 50 Hz fine gesture step also remains subject to the codec's coarser quantization above 500 Hz.
Validation of the repaired current-main tree (
a4227e68plus repairb77b84e5): full macOS ARM64 application build with RADE enabled passed;icom_civ_test,filter_passband_widget_test,filter_passband_math_test, andbridge_docs_checkpassed (4/4). Strict engine-boundary, test-registration, and frozen CI-gate checks passed. Both added commits are signed. The earlier full-app offscreen launch could not expose the bridge after QRhi initialization failures; runtime evidence here is the socket-free production widget event test, not a new hardware or full-app demo sweep.