Repository navigation
feat(kpa500): add serial/network applet and Advanced maintenance - #5991
Conversation
|
Thanks for this — a complete serial-amp applet with protocol, transport, UI, and live-hardware verification is a lot of work, and the Principle II discipline (no button latches its own state) is exactly right. Only one job is red, and it's Static checks, not a compile error: run 36360337692. 1.
|
953b83d to
2dc1f97
Compare
|
CI fix (amend): Added peripheral(kpa500) semantic tags to aetherd-touchpoint-tags.json for both new headers; added setAccessibleDescription/setStatusTip to pre-existing disabled controls in RadioSetupDialog.cpp and MainWindow.cpp that the a11y ratchet flagged when those files appeared in the diff. |
2dc1f97 to
6c7f825
Compare
|
Fixed both static-check failures in the follow-up amend (6c7f825): aetherd-touchpoint-tags.json: tag changed to peripheral(elecraft), manifest regenerated |
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
1. Issue fit
Partially. The KPA500 commit is a faithful third instance of the ACOM/SPE peripheral pattern the #4953 triage prescribed, and it honours the two hard rulings in that triage: no shared ElecraftStationController, and no band-push to the amp (display + explicit command only). Protocol, transport, applet, settings row, wiring, touchpoint tags and a socket-free codec test are all present and land on the named files. #4953 correctly stays open for the KAT500 half.
Two requirements from the triage are not met. It asked for docs/architecture/elecraft-kpa500-kat500-design.md; no design note is added, yet four places in this PR cite a docs/architecture/kpa500-amplifier-design.md that does not exist in the tree (nit 1). More seriously, the triage's safety-labelled ruling — "if the link drops mid-QSO, the applet must show stale/unknown, not the last good SWR" — is only half implemented (blocker 3).
And four of the five commits on this branch are not this feature at all.
2. Scope
| File / group | What it changes | Claimed by title/issue? | Verdict |
|---|---|---|---|
src/core/Kpa500{Protocol,Connection}.{h,cpp} |
New codec + serial transport | Yes (#4953) | In scope |
src/gui/Kpa500Applet.{h,cpp}, AppletPanel.{h,cpp} |
New applet + registration | Yes | In scope |
RadioSetupDialog KPA500 row, MainWindow_Wiring KPA500 block, MainWindow.h, CMakeLists.txt |
Settings + wiring | Yes | In scope |
tests/kpa500_protocol_test.cpp + tests.cmake |
Codec test, registered | Yes | In scope |
docs/architecture/aetherd-touchpoint-* |
peripheral(elecraft) tags |
Yes | In scope (cites a missing doc — nit 1) |
Fan minimum-speed control (^FC, applet combo) |
New user-facing control | In the #4953 thread (your own comment, 2026-09-16), not the triage change list | Name it in the PR body |
AmpAppletStyles.h Standby pill → theme token |
Re-colours the existing ACOM/SPE STANDBY pill blue→amber | No | Needs maintainer decision (§6) |
src/core/Wsjtx{Client,StatusFreq,WireHelpers}, SpectrumWidget, MainWindow_Spots, DxClusterDialog, MainWindow.cpp listener, tests/wsjtx_status_freq_test.cpp |
WSJT-X Rx/Tx frequency overlay (#4526) — ~800 lines, 4 commits, a second new test target | No | Blocker — unbundle |
MainWindow.cpp updateToolsMenuState setStatusTip ×4; RadioSetupDialog setAccessibleDescription ×5 |
a11y/status-tip improvements on unrelated controls | No | Out of scope (good changes, own PR) |
git log on the head checkout: 706a422a, e2a0e281, 795a5fee, 6f4abb8d are all #4526 WSJT-X overlay work; only 6c7f825f is the KPA500. Two different feature reviews are stacked in one PR.
3. Blockers
-
Compile break in the build configuration where Qt6 SerialPort is absent.
Kpa500Connection.cpp:147dereferencesm_serialoutside any guard, but the member is declared only under#ifdef HAVE_SERIALPORT(Kpa500Connection.h).CMakeLists.txt:328-336finds SerialPort withfind_package(... QUIET ...)unlessREQUIRE_SERIALPORT=ON, andHAVE_SERIALPORTis defined onlyif(Qt6SerialPort_FOUND)(:2351), whileKpa500Connection.cppsits unconditionally inCORE_SOURCES. The file's ownsendRaw()#else Q_UNUSED(bytes)arm shows the no-serialport build is meant to compile. CI is green here only because its runners have Qt6::SerialPort — that green does not cover this. Fix in the inline suggestion. (Reasoned from the source and the CMake conditional; I did not build it.) -
The WSJT-X Rx/Tx overlay (#4526) is bundled into a KPA500 PR. Four commits, a new public signal shape on
WsjtxClient::statusReceived, three new headers, a new registered test target, and rendering changes inSpectrumWidget. Nothing in the title, in #4953, or in the KPA500 commit explains any of it, and it carries its own RFC-review history that deserves its own thread. Please split it out — I've deliberately not reviewed it on its merits here beyond noting it exists. -
The amp going silent leaves both the applet readouts and the station TX meters stale — the exact case #4953's triage flagged under
safety.Kpa500Connectionalready models this correctly:respondingChanged()exists precisely because "a USB-serial link can outlive the amp being switched off". Neither consumer uses it for anything but a pill caption:Kpa500Applet::setResponding()(Kpa500Applet.cpp:312-316) repaints the status pill only. PWR/SWR/TMP/V/I keep their last-good text, so the panel readsNO RESPONSEnext to a confidentSWR 1.2:1— the literal thing the triage said must not happen.setConnected(false)clears everything;setResponding(false)should clear the same live readouts.MainWindow_Wiring.cpp:6363and:6387suppress the radio's owntxMetersChanged/directionalPowerMetersChangedwheneverm_kpa500Conn.isConnected() && lastStatus().operate, with norespondingterm, and:7179pins the gauge to 500 W on the same condition. Switch the KPA500 off at its front panel while the USB-serial adapter stays enumerated andisConnected()stays true,lastStatus().operatestaystrueforever (m_statusis only reset inonTransportUp()), and no replacement telemetry ever arrives — so every subsequent barefoot transmission shows a frozen or dead forward-power/SWR meter, on a 500 W scale, with no indication why. Gating these on responding needs a public accessor;Kpa500Connectioncurrently exposes only the signal.
4. Nits (non-blocking)
- Missing design note.
docs/architecture/kpa500-amplifier-design.mdis cited inaetherd-touchpoint-tags.json,aetherd-touchpoints.md,Kpa500Applet.h:18andAppletPanel.cpp:877, and does not exist. ACOM, SPE and the LP-100A each have one; the triage namedelecraft-kpa500-kat500-design.md. static int s_fastIndex(Kpa500Connection.cpp:204) is process-wide, shared by every instance, and survives disconnect/reconnect — whilem_slowIndexnext to it is a member and is reset inonTransportUp(). Make it a member for consistency.buildPowerOff()has no caller outside the test (^ON0;). #4953 lists "Power ON/OFF" as a requirement; either wire it to a control or say in the body that power-off is deferred (power-on genuinely can't be done — it's a boot-loader command, as your header notes).- Test gaps.
buildSetFanSpeed()is the only builder with logic (qBound(0, n, 6)) and has no test; neitherkMaxFrameBytescap is exercised; and theapplyMessage("FL", "B0", s)case at the end of the fault block runs with noreport()at all, so it asserts nothing. All cheap to add in the existing socket-free target. FrameParser::feed()(Kpa500Protocol.cpp:27-29) clears the buffer before appending whenm_buf.size() + bytes.size() > 256, which discards a buffered partial-frame prefix that the incoming chunk would have completed. Unlikely at 4800 baud with 200 ms polls, but clearing after append — or only clearing the un-framed tail — avoids it entirely.- Contradictory comments at
RadioSetupDialog.cpp:9563: "it is not user-configurable here because the spec documents no alternative rate negotiation" sits directly above a four-rate baud combo whose own comment says the KPA500 supports 4800/9600/19200/38400 via^BRP. isalpha()is used inKpa500Protocol.cppwithout including<cctype>(currently transitive) and is locale-sensitive; an explicitc >= 'A' && c <= 'Z'range test would be both.- I could not verify the
^WS/^VI/^FC/^FLscalings against the KPA500 Programmer's Reference Rev. A2 — I have no copy. They are internally consistent between header comment, code and test, and the commentary cites the section for each, which is the right shape under Principle I. Confirming them against the PDF is a maintainer/hardware check.
5. Socket tests
tests/kpa500_protocol_test.cpp is socket-free — it drives FrameParser/applyMessage over hand-built QByteArrays with no transport at all — and is registered in tests.cmake linking only Qt6::Core. Correct layer for wire parsing per AGENTS.md, and no fake KPA500 peer anywhere, which is right. tests/wsjtx_status_freq_test.cpp (from the bundled #4526 commits) is likewise socket-free; its only mention of QUdpSocket is a comment explaining what it avoids. Nothing here binds, listens or connects. Neither is in the per-PR ctest -R gate.
6. What I tried to break, and could not
- Principle VI / TX safety. Nothing in this diff keys a transmitter.
setOperate()fires only from the two applet buttons, there is no auto-OPERATE on connect or reconnect, and the poll loop is pure GET traffic. The Constitution II claim in the header holds: no control method latches —setOperate()sends^OS1;/^OS0;and the pill only moves when the^OSreply lands. - Lifetime and teardown. All
RadioSetupDialogconnections to the longer-livedm_kpa500Connpassthisas receiver context, so they die with the dialog; the reseed lambdas holdQPointers.onTransportDown()callsm_serial.disconnect(this)beforeclose(), so the error-then-close path can't re-enter.m_reconnectTimeris stopped on deliberate disconnect andarmReconnect()re-checks both flags. I found no dangling-pointer or double-fire path. - Reconnect and repeat-click.
connectSerial()no-ops while connected;onTransportDown()returns early when already down; clicking Connect twice, or Disconnect during a pending reconnect, both resolve cleanly. - Parser hostile input. Garbage before
^, a bare;echo, a frame split across feeds,^;, a >3-letter token, a body of only digits — all either resync or are dropped without a callback. The 64/256-byte caps do bound growth. The one hole is the pre-append clear noted above. toLatin1().constData()in the poll ticks — I checked for a dangling temporary; theQByteArraylives to the end of the full expression, so the pointer is valid for thebuildQuery()call.- Applet signal loops.
m_fanCombo's poll-drivensetCurrentIndex()is wrapped in aQSignalBlocker, and thecurrentIndexChangedconnection is made after the items are added, so neither construction nor a poll reply can echo a^FCback to the amp. - Accessibility and the colour ratchet.
tools/check_a11y.py src/gui/Kpa500Applet.cpp→ 0 findings.tools/audit_colours.py --strict --compare-src <base>→ all three counts down or flat (unique −1, refs −1, setStyleSheet ±0), so the new hardcoded hexes inupdateStatusPill()do not trip the gate. - CodeGuard's five
CG-PATH-001hits — all refuted.MainWindow.cpp:9551is aQLabelstring in the FFTW-wisdom dialog,:9663a progress-label update,:9782-9784the macOS bundle-relaunch walk up fromQCoreApplication::applicationFilePath(). None takes external input and none is touched by this PR. - Could not check: the PR body (I have no read path to it from here, so I could not test its claims or its checklist), any on-hardware behaviour, and the Elecraft PDF. Every finding above is reasoned from the head checkout at
6c7f825f, not reproduced at runtime.
7. Recommendation
Request changes, and the first request is structural: split the four #4526 WSJT-X commits into their own PR so each feature gets a real review. On the KPA500 half, the architecture is right and the pattern-matching to ACOM/SPE is genuinely well done — blocker 1 is a one-line guard, and blocker 3 is a small accessor plus wiring the respondingChanged signal you already built through to the readouts and the meter gates. With those two and the design note, this is close. Given #4953 is labelled safety and maintainer-review, and that the triage explicitly wanted a hardware cross-check before an amplifier control path lands, the merge call and the bench validation both sit with the maintainer.
Thanks for taking this on — the protocol layer in particular is carefully sourced and the poll-silence model is more thoughtful than either sibling integration's.
One more thing: commit signing
main requires verified signatures, and 4 of the 5 commits on this branch are unsigned (only 6c7f825f, the KPA500 commit, is signed). Quickest setup is SSH key signing, no GPG needed:
git config --global gpg.format ssh
git config --global user.signingkey ~/.ssh/id_ed25519.pub
git config --global commit.gpgsign true(If you have no SSH key: ssh-keygen -t ed25519 first.) Then on GitHub: Settings → SSH and GPG keys → New SSH key → set the key type dropdown to Signing Key → paste the .pub.
To re-sign what's already on the branch:
git rebase main --exec "git commit --amend --no-edit -n -S"
git push --force-with-leaseIf you unbundle the WSJT-X commits first, that rebase covers whatever is left here. Full reference, including GPG and troubleshooting: https://docs.github.com/authentication/managing-commit-signature-verification
🤖 aethersdr-agent · cost: $8.7820 · model: claude-opus-5
|
Cisco CodeGuard — static analysis of this PR (5 finding(s))
Automated static scan by Cisco DefenseClaw CodeGuard on the changed files. Advisory — some may be false positives; the review above verifies them. 🤖 aethersdr-agent · cost: $8.9670 · model: claude-opus-5 |
6c7f825 to
1c3931b
Compare
1c3931b to
fbdb061
Compare
…yle token
Revert AmpAppletStyles.h Standby pill to blue hex (ACOM/SPE unaffected).
Add ampStandbyBtnStyle() using {{color.accessory.key.standby.*}} amber
tokens, matching AmpApplet's kPanelKeyStandbyStyle. KPA500 standby button
now uses it instead of hardcoded #12222e/#2a5a70.
…LPORT Fixes compile break when Qt6 SerialPort is absent. m_serial is declared only under #ifdef HAVE_SERIALPORT but the guard clause read it unconditionally. No-serialport build now falls back to checking m_connected only, matching the intent of the sendRaw() #else arm.
…aethersdr#4953) Add isResponding() accessor to Kpa500Connection (beside isConnected()). setResponding(false) in Kpa500Applet now clears PWR/SWR/TMP/V/I labels and gauge immediately, matching setConnected(false). Band, fault banner and firmware tooltip are kept — they describe conditions, not live values. Prevents 'NO RESPONSE' pill next to a confident SWR 1.2:1 (aethersdr#4953 triage). MainWindow_Wiring: gate all three KPA500 meter-suppression paths on isResponding() as well as isConnected(). Without this, switching the amp off at the front panel while USB-serial stays enumerated leaves the radio's TX meters frozen on the 500W scale indefinitely. Also connect respondingChanged to updatePowerScale so the scale restores immediately.
… comment) - s_fastIndex: promote to member m_fastIndex beside m_slowIndex; reset in onTransportUp() so reconnect restarts the rotation and multiple instances do not share state. - Kpa500Protocol.cpp: add <cctype> include for isalpha(); append before overflow-guard clear in FrameParser::feed() so a chunk that would complete a buffered partial frame is not discarded first. - RadioSetupDialog.cpp: remove contradictory 'not user-configurable' sentence that sat directly above the four-rate baud combo.
Tests: add buildSetFanSpeed bounds (clamp to 0/6), kMaxFrameBytes overflow recovery (buffer cleared, next valid frame still decodes), and a proper report() for the ^FL 'B0' hex-notation case (toInt decimal fails → field unchanged). Design doc: add docs/architecture/elecraft-kpa500-kat500-design.md covering peripheral rationale, protocol summary, command scope, TX safety, KAT500 deferral, touchpoint tagging, and resolved decisions (stale-readout safety, standby pill/button, no band push, no shared controller). Citation rename: four source files and two doc files referenced the wrong filename (kpa500-amplifier-design.md); updated to the correct elecraft-kpa500-kat500-design.md.
The header comment incorrectly described ^ON1 as a bootloader-only command. Both ^ON0 and ^ON1 work over the normal serial port, confirmed by KPA500 Remote and third-party applications (PGTG). Add buildPowerOn() alongside buildPowerOff(); both tested. No UI wired for either in this PR — power-off needs a confirmation dialog, power-on needs a hardware round-trip test. Design doc updated to match.
|
Thanks for the thorough review. I've pushed a series of follow-up commits addressing everything flagged. Summary: Blockers Split: The four #4526 WSJT-X overlay commits have been removed from this branch via git rebase --onto. The branch now contains only the KPA500 work. The overlay commits live on wsjtx-freq-overlay and will come through as a separate PR once RFC #5586 gets maintainer sign-off. Compile break (no SerialPort): onTransportDown() now wraps the m_serial.isOpen() check under #ifdef HAVE_SERIALPORT exactly as suggested, with if (!m_connected) return; in the #else arm. Safety (stale readouts + frozen meters): Added bool isResponding() const to Kpa500Connection alongside isConnected(). setResponding(false) in Kpa500Applet now clears PWR/SWR/TMP/V/I labels and gauge immediately — band, fault banner, and firmware tooltip are kept. All three meter-suppression gates in MainWindow_Wiring now check isResponding() as well as isConnected(), and respondingChanged is connected to updatePowerScale so the 500 W scale restores as soon as the amp goes silent. Style alignment (AmpAppletStyles.h) Reverted the Standby pill to the original blue hex — ACOM and SPE pills are unchanged. Added ampStandbyBtnStyle() using {{color.accessory.key.standby.*}} amber tokens, matching AmpApplet's own kPanelKeyStandbyStyle. The KPA500 standby button now uses it; the pill stays in the family blue. Nits s_fastIndex → member m_fastIndex, reset in onTransportUp() Design doc Added docs/architecture/elecraft-kpa500-kat500-design.md covering peripheral rationale, protocol summary, command scope, TX safety, KAT500 deferral, touchpoint tagging, and resolved decisions. All four dangling citations to the wrong filename are fixed. Power on/off correction The header comment incorrectly described ^ON1 as a bootloader-only command. Both ^ON0 and ^ON1 work over the normal serial port (confirmed by KPA500 Remote and at least one other third-party application). Added buildPowerOn() alongside buildPowerOff(), both tested. Neither is wired to a UI control in this PR — power-off needs a confirmation dialog, power-on needs a hardware round-trip test. Both are documented as deferred in the design doc. Build + test Full build clean. ctest -R kpa500_protocol_test passes (all cases including the new ones). Verified on hardware: standby button renders amber in standby, readouts clear correctly when the amp is switched off at the front panel, TX meters restore immediately. The remaining hardware cross-check of ^WS/^VI/^FC/^FL scalings against Rev. A2 is a maintainer/owner step as noted — I don't have a copy of the PDF. |
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Reviewed head da95a70e1fec08ce10c67ebceff8b6b583df8f3d.
Issue fit: Partially addresses #4953: KPA500 telemetry, explicit standby/operate, faults and fan control are delivered; KAT500, power controls and band pushing are deferred as the design note says. The standalone-peripheral shape follows the issue discussion. The stale-readout remediation works at the instant silence is declared, but production state injection shows that stale measurements become current again without fresh measurement replies. The parser also fails two claimed recovery properties.
Scope
| File/group | Change | Claimed? | Verdict |
|---|---|---|---|
Kpa500Protocol.{h,cpp}, kpa500_protocol_test.cpp, test registration |
Codec, commands, polls and socket-free checks | Yes | In scope; defects below |
Kpa500Connection.{h,cpp} |
Serial lifecycle, polling and silence tracking | Yes | In scope; stale-state defect |
Kpa500Applet.{h,cpp} |
Dedicated controls and telemetry | Yes | In scope; owner must approve final feature UI |
AppletPanel.{h,cpp}, KPA parts of MainWindow.{h,cpp}, MainWindow_Wiring.cpp |
Register/connect applet, meter/scale integration | Yes | In scope |
KPA parts of RadioSetupDialog.{h,cpp} |
Port/baud settings and connection | Yes | In scope |
AmpAppletStyles.h, AcomApplet.cpp |
Shared clear-fault style and KPA standby-button style | Follow-up comment explains | Related deduplication; ACOM style preserved |
CMakeLists.txt, architecture tags/manifest/design note |
Build and peripheral documentation | Yes | In scope |
| Other MainWindow/RadioSetup a11y edits | SWR/ATU/AGC menu status tips and automation/network control descriptions | No | Unrelated; unbundle |
Earlier WSJT-X overlay changes are no longer in this diff. The body's claim that the shared Standby pill turns orange is stale: the follow-up reverted that; this head changes the KPA button and leaves the family pill blue. No CHANGELOG change or baseline weakening found.
Blockers
-
Do not resurrect stale amplifier telemetry on an unrelated reply —
src/core/Kpa500Connection.cpp:27–34,245–251; consumersrc/gui/MainWindow_Wiring.cpp:6834–6839. Silence changesm_respondingbut retains allm_statusvalues. Any parsed command sets responding true before validation; a subsequent band/temperature response emits the entire old snapshot. Socket-free production-object injection reproduced:status responding=1 operate=1 power=500.0 swr=1.5 band=-1 after silence responding=0 status responding=1 operate=1 power=500.0 swr=1.5 band=5The final input was only
^BN05;, with no fresh OS or WS. The MainWindow consumer then applies those old watts/SWR, and its responding+operate gates suppress the exciter again. This is the precise stale/unknown safety requirement in #4953, not just cosmetic repainting. Invalidate live fields on silence, accept only validated replies as liveness, and retain per-field unknown/fresh state until the relevant reply arrives. Add a socket-free silence→partial-resume regression. The observed evidence is the production connection state/signal; the MainWindow pixel consequence is traced from its consumer, not a claimed GUI reproduction. -
Parse valid coalesced frames before applying a partial-frame cap, and really resynchronize at a new caret —
src/core/Kpa500Protocol.cpp:30–49. The initial 256-byte whole-buffer clear discards arbitrarily many complete valid frames;indexOf('^')chooses the first caret, consuming a later valid frame as malformed payload. Production parser probes returned:input: 60 concatenated ^OS0; frames coalesced callbacks=0 expected=60 input: ^BAD^OS0; frame BAD:^OS0Serial read chunk size is not a frame boundary. A delayed GUI read can coalesce replies; corrupted/incomplete input must not swallow the next valid standby/fault state. Use bounded incremental framing that resets on a new
^, applies the 64-byte cap to an individual incomplete frame, and processes all complete frames. Extend the pure codec tests with both cases. The current suite passes despite these counterexamples. -
Unbundle unrelated a11y edits —
src/gui/MainWindow.cpp:10045–10111,src/gui/RadioSetupDialog.cpp:2095–2099,2192–2203,2273–2277,2419,2433. The new SWR/ATU/AGC menu status tips and network/automation control accessibility descriptions may be useful fixes, but neither KPA500 support nor #4953 explains them. CONTRIBUTING.md says “One issue per PR. Keep changes focused and reviewable.” Move these hunks to their own focused change; keep the KPA connection-constructor plumbing here. This is a scope finding, not an objection to accessibility improvements.
Nits / limits (non-blocking)
src/core/Kpa500Protocol.cpp:217–222and its test assertion:buildPowerOn()emits^ON1;while citing Rev A2. That reference documents^ON0;for power-off and bootloaderPfor power-on, not^ON1;. The builder is unused by UI, so this is not a reproduced operational failure; remove the unused builder or cite captured hardware/newer documented evidence for it. See Elecraft Rev A2, pages 3 and 5.src/core/Kpa500Protocol.cpp:115–140: validate the documented maximums as well as non-negativity (SWR raw ≤990; voltage/current raw ≤999). Extremely large positive integers currently become accepted telemetry. The official reference above gives the ranges; malformed-input coverage should include them.src/gui/RadioSetupDialog.cpp:9730–9769: without HAVE_SERIALPORT, the Connect button remains enabled with a handler that does nothing. Dim unavailable controls with a stated reason. This was identified by inspection of the no-SerialPort compile path, not driven in the GUI.docs/architecture/elecraft-kpa500-kat500-design.md:78–117,193–197,src/gui/Kpa500Applet.cpp:78: reconcile stale polling/silence/gauge/validation descriptions. Head uses WS/OS/BN fast polling, 15 silent ticks, and a 500 W applet gauge; body says 600 W and the design note describes older schedules/three ticks. The head's codec table can now be checked against the public PDF; claims that it was both verified and unavailable should not coexist.
Verification and attacks
- Rechecked added test and registration: pure codec, no sockets or peer; built the actual
kpa500_protocol_testusing GCC16.2.1/Qt6.11.2:ALL PASSED (0 failure(s)). Changed only SWR scaling /10→/100 in a scratch implementation: two checks failed, exit1. The existing decode test genuinely detects a scaling defect. - Built a focused harness with the actual Kpa500Connection/Protocol and QObject event infrastructure. Injected parser/state and called timer handlers directly, with no SerialPort support, opened port, listener, synthetic firmware or TX. It reproduced both parser failures and stale-state revival above. No base failure comparison is applicable to these entirely new classes.
- Independently compared BN, FC, FL, OS, WS and VI semantics against Elecraft's official reference. Valid sample scalings agree; the bounds/power-on caveats above remain. Did not reproduce the author's serial hardware or wattmeter observations.
- Read poll rotation, reconnect reset, disconnect timer cancellation, no automatic OPERATE, no band SET, command echo ownership, applet registration and settings ownership. Those paths retain explicit user intent; the shared ACOM style extraction preserves its literal style.
- Four engine gates pass at base and head. Capability, command-plane and manifest outputs are identical; engine output changes only line numbers and new file counts, retaining 102 legacy findings and zero blockers. New touchpoints are disclosed standalone peripherals, not vendor-header declassification. No baseline grows.
- No full-app/bridge session: SimBackend does not implement this serial peripheral, and no live accessory access was authorized. The connection/parser evidence is reproduced; UI integration remains code-reviewed. Automated code-review skill unavailable; older bot findings were rechecked, including the now-fixed no-SerialPort member reference and reverted shared pill restyle.
CI and recommendation
Request changes for stale-state revival, framing recovery and unrelated scope. Five displayed checks pass, but earliest head run 2026-10-02T21:48:38Z predates current main 2026-10-04T18:26:40Z; green applies to that frozen merge result and non-strict protection does not force refresh. Fix the reproduced state/parser paths with socket-free regressions, unbundle the unrelated hunks, update the body/design note, then obtain the maintainer's final peripheral/UI acceptance and bench validation.
— Jeremy KK7GWY + Codex
ten9876
left a comment
ten9876
left a comment
There was a problem hiding this comment.
Independent re-review at head da95a70e1fec08ce10c67ebceff8b6b583df8f3d. Same head as the earlier ten9876 + Codex review. I reproduced its main findings with my own harness rather than adopting them. On one of them I disagree about severity, and I've added findings it did not have.
Issue fit: partial, as scoped. For #4953 this delivers KPA500 telemetry, explicit STANDBY/OPERATE, faults and fan control over serial, as a standalone peripheral(elecraft) accessory, which is the shape the triage proposed. KAT500, band push and power control are deferred in the design note. No control is ever automated: nothing selects OPERATE on its own, and buttons are requests repainted from the ^OS reply (Principle II holds).
Scope
| File/group | Claimed? | Verdict |
|---|---|---|
Kpa500Protocol.{h,cpp}, kpa500_protocol_test.cpp, tests.cmake |
Yes | In scope; socket-free codec test |
Kpa500Connection.{h,cpp} |
Yes | In scope; blocker 2 |
Kpa500Applet.{h,cpp}, AppletPanel.{h,cpp}, KPA parts of MainWindow*.{h,cpp} and RadioSetupDialog.{h,cpp} |
Yes | In scope |
AmpAppletStyles.h, AcomApplet.cpp |
Follow-up commit | Related deduplication; ACOM keeps its literal style |
CMakeLists.txt, touchpoint tags and manifest, design note |
Yes | In scope |
MainWindow.cpp:10044-10111 (SWR-scan, pre-tune, clear-ATU and AGC-T setStatusTips) and RadioSetupDialog.cpp buildNetworkTab/buildIpConfigGroup (setAccessibleDescriptions) |
No | Unrelated: blocker 3 |
On the body:
- Its claim that the shared Standby pill turns orange is stale. That restyle was reverted, and this head adds a KPA-only
ampStandbyBtnStyle(). - "Fast poll (200 ms:
^WS,^OS,^BN)" is wrong; see the nits. - All 11 commits are signed, and there's no CHANGELOG entry.
Blockers
-
This no longer merges with
main.git merge-tree origin/main <head>reports conflicts in 9 files:aetherd-touchpoint-tags.json,aetherd-touchpoints.md,AppletPanel.{h,cpp},MainWindow.{h,cpp},RadioSetupDialog.{h,cpp}andtests/tests.cmake. Rebase, and regenerate the touchpoint manifest afterwards rather than hand-merging it. -
Silence doesn't clear the stale amp state, contrary to what commit
186d6b97("safety — clear stale readouts and meter gates on silence") claims. This confirms the earlier review.- On silence,
Kpa500Connection.cpp:204flipsm_respondingbut keeps all ofm_status. - The parser callback (
:27-34) then marks the amp responding again on any parsed frame and emits the whole old snapshot.
I reproduced it with a harness built from this head's
Kpa500Connection.cpp/Kpa500Protocol.cpp, injecting into the parser and drivingfastPollTick()directly (no serial port, no peer):statusUpdated responding=1 operate=1 power=500.0 swr=1.5 band=-1 after silence responding=0 operate=1 power=500.0 swr=1.5 band=-1 statusUpdated responding=1 operate=1 power=500.0 swr=1.5 band=5 <- input was only ^BN05;MainWindow_Wiring.cpp:6838then puts 500 W on the main PWR meter, and the gates at:6107/:6132suppress the radio's own TX meters. This lasts until fresh^OSand^WSreplies arrive, which takes up to ~600 ms (see the poll-rate nit). Typically the amp has just been power-cycled into STANDBY.Fix: on silence, clear
m_status's live fields (operate,forwardPowerW,swr,paVoltageV,paCurrentA,temperatureC,faultCode) and emitstatusUpdatedso the applet blanks. Add a socket-free silence → partial-resume test tokpa500_protocol_testor a sibling test. - On silence,
-
Unbundle the unrelated a11y edits. This confirms the earlier review.
git log -S'setStatusTip(swrTip)'shows these hunks arrived in the PR's first commitfbdb061d, not through the merge frommain, andmaindoesn't carry them. CONTRIBUTING.md: "One issue per PR. Keep changes focused and reviewable." The edits may well be good; they belong in their own PR.
Nits (non-blocking)
- Framing parser: confirmed, but I'd call it a nit, not a blocker. My harness gives the same two counterexamples as the earlier review:
^BAD^OS0;deliversBAD:^OS0and drops theOS, and 60 coalesced^OS0;frames (300 B) deliver 0 callbacks, while 40 (200 B) deliver all 40. Both recover on the next poll: the first loses one reply, and the second needs ≥ ~1.5 s of event-loop stall. A lost^FLdelays a fault display by one slow rotation, and the amp protects itself. Still worth fixing, and it's small. AtKpa500Protocol.cpp:44take the last caret before the;, and drop the pre-loop clear at:30-32(the post-loop cap at:71already bounds a partial frame):// A new '^' starts a new frame: anything before the last one is the // remains of a corrupt frame (includes the null-command echo). int caret = raw.lastIndexOf('^');
- Merge leftover in a comment.
MainWindow_Wiring.cpp:6864reads// cache here could skip a needed re-apply. upstream/main. That's a conflict markerda95a70emissed, and a whitespace-only line was added near:6893. - Poll rate.
fastPollTick()sends one command per 200 ms tick, rotating WS → OS → BN, so each is read every 600 ms. The comment atKpa500Protocol.cpp:244("queried every ~200 ms") and the body are wrong. The consequence: in OPERATE the radio's own TX meters are gated off (:6107,:6132), so the main PWR meter updates at ~1.7 Hz from^WS. Either say so, or putWSon every tick and rotate onlyOS/BN. ^WSSWR of000is decoded as 0.0. Elecraft's KPA500 Programmer's Reference Rev A2 says "sss will return as 000 when not transmitting".Kpa500Protocol.cpp:123-125stores that as SWR 0.0, and:6838passes it toapplyAmpTxMeters(value_or(1.0f)doesn't apply, because the optional is set). Leaveswrunset on000.- Range checks.
:123and:139check only>= 0. The same reference gives SWR 1.0–99.0 and V/A 00.0–99.9, so rejectsss > 990andvvv/iii > 999. buildPowerOn()(:217) emits^ON1;. Rev A2 documents only^ON0;(off) and says power-on is the boot-loaderPcommand. It's unused outside its own test, so remove it or cite hardware evidence for it.- Stale docs. The body's 600 W and the orange shared pill, and the design note's three-tick silence and older poll schedule, don't match this head (500 W, 15 ticks, the rotation above).
Verification and what I tried to break
- Socket preflight:
kpa500_protocol_testis codec-only (noQSerialPort, no socket, noKpa500Connection, no fake amp), so it's allowed under canon. It is registered intests.cmake. - Built the exact head (app and
kpa500_protocol_test):ALL PASSED. Mutation: decoding^WSwatts asppp / 10fails 2 checks ([FAIL] ^WS250 015; → 250 W, SWR 1.5). Restored (sha256 matchesHEAD) and it passes again, so the decode table really is pinned. - Harness: built against this head's real
Kpa500ConnectionandKpa500Protocolwith moc (Qt Core only,HAVE_SERIALPORToff). It reproduced blocker 2 and both parser cases. - Protocol against the vendor document: I checked
^BNbands,^FC0–6,^WS/^VIimplied decimals and the^BRPrate table against Elecraft's Rev A2 PDF. The decoders agree apart from the000and range nits above. - Read for TX safety: no automatic OPERATE on connect or reconnect, no band SET, the exciter-meter gates require connected && responding && operate, and reconnect resets
m_status(onTransportUp). - Not driven in the app: SimBackend has no serial peripheral path, and I did not touch any serial hardware. The author's on-air observations (260 W, band latency) remain their evidence.
- CI: green on 2026-10-02 against a base that the 9-file conflict shows no longer exists.
Recommendation: request changes. Rebase; clear live state on silence and add a test for it; split out the a11y hunks. After that this is a maintainer decision on accepting a new amplifier peripheral and its applet UI, plus bench validation on a real KPA500.
— Jeremy KK7GWY + Claude
|
Thanks Jeremy, requested changes will be made as soon as I return from Japan. |
|
Bot-review follow-up is pushed at The new issue routed from #6452 was valid: malformed KPA reply arguments could put control characters into decoded diagnostics. The parser now rejects non-printable bytes before delivery and recognizes command letters using ASCII ranges. Raw RX diagnostics retain percent-escaped bytes. The two new injected-transport/parser regressions failed before the fix and now pass, covering every byte outside printable ASCII and recovery on the next valid frame. I also checked and replied individually to all eight older inline bot threads. Their earlier fixes are present at the current head: optional-SerialPort compilation, stale applet/meter handling, shared standby colors, per-instance poll rotation, bounded frame handling, design-note references and baud documentation. Those addressed threads are now resolved; existing human reviews are unchanged. All six native Mac KPA500 suites and the documentation production build passed. The connection suite compiles without 73, Ozy K6OZY · prepared with Codex |
|
Rechecked the current head The description now records Jeremy's direction: TX Controls retains radio metering and native ATU/MEM; the accepted percent readouts remain. Amplifier output stays in the dedicated applet and VU/PWR. RFC #6477 has now been closed as withdrawn, with the direction relayed by Ozy and Ozy's confirmation of the reverted UI. This does not dismiss the earlier changes-requested review: Jeremy's fresh review is still needed. WA8PAM's attribution, screenshots and the forthcoming aether-serial-bridge note are retained. No KPA500 source change or new RF test in this pass. 73, Ozy K6OZY · prepared with Codex |
11517ad to
874cf21
Compare
jensenpat
left a comment
jensenpat
left a comment
There was a problem hiding this comment.
Reviewed exact head 874cf214e19aae344682b57996f8a77a76d1e2b1 against main 7864ef544ffbb465bb2136438d748f9da58aa30c.
This delivers the KPA500 portion of #4953: an independent serial/raw-TCP peripheral, confirmed-state applet, Advanced maintenance, shared VU/PWR metering and user documentation. KAT500 remains in #6452. The landing request accepts the proposed feature; this review checks its implementation and merge readiness.
| File/group | Scope and verdict |
|---|---|
Kpa500Protocol, Kpa500Connection, maintenance/fault codecs |
Claimed KPA500 protocol, transport, freshness and guarded maintenance; in scope |
| KPA500 applet/Advanced, AppletPanel, Peripherals and MainWindow wiring | Claimed device setup, controls and metering; in scope |
HGauge, shared clear-fault style |
Opt-in KPA scale strokes and equivalent ACOM style factoring; existing behavior retained |
| TX Controls percentage labels | Explicit direction recorded in #6477; radio-only metering/control policy retained |
| CMake/test registration, touchpoints, design/user docs/screenshots | Supporting feature integration and evidence; in scope |
Blockers remaining: none. The review found and fixed one additional stale-power path in src/core/Kpa500Connection.h:47 and the four MainWindow shared-meter sites: unrelated valid replies could keep expired amplifier watts selected. hasLiveTxMeters() now requires current powered-on/Operate/forward-power readback for both routing and scale. tests/kpa500_connection_test.cpp:239 exercises expiry, unrelated partial replies, fresh zero and Standby. Fix: 874cf214e.
The older review blockers are addressed: stale snapshots are invalidated, only validated replies establish liveness, framing is bounded/resynchronizing, unrelated WSJT-X/a11y changes are removed, optional-SerialPort compilation works, and the generated index was reconciled with current main. Both old changes-requested reviews were dismissed with this evidence; all eight pre-existing inline threads are resolved with replies.
Verification:
- Native macOS/Qt 6.12 full app build passed; 11/11 selected headless CTests passed (six KPA500 suites, MeterModel, TX Controls reconciliation, both HGauges and Peripherals automatic setup). The connection suite uses in-memory injection and compiles without HAVE_SERIALPORT. No new socket peer or fake firmware ran.
- Mutation: removing forward-power freshness produces two failed stale-selection assertions; restoring it passes. The tests also cover malformed/coalesced/truncated replies, per-field and total silence, reconnect/cancellation, transport write failures, maintenance preflight/TX refusal, bounded transfers, no write retry, identity mismatch and byte-for-byte verification.
- Clean-bundle bridge on isolated, offscreen, TX-disabled DEMO-0001 passed 10 assertions for disconnected/unknown device state, no demo TX-band target, Serial/Network setup, and disabled hardware maintenance. Radio TX/MOX/Tune stayed false. The owned process was disconnected and closed. This is app/demo evidence; amplifier-positive behavior remains injected/source-reviewed, not live hardware proof.
- Base/head engine findings remain identical after line normalization: 102 tracked warnings, zero blockers. Capability/command-plane counts and vendor classifications do not grow. Touchpoints, test registration, frozen CI gate, colour ratchet, accessibility, generated docs/references and user-doc production build pass. Log rules pass 707 checks; their existing moved-line advisory is non-blocking.
- Standard command scalings, ranges, SWR 000 and bootloader P agree with Elecraft Rev. A2. The added maintenance format remains explicitly scoped to observed firmware 01.54. Physical restore/banner-write persistence, higher-power accuracy and live RF on this final revision remain unverified; no hardware/TX was used.
Non-blocking limits: the history screenshot predates DRV and is disclosed as such; final high-power/hardware-write qualification remains separate. No new automated review skill was available; the pass was performed manually and existing bot findings were checked against the current diff.
CI and merge freshness: all required Linux build, Windows check-windows, macOS check-macos and Static checks passed at this exact head; docs and sanitizer configuration also passed. The earliest head run was created at 2026-10-17T15:16:10Z, after current main 7864ef544 (2026-10-16T05:46:18Z). Main is unchanged, all 25 PR commits are GitHub-verified, no unresolved threads remain, and git merge-tree is clean. Post-merge CI on the squash commit remains a separate required verification.
Approved for the requested guarded squash merge at this exact head.
Generated with OpenAI Codex (GPT-6 Astra)
4 participants
Summary
Tracking: Refs #4953 (KPA500 portion). #6452 delivers the KAT500 portion and carries the closing reference, so #4953 remains open until both device implementations land.
Adds a complete KPA500 peripheral with local serial or raw-TCP network access, a resizable applet, and Peripherals → Advanced maintenance. The applet displays confirmed amplifier state, keeps stale measurements out of the UI and main power meter, and distinguishes amplifier RF output from the radio's TX flag.
This PR delivers the KPA500 only. The KAT500 implementation is tracked separately in #6452.
Credits
Bob Holzer / @WA8PAM contributed the original KPA500 protocol/parser and command builders, serial connection and polling, first applet, Peripherals setup, main power-meter integration, initial tests, and subsequent fixes to the original review findings. His original commits remain in this branch.
@Ozy311 extended that foundation with the redesigned full/Mini applet, history and debug views, the radio-sourced DRV meter, confirmed power handling, Follow TX band selection, raw-TCP transport, Advanced maintenance, native-dialog crash fix, additional regression coverage, and Windows/Mac hardware validation. This is a continuation of WA8PAM's contribution, with explicit credit to both contributors.
TX Controls and meter separation
Following Jeremy’s direction recorded in #6477, the KPA500 affects the VU/PWR meter source and scale, not the radio-side TX Controls gauge. TX Controls continues to display exciter watts and SWR; RF Power and Tune Pwr values include %. Existing radio-integrated amplifier priority is retained. The dedicated KPA500 applet keeps amplifier readback and its Full-mode radio-sourced DRV row.
Applet and connection behavior
Remote access and aether-serial-bridge
Network mode supports remote KPA500 access through a raw TCP serial bridge. The companion aether-serial-bridge project is coming soon: it runs on the computer connected to the equipment's serial port and makes that port accessible remotely to AetherSDR. Compatible raw-TCP ser2net endpoints also work. Local serial access remains available without the companion project.
The bridge owns serial baud/framing settings. This PR does not implement Telnet/RFC2217 negotiation, Elecraft Remote Server framing, or a split GUI/AetherD peripheral connection. Peripherals labels local serial as DIRECT and TCP as NETWORK.
Advanced maintenance
Peripherals → Elecraft KPA500 → Advanced adds configuration backup/restore, stored fault history with CSV export, and power-on banner read/edit. Device information also appears in applet Settings.
Maintenance is gated to the observed firmware 01.54. Restore and banner writes require explicit confirmation, identity checks, fresh Standby/zero-RF readback, bounded transfers and matching readback before success. Polling and ordinary controls pause during maintenance; writes are not retried or resumed after reconnect. Cancellation, TX, malformed replies and transport failure abort the operation and report possible partial completion.
The native file chooser is asynchronous, parent-owned and limited to one open chooser. This fixes the reproduced Windows COM/Qt crash caused by duplicate blocking file dialogs.
Screenshots
Full below shows the current DRV layout using explicitly replayed telemetry, not a hardware power measurement. Mini, History and Advanced retain earlier native Mac captures from the real KPA500 with zero RF; station identifiers are redacted. The History capture predates the added DRV row and remains a screenshot-refresh follow-up when the bridge is available. The images are included in the amplifier user guide.
Full applet
Mini applet
Expanded history
Peripherals and Advanced
Current radio-side TX Controls, captured on the Mac while disconnected with no RF:
Constitution principles honored
Validation
Reviewed and rebased onto main
7864ef544; current head is874cf214e19aae344682b57996f8a77a76d1e2b1.The rebase preserves the original contributor commits. Its generated-index conflict was resolved by regeneration, and the rebased tree matched a clean merge result before the landing fix.
The landing review found and fixed an additional partial-telemetry failure: expired
^WSmeasurements could leave the shared meters suppressing radio readings while other amplifier replies remained live. Shared meter routing, suppression and scale now use the same current power-on/Operate/forward-power predicate. Fresh zero watts remains valid.HAVE_SERIALPORTand uses only in-memory transport injection.npm ciand the user-doc production build pass.874cf214enative bundle passed 10 assertions in an offscreen, isolatedDEMO-0001session: unknown amplifier readings, no TX-band target, Network setup, and disabled backup/restore/banner writes without hardware. TX/MOX/Tune stayed off; the owned instance was disconnected and closed. External-amplifier telemetry remains socket-free injection/source-review evidence, not demo or hardware proof.Earlier contributor hardware evidence covers firmware 01.54 identity/telemetry, configuration backup, banner reads and stored faults on Windows serial and Mac raw TCP. It is separate from this landing pass. No hardware was contacted or changed in this review.
Not validated on this final revision: physical restore/banner-write persistence, higher-power RF accuracy, or end-to-end transmitted-power behavior. No transmission, Tune, MOX/PTT, restore or banner writes were performed for this pass. The demo backend cannot validate external amplifier firmware.
Review scope
KPA500 protocol/transport, its applet and Advanced panel, Peripherals setup, meter integration, source registration, tests and user documentation are included. KAT500 remains separate in #6452. Unrelated WSJT-X/a11y edits and the shared ACOM/SPE Standby recolouring from earlier revisions are absent. Shared clear-fault styling is factored without changing ACOM behavior. No CHANGELOG entry, dependency, workflow or gate weakening is included.
The radio-only TX Controls direction and percentage labels follow #6477. Existing radio-integrated amplifier priority remains first. The operator requested the review, rebase, blocker repair and guarded merge of this proposed feature.
Generated with OpenAI Codex (GPT-6 Astra)