Skip to content

feat(aetherd): 2.3 — complete SliceModel conversion (full canonical slice-status decode behind the seam)#4068

Merged
ten9876 merged 3 commits into
mainfrom
aetherd-step2.3-slice
Jul 6, 2026
Merged

feat(aetherd): 2.3 — complete SliceModel conversion (full canonical slice-status decode behind the seam)#4068
ten9876 merged 3 commits into
mainfrom
aetherd-step2.3-slice

Conversation

@ten9876

@ten9876 ten9876 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Completes the SliceModel touchpoint of aetherd RFC 2.3 — the largest of the five mixed-model splits. ~70 fields of Flex slice-status wire decode move out of SliceModel into FlexBackend, with a full vendor-neutral rename of the seam contract (per the 2026-07-05 scope decision). Behavior-neutral.

What moved

  • FlexBackend::decodeSliceStatus(sliceId, kvs) owns ALL the SmartSDR wire knowledge and emits the normalized, canonically-named, typed change map via IRadioBackend::sliceChanged:
    • key renames (RF_frequencyfrequency, filter_lo/filter_hifilterLow/filterHigh, locklocked, audio_levelaudioGain, txtxSlice, daxdaxChannel, index_letterletter, …)
    • "1"→bool; esc "1"/"on"→bool; comma-split + trim for antenna/mode lists (with rx_ant_list precedence over ant_list); fm_tone_mode/repeater_offset_dir lowercase.
  • SliceModel::applyStatusapplyChanges(const QVariantMap&) reads only canonical keys; every piece of model business logic is unchanged — the mode-dependent filter-polarity flip, the audio_mute / speex_nr_level / rtty_mark override re-pushes, the diversity change-detection block, fmToneValue formatting, and each field's exact emit gate (unconditional-when-present vs change-gated).
  • RadioModel::handleSliceStatus routes both applyStatus call sites through decodeSliceStatus; a ctor-wired sliceChanged handler applies to slice(id). FlexBackend is main-thread → synchronous DirectConnection, so a freshly-appended slice is populated before the sliceAdded UI notify (ordering preserved).
  • splitAntennaList relocated from SliceModel into the backend decode.

Behavior-neutral — verified three ways

  1. Programmatic key symmetry: the backend inserts 69 canonical keys, the model reads 69 — set-difference is empty in both directions, so there is no name mismatch (silent field death) and no key the model waits forever for.
  2. Adversarial field-by-field review across 8 dimensions (name consistency, no dropped field, type correctness incl. the floatdouble gain fields, "1"→bool + the esc/play/in_use special cases, present-only + combined-block "keep current when absent", all side-effects, per-field emit conditionality, RadioModel routing + synchronous ordering) — no regressions.
  3. Tests: slice_model_letter_test + antenna_alias_test converted to canonical applyChanges; new aetherd_slice_decode_test pins the wire→canonical mapping (renames, "1"/"on"→bool, list split, rx_ant_list precedence, lowercase, raw play/step_list, present-only).

Gates

  • 66/66 tests green, engine-boundary --strict green, full app builds clean.

