Fix PGXL startup status synchronization

[w5jwp]

Summary

Fixes #5336.

Apply an amplifier delta's operate state before publishing first presence. This prevents the PGXL applet's presence observer from reading the model's default standby value while the same initial delta already contains the radio-authoritative operate state.

Keep SmartSDR's 0x00000000 startup sentinel inside FlexBackend by normalizing it to an empty neutral handle. AmpModel now establishes identity only from a model-bearing PGXL delta; a model-less update cannot be adopted as the amplifier handle and therefore cannot bind a mis-routed TGXL update.

Extend the existing socket-free state-machine and decoder tests to cover startup ordering, placeholder normalization, removal preservation, rejection of model-less handle adoption, and later recovery through a model-bearing PGXL status.

Constitution principle honored

Principle XI — Fixes Are Demonstrated. The focused regression test fails when the old presence-before-state ordering is restored, passes with the fix, and the corrected startup behavior was verified against a real PGXL. The added identity regression also proves an unidentified model cannot adopt a model-less TGXL handle.

Test plan

• Rebased onto current origin/main
• Focused targets build (amp_model_test, aetherd_amp_decode_test, aetherd_amp_tuner_encode_test)
• Focused tests pass (ctest --test-dir build -R '^(amp_model_test|aetherd_amp_decode_test|aetherd_amp_tuner_encode_test)$' --output-on-failure --no-tests=error)
• Registration check passes (python3 tools/check_test_registration.py --strict)
• Engine-boundary check reports no blockers (python3 tools/check_engine_boundary.py --strict)
• Diff whitespace check passes (git diff --check)
• Original startup-order assertions mutation-checked against the old presence-before-state ordering; the mutation fails
• Behavior verified on a FLEX-8600 with PGXL firmware 3.8.9 on macOS
• Original reporter confirmed the startup issue no longer reproduces

Checklist

• Commit is signed (docs/COMMIT-SIGNING.md)
• No new AppSettings calls
• Code is clean-room and based on existing AetherSDR model behavior
• No meter UI changes
• Reproduction and user-visible behavior are documented in the linked issue and this PR; CHANGELOG.md is unchanged
• No security-sensitive changes

[aethersdr-agent]

Issue fit

Partially. The diff does exactly what issue #5336's "Suggested correction" asked for — apply operate before publishing first presence, plus a socket-free regression test that fails under the old ordering. That part is correct and I could not break it.

But I don't think it closes the reporter's symptom. Tracing the two stateChanged consumers in the head checkout: MainWindow_Wiring.cpp:6591-6598 re-runs updatePgxlStyle() and ampApplet()->setState(...), and :6546-6547 re-runs updatePowerScale. Both are direct, same-thread connections. Under the pre-fix ordering, emit presenceChanged(true) and the change-gated emit stateChanged() happen in the same applyChanges() call stack with no return to the event loop between them — so the wrong STANDBY paint is overwritten before Qt can repaint. The plain first-delta case was a sub-frame transient, not the lasting STANDBY the reporter described ("until later status or interaction updates it").

The triage comment on #5336 reached the same conclusion independently and named the likely persistent cause — the 0x00000000 placeholder-handle latch — and explicitly asked the maintainer to decide on it. That half is not in this diff. Details in the inline comment.

This is not an argument against merging the ordering fix: it is correct, it matches FlexLib's own order (status applied before the amp is announced), and it removes a real latent trap. It is an argument against closing #5336 on it.

Scope

File	What it changes	Claimed by title/body?	Verdict
src/models/AmpModel.cpp	Moves the operate apply/emit ahead of the presence latch; splits the d.detectedModel block in two	Yes	In scope
tests/pgxl_status_state_test.cpp	New 48-line test: presence observer sees decoded operate	Yes (Principle XI)	In scope; see nit 2
tests/tests.cmake	Registers the new target	Yes	In scope

Everything in the diff is explained by the issue. No CHANGELOG.md entry (correct). No new public/protocol surface — AmpDelta, the decode, and every signal signature are untouched. No deleted guard: the removed lines are the same operate block, relocated verbatim including its behavior, and I checked the deleted comment ("Operate is change-gated; a status without a state leaves it as-is") — that behavior is preserved by the identical if (d.operate && …) condition, though the comment itself did not travel with it. No preference change: this is a defect fix with a repro and a stated authority.

