-
-
Notifications
You must be signed in to change notification settings - Fork 114
fix(icom): Restore durable Icom memory ownership and synced recall #5328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,15 +9,15 @@ | |
|
|
||
| namespace AetherSDR { | ||
|
|
||
| // Portable, versioned JSON persistence for the CLIENT-side memory bank — the | ||
| // channels an operator saves on a radio that has no memory storage of its own | ||
| // (Hermes-Lite 2, Kiwi, the demo backend). On a Flex the radio owns the slots | ||
| // and this file is never touched; see RadioCapabilities::persistsMemories. | ||
| // Portable, versioned JSON persistence for the CLIENT-side memory bank — both | ||
| // channels the operator creates here and snapshots explicitly imported from a | ||
| // radio. On a Flex the radio owns and mutates the active slots, so this document | ||
| // is not the session store; see RadioCapabilities::persistsMemories. | ||
| // | ||
| // Envelope: | ||
| // { | ||
| // "format": "aether.memories", | ||
| // "version": 1, | ||
| // "version": 2, | ||
| // "savedAt": "2026-07-29T14:00:00Z", | ||
| // "savedBy": "AetherSDR", | ||
| // "memories": [ { "index": 0, ...MemoryEntry... } ] | ||
|
|
@@ -35,7 +35,7 @@ namespace AetherSDR { | |
| // bank is sparse. | ||
| class LocalMemoryStore { | ||
| public: | ||
| static constexpr int kFormatVersion = 1; | ||
| static constexpr int kFormatVersion = 2; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocker 4 — a one-way door for purely additive fields. Every v2 field is read |
||
| static constexpr const char* kFormatId = "aether.memories"; | ||
|
|
||
| // The bank's home since RFC #4603 PR 6: ONE shared feature document in | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -461,10 +461,14 @@ RadioCapabilities IcomCivBackend::capabilities() const | |
| c.hasRadioSideWaterfallAutoBlack = false; | ||
| const MemoryProfile* memory = m_model && profileFor(*m_model).memory | ||
| ? &*profileFor(*m_model).memory : nullptr; | ||
| c.persistsMemories = memory != nullptr; | ||
| // The AetherSDR memory model is always the shared client database for | ||
| // Icom. A model-specific codec only adds an explicit radio-to-database | ||
| // Sync source; it does not hand ownership of the working store to the | ||
| // radio. | ||
| c.persistsMemories = false; | ||
| c.canWriteMemories = false; | ||
| c.canApplyMemories = false; | ||
| c.canRefreshMemories = c.persistsMemories; | ||
| c.canRefreshMemories = memory != nullptr; | ||
| if (memory) { | ||
| c.memoryGroupColumnTitle = QString::fromLatin1(memory->groupColumnTitle.data(), | ||
| static_cast<qsizetype>(memory->groupColumnTitle.size())); | ||
|
|
@@ -726,6 +730,11 @@ void IcomCivBackend::connectRadio(const RadioConnectRequest& request) | |
| { | ||
| disconnectRadio(); | ||
|
|
||
| // The stable import identity arrives in the authenticated RS-BA1 | ||
| // capabilities record. An endpoint is deliberately not used here: DHCP, | ||
| // mDNS and NAT changes must not turn one radio into a second import source. | ||
| m_memoryImportSource.clear(); | ||
|
|
||
| IcomSession::Params p; | ||
| p.host = QHostAddress(request.host); | ||
| p.controlPort = request.port ? request.port : kControlPort; | ||
|
|
@@ -1110,6 +1119,14 @@ void IcomCivBackend::refreshMemories(const QString& groupName) | |
| return; | ||
| } | ||
| const MemoryProfile& memory = *profileFor(*m_model).memory; | ||
| if (m_memoryImportSource.isEmpty()) { | ||
| qCWarning(lcIcomCiv) | ||
| << "memory sync refused: RS-BA1 supplied no stable radio identity"; | ||
| emit configurationWarning( | ||
| QStringLiteral("This radio did not provide a stable RS-BA1 identity, so its " | ||
| "memories cannot be synced safely.")); | ||
| return; | ||
| } | ||
| int selectedGroup = -1; | ||
| if (!groupName.isEmpty() && memory.firstGroup >= 0) { | ||
| for (int group = memory.firstGroup; group <= memory.lastGroup; ++group) { | ||
|
|
@@ -1120,6 +1137,10 @@ void IcomCivBackend::refreshMemories(const QString& groupName) | |
| } | ||
| } | ||
| if (memory.requiresGroupSelection && selectedGroup < memory.firstGroup) { | ||
| qCWarning(lcIcomCiv) | ||
| << "memory sync refused: invalid group selection" << groupName; | ||
| emit configurationWarning( | ||
| QStringLiteral("Choose a valid Icom memory group before syncing.")); | ||
| return; | ||
| } | ||
| m_memoryRefreshActive = true; | ||
|
|
@@ -1379,6 +1400,13 @@ void IcomCivBackend::adoptReportedCivAddress(std::uint8_t reported) | |
| void IcomCivBackend::onSessionConnected(const QString& deviceName) | ||
| { | ||
| m_deviceName = deviceName.trimmed(); | ||
| const std::string stableRadioId = radioIdHex(m_session->radioId()); | ||
| if (stableRadioId.empty()) { | ||
| m_memoryImportSource.clear(); | ||
| } else { | ||
| m_memoryImportSource = QStringLiteral("icom:%1").arg( | ||
| QString::fromStdString(stableRadioId)); | ||
| } | ||
| m_connected = true; | ||
| m_connectedAtMs = nowMs(); | ||
| m_lastIncident.clear(); | ||
|
|
@@ -2546,6 +2574,14 @@ void IcomCivBackend::onCivFrame(const CivFrame& frame, | |
| } | ||
| MemoryDelta delta; | ||
| delta.index = index; | ||
| delta.importSource = m_memoryImportSource; | ||
| delta.importKey = QStringLiteral("%1:%2") | ||
| .arg(memory->group) | ||
| .arg(memory->channel); | ||
| delta.owner = m_model | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Blocker 2 — this re-stamp lands on rows the operator has since edited. 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. |
||
| ? QString::fromLatin1(m_model->name.data(), | ||
| static_cast<qsizetype>(m_model->name.size())) | ||
| : QStringLiteral("Icom"); | ||
| if (!memory->occupied) { | ||
| delta.removed = true; | ||
| emit memoryChanged(delta); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -161,7 +161,13 @@ std::optional<IcomMemoryChannel> decodeMemory( | |||||
| return std::nullopt; | ||||||
| } | ||||||
|
|
||||||
| memory.split = payload.size() == static_cast<std::size_t>(layout.splitBytes); | ||||||
| // The IC-7300MK2 always returns the second 4..17 block documented by its | ||||||
| // CI-V guide, even when Split is OFF. Its byte-3 SPLIT flag, decoded | ||||||
| // below, is therefore the authority; treating the 47-byte reply length as | ||||||
| // Split made every live channel display-only and prevented spot recall. | ||||||
| // The older dialects retain their established variable-length contract. | ||||||
| memory.split = dialect != MemoryDialect::Ic7300Mk2 | ||||||
| && payload.size() == static_cast<std::size_t>(layout.splitBytes); | ||||||
| const std::optional<std::uint64_t> frequency = decodeFreqExact( | ||||||
| payload.subspan(static_cast<std::size_t>(layout.frequencyOffset), kFreqBytes), | ||||||
| kFreqBytes); | ||||||
|
|
@@ -217,7 +223,12 @@ std::optional<IcomMemoryChannel> decodeMemory( | |||||
| } | ||||||
| memory.split = memory.split || ((selectByte >> 4) != 0); | ||||||
| } | ||||||
| memory.recallable = mode.mode.has_value() && !memory.split && duplex != 3; | ||||||
| // AetherSDR recalls the RX side of a stored channel onto the active slice. | ||||||
| // Split/RPS metadata is useful provenance, but it must not make a memory | ||||||
| // 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(); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 Until the second block is decoded (or its semantics evidenced à la
Suggested change
|
||||||
|
|
||||||
| if (layout.dtcsOffset >= 0) { | ||||||
| const std::optional<RepeaterToneRegister> dtcs = decodeRepeaterToneRegister( | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 inmodes()— so rows poisoned by the first build in those modes stay display-only while their neighbours heal. (b) It sits belowif (!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/FDVare already inmodes()), every deliberately display-only row flips recallable forever, and no codec decision can stick. It also makesload()a writer — which can trip the foreign-write guard in the two-window case — and logs "repaired N" even when the flush refused.The
kFormatVersion1→2 bump this PR makes is the one-shot hook the comment's "once" wants: gate onstoredRowVersion < 2, key the predicate on the codec's own rule, and do it in the store's schema layer.