Notes

  • The seam-contract keys are now fully canonical (vendor-neutral) — an HL2 or other backend emits the same map for the fields it supports; unknown/absent keys are simply not applied.
  • client_handle slice ownership + slice lifecycle remain RadioModel-level (the doc's SliceModel "extension" bucket), unchanged by this PR.

Refs #3849 (aetherd RFC tracking) — part of the step-2.3 touchpoint burndown.

2.3 progress

PanadapterModel ✅ (#4065) · MeterModel 🟡 (#4066) · SliceModel (this PR) · TransmitModel + RadioModel residual next.

🤖 Generated with Claude Code

@ten9876 ten9876 requested review from a team as code owners July 6, 2026 01:58
…lice-status decode behind the seam)

The largest of the five mixed-model splits: ~70 Flex slice-status fields move out
of SliceModel into FlexBackend, with a full vendor-neutral rename of the seam
contract. Behavior-neutral (verified field-by-field + programmatic key-symmetry +
adversarial review).

- FlexBackend::decodeSliceStatus(sliceId, kvs): owns ALL the SmartSDR wire
  knowledge — key names (RF_frequency, filter_lo/hi, lock, audio_level, …),
  "1"→bool, esc "1"/"on", comma-split + trim antenna/mode lists (rx_ant_list
  precedence), fm_tone_mode/repeater_offset_dir lowercase — and emits the
  normalized, canonically-named, typed change map via IRadioBackend::sliceChanged.
- SliceModel::applyStatus → applyChanges(const QVariantMap&): reads only canonical
  keys; keeps every piece of model business logic unchanged — filter-polarity
  normalization, the audio_mute / speex_nr_level / rtty_mark override re-pushes,
  diversity change-detection, fmToneValue formatting, per-field emit gates.
- RadioModel::handleSliceStatus routes both call sites through decodeSliceStatus;
  a ctor-wired sliceChanged handler applies to slice(id). FlexBackend is main-
  thread → synchronous DirectConnection, so a freshly-appended slice is populated
  before the sliceAdded UI notify (ordering preserved).
- splitAntennaList moved from SliceModel into the backend decode.
- Tests: slice_model_letter_test + antenna_alias_test converted to canonical
  applyChanges; new aetherd_slice_decode_test pins the wire→canonical mapping
  (renames, "1"/"on"→bool, list split, rx_ant_list precedence, lowercase, raw
  play/step_list, present-only).

Verification: backend inserts 69 canonical keys, model reads 69 — set-diff empty
both ways (no silent field death). 66/66 tests green; boundary --strict green;
full app builds clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full diff plus the PR-head sources (not just the truncated diff). This is a clean, genuinely behavior-neutral refactor — nice work, @ten9876. The mechanical wire→canonical split is executed carefully and the model's business logic is preserved intact.

What I verified holds

  • Key symmetry is real. Cross-checked every canonical key decodeSliceStatus inserts against every key applyChanges reads — the two sets match exactly, including inUse (emitted present-only, consumed only in the audio_mute-absent resync branch). No silent field death in either direction.
  • The subtle business logic survived the move verbatim: the rtty_mark/speex_nr_level "default-recall → re-push user override" workarounds, the play 3-state (disabled/1/0), the filter one-edge-present "keep current for the absent edge" fallback, rx_ant_list precedence over ant_list, and the fmToneValue 'f',1 formatting.
  • Signal wiring is sound. sliceChanged is declared once on IRadioBackend, emitted once, connected once. Since FlexBackend is constructed on and never moved off RadioModel's thread, the AutoConnection resolves to a DirectConnection — so the append path's decodeSliceStatus()sliceChangedapplyChanges() runs synchronously before emit sliceAdded(s), preserving the populate-before-notify ordering. The handler's if (SliceModel* s = slice(id)) guard also makes a spurious emit harmless.
  • The if (m_flexBackend) gate is not a regressionm_flexBackend is set unconditionally in the ctor and only nulled during teardown (fail-closed), matching the established #4065 idiom.
  • CI is green (7/7), and aetherd_slice_decode_test is a real QSignalSpy-based pin, including a "no wire key leaks through" assertion.

Minor

  1. Stale comment at RadioModel.cpp:5841: return; // applyStatus already called below; skip second call. applyStatus no longer exists, and the decode now happens on the line just above the return (not below). Worth updating to reference decodeSliceStatus for the next reader.

  2. Merge conflict with main (mergeable_state: dirty). The 2.3 series has advanced RadioModel.cpp/SliceModel.cpp under you (#4065), so this needs a rebase before it can land — a git merge-tree shows conflicts in the touched files. Not a code issue, just a heads-up.

Neither blocks the approach. Thanks for the thorough behavior-neutrality write-up and the field-by-field discipline — it made verification straightforward.


🤖 aethersdr-agent · cost: $6.3732 · model: claude-opus-4-8

@ten9876 ten9876 force-pushed the aetherd-step2.3-slice branch from 2d3b9a1 to d45eac2 Compare July 6, 2026 02:42

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the full decode↔model seam, not just the summary. This is a faithful, behavior-neutral split — nice work, and thanks for the meticulous field-by-field bookkeeping.

What I verified independently:

  • Key coverage & naming. Every wire key the old applyStatus consumed is emitted by FlexBackend::decodeSliceStatus, and every canonical name lines up with the corresponding applyChanges read (walked both halves of the diff, incl. the DSP toggle/level block, AGC, squelch/rit/xit, DAX, RTTY, FM, and step). No name drift in either direction.
  • Emit-gate fidelity. The subtle part, and it holds: unconditional emits stay unconditional (nb/nr/anf/qsk/lock/agc/antenna), change-gated stay gated (apf, all *Level, dax, rtty, digl/digu), and the combined blocks (squelch/squelchLevel, rit, xit) keep their either-present gate. The audio_mute-absent → inUse resync else-branch and the 3-state play (disabled/1/other, carried raw) are both preserved verbatim.
  • Business logic stays model-side as claimed: mode-dependent filter-polarity normalization, the speex_nr_level / rtty_mark default override re-pushes (still via sendCommand), the diversity change-detection block, and fmToneValue formatting.
  • Routing & ordering. if (m_flexBackend) matches the established seam pattern (panState/meterStatus already guard this way); it's non-null outside teardown, so the guard replacing the old unconditional applyStatus isn't a regression. The append path appends to m_slices before decodeSliceStatus, and since FlexBackend lives on the main thread the sliceChanged AutoConnection resolves to DirectConnection → the slice is populated synchronously before sliceAdded. Ordering claim confirmed.

All 7 CI checks are green (build/macos/windows/CodeQL/a11y/dependency-direction), and the new aetherd_slice_decode_test pins the wire→canonical mapping.

Non-blocking:

  • GitHub reports this as mergeable_state: dirty — it'll need a rebase on main now that #4065/#4066 have landed.
  • Same transitional note as #4063 applies: routing through m_flexBackend means a hypothetical non-Flex backend would silently drop slice status until slice decode is fully backend-owned. Fine while Flex is the only backend; just flagging it stays on the 2.x burndown.

No changes requested from me — LGTM pending the rebase.


🤖 aethersdr-agent · cost: $5.7600 · model: claude-opus-4-8

@ten9876 ten9876 left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review — high-effort recall pass (8 finder angles → verify)

Verdict: clean, faithful, well-tested — no functional regression. The largest 2.3 touchpoint: ~70 Flex slice-status keys move out of SliceModel::applyStatus into FlexBackend::decodeSliceStatus, which translates wire→canonical typed keys and emits sliceChanged; SliceModel::applyChanges now consumes only vendor-neutral keys. Two independent key audits confirmed every wire key round-trips 1:1 — no dropped/misnamed/orphan key. The == "1"→bool semantics stay backend-side (not .toBool() on raw strings), esc keeps 1/on, and the filter one-edge default, fm_tone_value formatting, play/step_list raw carry, plus all preserved business logic (rtty_mark re-push, speex_nr override, filter-polarity normalization, audio override re-pushes, audio_mute in_use resync) are intact. The rename layer genuinely buys vendor-neutrality — applyChanges contains no Flex wire string. Good coverage from the new decode test + converted model tests.

Five findings inline, all low/medium — none blocking. Noted-not-flagged: the rx_ant_list branch is a guessed spelling (FlexLib Slice.cs only has ant_list) but pre-existing, not PR-introduced; the wire→map→apply round-trip roughly doubles map probes + a QVariantMap alloc per status, mitigated by present-only (most updates are 1–3 keys); m_flexBackend teardown gate is intended fail-closed.

Comment thread src/core/backends/flex/FlexBackend.cpp Outdated
Comment on lines +421 to +428
const auto integer = [&](const char* wire, const char* canon) {
if (kvs.contains(QLatin1String(wire)))
c.insert(QLatin1String(canon), kvs.value(QLatin1String(wire)).toInt());
};
const auto real = [&](const char* wire, const char* canon) {
if (kvs.contains(QLatin1String(wire)))
c.insert(QLatin1String(canon), kvs.value(QLatin1String(wire)).toDouble());
};

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[fail-closed — the stakes are higher here] Boundary numeric parses are unguarded.

integer()/real() use bare .toInt()/.toDouble(), so a malformed present field → 0/0.0, silently applied. Behavior-preserving (old applyStatus was equally unguarded), but this is now the Flex slice boundary decoder, and unlike the pan/meter cases the fields are tuning-relevant: a garbled RF_frequency retunes the slice to 0 Hz, and filter edges/offsets follow. FlexLib fails closed on exactly these (Slice.cs uses TryParseDouble/int.TryParse + continue, keeping the prior value). Same call I flagged on #4065 (decodePanRange/RfGain) and #4066 (meter low/hi/num) — closing it in the two carriers fixes every numeric field at once:

Suggested change
const auto integer = [&](const char* wire, const char* canon) {
if (kvs.contains(QLatin1String(wire)))
c.insert(QLatin1String(canon), kvs.value(QLatin1String(wire)).toInt());
};
const auto real = [&](const char* wire, const char* canon) {
if (kvs.contains(QLatin1String(wire)))
c.insert(QLatin1String(canon), kvs.value(QLatin1String(wire)).toDouble());
};
const auto integer = [&](const char* wire, const char* canon) {
if (kvs.contains(QLatin1String(wire))) {
bool ok = false;
const int v = kvs.value(QLatin1String(wire)).toInt(&ok);
if (ok) c.insert(QLatin1String(canon), v);
}
};
const auto real = [&](const char* wire, const char* canon) {
if (kvs.contains(QLatin1String(wire))) {
bool ok = false;
const double v = kvs.value(QLatin1String(wire)).toDouble(&ok);
if (ok) c.insert(QLatin1String(canon), v);
}
};

Low real-world probability (radio emits well-formed), reachable over SmartLink/WAN.

Comment thread src/core/backends/flex/FlexBackend.cpp Outdated
// key names, "1"→bool, comma-split lists, lowercase normalization — so
// SliceModel::applyChanges speaks only vendor-neutral keys. Present-only:
// each canonical key rides only when its wire key was reported.
QVariantMap c;

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[altitude / simplification — the one strategic item] Untyped 70-field QVariantMap is the #4066 fragility at 10×.

The canonical schema now lives in two hand-maintained string tables — the str/integer/real/boolean calls here and the ~70 c.contains(QStringLiteral(...)) reads in applyChanges — with the compiler blind to drift between them. A typo ("filterlow" on emit vs "filterLow" on read) compiles, passes the emit-only decode test, and silently drops that field forever (stale UI, no error). All 70 currently match, but this is exactly the smell reviewers raised on #4066's MeterDef, now far more load-bearing.

A typed SliceDelta { std::optional<double> frequency; std::optional<int> filterLow; … } single-sources the schema and makes the seam compiler-checked — and keeps the vendor-neutrality win (the rename layer is good; it's the payload type that doesn't survive at this field count). The generic sliceChanged(int, QVariantMap) signal cardinality is fine — slice state is one cohesive object. Recommend for this PR or a fast-follow: the natural moment to fix the compounding cost of the series is before a 5th touchpoint copies the pattern.

if (kvs.contains(QLatin1String(wire)))
c.insert(QLatin1String(canon), kvs.value(QLatin1String(wire)) == QLatin1String("1"));
};
const auto splitList = [](const QString& raw) {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[cleanup — low] Duplicated split/carry helpers.

splitList is a verbatim copy of the splitAntennaList this PR deletes from SliceModel.cpp, and the str/integer/real/boolean present-only carriers re-implement the carry pattern already in decodePanState/decodePanExtensions. A shared typed carry-helper set would centralize the present-only contract and the ok-guard from the numeric-parse comment above in one place. (Minor: the inline mode_list split doesn't trim while splitList does — pre-existing asymmetry, old code didn't trim mode_list either, so not a regression.)

Comment thread src/models/RadioModel.cpp
// it to the addressed slice. Driven synchronously from handleSliceStatus's
// decodeSliceStatus() calls (main-thread DirectConnection), so a slice just
// appended to m_slices is populated before the sliceAdded UI notify.
connect(m_backend.get(), &IRadioBackend::sliceChanged, this,

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[latent — low] connect() is AutoConnection, but the comment two lines up says "DirectConnection".

Synchronous today only because FlexBackend shares RadioModel's thread. But FlexBackend already moves its sub-objects (m_connection/m_panStream) to worker threads — if it's ever itself moved to a worker thread, this AutoConnection silently degrades to QueuedConnection: emit sliceChanged returns before applyChanges runs, so the "slice populated before sliceAdded UI notify" guarantee (just below) breaks and the UI briefly sees a slice with default frequency/mode. No compile error, no test failure (the decode test never exercises the connect). Either pin Qt::DirectConnection explicitly to make the invariant load-bearing, or soften the comment to match the actual AutoConnection.

Comment thread src/models/RadioModel.cpp Outdated
@@ -5853,7 +5868,9 @@ void RadioModel::handleSliceStatus(int id,
return; // applyStatus already called below; skip second call

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[doc nit — trivial] Stale comment. applyStatus already called below; skip second callSliceModel::applyStatus no longer exists after this PR; the call this return guards against is now m_flexBackend->decodeSliceStatus(id, kvs) on the fall-through. Reword to avoid pointing a future reader at a removed method.

@nigelfenton

Copy link
Copy Markdown
Contributor

Took a red-team pass at the behavior-neutrality claim — decoded-vs-original applyStatus compared field-by-field, plus the type-coercion and emit-gating angles. It holds up. Notes on where I pushed hardest, in case they're useful:

  • Key symmetry / silent field death — traced every wire key through decodeSliceStatus → canonical key → applyChanges. No orphans in either direction; the 69↔69 claim checks out. play and step_list correctly carried raw for the model to interpret, rx_ant_list precedence over ant_list preserved.
  • Type coercion — the gain fields (rfgain/audio_level/esc_*) now round-trip string→double(QVariant)→float instead of string→float directly; harmless at these ranges/comparisons. QVariant::toBool() matches the old == "1" everywhere it's used.
  • Emit gates & the combined blocks — the "one edge present without the other" cases are the easy place to regress this; filter_lo/filter_hi, squelch/squelch_level, rit/xit, step/step_list, and the diversity shared change-detection all keep the old present-only + keep-current-when-absent semantics.
  • The audio_mute else-branchelse if (c.value("inUse").toBool() && m_audioMute) is equivalent to the old kvs.value("in_use") == "1" across all three cases (in_use absent → invalid QVariant → false, matching "" != "1"; present "0"; present "1"), so the reconnect resync still fires exactly when it used to.

One thing that's out of scope for this PR but worth a mental note for the FlexBackend lifecycle: both routed call sites are now if (m_flexBackend) …decodeSliceStatus(…). That's fine here — the pre-image already had the seam and never dereferenced it unconditionally — but it does mean if m_flexBackend is ever null on a path that carries real Flex slice status, that status is silently dropped rather than applied. Not a regression from this diff; just the one spot I'd keep an eye on as more backends land.

Nothing blocking from me. (It's showing conflicting against main — presumably just the sibling 2.3 touchpoints landing around it.)

@rfoust rfoust self-assigned this Jul 6, 2026
…ok-guarded numeric parses

Resolves ten9876's #4068 findings.

- TYPED PAYLOAD (#2, strategic): the slice-status payload becomes a compiler-
  checked SliceDelta struct (std::optional per field) instead of the stringly-
  keyed QVariantMap. A field-name typo between FlexBackend::decodeSliceStatus and
  SliceModel::applyChanges is now a compile error, not a silently-dropped field.
  IRadioBackend::sliceChanged(int, const SliceDelta&); SliceDelta lives in
  src/core/backends/ so the interface stays free of model deps. The vendor-neutral
  rename win is unchanged — applyChanges still reads only canonical fields.
  Verified: 69 delta fields set in the decode, 69 read in the model, set-diff
  empty both ways (now also enforced by the compiler).
- PARSE GUARDS (#1): the decode's numeric parses are ok-guarded — a malformed
  present field is dropped, not applied as 0/0.0. Higher-stakes here than
  pan/meter: a garbled RF_frequency would otherwise retune the slice to 0 Hz.
  Matches FlexLib's TryParse+continue and the standard set on #4065/#4066.
- DOC FIXES: the sliceChanged connect comment now accurately describes the
  AutoConnection + same-thread assumption (not a hard "DirectConnection", which
  would be wrong if a backend ever moves to a worker thread) (#4); the stale
  "applyStatus already called below" comment reworded to reference
  decodeSliceStatus (#5).
- Tests: aetherd_slice_decode_test now asserts on the typed delta (incl. a
  malformed-RF_frequency-dropped case); slice_model_letter_test / antenna_alias_test
  build SliceDelta directly.

67/67 tests green; boundary --strict green; full app builds clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@rfoust

rfoust commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the detailed split. I walked the merge ref against current main, not just the PR head. The field mapping looks symmetric and the focused local checks pass, but I would not merge this until the boundary parsing issue is closed.

Blocking

src/core/backends/flex/FlexBackend.cpp:421 / :425: integer() and real() still use bare toInt() / toDouble(), so malformed present values become 0 / 0.0 and are applied as real radio state. Because this is now the Flex slice boundary decoder, the impact is higher than the old interior SliceModel::applyStatus parser: a malformed RF_frequency can become 0.0, filter edges and offsets can collapse to zero, and other tuning-relevant values follow the same path.

Please make the numeric carriers fail closed with toInt(&ok) / toDouble(&ok) and only insert canonical fields when parsing succeeds. I would also cover step_list; it is still parsed later in SliceModel.cpp with bare toInt(), so a malformed step token can still enter the tuning-step list as 0. Add invalid numeric/list coverage to aetherd_slice_decode_test so this boundary contract is pinned.

Non-blocking

The 69-field QVariantMap seam is mechanically correct today, but it leaves the schema split across emitter string literals and SliceModel::applyChanges() reader string literals. That is manageable for this PR if the parse guard lands, but I would strongly prefer a typed SliceDelta or shared schema/helper before the next large touchpoint repeats the pattern.

RadioModel.cpp:537 says this is a DirectConnection path, but the code uses AutoConnection. It is synchronous today because FlexBackend shares the model thread; either pin Qt::DirectConnection for the ordering invariant or soften the comment.

There are also stale applyStatus() comments in RadioModel.cpp:5868 and SliceModel.h:443.

Local checks I ran

  • cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=RelWithDebInfo
  • cmake --build build --target aetherd_slice_decode_test slice_model_letter_test antenna_alias_test --parallel
  • ctest --test-dir build -R "^(aetherd_slice_decode_test|slice_model_letter_test|antenna_alias_test)$" --output-on-failure
  • python3 tools/check_engine_boundary.py --strict
  • git diff --check upstream/main...HEAD

All passed locally. GitHub CI is now green, but the fail-closed parse issue should be fixed before merge.

@ten9876

ten9876 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Review response — pushed 052c465

Thanks @ten9876 (and @aethersdr-agent for the two clean passes). All findings resolved, including the strategic one:

#2 — typed payload (done). The slice payload is now a compiler-checked SliceDelta struct (std::optional per field) instead of the stringly-keyed QVariantMap. A field-name typo between decodeSliceStatus and applyChanges is now a compile error, not a silently-dropped field — exactly the fragility you flagged, eliminated structurally rather than by a one-time check. SliceDelta lives in src/core/backends/ so IRadioBackend stays free of model deps; the vendor-neutrality win is unchanged (applyChanges reads only canonical fields). Cross-check still holds and is now compiler-enforced: 69 fields set in the decode, 69 read in the model, empty set-diff both ways.

#1 — parse guards (done). decodeSliceStatus's numeric parses are ok-guarded via the oInt/oReal carriers — a malformed present field is dropped, not applied as 0/0.0. As you noted, higher-stakes here (a garbled RF_frequency → 0 Hz retune); matches FlexLib's TryParse+continue and the #4065/#4066 standard. New test case asserts a malformed RF_frequency is dropped.

#4 — AutoConnection comment (done). Reworded to describe the actual AutoConnection + same-thread assumption. Deliberately did not pin Qt::DirectConnection — that would be a thread-safety bug if a backend ever moves to a worker thread; the comment now says the ordering guarantee would need an explicit populate step in that case.

#5 — stale comment (done). "applyStatus already called below" → now references decodeSliceStatus and the fall-through.

#3 — duplicated carry helpers: deferred to a dedicated cleanup that unifies the carry/split helpers across all the decodePan*/decodeSliceStatus methods — didn't want to widen this PR's blast radius into the pan decoders.

67/67 tests green, boundary --strict green, full app builds clean.

@aethersdr-agent aethersdr-agent Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the head commit (052c465), which already folds in your two self-flagged items — the typed SliceDelta payload and the ok-guarded numeric parses. This is a clean, careful split. Thanks for the thoroughness, @ten9876.

Verification

Seam correctness — the typed struct closes the real risk. Adopting SliceDelta (instead of a 70-key QVariantMap) means a decode/apply name drift is now a compile error rather than a silently-dropped field. That was the one structural concern worth blocking on, and it's resolved. SliceDelta.h living in core/backends/ with only Qt deps keeps IRadioBackend free of model dependencies — and the Engine/UI dependency-direction CI check passed, confirming SliceModel.h including the backend header doesn't invert the layering.

Behavior-neutrality — spot-checked the tricky spots, all preserved:

  • Per-field emit gating (unconditional-when-present vs change-gated) matches old code field-by-field — e.g. rxant/agc_mode/fm_* still emit unconditionally on presence; apf/dax/rtty_mark still change-gated.
  • The audio_mute-absent → in_use==1 resync branch: d.inUse.value_or(false) is exactly equivalent to the old kvs.value("in_use") == "1" (engaged-and-true iff present-and-"1").
  • Filter one-edge-present fallback, the speex_nr_level/rtty_mark override re-pushes, diversity change-detection block, and fm_tone_value → 1-decimal formatting all stay model-side, unchanged.
  • esc "1"/"on"→bool and rx_ant_list precedence over ant_list correctly moved wire-side.

The ok-guard is a deliberate (good) behavior change, not neutral — a malformed present RF_frequency now keeps the prior value instead of retuning to 0 Hz. Correctly matches FlexLib's TryParse+continue, and your comment documents it. Reachable over SmartLink, so worth having. 👍

Routing/ordering: m_flexBackend is the same object as m_backend (the transitional alias set at construction), so the emit sliceChanged reaches the DirectConnection handler synchronously — the freshly-appended slice is populated before the sliceAdded notify, as claimed. The if (m_flexBackend) guard matches the merged pan/meter precedent (#4065/#4066). Confirmed no stale applyStatus callers remain for SliceModel.

New aetherd_slice_decode_test is substantive (real QSignalSpy on the signal, qRegisterMetaType present) and pins the wire→typed mapping.

Minor (non-blocking)

  • Your own comment-3 still partly stands: splitList in the decode is a verbatim copy of the deleted splitAntennaList, and oStr/oInt/oReal/oBool re-implement the present-only carry already in decodePanState/decodePanExtensions. A shared typed carry-helper set (folding in the ok-guard) would centralize the present-only contract across all three decoders. Fine as a fast-follow — no need to expand this PR.
  • Pre-existing asymmetry you already noted: inline mode_list split doesn't trim while splitList does. Matches old behavior, so not a regression.

Nothing here blocks. Waiting on build/macos/windows/CodeQL (still in progress) to go green. Nice work on the largest of the five splits.


🤖 aethersdr-agent · cost: $3.4552 · model: claude-opus-4-8

…stale applyStatus comments

Closes rfoust's remaining #4068 items (their review raced the 052c465 push that
already landed the integer/real guards, typed SliceDelta, AutoConnection comment,
and the RadioModel stale comment).

- SliceModel::applyChanges: the per-token step_list parse now fails closed —
  a malformed step token is skipped, not admitted to the tuning-step list as a
  bogus 0-Hz step. (Was the one remaining bare toInt() on the slice boundary.)
- Reword the last stale applyStatus() comment references in SliceModel.{h,cpp}
  to applyChanges().
- Test: slice_model_letter_test pins the malformed-step-token drop.

67/67 tests green; boundary --strict green; full app builds clean.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@ten9876

ten9876 commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator Author

Re: @rfoust — all items closed (06a4c12)

Thanks for the thorough merge-ref review. Your pass landed ~1 min after the 052c465c push, so it reviewed the pre-fix version — the blocking parse-guard is already resolved there, along with the typed payload and the comment nits. Confirming each against the current head, plus closing your additional step_list item:

Blocking — parse guards: resolved in 052c465c. integer()/real() are now the oInt/oReal carriers with toInt(&ok)/toDouble(&ok), inserting the field only on success — a malformed RF_frequency is dropped (no 0 Hz retune). The decode test asserts this.

Your additional step_list catch (new, now fixed in 06a4c12): good find — that was the one remaining bare toInt() on the slice boundary, model-side in applyChanges. It now fails closed (skips a malformed step token instead of admitting a bogus 0-Hz step), and slice_model_letter_test pins it with a "10,abc,1000" → {10,1000} case.

Typed SliceDelta: done in 052c465c — the payload is now a compiler-checked struct, so the emit/read schema drift you (and ten9876) flagged is a compile error, not a silent drop. This landed before your "strongly prefer before the next large touchpoint" note, so we're good there.

AutoConnection comment: reworded (not pinning Qt::DirectConnection — that'd be a cross-thread bug if a backend ever moves off the main thread; the comment now documents the same-thread assumption).

Stale applyStatus() comments: RadioModel.cpp:5868 fixed in 052c465c; the SliceModel.h:445 one you spotted plus two more in SliceModel.cpp reworded in 06a4c12a. Grep for applyStatus in the slice model is now clean.

67/67 tests green, boundary --strict green, full app builds clean.

@ten9876 ten9876 merged commit a9d3546 into main Jul 6, 2026
7 checks passed
@ten9876 ten9876 deleted the aetherd-step2.3-slice branch July 6, 2026 04:15
ten9876 added a commit that referenced this pull request Jul 6, 2026
…tDelta, last mixed model) (#4071)

Completes the **TransmitModel** touchpoint — the **last of the five
mixed-model splits** in aetherd RFC 2.3. All five Flex transmit-family
status decoders move out of `TransmitModel` into `FlexBackend`, behind a
typed, compiler-checked `TransmitDelta` payload (the pattern established
for slices in #4068). Behavior-neutral.

## What moved

- **`FlexBackend::decodeTransmitStatus` / `decodeInterlockStatus` /
`decodeAtuStatus` / `decodeApdStatus` / `decodeApdSamplerStatus`** own
all the SmartSDR wire knowledge: key names, `"1"`→bool, **ok-guarded +
clamped** numeric parses (per-field ranges), uppercase, list split, the
**compander/dexp alias precedence** (`compander` wins;
`dexp`/`noise_gate_level` alias only when the compander key is absent),
and the sampler `INTERNAL`-prepend. Each emits
`IRadioBackend::transmitChanged(const TransmitDelta&)`.
- **`TransmitModel`**: the five `apply*Status` methods → one merged
**`applyChanges(const TransmitDelta&)`**. Every emit group is preserved
exactly — `stateChanged` / `tuneChanged` / `micStateChanged` /
`phoneStateChanged` / `txFilterCutoffChanged` / inline
`maxPowerLevelChanged`+`txSliceModeChanged` / `atuStateChanged` /
`apdStateChanged` / `apdEqualizerResetReceived` / `apdSamplerChanged` —
along with the model-side business logic: `compander` → both
`companderOn`/`dexpOn` (+ mic/phone flags), the **ATU enum parse**, and
the **stateful per-antenna APD-sampler map** +
selected-not-in-available→`INTERNAL` fallback.
- **`RadioModel`**: the 6 transmit-family call sites route through the
decode methods; a ctor-wired `transmitChanged` handler drives the model
(synchronous, main-thread).

## Behavior-neutral — verified

- **Adversarial field-by-field review** across the 5-method merge's
central risks — emit-group mapping (every field lands in its original
group; `met_in_rx`→`changed` not phone; compander sets both mic+phone;
interlock emits nothing), cross-contamination (single-plane deltas
isolate each group's emits), clamp ranges (all match), ATU/sampler logic
+ idempotency, routing, and the `optional<bool>` presence-vs-value
footgun — **no regressions**.
- **57 delta fields set in the decode = 57 read in the model**, empty
set-diff both ways (now compiler-enforced by the typed struct).

## Fail-closed (intentional, not a regression)
Malformed *present* numerics now drop instead of applying `0`/`0.0` —
`freq`, `max_power_level`, and the interlock ints were previously
unguarded (a garbled `max_power_level` would have emitted
`maxPowerLevelChanged(0)`). No well-formed input changes behavior.
Consistent with the slice/pan standard.

## Also in this PR
- **Folds in the deferred #4066 meter parse-guards** (`num`/`low`/`hi`
ok-guarded — a live fail-closed gap on `main`) + a malformed-value meter
test.
- **Registered `transmit_model_apd_test`** — it was built but never run
(pre-existing CMake omission).

## Tests / gates
- `transmit_model_test` + `transmit_model_apd_test` converted to typed
deltas; new **`aetherd_transmit_decode_test`** pins the wire→delta
mapping (clamps, ok-guard-drop, compander/dexp aliasing, ATU raw token,
APD bare-flag, sampler `INTERNAL`-prepend + empty-`tx_ant` no-emit).
- **68/68 tests green**, engine-boundary `--strict` green, full app
builds clean.

## Scope note — "RadioModel residual"
This finishes the five mixed models. RadioModel's own remaining *direct*
Flex decoders are the radio-**global** status handlers
(`handleRadioStatus`/`handleGpsStatus`/`handleMemoryStatus`/`handleProfileStatus`)
— not a sub-model, and a distinct smaller concern. Kept out of this PR
(already the largest) as a separate follow-up rather than bloating it.

Refs #3849 (aetherd RFC tracking). Related: #4065, #4066, #4068, #4070
(typed-payload parity for pan/meter).

## 2.3 scoreboard
Panadapter ✅ (#4065) · Meter ✅ (#4066) · Slice 🟡 (#4068) · **Transmit
(this PR)** — the five mixed models complete.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ten9876 added a commit that referenced this pull request Jul 6, 2026
…y decode helpers (#4075)

Closes #4070 — the seam-consistency follow-up from the RFC 2.3
mixed-model series. Behavior-neutral (the three decode tests pin the
exact wire→output mapping).

## Typed meter payload
`MeterDef` moves from `models/MeterModel.h` to
**`core/backends/MeterDef.h`** (pure data), so `IRadioBackend` carries
it directly: **`meterDefined(const MeterDef&)`** instead of the
stringly-keyed `QVariantMap`. `FlexBackend::decodeMeterStatus` builds
the `MeterDef`; `RadioModel` hands it straight to `defineMeter()` — the
~15-line key-string reconstruction is gone, and a field-name typo is now
a **compile error**. `MeterModel.h` re-includes the core header so its
existing users are unaffected.

## Shared decode carriers
New **`core/backends/flex/FlexKvCarry.h`** single-sources the
present-only + fail-closed parse contract
(`carryStr/Bool/Int/Clamp/Real` for optional delta fields;
`carryInto/IntInto/RealInto` for plain struct fields; `splitList`). The
slice (`oStr/oInt/…`), transmit (`tBool/tInt/…`), and meter (inline
guards) decoders now **all route through it** — resolving the "3rd/4th
re-roll of the same guard" that ten9876 flagged across
#4066/#4068/#4071. A future tweak (base-0 parse, whitespace trim) can no
longer be applied to one copy and silently skipped in the others.
`FlexBackend.cpp` net-shrinks ~90 lines.

## Deliberately NOT done (documented decisions)
- **`panState`/`panWnb` stay on the generic `extensionStatus(ns, kind,
QVariantMap)` channel** — that channel exists precisely to keep
Flex-specific status *off* the vendor-neutral typed interface; typing
them would invert that design.
- **`TransmitDelta`/`SliceDelta` kept flat** (not per-plane sub-structs
— ten9876's #4071 option): the flat structs are verified/tested, and the
`assign()` helper + these shared carriers already address the
maintainability concern; a sub-struct rewrite of two just-merged deltas
isn't worth the churn. Revisitable if a 6th touchpoint ever copies the
pattern.

## Gates
- `qRegisterMetaType<MeterDef>()` added at startup alongside
`Slice`/`TransmitDelta`.
- `aetherd_meter_decode_test` asserts on the typed `MeterDef` (incl. the
malformed-drop guard → default).
- **69/69 tests green**, engine-boundary `--strict` green, full app
builds clean.

Refs #3849. Concludes the RFC 2.3 mixed-model split seam cleanup — the
`IRadioBackend` seam now carries pan/meter/slice/transmit with
compiler-checked typed payloads and one shared decode-helper set.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants