fix(icom): Restore durable Icom memory ownership and synced recall - #5328
fix(icom): Restore durable Icom memory ownership and synced recall#5328jensenpat wants to merge 3 commits into
Conversation
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Issue fit
There is no fixes/closes #NNNN in the commit (9a17060 Restore durable Icom memory ownership (Principle XI)), so I reviewed against the PR's own stated intent: make AetherSDR's shared client database the working memory store for every Icom, turn 1A 00 reads into an ingestion path rather than an ownership handover, and have repeated syncs update the same row instead of duplicating it. The first two are delivered cleanly and are, in my read, the right architectural call — persistsMemories was doing double duty as "who owns the store" and "can this radio be read", and splitting canRefreshMemories out of it is a genuine improvement over the previous profile-gated persistsMemories. The third claim does not hold in the one case that matters most; see Blocker 1.
Per GOVERNANCE.md this is close to an architectural change (it reverses which store owns Icom memories and bumps a persisted schema version), and it arrives with no linked issue or RFC. That is a maintainer call, not something I'd block on by itself, but it is worth noting that the capability semantics being changed here were themselves ratified through RFC #4603.
Scope
| File / group | What it changes | Claimed? | Verdict |
|---|---|---|---|
IcomCivBackend.{h,cpp} |
persistsMemories→false, canRefreshMemories←profile, import identity + importSource/importKey/owner on the memory delta |
Yes | In scope |
RadioModel.cpp (applyMemoryChanges) |
Import routing: slot lookup, create-on-miss, persist | Yes | In scope |
LocalMemoryBank.{h,cpp} |
importedSlot() |
Yes | In scope |
LocalMemoryStore.{h,cpp} |
Persist 10 previously-unpersisted fields; kFormatVersion 1→2 |
Implied | In scope — see Nit 1 |
MemoryEntry.h, MemoryDelta.h |
importSource / importKey |
Yes | In scope |
MemoryDialog.cpp |
Comment; radio groups folded into the filter combo | Yes | In scope, but see Blocker 2 |
RadioCapabilities.h, both docs |
Comment/doc de-drift | Yes | In scope — one stale row left, see Nit 2 |
tests/* (3 files) |
New import-identity, round-trip, and gating assertions | Yes | In scope |
Nothing here is unrelated to the stated change. MemoryDelta::importSource/importKey is new cross-seam surface (backend → model), but it is internal C++, not a protocol verb or a settings key, and it is the mechanism the change requires. The persisted JSON schema bump is new durable surface — flagged below rather than waved through.
I did not find a false self-certification: no CHANGELOG.md entry (correct), no unrelated files, no formatting churn, no removed guards. Reading the - lines specifically: the only deleted logic is c.persistsMemories = memory != nullptr and c.canRefreshMemories = c.persistsMemories, both replaced deliberately; no comment naming a fixed symptom was dropped.
Blockers
1. The import identity is an IP address for every Icom, so a DHCP lease change permanently duplicates the whole channel set. (inline: IcomCivBackend.cpp:711)
The new comment says "Prefer the discovery identity so repeated syncs from the same radio update their rows; retain a deterministic endpoint fallback for a manually-entered radio that reports no serial." I could not reproduce a case where the first branch is ever taken. Icom has no discovery — ConnectionPanel.cpp:2739-2741 is the only producer of an Icom RadioInfo, and it always synthesizes info.serial = "icom:<ip>" (or "icom:<ip>:<port>" off the default base port), with its own comment saying "No discovery means no MAC and no reported serial, so the host is the only stable identity this radio has for us." That flows to req.serial at RadioModel.cpp:2436/:3456, so request.serial.trimmed() is never empty and the request.host fallback is dead code.
Net effect: m_memoryImportSource is always "icom:icom:<ip>". Failure scenario — an IC-9700 on DHCP, synced at 192.168.1.50, gets a new lease as 192.168.1.71, operator hits Sync again: importedSlot() matches nothing, applyMemoryChanges takes the targetIndex < 0 branch and calls memory create for every occupied channel, up to 297 fresh rows land beside the 297 already there. Same on the same radio reached via a hostname vs. an IP, or on the default base port vs. a custom one (different synthesized serial string). Nothing prunes the old rows: the d.removed path only forgets rows matching the current source. The ghosts are permanent, live in the durable settings-backed document that settings backup/export covers, and are visually indistinguishable in the dialog from the operator's own memories.
This falsifies the PR's and the design doc's central claim ("Repeated syncs update rows from the same radio/channel rather than duplicating them") — it is true only while the IP never changes, which is not a property a DHCP client has.
In fairness to the author: RadioModel::settingsScope() already keys per-radio state off this same IP-derived serial, so the PR is consistent with existing practice. The consequence differs, though. For settings, an address change means "per-radio settings reset" — annoying and self-limiting. Here it means the shared, backed-up memory database silently doubles and mixes ghost rows into the operator's list, with hand-deletion the only cleanup.
Fixes I'd consider, in preference order: (a) key importSource on something the radio actually reports — the CI-V address plus m_model->name is already decoded by adoptReportedCivAddress() and is address-independent; (b) keep the IP key but, at the end of a successful, complete refresh, reconcile: prune rows whose importSource names this same model but a stale endpoint; (c) at minimum, document the duplication and give the dialog a way to select all rows from one importSource. Option (a) also makes the "discovery identity" comment true.
2. On an IC-705, Sync now silently does nothing for most filter-combo selections. (inline: MemoryDialog.cpp:1484)
IcomCivBackend::refreshMemories() resolves groupName against the profile's group names and, when memory.requiresGroupSelection (the IC-705), bails with a bare return if it did not match (IcomCivBackend.cpp:1097-1099) — no signal, no toast, no memoryRefreshStarted. Before this PR the Icom combo contained only group names, so the sole way to hit that return was leaving it on "All Memories". This diff moves Icom onto the else branch, where the combo is relabelled "Profile:" and now carries global profiles + TX profiles + radio groups in one flat sorted list, and MemoryDialog.cpp:467 passes whatever is selected straight to refreshMemories().
Failure scenario: IC-705 operator with any global or TX profile defined, combo left on (or sorted to) a profile name, clicks Sync — the button appears to work and nothing whatsoever happens, with no way to tell that from a radio that answered with zero occupied channels. The blast radius is bigger than the old one-value case, and it is introduced by this hunk. Either gate/label the Sync affordance on the selection being a member of capabilities.memoryGroups, or have refreshMemories() report the refusal rather than returning silently.
Nits (non-blocking)
- The schema bump is a one-way door and is not called out.
kFormatVersion 1→2is backward-compatible on read (parse()only rejectsversion > kFormatVersion, so v1 documents load fine, and every new field reads through a defaulted accessor). It is not forward-compatible:LocalMemoryBank::load()'s row-version guard andparse()'s body check both make a v2 document read-only on any older build, so a downgrade after one sync leaves the operator's whole bank refusing edits with a "newer than this build" warning. That is the designed behaviour and the bump is defensible — dropping the new fields on an old build's write-back would be worse — but it deserves a sentence in the PR body. - Dead code the change leaves behind. With
persistsMemoriesnow false on every Icom and Flex the only backend setting it true,usesNativeMemorySchema(MemoryDialog.cpp:749-750and:1456-1457) is unconditionally false, so ~45 lines of native-column layout and the entireRadioCapabilities::memoryGroupColumnTitlefield (its only reader isMemoryDialog.cpp:757) are now unreachable. Likewise the whole!usesLocalMemoryBank()read-only-radio-store branch intryMemoryCommand(RadioModel.cpp:7797-7827): reaching it needspersistsMemories, which only Flex has, and Flex setscanWriteMemories/canApplyMemoriestrue so it falls straight toreturn nullopt. Not wrong, but it is a subsystem with no users, and the caps doc'scanApplyMemoriesrow still describes that path's behaviour as live ("applies recallable cached fields through the existing neutral slice setters") — worth either deleting or marking as reserved-for-future while the docs are being de-drifted anyway. - Lookup key is not normalized the way the stored key is.
applyMemoryChangesstoressanitize(*d.importSource)/sanitize(*d.importKey)(RadioModel.cpp:8078-8079) butimportedSlot()is called with the raw values (:8027).sanitizeTextstrips C0 and0x7f, which today's values never contain, so this is latent — but if it ever fires, the symptom is a fresh row on every sync forever, i.e. Blocker 1's failure mode with a harder-to-find cause. Cheapest fix is to sanitize once before both uses. - CodeGuard's two
CG-PATH-001hits are false positives —MemoryDialog.cpp:1194and:1420areQProgressDialog::setLabelTextformat strings ("Importing %1 of %2 memories…" / "Deleting %1 of %2 memories…"). No filesystem path is involved and neither line is in this diff. Dropping them.
What I tried to break
- "Repeated syncs update rows rather than duplicating them." Broke it — Blocker 1. Traced
importSourceback throughRadioConnectRequest.serialto its only producer to confirm thehostfallback is unreachable and the identity is always endpoint-derived. - "A radio's channel 1 cannot overwrite the operator's client slot 1." This one held, and I pushed on it.
allocateSlot()picks the lowest free index in the bank,publishLocalMemories()seedsm_memoriesfrom the bank on the connect edge viaonConnected → syncMemoryStoreForSession, and I confirmedonConnectedis wired toIRadioBackend::connected(RadioModel.cpp:1523) and not Flex-only — so the two maps are in sync before any create, and no aliasing is possible. Within one sync the ordering is safe too:record()writes the provenance pair before the next channel'simportedSlot()scan. - Radio-swap and disconnect lifecycle. Checked Flex→Icom and Icom→Flex.
onDisconnectedclearsm_memories, flushes the bank, and re-publishes (RadioModel.cpp:7053-7066), andsyncMemoryStoreForSessionclears before a radio-owned dump — so no leakage of Flex slots into the bank-published cache, in either direction. Them_sessionRadioOwnsMemorieslatch behaves correctly across a link blip on the new Icom path. - Failure paths. An unreadable/unwritable bank:
memory createrejects, the new code logs viaqCWarning(lcProtocol)and returns without touching the cache — correct.d.removedfor a channel never imported:targetIndex < 0, early return, no spuriousmemoryRemoved— correct. Non-occupied channels during an IC-705 group sweep behave. - Recall after import. Confirmed
MemoryEntry::recallabledefaults totrue, so manual/CSV rows still tune on an Icom now thatmemory applyroutes through the local bank intorecallCachedMemory()instead of the old read-only branch; and that the newly-persistednativeFilter/dataMode/rxToneValue/DTCS fields are exactly the onesrecallCachedMemory()feeds toapplyMemoryRecallDetails(), so a synced repeater channel survives a restart intact. This is the part of the PR I'd most want kept. - Filter/group namespace collision.
populateTablefilters onm.group, and the backend setsdelta.group = memoryGroupName(...), so the new combo entries do match imported rows — the added loop is necessary, not cosmetic, and it is in the liveelsebranch rather than the dead one. It did surface Blocker 2. - Attacked the tests.
radio_capability_gating_testadditions genuinely fail against the unfixed code (oldpersistsMemorieswastruefor all three profiles).local_memory_bank_test's reopen case is stronger than it looks — the second bank uses a different file path, so it proves the round trip goes through the shared settings document, not the legacy file. But nothing coversRadioModel::applyMemoryChanges's import routing, which is the actual headline behaviour: no test asserts that a radio channel lands in a fresh slot instead of slot 1, that a second sync reuses the row, or that an unoccupied channel forgets it.importedSlot()being correct is necessary, not sufficient.grepovertests/showsMemoryDeltaappears only inaetherd_residual_decode_test.cpp, so there is no existing harness to extend — which is probably why, but it is the test that would have caught Blocker 1. - What I could not check: CI is green on all four checks for
9a17060(build, check-windows, check-macos, Static checks), butctestinci.ymlis-R-filtered, so green does not mean the ~240-test suite passed. I have no build and no GUI here — every finding above is reasoned from the code at/tmp/aetherclaude/pr-5328, not reproduced at runtime. In particular I could not observe an actual IC-705/IC-9700 sync, so Blockers 1 and 2 are read off the call graph rather than watched happening.
Recommendation
Request changes, narrowly. The architecture is right and I'd like to see it land: separating "who owns the working store" from "can this radio be read as an import source" is a real improvement, the durable-recall field set is the part operators will feel, and the lifecycle handling around connect/disconnect/radio-swap survived everything I threw at it. But Blocker 1 makes the PR's own headline guarantee conditional on a stable IP, and the failure is silent, permanent, and lands in a backed-up document — that wants fixing before merge, ideally by keying the identity on the CI-V-reported model instead of the endpoint. Blocker 2 is a smaller, self-contained fix in the same area. Concrete next step: re-key m_memoryImportSource, add one regression test over applyMemoryChanges covering create-then-resync-then-remove, and either gate the Sync button on a real group selection or make refreshMemories say why it refused. Blocker 1's fix option (a) vs (b) is worth a maintainer opinion first, since (b) implies a pruning policy nobody has ruled on.
Thanks for the unusually careful doc and comment work here — the radio-capabilities-map.md rewrite of the persistsMemories row is clearer than what it replaced, and the reasoning comments in applyMemoryChanges made this reviewable.
🤖 aethersdr-agent · cost: $10.4712 · model: claude-opus-5
| && d.importKey && !d.importKey->isEmpty(); | ||
| if (isImport) { | ||
| m_localMemories.load(); | ||
| targetIndex = m_localMemories.importedSlot(*d.importSource, *d.importKey); |
There was a problem hiding this comment.
The lookup uses the raw delta values, but :8078-8079 stores them through sanitize():
if (d.importSource) m.importSource = sanitize(*d.importSource);
if (d.importKey) m.importKey = sanitize(*d.importKey);MemoryFields::sanitizeText strips C0 controls and 0x7f, which neither of today's values contains, so this is latent rather than live. But if it ever does fire the symptom is a brand-new row on every sync forever — the same failure mode as the import-identity blocker, with a harder-to-trace cause. Cheapest fix is to sanitize once up front and use the normalized pair for both the lookup and the store.
Separately (non-blocking): this whole import branch is the headline behaviour of the PR and has no test. local_memory_bank_test proves importedSlot() in isolation, which is necessary but not sufficient — nothing asserts that a radio channel lands in a fresh slot rather than clobbering client slot 1, that a second sync reuses the row, or that an unoccupied channel forgets it. MemoryDelta appears in only one test file today, so there is no harness to extend, but a small direct test over applyMemoryChanges covering create → resync → remove is what would have caught the identity problem.
|
Cisco CodeGuard — static analysis of this PR (2 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $11.5923 · model: claude-opus-5 |
9a17060 to
786c87d
Compare
jensenpat
changed the title
786c87d to
776c7ca
Compare
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Issue fit
#5327's regression (durable database hidden, Tune/Import broken, transient sync rows) is genuinely fixed, and the live automation-bridge validation on the IC-7300MK2 — 221 rows across restart, idempotent double-Sync, panadapter-spot recall — is exactly the right kind of evidence and covers what it claims. The provenance design (icom:<guid> + native <group>:<channel> key, all-zero GUID refused) is sound. The problems are at the edges the live run's own coverage-limits section honestly lists as untested — and several of them are the sharp edges.
Scope
All files map to the issue. One out-of-band item: none — the diff is clean. Preflight: no sockets, no fake peers; every new test is socket-free.
Blockers
1. Split/RPS recall is unevidenced, and recalling one pushes wrong state at a live radio (inline at the codec). The deleted rule was recallable = mode && !split && duplex != 3; the new rule keeps only the mode test, justified by a comment claiming "the local database already has a complete, neutral RX frequency" — but IcomMemoryChannel has no field for the second frequency block (it is parsed for nothing), and duplex == 3 falls through the backend's default: arm to offsetDir = "simplex". Tune on an RPS/split row therefore sets the slice to the first-block frequency and pushes DUP=simplex to the radio — clearing the stored split and, on transmit, keying on the wrong frequency. The PR body's own coverage-limits section says split/reverse-split recall was never live-tested, and the changed test asserts the flag, not that the recalled frequency is the RX side. Until the second block is decoded (or its semantics evidenced per the repo's docs/data/ precedent), duplex == 3 and split rows should stay display-only.
2. Re-Sync silently destroys operator edits on imported rows (inline at the stamp). delta.owner/delta.group/delta.name are unconditionally re-stamped on every pass, and applyMemoryChanges applies any present field to the matched row — so an operator who annotates Owner, fixes a name, or re-groups a synced channel loses those edits on the next Sync, persisted, with no prompt or diff. The body's "does not overwrite manual/CSV memories" is true and misses this class. Either skip fields the operator has touched (a dirty mask), stamp only on first insert, or warn.
3. The recallability repair heals partially, can be skipped entirely, and re-fires forever (inline). Three verified defects in one loop: (a) it keys on MemoryFields::isKnownMode, but the codec emits "CWL" and "WFM", neither in modes() — so CW-R/WFM rows poisoned by the first build stay display-only while their neighbours heal ("the fix mostly worked" bug reports); (b) it sits below if (!parsed.ok()) return;, so one non-fatal bad row anywhere in the document disables the whole repair; (c) it is un-versioned — it runs on every launch, so the moment DV/DD gain a neutral mapping (DSTR/FDV are already in modes()) every deliberately display-only row silently flips recallable, permanently, and no future codec decision can stick. The store's own kFormatVersion bump this PR makes is the migration hook the comment's "once" wants — gate the repair on storedRowVersion < 2, key it on the codec's rule, and move it into the store's schema layer.
4. The format-version bump is a one-way door for purely additive fields — and the repair walks users through it without any action of theirs. Every v2 field is read via value(...).toX(default), so a v1 build would parse a v2 document harmlessly; but flush() stamps kFormatVersion=2 on any save, load() refuses to write when stored > built, and the load-time repair itself triggers a flush. An operator who merely launches this build with synced rows and then rolls back finds an empty, read-only memory bank — indistinguishable from data loss, in the PR that exists because of an indistinguishable-from-data-loss bug. Don't bump the version for additive fields, or at minimum never auto-upgrade from load().
5. Tune half-applies then fails on a disconnected Icom. Imported rows always carry nativeFilter/offsetDir, so recall always reaches applyMemoryRecallDetails, which returns false without a session — after setMode/setFilterWidth/setFrequency have already run. The dialog is deliberately fully usable while disconnected, so this is a reachable ordinary path: half-applied slice plus a Tune error on a row presented as a normal database memory. Validate the backend requirement before mutating the slice.
6. Sync reports 100% success when zero rows were stored. The create-refusal and non-writable-bank paths are qCWarning-only, while the reply counter that feeds memoryRefreshFinished(true, n, n) increments before the delta is applied — an unreadable or full bank yields "297 of 297" and a success banner with nothing persisted. This is the exact silent-drop shape #5263 made loud on the command plane; thread a stored-count (or failure reason) back into the finished signal.
Also needing your ruling, not a blocker: persistsMemories = false for a radio that demonstrably persists and recalls its channels is in tension with Principle III's letter ("the deciding test is simply whether the radio can save and restore the value"). The mitigations are real — the client cannot write native memories, sync is one-directional, and both #5327 and the pre-#5283 design note prescribe this model — but III outranks the design doc, so the stale-recall trade (front-panel edit vs. un-resynced client row) should be ruled on explicitly rather than passed silently.
Nits (non-blocking, condensed)
usesNativeMemorySchemais now compile-time impossible (Icom hardcodespersistsMemories=false; only Flex sets true), leaving ~150 lines of dead two-schema table code, a write-onlymemoryGroupColumnTitlecapability, and — the real cost — the newly persistedchannel/nativeFilter/dataMode/rxToneValuefields rendered nowhere: a split-tone repeater's RX tone is stored, un-viewable, un-editable.- The Group-cell editor lost stored-group suggestions wherever
hasProfilesis true — including the never-connected default-Flex state, where the dropdown is now empty for an operator with 40 grouped rows (typo-fork risk); andhasProfilesis the wrong predicate for "who owns the memory vocabulary" (the deleted code keyed onpersistsMemories;MainWindow.cpp:7230shows the!connected ||form). The enable-check (case-insensitive, trimmed) and the backend accept-check (exact) also disagree, with the new gating test enshrining a string the backend rejects; and on IC-9700/MK2 a stored-group selection passes the GUI check and silently sweeps all groups. - Import identity (
importSource/importKey) is absent from the kv wire-codec and the CSV export, so a row round-tripped through either path loses its identity and the next Sync duplicates it — state the fields as backend→model-only inMemoryDelta.hor carry them explicitly. - Sync-time UI cost compounds: per-row
memoryChanged→ 50 ms full-table rebuilds × up to five by-valueRadioCapabilitiesconstructions per rebuild ≈ thousands of 700-field builds per 297-channel sweep; the already-wired refresh-finished signal should own the single repopulate. - The 47-byte IC-7300MK2 claim is live-radio fact with no
docs/data/evidence artifact (the repo's own IC-9700 precedent),layoutForstill sayssingleBytes=33while the comment says 33 never occurs, and the split-length fact belongs in the layout table (the inlinedialect !=test is exactly how the bug being fixed got introduced);docs/architecture/radio-capabilities-map.md'scanApplyMemoriesrow still says split/RPS are display-only, which this PR's own test now falsifies. - The identity-refusal guard — the advertised anti-aliasing protection — has no test reaching
refreshMemories, while the PR's ownradio_capability_gating_testpattern proves backend refusals are testable pre-transport; smaller:radioIdHexhand-rolls hex whereQByteArray::toHex()is the idiom two files away, imports insert twice via the command-string round-trip,flush()pretty-prints then re-parses the whole bank, and the repair logs "repaired N" even when the flush was refused.
What was verified vs read
- Verified by me in primary sources: the old vs new recallable rules and the absent second-frequency field; the
default:→simplex mapping; the owner/group/name stamps and their unconditional application;modes()vs the codec's"CWL"/"WFM"; the repair's position below theparsed.ok()early-return;persistsMemories=falsewith Flex as the onlytrueproducer and both renderers of the new fields behind the dead gate; main's editor-suggestion union vs the new spec. - Refuted along the way (claims the pass itself killed): the radioId ordering race, the 297×
load()/O(n²)-flush cost (loads latch, saves debounce), stale-Flex-rows leaking into imports, and my own initial "repair runs per command" reading (once per process). - Not run: no hardware, no bridge session of my own — the author's live IC-7300MK2 bridge run covers the happy paths well; every blocker above lives in the paths that run's coverage-limits section names as untested.
| // impossible to navigate to: the local database already has a complete, | ||
| // neutral RX frequency and mode. Modes with no neutral representation | ||
| // (currently DV/DD) remain display-only. | ||
| memory.recallable = mode.mode.has_value(); |
There was a problem hiding this comment.
Blocker 1 — the deleted guard's cases were never given the decode they need. The comment above claims "the local database already has a complete, neutral RX frequency", but IcomMemoryChannel has no field for the second 4..17 block (it is parsed for nothing), and in the backend duplex == 3 falls through the default: arm to offsetDir = "simplex". Recalling an RPS/split row therefore tunes the first-block frequency and pushes DUP=simplex at the live radio — clearing the stored split and, on TX, keying the wrong frequency. Your own coverage-limits section lists split/reverse-split recall as untested, and the changed test row asserts the flag, not which frequency recalls.
Until the second block is decoded (or its semantics evidenced à la docs/data/icom-ic9700-fm-repeater-*), keep duplex == 3 and split rows display-only:
| memory.recallable = mode.mode.has_value(); | |
| memory.recallable = mode.mode.has_value() && !memory.split && duplex != 3; |
| delta.importKey = QStringLiteral("%1:%2") | ||
| .arg(memory->group) | ||
| .arg(memory->channel); | ||
| delta.owner = m_model |
There was a problem hiding this comment.
Blocker 2 — this re-stamp lands on rows the operator has since edited. importedSlot now matches existing rows, owner/group/name are stamped on every pass, and applyMemoryChanges applies any present field — so an annotation ("W6ABC trustee — verify PL"), a corrected name, or a re-grouping is silently reverted and persisted on the next Sync. The body's "does not overwrite manual/CSV memories" guarantee doesn't cover the imported rows' operator edits, which are exactly the rows Sync revisits.
Options: stamp identity-ish fields only on first insert; carry a per-row operator-touched mask; or diff-and-warn. Any of the three preserves the idempotent-upsert property the live test proved.
| // row: if its mode can be applied to an AetherSDR slice, stale Icom import | ||
| // metadata must not permanently prevent navigation. Repair the rows once | ||
| // on load and persist the corrected document atomically. | ||
| int repairedRecallability = 0; |
There was a problem hiding this comment.
Blocker 3 — three verified defects in this loop. (a) It keys on MemoryFields::isKnownMode, but the codec emits "CWL" (Cw-R) and "WFM" — neither is in modes() — so rows poisoned by the first build in those modes stay display-only while their neighbours heal. (b) It sits below if (!parsed.ok()) return;, so one non-fatal bad row (a duplicate slot in a hand-edited export) disables the entire repair. (c) It is un-versioned and runs on every launch: the moment DV/DD gain a neutral mapping (DSTR/FDV are already in modes()), every deliberately display-only row flips recallable forever, and no codec decision can stick. It also makes load() a writer — which can trip the foreign-write guard in the two-window case — and logs "repaired N" even when the flush refused.
The kFormatVersion 1→2 bump this PR makes is the one-shot hook the comment's "once" wants: gate on storedRowVersion < 2, key the predicate on the codec's own rule, and do it in the store's schema layer.
| class LocalMemoryStore { | ||
| public: | ||
| static constexpr int kFormatVersion = 1; | ||
| static constexpr int kFormatVersion = 2; |
There was a problem hiding this comment.
Blocker 4 — a one-way door for purely additive fields. Every v2 field is read value(...).toX(default), so a v1 build parses a v2 document harmlessly — yet flush() stamps 2 on any save, load() refuses to write when stored > built, and the load-time repair triggers a flush without any user action. Launch this build once with synced rows, roll back, and the previous release shows an empty, read-only memory bank — indistinguishable from data loss, in the PR fixing an indistinguishable-from-data-loss bug. Keep additive fields at version 1 (or bump only on an actual incompatibility, and never auto-upgrade from load()).
2 participants
Closes #5327.
Summary
Restore AetherSDR's durable, writable memory database as the working memory model for every Icom radio, while retaining explicitly model-gated radio Sync readers for the IC-705, IC-7300MK2, and IC-9700.
This fixes the regression introduced by #5283 where connecting a profiled Icom radio replaced the client database view with a transient, read-only radio cache. Existing memories appeared lost, Import/Add/Edit/Remove were disabled, and synced channels could not reliably tune.
Model
MemoryProfilecodec.Changes
Durable database ownership
persistsMemories=false, keeping AetherSDR's sharedMemoryBankactive across connection, disconnect, and restart.canRefreshMemoriesis independent and enabled only for the IC-705, IC-7300MK2, and IC-9700 profiles.Stable Sync upserts
Synced rows carry stable provenance:
importSource: authenticated RS-BA1 radio identity (icom:<16-byte-radio-guid>)importKey: native<group>:<channel>identitySync updates the matching database row or allocates the lowest free client slot. It does not overwrite manual/CSV memories with the same numeric slot, duplicate rows on repeated Sync, alias channels from different radios, or change identity when DHCP changes an address. An absent/all-zero authenticated GUID refuses Sync with a configuration warning rather than falling back to a mutable endpoint. No migration is included because the earlier identity scheme has not shipped.
Radio-family filter isolation
memoryGroupsvalue. The backend also rejects an invalid group with a visible configuration warning rather than silently returning.Complete persisted recall state
Memory document schema 2 persists native channel, import provenance, native filter/data-mode selection, RX tone, DTCS fields, and recallability. Schema-1 documents remain readable and upgrade on their next save; future/unreadable document protections are unchanged.
IC-7300MK2 record correction
The IC-7300MK2 returns its documented second RX/TX data block even when Split is off, so ordinary live records are 47 bytes. Reply length is no longer treated as proof of Split; the documented Split flag is decoded instead.
Persisted display-only repair
The first test build had already saved all 66 synced IC-7300MK2 rows as
recallable=false, including plain AM/USB/DIGU/DIGL channels. On load, Icom-imported rows with a neutral AetherSDR mode are now repaired and atomically persisted. No database deletion or repeat Sync is required.Split and reverse-split rows may recall their neutral RX side. DV/DD remain display-only because they have no neutral mode mapping.
Evidence
The failed first live run was diagnosed from its logs and settings database:
memory apply;display-only;recallable=false.The memory-recall implementation was subsequently validated live on an IC-7300MK2 from the Mac Studio using the AetherSDR automation bridge. The later review-fix delta (stable GUID identity and profile/group isolation) is covered by focused socket-free tests and mutation checks; it has not been rerun against live hardware.
Live automation bridge validation — IC-7300MK2
Tested AetherSDR v26.9.1 at exact PR head
786c87d5891124696efdd692c152adbb4b5f0c42against a network-connected IC-7300MK2. Automation ran with transmit disabled; no TX occurred and final radio state was unkeyed.NMC WEFAX NIGHT) to 4.344100 MHz DIGU from the Memory dialog.No product defect was found in the live IC-7300MK2 paths exercised above.
Live-test coverage limits
Validation
origin/main(0f78183c). Current head:776c7ca796636e52728270782e4b610fde74d9bc.git merge-tree --write-tree origin/main HEAD: clean.-j22: passed; ONNX Runtime detected normally.icom_protocol_testicom_memory_testlocal_memory_bank_testlocal_memory_store_testmemory_recall_policy_testmemory_csv_compat_testradio_capability_gating_testgit diff --check: passed.The local-bank regression test writes an Icom DIGU row with the poisoned
recallable=falsevalue, reopens the bank, verifies it becomes recallable, and verifies the repaired value is persisted.Safety
Automation follow-up
Extend the automation bridge with first-class
memory list/create/remove/sync/importoperations, expose each row's recallability/import provenance/native channel, assign a stable object name to the Memory dialog search field, and expose memory-spot geometry/hit targets. These would eliminate native-picker and coordinate-selection gaps in future end-to-end memory testing.Generated with OpenAI Codex (Daybreak Blue)