Blockers

None.

Nits (all non-blocking)

• stateChanged now fires while m_present is still false. On a first-detect delta the new block runs before the presence latch, so updatePowerScale (MainWindow_Wiring.cpp:6539-6547) evaluates present() && operate() as false on that emission and computes the barefoot scale, then immediately recomputes correctly when presenceChanged follows. Harmless today — I checked both stateChanged consumers and neither has a side effect that isn't redone at presence. But #5336's suggested fix offered "suppressed on the first-detect delta … or emitted after presence," and announcing a state change before the object is announced present is the one shape that leaves a trap for a future consumer that gates on present(). Applying m_operate in place and moving the emit below the presence block gets the same fix without it.
• A new CMake target where an existing one already owns this state machine. tests/amp_model_test.cpp (registered at tests/tests.cmake:3285) already covers presence latch, operate change-gating, handle matching, removal and reset, with detected()/update() helpers that construct exactly the delta this new test hand-rolls. #5336's triage recommended adding the case there. The new file re-implements the CHECK macro and links Qt6::Test without using anything from it. tools/check_test_registration.py --strict passes either way — this is tidiness, not a violation.
• Neither test gates the merge. grep for amp_model|pgxl across every ctest -R filter in .github/workflows/ci.yml returns nothing, so the four green checks on bec8eb5a did not run this test. Worth knowing when reading the badge; not something to fix in this PR.
• The new test pins only that the presence observer sees operate == true. It would still pass if a later change stopped emitting stateChanged on that delta entirely. One QSignalSpy on stateChanged would close that.

What I tried to break

• The mutation claim. Verified against main's AmpModel.cpp:19-41: pre-fix, emit presenceChanged(true) precedes the operate apply, so operateAtPresence would capture false and check() would fail. The test is a genuine regression guard for the ordering, not an implementation-echo.
• Whether the reordering breaks the change-gate or handle matching. It does not. m_handle = d.handle still runs first, so the relocated !m_handle.isEmpty() && d.handle == m_handle guard evaluates identically in both positions; the split if (d.detectedModel) blocks are semantically the same as the original nested one. update()-style deltas (no detectedModel) take the same path as before.
• Second/replacement amp, and the removal path. applyChanges with d.removed returns early and is untouched. On re-detect after removal, m_operate is still deliberately left stale by the removal path — a delta carrying no state key will show the pre-removal operate value at first presence with nothing to correct it. Pre-existing, not introduced here, but it is the same failure class as the reported bug and the reordering does not help it.
• Both stateChanged consumers and both presenceChanged consumers (MainWindow_Wiring.cpp:6068, :6546, :6591, :6600) read for a side effect that the presence emission doesn't redo. None found — the PGXL auto-connect at :6068 is on presence only and is unaffected by operate.
• Empty and placeholder handles. Reproduced by reading, not running: an empty d.handle on a detectedModel delta leaves the operate block unreachable in both orderings, same as the placeholder case below.

All of the above is reasoned from the head checkout at /tmp/aetherclaude/pr-5337; I am headless and did not build, run the test, or drive the GUI. Nothing here is an observed runtime result.

Recommendation

Needs maintainer decision — on scope, not on code. The ordering change is correct, minimal, matches upstream FlexLib's order, has a test that genuinely fails without it, and survived everything I tried. Merge it on its own merits. The decision is whether #5336 stays open for the placeholder-handle guard, since the triage flagged that as the piece most likely responsible for the persistent STANDBY and this PR does not include it. Concrete next step: either add the 0x00000000 guard here (three lines, and the existing amp_model_test fixture already has the helpers for a test), or merge as-is and drop "Fixes #5336" from the body so the issue survives.

Nice, tight diff — thank you for the comment explaining why the order matters rather than just what moved.

---

_{🤖 aethersdr-agent · cost: $7.2811 · model: claude-opus-5}

[aethersdr-agent]

The ordering half is right, but the m_handle latch three lines above (AmpModel.cpp:21) is what likely produces the persistent stale STANDBY in #5336, and it isn't guarded here.

if (d.detectedModel) { m_handle = d.handle; } latches unconditionally, including the 0x00000000 placeholder a first amplifier status can carry before the real handle is assigned. Both siblings guard against exactly that value:

• RadioModel.cpp:9985 — if (handle != "0x00000000" && handle != m_tunerModel.handle()) on the TGXL route
• FlexBackend.cpp:950 — if (!handle.isEmpty() && handle != QLatin1String("0x00000000") && handle != m_tunerHandle) before caching m_ampHandle

AmpModel is the third copy and has no guard. If the placeholder lands in m_handle, every subsequent status arrives with the real handle and no model= key, so d.detectedModel is unset, m_handle never re-latches, and d.handle == m_handle fails — this block and the telemetryUpdated emit below are both skipped indefinitely. That reproduces the report precisely: stuck on STANDBY, corrected "later" when another model=-bearing status re-latches the handle, or when the operator presses OPERATE (which goes out on FlexBackend's correctly cached handle and draws a reply carrying model=).

A d.handle-is-empty first delta fails the same way, for the same reason.

I could not reproduce this at runtime — headless — so I can't tell you which of the two is what the reporter hit. But the guard is cheap and matches two existing call sites:

    if (d.detectedModel && !d.handle.isEmpty()
            && d.handle != QLatin1String("0x00000000")) {
        m_handle = d.handle;
    }

Worth a maintainer call on whether it belongs in this PR or a follow-up — but if it's a follow-up, #5336 shouldn't close on this one.

[aethersdr-agent]

This duplicates a harness that already exists. tests/amp_model_test.cpp (registered at tests/tests.cmake:3285, same aethercore + Qt6::Core + Qt6::Test link line) already owns this state machine — presence latch, operate change-gating, handle matching, removal, reset — and its detected(handle, model, ip, operate, telem) helper builds precisely the delta constructed by hand here. #5336's triage recommended putting the case there, and noted that the existing latch test at amp_model_test.cpp:54-60 uses operate=false, which is exactly why the ordering slipped through.

Adding it there instead of standing up a second target would reuse the CHECK macro, drop the extra AUTOMOC target, and put the new assertion next to the case it complements. Qt6::Test is also linked here but nothing from it is used (QSignalSpy is included but never instantiated).

Non-blocking either way — tools/check_test_registration.py --strict passes on the head checkout.

[w5jwp]

Addressed the bot review in signed commit 5dbe5acf:

• AmpModel no longer latches 0x00000000 as its command/status identity. It still applies the identified amplifier's initial operate state and telemetry, then adopts the first real handle from a later model-less status.
• stateChanged is emitted after first presence, so consumers that gate on present() no longer observe a state edge for an absent model.
• The regression was moved into the existing amp_model_test; the extra pgxl_status_state_test target and registration were removed.
• The test now asserts the operate value inside presenceChanged(true), verifies the state signal is deferred until after presence, and covers placeholder-to-real-handle recovery.
• Mutation checks confirmed both the original ordering and removal of real-handle adoption make the focused test fail.

Because the persistent placeholder-handle path is now covered, the PR continues to use Fixes #5336.

Focused tests, registration validation, and the strict engine-boundary check pass locally.

[aethersdr-agent]

CI failure on 5dbe5ac — not your code

Short version: nothing in this PR caused it. The compile succeeded; a single unrelated test step failed. Please re-run and it will most likely go green.

What actually failed

Of the four checks on 5dbe5ac, three passed (Static checks, check-macos, check-windows). The Linux build job failed — but not at the Build step, which is marked success. The only failing step is:

Test PSK Reporter map live updates

which runs (.github/workflows/ci.yml:268-271):

ctest --test-dir build -R "^map_live_update_test$" --no-tests=error --output-on-failure

Every step after it is skipped — that's just the job short-circuiting, not seven more failures.

Why it isn't related to this PR

Your diff touches exactly two files: src/models/AmpModel.cpp and tests/amp_model_test.cpp. The failing target compiles only the map sources (tests/tests.cmake:1022-1041):

add_executable(map_live_update_test
    tests/map_live_update_test.cpp
    src/gui/map/MapMarkerBatchItem.cpp
    src/gui/map/MapPathBatchItem.cpp
    src/gui/map/MapTerminatorItem.cpp
)

It links aethercore (so AmpModel is present in the binary), but the test never constructs an AmpModel, never touches applyChanges, and exercises only marker/path/terminator rendering. There is no path from an amplifier presence latch to a map tile cache.

The same build job is green on your merge base (7f3eb98c, run 33310983287), so the test isn't broken on main either.

Why it plausibly flaked

I want to be straight with you about the limits here: the tooling I have returns step-level results, not the log text, so I do not have the FAIL: … line and I can't tell you which specific assertion tripped. I'd rather say that than guess at an error message.

What I can say from reading the test is that it's wall-clock bounded around genuinely asynchronous work. waitUntil() polls a predicate with a 5000 ms default budget:

bool waitUntil(QApplication& app, const std::function<bool()>& predicate,
               int timeoutMs = 5000)

and five of its call sites wait on caches and images produced by QtConcurrent::run on a background pool (MapMarkerBatchItem.cpp:210, MapTerminatorItem.cpp:91) — including a full-world terminator image — all under QT_QPA_PLATFORM=offscreen inside a CI container. A noisy-neighbour runner blowing a 5-second budget on multi-megapixel software rendering is exactly the shape of failure that appears once and never again. That's a hypothesis, not a diagnosis.

What to do

1. Re-run the failed job (or push any new commit) to retrigger. Nothing to change in your diff.
2. If it reproduces on the re-run, that's worth knowing — the job runs with --output-on-failure, so the log will carry a FAIL: <message> line naming the assertion (e.g. replacement terminator image completes asynchronously). Drop that line in a comment here and I'll take another look; a reproducible failure would point at map_live_update_test itself, not at your change, and would deserve its own issue.

On the review comments

Both bot review notes look already addressed at this head, and neither has any bearing on the CI result:

• The m_handle placeholder latch is now guarded — d.detectedModel && !d.handle.isEmpty() && !placeholderHandle — matching the existing RadioModel.cpp and FlexBackend.cpp call sites, plus an adoption branch so the first real handle is picked up after a 0x00000000 first status.
• The new case lives in tests/amp_model_test.cpp alongside the existing latch coverage, rather than in a second target.

Thanks for putting the time into this one, and sorry the robots made you chase a ghost — the ordering fix plus the placeholder guard is a nice tidy change. Just give the job a re-run.

---

_{🤖 aethersdr-agent · cost: $3.3893 · model: claude-opus-5}

[w5jwp]

Attempted the recommended failed-job rerun for Actions run 33328642836, but GitHub rejected it because w5jwp does not have repository Actions administration permission.

I did not create an empty retrigger commit. The project workflow guidance explicitly rejects empty CI-retrigger commits, and the bot's analysis identifies the failure as the unrelated asynchronous map_live_update_test, not the PGXL changes. A maintainer will need to rerun the failed job.

[ten9876]

Issue fit

#5336's ordering defect — presence published before the same delta's operate state, so the applet's presence observer read default-standby — is precisely fixed, mutation-checked in both directions, live-verified on a FLEX-8600 + PGXL 3.8.9, and confirmed by the original reporter. The test is socket-free state-machine injection, exactly the right layer. The ordering half of this PR is model work.

The blocker is the second half: the new adopt-first-real-handle branch reopens a cross-device binding the old code made structurally impossible.

Scope

Two files, both explained by the issue; body checklist claims hold (signed commit verified via CI metadata, no settings calls, CHANGELOG untouched). Preflight: no sockets, no fake peers.

Blocker

1. The handle-adoption branch can bind the TGXL to the PGXL model (inline). The repo documents a routing edge (FlexBackend.cpp ~962, the #4203 comment): a model-less TGXL status arriving before the tuner handle is known falls through to decodeAmplifierStatus. The #4203 guard protects only the backend's own m_ampHandle — the emitted delta still carries the TGXL handle, no detectedModel, and the TGXL's state/kvs. The PR's new else if adopts any real handle while m_present && m_handle.isEmpty() — the exact state the PR's own test establishes (placeholder detect ⇒ present with empty handle). Sequence: PGXL detected with 0x00000000 → model-less TGXL status falls through → AmpModel adopts 0x2000 → TGXL operate/telemetry drive the PGXL applet, and a later amplifier 0x2000 removed tears the PGXL applet down. Old code adopted only under d.detectedModel, so this class could not occur; it self-heals only when the next model-bearing PGXL status re-binds. The smallest structurally-safe fix is to drop the else if entirely — the first branch already adopts on the next model-bearing status, and ignoring model-less updates until then is exactly the pre-PR behavior the issue never complained about. If earlier binding is genuinely needed, it has to key on something that distinguishes amp from tuner, which AmpModel by design does not know.

Nits (non-blocking)

• Latent, trigger unproven: with m_handle empty-by-design after a placeholder detect, no removal message can ever match (\S+ capture ⇒ removal handles are never empty), so presence can only clear on disconnect (reset() — which also skips presenceChanged(false)). Real mechanism, but nothing shows the radio ever emits a placeholder-handle removal, so this is a robustness gap, not a demonstrated bug — labeled accordingly.
• The 0x00000000 literal now lives in the vendor-neutral AmpModel — its own header says it "builds no SmartSDR strings" — as the tree's fourth copy, while decodeAmplifierStatus already knows and strips it three lines from where it builds the delta. Sanitizing at the decoder is behaviorally equivalent for the status path (verified across every consumer), with one caveat: don't blank it on the removed path, or add a !m_handle.isEmpty() guard to the removal compare.
• CI: the red build job is map_live_update_test (Timeout, 300 s) — the documented pre-existing flake (#5271), unrelated to this diff; a rerun should clear it.

What was verified vs read

Every finding above was verified against the PR head and the routing/decoder sources before adoption (the pass's three candidates came back CONFIRMED / PLAUSIBLE / CONFIRMED, and the "real PGXL updates dropped forever" overstatement in the first candidate was cut down to the actual self-healing window). The ordering fix itself was read, and its mutation claims are consistent with the test's structure; not re-run locally. No bridge session — model-layer change, and the author's live PGXL verification covers the runtime claim.

[ten9876]

Blocker — this branch can adopt the TGXL's handle. The documented routing edge (#4203 comment in FlexBackend::decodeAmplifierStatus) lets a model-less TGXL status reach the amp decode before the tuner handle is known; the guard there protects only the backend's cache, and the emitted delta carries the TGXL handle with no detectedModel. In the placeholder-detect state this PR's own test establishes (present && handle.isEmpty()), this predicate has no term that can tell amp from tuner — so the TGXL's operate/telemetry drive the PGXL applet, and its removal tears the applet down. The old if (d.detectedModel)-only adoption made this structurally impossible.

Smallest safe fix: delete this whole else if branch (all four lines through its closing brace) and let the first branch adopt on the next model-bearing status — that restores the pre-PR binding timing, which #5336 never complained about. (No suggestion block: the deletion spans more lines than one anchored suggestion can express correctly.)

[w5jwp]

Addressed the latest review in signed commit 40f2e1a8 and rebased the branch onto current origin/main.

• Deleted the unsafe model-less handle adoption path from AmpModel; only a model-bearing PGXL delta can now establish amplifier identity.
• Moved SmartSDR's 0x00000000 placeholder normalization into FlexBackend, while preserving the raw handle on removal deltas.
• Updated amp_model_test to prove an unidentified PGXL ignores a model-less TGXL-shaped update and later recovers only through a model-bearing PGXL status.
• Added decoder coverage for placeholder normalization and removal preservation.

Validation:

• amp_model_test, aetherd_amp_decode_test, and aetherd_amp_tuner_encode_test: pass
• check_engine_boundary.py --strict: pass (tracked legacy warnings only)
• check_test_registration.py --strict: pass
• git diff --check: pass

I also attempted the required assignee claim before posting, but GitHub rejected it because w5jwp lacks ReplaceActorsForAssignable permission.

The review's placeholder-removal observation remains deliberately unchanged: while identity is empty, treating an arbitrary removal as the PGXL removal would recreate the same TGXL ambiguity. There is no demonstrated wire sequence or safe identity signal for that behavior, so the fix continues to fail closed until a model-bearing PGXL status arrives.

[w5jwp]

We have pushed updates since the latest review: the latest blocker fixes are now on the branch and all checks are green (current head `40f2e1a8`). When convenient, @aethersdr-agent, could you please review the current head? Thank you.