Skip to content

feat(mqtt): per-topic control, CW keyer, radio state, AX.25, debug logging#3460

Merged
jensenpat merged 6 commits into
aethersdr:mainfrom
K5PTB:feat/mqtt-expansion
Jun 9, 2026
Merged

feat(mqtt): per-topic control, CW keyer, radio state, AX.25, debug logging#3460
jensenpat merged 6 commits into
aethersdr:mainfrom
K5PTB:feat/mqtt-expansion

Conversation

@K5PTB

@K5PTB K5PTB commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

Motivation

Addresses the feature requests and concerns raised in #3388. The existing MQTT implementation published only antenna/VFO topics with no user control over which topics were active. This PR adds opt-in control for all new topics, extends the CW decode payload, and adds bidirectional MQTT support for the CW keyer, radio state, and AX.25 packet radio.

Changes

Per-topic opt-in control

A new InternalMqttTopicDef struct serves as the single source of truth for the topic registry, replacing the previous ad-hoc approach. Each topic carries a gateable flag:

  • Non-gateable topics (the existing antenna alias topics) remain always-on and cannot be disabled, preserving the original behavior. They appear checked and greyed-out in the settings dialog.
  • aethersdr/cw/decode is gateable but defaults to enabled (defaultEnabled=true), preserving the behavior of existing relay scripts and loggers without requiring any user action after upgrade.
  • All other new topicsaethersdr/cw/transmit, aethersdr/radio/state, aethersdr/ax25/rx, aethersdr/ax25/tx — are gateable and default to false (disabled). Users explicitly opt in via the MQTT settings dialog.

isMqttTopicEnabled() / setMqttTopicEnabled() are backed by QSettings.

aethersdr/cw/decode — payload additions and gating change

pitch_hz and speed_wpm are now included in the payload when their values are non-zero, allowing subscribers to mirror the operator's keying characteristics in replies.

Note for existing users: aethersdr/cw/decode was introduced in v26.6.1 (#3216) as an always-active publish topic. This PR brings it under the same per-topic opt-in mechanism as all other gateable topics, but it defaults to enabled for backward compatibility. Users relying on this topic do not need to take any action after upgrading.

The kCwDecodeTopic string constant was consolidated per the feedback in the #3216 review; the topic path itself is unchanged.

aethersdr/cw/transmit — subscribe (opt-in)

AetherSDR subscribes to this topic and feeds received text to the CWX keyer.

Payload: {"text": "K5PTB DE W1ABC", "speed_wpm": 28, "pitch_hz": 600}

speed_wpm and pitch_hz are optional; omitting them uses the current keyer settings.

aethersdr/radio/state — publish (opt-in)

Published on any frequency, mode, or TX state change:

{"slice": "A", "freq": 14.025, "mode": "CW", "tx": false}

aethersdr/ax25/rx — publish (opt-in)

Each decoded AX.25 frame is published:

{
  "from": "K5PTB", "to": "APRS",
  "via": ["WIDE1-1", "WIDE2-1"],
  "payload": ">Running AetherSDR",
  "freq": 144.390,
  "confidence": 0.87
}

aethersdr/ax25/tx — subscribe (opt-in)

AetherSDR subscribes and queues received frames for 1200-baud Bell 202 transmission.

Payload: TNC2 string — K5PTB>APRS,WIDE1-1:>payload

Initialization note: The AX.25 MQTT handler lives in the AetherModem dialog, which is lazy-initialized. The ax25/tx topic will not be active until the dialog has been initialized — either by opening the AetherModem window, or by enabling Start KISS TNC on startup in the AetherModem KISS TNC settings. With that option enabled, the dialog is created silently at launch (no window appears) and ax25/tx is available immediately without any user interaction.

Debug logging

All MQTT publish and receive events are written to the AetherSDR debug console when the debug logging option is enabled. No applet-level display is added for these topics.

Design decisions (re: #3388)

On TCI overlap (radio/state): TCI and MQTT carry overlapping information, but they serve different clients. A subscriber using only MQTT for external tool integration (contest loggers, automation scripts) should not need a second TCI connection. The ordering concern — CW decode characters arriving before a tx=false state update — is not a practical issue: both originate from the same internal event bus, so MQTT message ordering between them is deterministic for a single publisher.

On keeping AX.25 topics: The ax25/rx and ax25/tx topics are not debug conveniences; they are the primary path for external tools (APRS clients, packet BBS clients, automation scripts) to consume decoded frames without maintaining a persistent KISS TCP connection. They have been in live use for multi-day AX.25 packet decode testing.

On PR structure: The per-topic gating infrastructure (InternalMqttTopicDef, isMqttTopicEnabled) is shared across all the new topics. Splitting would require duplicating or pre-landing that infrastructure separately. Happy to split if preferred — the topics are otherwise independent.

What does not change

  • Existing antenna/VFO alias topics remain always-on and non-disableable
  • aethersdr/cw/decode topic path is unchanged
  • No changes to KISS TNC, TCI server, or CAT/rigctld paths

Testing

Verified end-to-end on all new topics: publish events received by a Mosquitto subscriber, inbound cw/transmit messages keyed through CWX, radio/state updates on band change, and ax25/rx frames from live 144.390 MHz traffic. ax25/tx (transmit path) has not been exercised on-air pending transverter calibration.

The AX.25 MQTT path has been in continuous use for multi-day packet decode comparison testing on a Raspberry Pi 5 (2,000+ frames across 12-hour runs, three-decoder comparison against Graywolf and Direwolf). The cw/transmit and radio/state topics were used in live contest operating on a MacBook Air M2 during the 2026 Atlantic Canada QSO Party.

Screenshots of the updated MQTT settings dialog are in #3388.


Built and smoke tested on: Windows 11 on Intel i3 (integrated GPU)

@K5PTB K5PTB requested a review from a team as a code owner June 8, 2026 11:26
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 8, 2026
…profiles branch

MQTT: #ifdef HAVE_MQTT blocks removed from dialog .h/.cpp — ax25 branches
must compile independent of the unmerged PR aethersdr#3460 MQTT topics.

Tone polarity: radio buttons, selectedTonePolarity(), setTonePolarity(),
kPacketDecoderPolaritySetting, and helpers removed; setModemProfile now
hardcodes Ax25TonePolarity::Normal. Backend enum kept but no UI.

VHF mode combo: polarityCell panel removed entirely; kPacketDecoderVhfModeSetting
and applyVhfMode lambda moved to feat/ax25-direwolf-profiles (PR 4).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@ten9876

ten9876 commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

@AetherClaude review this PR

K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 8, 2026
…B+/AB+ demodulators

Add IAfskDemod interface + LibmodemAfskDemod, DirewolfAfskDemod, DirewolfFMDiscrimDemod wrappers.
VhfMode enum (Off/A/B/AB/APlus/BPlus/ABPlus) replaces the fixed 18-lane bank; default APlus
matches Direwolf's default profile. kVhf1200SpaceGains (9-step geometric) and
kVhf1200BSliceOffsets (9-step linear) reproduce exact Direwolf A+/B+ slicer banks.

UI: tone polarity radio buttons + VHF DEMOD MODE combo box (disabled for HF).
Settings: Ax25PacketDecoderVhfMode, Ax25PacketDecoderPolarity, both persisted.
Dialog: startTransmit() guards moved out of startTransmitFromUi() so MQTT TX
path goes through the same checks.

Receive gate: remove resetBitstreamStates() on open — each lane's HdlcCodec
tracks its own state since PR 2; spurious resets break mid-preamble decodes.
processBit: extract inFrame() transition count outside the completion branch.
recordReject: AX.25 minimum is 18 bytes (was 17) to include 1-byte PID.
recordDemodSymbol: signature changed to (uint8_t bit, double confidence).
demodDescription(): lane count now derived from config, not live lane vector.

#ifdef HAVE_MQTT: optional AX.25 MQTT publish/subscribe wired with local
topic-constant stubs; remove stubs once MqttSettings.h additions in aethersdr#3460 merge.

CMakeLists: aether_afskdemod STATIC library (GPL-3, Direwolf A+B);
linked to AetherSDR, ax25_libmodem_shim_test, ax25_replay, ax25_session_analyze.
hdlc_codec_test moved to sit after tnc_terminal_test (cosmetic).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 8, 2026
…profiles branch

MQTT: #ifdef HAVE_MQTT blocks removed from dialog .h/.cpp — ax25 branches
must compile independent of the unmerged PR aethersdr#3460 MQTT topics.

Tone polarity: radio buttons, selectedTonePolarity(), setTonePolarity(),
kPacketDecoderPolaritySetting, and helpers removed; setModemProfile now
hardcodes Ax25TonePolarity::Normal. Backend enum kept but no UI.

VHF mode combo: polarityCell panel removed entirely; kPacketDecoderVhfModeSetting
and applyVhfMode lambda moved to feat/ax25-direwolf-profiles (PR 4).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…5, debug logging

Squashed history (oldest → newest):
- feat(mqtt): per-topic enable/disable for internal MQTT topics
- fix(mqtt): plain text labels and dim styling for locked internal topics
- feat(mqtt): add pitch_hz and speed_wpm to aethersdr/cw/decode payload
- feat(mqtt): subscribe aethersdr/cw/transmit — inbound MQTT to CWX keyer
- feat(mqtt): publish aethersdr/radio/state on freq/mode/TX changes
- feat(ax25): publish decoded frames to MQTT and accept TX via MQTT
- feat(ax25-mqtt): add confidence to MQTT RX message
- feat(mqtt): add publish/receive debug logging and TX applet display
@K5PTB K5PTB force-pushed the feat/mqtt-expansion branch from 3fef707 to fba67f8 Compare June 8, 2026 22:55
@K5PTB

K5PTB commented Jun 8, 2026

Copy link
Copy Markdown
Contributor Author

@ten9876 Another PR (#3475) was approved and merged so I could continue, out of order with how I submitted it and this one. There was a merge conflict, so I had Claude resolve them. I don't know if that was what is blocking @AetherClaude from reviewing the PR.

@ten9876

ten9876 commented Jun 9, 2026

Copy link
Copy Markdown
Collaborator

@AetherClaude review this PR

@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.

Thanks for the thorough PR @K5PTB — the InternalMqttTopicDef registry is a clean replacement for the ad-hoc list, the opt-in default for cw/decode is the right call (and clearly called out in the description), and the AX.25 / radio-state / cw-transmit wiring all read sensibly. A few things worth a look before merging:

Dead declaration in Ax25HfPacketDecodeDialog.h

Ax25TonePolarity selectedTonePolarity() const;

is added at Ax25HfPacketDecodeDialog.h:99 but has no definition and no caller (the only use of Ax25TonePolarity in the file is the hardcoded Ax25TonePolarity::Normal at the call site). Looks like a leftover from earlier work — please drop it before merge so we don't ship an unresolved declaration.

aethersdr/radio/state will flood on tuning

SliceModel::frequencyChanged fires on every intermediate step while the user drags the VFO knob, scans, or uses RIT — there's no debounce. Each tick produces a JSON publish, a qCDebug line, and a messagePublished signal that the MqttApplet log buffer trims one block at a time. On a fast scan or even a normal mouse drag this will be hundreds of msg/sec to the broker. Consider a small coalescing timer (e.g. 100–200 ms QTimer::singleShot "dirty flag" pattern) before publishing, or only publish on frequencyChanged after the user releases — either is fine, but the current shape is likely to surprise downstream subscribers.

setMqttTopicEnabled flushes to disk per call

void setMqttTopicEnabled(const QString& topic, bool enabled) {
    AppSettings::instance().setValue(topicEnabledKey(topic), enabled);
    AppSettings::instance().save();   // ← XML write
}

MqttSettingsDialog::saveSettings calls this in a loop over all gateable topics, so a single OK click does N full XML writes (plus the regular settings save()). Suggest dropping the save() here and letting the caller (saveSettings) do one final AppSettings::instance().save() after the loop, matching how the rest of the dialog persists config.

Possible feedback loop between cw/decode and cw/transmit

If a subscriber relays cw/decode events back through cw/transmit (a plausible "remote echo" automation), the keyer text gets re-decoded by m_cwDecoderTx, re-published on cw/decode, and the cycle repeats. Both topics default to off so this only bites users who deliberately enable both, but a one-line note in the topic documentation or an origin field on the cw/decode payload would head it off.

Minor

  • Ax25HfPacketDecodeDialog::handleMqttMessage doesn't recheck isMqttTopicEnabled before calling startTransmit, unlike the CW transmit handler in MainWindow. In practice the broker won't deliver to a disabled-and-thus-unsubscribed topic, so this is just a consistency nit — pick one pattern.
  • m_cwLastPitchHz / m_cwLastSpeedWpm are written from the RX m_cwDecoder.statsUpdated only (the wire at MainWindow.cpp:13125), so the pitch_hz/speed_wpm fields on TX-direction cw/decode payloads will reflect the last RX-side measurement, not the operator's actual TX settings. If the intent (per the PR description) is to mirror operator keying for the subscriber to echo back, consider pulling from transmitModel().cwSpeed() / cwPitch() directly when rx=false.

Otherwise the per-topic gating mechanism, the dialog plumbing, and the AX.25 JSON shape all look reasonable, and the AppSettings-backed persistence follows project convention. Thanks for the careful release-note on the cw/decode default-disabled behavior change.


🤖 aethersdr-agent · cost: $9.5183 · model: claude-opus-4-7

K5PTB and others added 2 commits June 8, 2026 23:02
- Drop dead selectedTonePolarity() declaration from Ax25HfPacketDecodeDialog.h
  — leftover from conflict resolution during rebase onto main after aethersdr#3473 merged
- handleMqttMessage in Ax25HfPacketDecodeDialog now guards with
  isMqttTopicEnabled, consistent with the CW transmit handler in MainWindow
- cw/decode TX payloads (rx:false) now pull pitch_hz/speed_wpm from
  transmitModel().cwPitch()/cwSpeed() rather than the last RX measurement
- radio/state publish coalesced through a 150 ms single-shot timer to prevent
  flooding on VFO drag/scan (frequencyChanged fires on every intermediate step)
- setMqttTopicEnabled no longer saves to disk per call; saveSettings() does
  one AppSettings::save() after the full loop
- Add comment near kCwDecodeTopic/kCwTransmitTopic noting relay scripts should
  filter on the aethersdr/ namespace to avoid cw/decode→cw/transmit feedback
  loops — the full topic path serves as the origin identifier.
  @aethersdr-agent: was an explicit origin field your intent, or is the
  namespace sufficient?

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Six fixes from /code-review ultra:
- startKissTncOnStartupIfConfigured: call setMqttClient() so
  auto-started KISS TNC has MQTT wired up
- kCwDecodeTopic: set defaultEnabled=true so upgrades don't silently
  disable previously-always-on CW decode
- routeCwDecoderOutput: save m_cwStatsConn handle to properly
  disconnect statsUpdated lambda (connection leak)
- handleMqttMessage: add isVisible() guard so hidden dialog can't
  key the radio via MQTT
- publishFrameMqtt: add isMqttTopicEnabled(kAx25RxTopic) guard
- cw/transmit: save/restore cwSpeed/cwPitch after MQTT-commanded TX

Four fixes from smoke testing:
- queueEmpty doesn't fire with sync_cwx=0; replace with 1-second
  debounce timer (m_cwxTxEndTimer) — queueEmpty kept as fast path
- Speed restore uses cwxModel().setSpeed() (cwx wpm N); CWX ignores
  transmitModel().setCwSpeed() (cw wpm N)
- radio/state publishes one tx:true at keying start and one
  tx:false after 1 s silence, suppressing inter-element transitions
- Remove flooding "MQTT CW decode suppressed" debug log

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@K5PTB

K5PTB commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@AetherClaude — following up on the review items — all six have been addressed, plus four more found during smoke testing. Summary:

From the review (commit 2cfb8adb):

  1. Dead selectedTonePolarity() declaration removed from header (merge artifact)
  2. radio/state flooding fixed: 150 ms coalescing timer (m_radioStateCoalesceTimer)
  3. setMqttTopicEnabled no longer saves on every call; one save at end of saveSettings()
  4. Comment added near CW topic constants re: feedback loop / origin field
  5. handleMqttMessage in AX.25 dialog guards with isMqttTopicEnabled
  6. cw/decode TX payloads use transmitModel().cwPitch()/cwSpeed() not last RX measurement

From smoke testing and /code-review ultra (commit 81bd3247):

  1. startKissTncOnStartupIfConfigured: call setMqttClient() so auto-started KISS TNC has MQTT wired up
  2. kCwDecodeTopic: set defaultEnabled=true in InternalMqttTopicDef so upgrades don't silently disable previously-always-on CW decode
  3. routeCwDecoderOutput: save m_cwStatsConn handle to properly disconnect statsUpdated lambda (connection leak)
  4. handleMqttMessage: add isVisible() guard so hidden AX.25 dialog can't key the radio via MQTT
  5. publishFrameMqtt: add isMqttTopicEnabled(kAx25RxTopic) guard
  6. cw/transmit speed/pitch save-restore: queueEmpty doesn't fire with sync_cwx=0 (break-in mode); replaced with 1-second debounce timer (m_cwxTxEndTimer) — queueEmpty kept as fast path for sync_cwx=1 radios. Uses cwxModel().setSpeed() (cwx wpm N), not transmitModel().setCwSpeed() (cw wpm N), which CWX ignores
  7. radio/state inter-element suppression: publishes exactly one tx:true at keying start and one tx:false after 1 s of silence, suppressing the per-element PTT transitions
  8. Removed flooding "MQTT CW decode suppressed" debug log

@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.

Thanks for this @K5PTB — the per-topic gating approach is clean, the InternalMqttTopicDef registry is a nice generalization, and the PR description does a good job laying out the design tradeoffs and the back-compat note for cw/decode. Below are a handful of observations from a read-through, none of them blocking.

Behavior question: ax25/tx requires the dialog be visible

In Ax25HfPacketDecodeDialog::handleMqttMessage:

if (!isVisible())
    return;
startTransmit(QString::fromUtf8(payload).trimmed());

This means an inbound aethersdr/ax25/tx message is silently dropped unless the user has the AX.25 dialog open. RX publish (appendFrame → publishFrameMqtt) does not have the symmetric requirement, and the corresponding aethersdr/cw/transmit topic in MainWindow.cpp also has no visibility precondition. This asymmetry will likely surprise integrators — automation that drives the radio is exactly the scenario where the user is not looking at AetherSDR. Either drop the isVisible() guard, or at minimum emit a debug log line + a system line in the dialog so it's diagnosable when a message gets dropped. If this is deliberate (don't auto-TX without operator awareness), the PR description should call it out.

Race: user-initiated WPM/pitch change during MQTT CW keying

In the cw/transmit lambda (MainWindow.cpp:~2042), the saved WPM/pitch are restored unconditionally when the keyer finishes:

if (m_cwxSavedWpm > 0) m_radioModel.cwxModel().setSpeed(m_cwxSavedWpm);
if (m_cwxSavedHz  > 0) m_radioModel.transmitModel().setCwPitch(m_cwxSavedHz);

If the operator manually changes WPM or CW pitch via the UI while a long MQTT-driven CWX run is still keying, the restore overwrites their new value when queueEmpty/timer fires. Edge case, but easy to imagine in contest operation. Cheapest fix: on the user-initiated speed/pitch signal, clear m_cwxSavedWpm / m_cwxSavedHz so the restore becomes a no-op.

Per-topic gate keys could collide for + wildcards

topicEnabledKey() maps /_ and +x. Today's topic set is fine, but aethersdr/cw/decode and a hypothetical future aethersdr/cw+decode (or any topic containing x at the spot a + lived) would alias. Trivial to make robust now — e.g. percent-encode non-alnum chars, or hash the topic — much cheaper than migrating user settings later.

Settings dialog: index-based correspondence

In MqttSettingsDialog::loadSettings / saveSettings, the checkboxes are mapped to internalMqttSubscribeTopicDefs() by positional index. It works because buildUi() builds the boxes 1:1 in the same order and the defs vector is static const, but a future reorder or filter in buildUi() would silently flip user settings between topics. Storing the topic string on the checkbox (cb->setProperty("topic", def.topic)) and looking it up at save time would make this resilient.

Style nits (non-blocking)

  • The new member fields in MainWindow.h (m_cwxTransmitting, m_cwxTxEndTimer, m_radioStateFreqConn, etc.) are added outside the existing #ifdef HAVE_MQTT block, unlike m_mqttClient. Functionally fine (Connection/QTimer default-construct cheaply) but inconsistent with the established pattern.
  • qCDebug(lcMqtt) << "MqttClient: publish" << topic << payload; in MqttClient::publish — payload for cw/transmit or ax25/tx could be operator text; harmless on a debug category but worth being aware that turning the category on logs all message contents.

The core change (per-topic gating + opt-in defaults for the new topics, with the cw/decode migration called out) is the right shape. The CWX restore-on-completion dual-path (debounce timer + queueEmpty) is appropriately commented and the rationale is clear.


🤖 aethersdr-agent · cost: $8.5000 · model: claude-opus-4-7

- AX.25 TX: drop isVisible() guard so headless automation works
- CWX speed/pitch restore: track what value was SET (m_cwxSentWpm/Hz)
  and only restore if the current value still matches; prevents
  clobbering user changes made during TX
- topicEnabledKey: use %2B for '+' to avoid key collisions
- MqttSettingsDialog: bind topic string as Qt property on each checkbox
  so load/save is order-independent (resilient to def-vector reorders)
- MainWindow: guard all new MQTT members and connections with
  #ifdef HAVE_MQTT to preserve non-MQTT build correctness

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@K5PTB

K5PTB commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@AetherClaude — follow-up commit 11dfc48c addresses your five review items:

1. AX.25 TX visibility guard — removed if (!isVisible()) return; from handleMqttMessage(). Headless automation no longer depends on the dialog being open, consistent with how CW/transmit works.

2. CWX speed/pitch restore race — added m_cwxSentWpm / m_cwxSentHz to track what values were actually commanded. Both the 1-second debounce timer and the queueEmpty fast-path now check current == sent before restoring; if the user changed speed/pitch during TX, the restore is skipped. Both paths zero the sent-values at cleanup.

3. topicEnabledKey() collision — replaced '+''x' with '+'"%2B" (percent-encoding), eliminating the collision risk with topic names that contain a literal x.

4. MqttSettingsDialog index fragility — each checkbox now carries its topic string as a Qt widget property ("topic"). loadSettings() and saveSettings() look up by property rather than positional index, so adding or reordering topic definitions in the future won't silently flip settings.

5. #ifdef HAVE_MQTT consistency — all new MQTT-specific members in MainWindow.h are now inside the guard. The two disconnect() calls for m_radioStateFreqConn / m_radioStateModeConn that were outside the guard, and the m_cwStatsConn connect/disconnect pair in routeCwDecoderOutput(), are all properly wrapped.

Items 1, 2, and 4 smoke-tested on macOS. Build clean with HAVE_MQTT=ON.

@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.

Nice work on the iteration here, @K5PTB — the design is well thought out and the prior review items all look properly addressed. The opt-in gating, InternalMqttTopicDef registry, property-keyed checkboxes, and the dual queueEmpty / debounce-timer end-of-CWX detection all read cleanly. AppSettings::instance() is used correctly throughout, the #ifdef HAVE_MQTT guards are consistent, and the topicEnabledKey percent-encoding fix is a solid call.

One bug worth a fix before merge, plus a small note:

1. cw/transmit back-to-back messages clobber the saved WPM/pitchMainWindow.cpp:2046-2050

m_cwxSavedWpm = changeWpm ? m_radioModel.cwxModel().speed() : 0;
m_cwxSavedHz  = changeHz  ? tx.cwPitch() : 0;

If a second cw/transmit message arrives while m_cwxTransmitting is still true (queue not yet drained, debounce not yet fired), this re-reads the already-modified speed/pitch as the "saved" value. The user's original setting is then lost — restore at end of TX will leave the radio at message-1's values.

Suggest only saving on the transition into transmitting:

if (!m_cwxTransmitting) {
    m_cwxSavedWpm = changeWpm ? m_radioModel.cwxModel().speed() : 0;
    m_cwxSavedHz  = changeHz  ? tx.cwPitch() : 0;
}

Realistic scenario: an automation script firing two short messages in quick succession with different speed_wpm. The m_cwxSentWpm guard you added correctly prevents restoring a stale "sent" value, but the saved-original side has the same race in the opposite direction.

2. (Minor) messagePublished is emitted regardless of publish successMqttClient.cpp:236

mosquitto_publish's return value isn't checked before emit messagePublished(...), so the MqttApplet log will show a TX line for a publish that the broker never actually accepted. Not blocking — just something to consider if the broker drops it (offline, ACL deny, etc.).

Everything else looks good — happy to take another pass once the WPM/pitch save guard is in.


🤖 aethersdr-agent · cost: $6.5486 · model: claude-opus-4-7

AetherClaude review items:
- Back-to-back cw/transmit: guard saved WPM/pitch with !m_cwxTransmitting
  so only the first message in a sequence captures the original radio values
- MqttClient::publish: check mosquitto_publish return code; only emit
  messagePublished (and log TX in applet) on success

Self-review of full branch diff (upstream/main..HEAD):
- m_cwxSentWpm/Hz: use if(changeWpm)/if(changeHz) guards instead of
  ternary-to-zero so a second message without speed/pitch doesn't clobber
  the sentinel and break the restore check
- setActiveSliceInternal: merge two adjacent #ifdef HAVE_MQTT blocks into one

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@K5PTB

K5PTB commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@AetherClaude — follow-up commit 219db3bd addresses your two remaining items, plus two findings from a self-review of the full branch diff:

Your items:

1. Back-to-back cw/transmit save race — wrapped the m_cwxSavedWpm/Hz capture in if (!m_cwxTransmitting) so only the first message in a sequence reads the original radio values. Subsequent messages update m_cwxSentWpm/Hz (so the restore check stays current) without overwriting the saved originals.

2. mosquitto_publish return value — captured the return code; messagePublished (and the applet TX log) now only fires on MOSQ_ERR_SUCCESS.

Self-review findings on the full branch diff:

3. m_cwxSentWpm/Hz zeroed by second message without speed/pitch — the previous changeWpm ? wpm : 0 ternary would set m_cwxSentWpm to 0 if a second message arrived without speed_wpm, defeating the speed() == m_cwxSentWpm restore check. Changed to if (changeWpm) m_cwxSentWpm = wpm so the sentinel is only updated when a value is actually commanded. Validated: second message with speed_wpm: 0 (→ changeWpm=false) correctly restores to original on queue drain.

4. Adjacent #ifdef HAVE_MQTT blocks — merged two consecutive guards in setActiveSliceInternal into one.

@K5PTB

K5PTB commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@ten9876 Jeremy, my apologies for the iteration. I finally figured out how to initiate a review locally, so I can identify these things before submitting a PR.

@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.

Thanks for the iteration here — the per-topic gating registry, the careful CWX save/restore tracking against m_cwxSentWpm/m_cwxSentHz, the QPointer<MqttClient> in the AX.25 dialog, the #ifdef HAVE_MQTT discipline, and the resubscribe on settings save (MainWindow.cpp:5984) all look good. AppSettings (not QSettings) is used throughout, and the explicit AppSettings::instance().save() in MqttSettingsDialog::saveSettings is the right call.

A couple of items worth addressing:

1. PR description disagrees with code on cw/decode default. The PR body says:

All other topics — including aethersdr/cw/decode and all new topics — are gateable and default to false (disabled) ... Users relying on this topic will need to enable it in the MQTT settings dialog after upgrading.

But in src/core/MqttSettings.cpp the entry is:

{ QString(kCwDecodeTopic),    QStringLiteral("CW decoded text"),             true, true },

i.e. gateable=true, defaultEnabled=true — existing subscribers keep working on upgrade with no user action. That's actually the right behavior for backward compatibility (and your commit message on 219db3bd says as much), but the PR description still tells users they need to opt back in. Please update the description so users with relay scripts or loggers reading aethersdr/cw/decode don't go hunting for a checkbox they don't need to toggle.

2. m_radioStateCoalesceTimer configured after first use. The timer's setSingleShot(true) / setInterval(150) lives at MainWindow.cpp:4413, but the slice activation that wires m_radioStateCoalesceTimer.start() into SliceModel::frequencyChanged / modeChanged runs from setActiveSliceInternal() during early constructor setup. If a slice signal fires between then and line 4413, the timer ticks immediately (interval 0) and not single-shot. Probably harmless during startup, but cheap to fix — move the setSingleShot/setInterval block above the slice-init path, or do it inline at the timer's member declaration in MainWindow.h.

Minor: the TX-side log truncation lambda added in MqttApplet::setMqttClient is a verbatim copy of the loop in onMessageReceived. Not a blocker — feel free to extract a helper if you're touching the file again, otherwise leave it.

Scope, error handling at the mosquitto boundary (the new rc != MOSQ_ERR_SUCCESS early return), and the connection cleanup paths all look correct to me. Nice work iterating on this one, @K5PTB.


🤖 aethersdr-agent · cost: $9.2917 · model: claude-opus-4-7

- m_radioStateCoalesceTimer: move setSingleShot/setInterval/connect to
  before the radio-state MQTT wiring so the timer is fully configured
  before any code path can call start() on it
- MqttApplet: extract duplicated log-trimming loop into appendMessageLog()
  helper; called from both messagePublished and onMessageReceived paths

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@K5PTB

K5PTB commented Jun 9, 2026

Copy link
Copy Markdown
Contributor Author

@AetherClaude — commit 64905cb5 addresses your two items and the minor:

1. PR description / cw/decode default — updated the PR body to correctly reflect defaultEnabled=true. The code behavior was right; the description was wrong. Added a separate note under aethersdr/ax25/tx documenting the lazy-initialization behavior (dialog must be opened once, or KISS TNC auto-start enabled, before headless TX works).

2. m_radioStateCoalesceTimer initialization order — moved setSingleShot(true) / setInterval(150) / connect to before the radio-state MQTT wiring in the constructor, so the timer is fully configured before any code path can call start() on it.

Minor — appendMessageLog helper — extracted the log-trimming loop from both the messagePublished and onMessageReceived handlers into a shared appendMessageLog() method.


@AetherClaude — I'd like to perform a code review locally before submitting a PR or amendment, so we catch issues earlier in the cycle. Can you share what review process or Claude commands you use, so I can run the same thing on my end?

@jensenpat jensenpat left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

All 4 rounds of AetherClaude review feedback addressed; CI green across CI/macOS/Windows/CodeQL/Accessibility; per-topic gating registry + backward-compatible cw/decode default + property-keyed checkboxes + percent-encoded topic keys + CWX save/restore race fixes all land cleanly. Approving for merge.

@jensenpat jensenpat merged commit 024ce26 into aethersdr:main Jun 9, 2026
6 checks passed
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 9, 2026
…B+/AB+ demodulators

Add IAfskDemod interface + LibmodemAfskDemod, DirewolfAfskDemod, DirewolfFMDiscrimDemod wrappers.
VhfMode enum (Off/A/B/AB/APlus/BPlus/ABPlus) replaces the fixed 18-lane bank; default APlus
matches Direwolf's default profile. kVhf1200SpaceGains (9-step geometric) and
kVhf1200BSliceOffsets (9-step linear) reproduce exact Direwolf A+/B+ slicer banks.

UI: tone polarity radio buttons + VHF DEMOD MODE combo box (disabled for HF).
Settings: Ax25PacketDecoderVhfMode, Ax25PacketDecoderPolarity, both persisted.
Dialog: startTransmit() guards moved out of startTransmitFromUi() so MQTT TX
path goes through the same checks.

Receive gate: remove resetBitstreamStates() on open — each lane's HdlcCodec
tracks its own state since PR 2; spurious resets break mid-preamble decodes.
processBit: extract inFrame() transition count outside the completion branch.
recordReject: AX.25 minimum is 18 bytes (was 17) to include 1-byte PID.
recordDemodSymbol: signature changed to (uint8_t bit, double confidence).
demodDescription(): lane count now derived from config, not live lane vector.

topic-constant stubs; remove stubs once MqttSettings.h additions in aethersdr#3460 merge.

CMakeLists: aether_afskdemod STATIC library (GPL-3, Direwolf A+B);
linked to AetherSDR, ax25_libmodem_shim_test, ax25_replay, ax25_session_analyze.
hdlc_codec_test moved to sit after tnc_terminal_test (cosmetic).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 9, 2026
…profiles branch

MQTT: #ifdef HAVE_MQTT blocks removed from dialog .h/.cpp — ax25 branches
must compile independent of the unmerged PR aethersdr#3460 MQTT topics.

Tone polarity: radio buttons, selectedTonePolarity(), setTonePolarity(),
kPacketDecoderPolaritySetting, and helpers removed; setModemProfile now
hardcodes Ax25TonePolarity::Normal. Backend enum kept but no UI.

VHF mode combo: polarityCell panel removed entirely; kPacketDecoderVhfModeSetting
and applyVhfMode lambda moved to feat/ax25-direwolf-profiles (PR 4).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 9, 2026
Adds a Direwolf-derived AFSK demodulator alongside the existing libmodem
HF 300-baud path. Profile selection is exposed via a VhfMode enum (Off,
A, APlus) with a VHF combo box in the AetherModem dialog. Profile B/B+
are deferred to the follow-on direwolf-profiles branch.

- AetherAFSKDemod: Direwolf Bell 202 AFSK demodulator ported to C++/Qt,
  with AetherSDR lane/confidence interface
- IAfskDemod: abstract demodulator interface allowing libmodem (HF) and
  Direwolf (VHF) lane types to coexist in the same shim lane vector
- VhfMode enum: Off / A / APlus; drives lane construction and lane-count
  calculation in AetherAx25LibmodemShim
- Profile A+: 9-slicer space-gain diversity (kVhf1200SpaceGains) matching
  Direwolf MAX_SUBCHANS=9 geometry
- AetherFMDiscrimDemod excluded from this branch; present only in
  feat/ax25-direwolf-profiles where B/B+ profiles are implemented
- Ax25HfPacketDecodeDialog: VHF profile combo wired to VhfMode; MQTT
  topic constants sourced from MqttSettings.h (merged via aethersdr#3460)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 10, 2026
publishFrameMqtt() was defined but never connected to the frameDecoded
signal, so decoded AX.25 frames were never published to MQTT. Both HF
and VHF paths share the same emission point, so one connection covers
both. Introduced in aethersdr#3460.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 10, 2026
…Principle VIII.

Adds a Direwolf-derived AFSK demodulator alongside the existing libmodem
HF 300-baud path. Profile selection is exposed via a VhfMode enum (Off,
A, APlus) with a VHF combo box in the AetherModem dialog. Profile B/B+
are deferred to the follow-on direwolf-profiles branch.

- AetherAFSKDemod: Direwolf Bell 202 AFSK demodulator ported to C++/Qt,
  with AetherSDR lane/confidence interface
- IAfskDemod: abstract demodulator interface allowing libmodem (HF) and
  Direwolf (VHF) lane types to coexist in the same shim lane vector
- VhfMode enum: Off / A / APlus; drives lane construction and lane-count
  calculation in AetherAx25LibmodemShim
- Profile A+: 9-slicer space-gain diversity (kVhf1200SpaceGains) matching
  Direwolf MAX_SUBCHANS=9 geometry
- AetherFMDiscrimDemod excluded from this branch; present only in
  feat/ax25-direwolf-profiles where B/B+ profiles are implemented
- Ax25HfPacketDecodeDialog: VHF profile combo wired to VhfMode; MQTT
  topic constants sourced from MqttSettings.h (merged via aethersdr#3460)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 10, 2026
publishFrameMqtt() was defined but never connected to the frameDecoded
signal, so decoded AX.25 frames were never published to MQTT. Both HF
and VHF paths share the same emission point, so one connection covers
both. Introduced in aethersdr#3460.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@K5PTB K5PTB deleted the feat/mqtt-expansion branch June 10, 2026 05:14
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 10, 2026
…Principle VIII.

Adds a Direwolf-derived AFSK demodulator alongside the existing libmodem
HF 300-baud path. Profile selection is exposed via a VhfMode enum (Off,
A, APlus) with a VHF combo box in the AetherModem dialog. Profile B/B+
are deferred to the follow-on direwolf-profiles branch.

- AetherAFSKDemod: Direwolf Bell 202 AFSK demodulator ported to C++/Qt,
  with AetherSDR lane/confidence interface
- IAfskDemod: abstract demodulator interface allowing libmodem (HF) and
  Direwolf (VHF) lane types to coexist in the same shim lane vector
- VhfMode enum: Off / A / APlus; drives lane construction and lane-count
  calculation in AetherAx25LibmodemShim
- Profile A+: 9-slicer space-gain diversity (kVhf1200SpaceGains) matching
  Direwolf MAX_SUBCHANS=9 geometry
- AetherFMDiscrimDemod excluded from this branch; present only in
  feat/ax25-direwolf-profiles where B/B+ profiles are implemented
- Ax25HfPacketDecodeDialog: VHF profile combo wired to VhfMode; MQTT
  topic constants sourced from MqttSettings.h (merged via aethersdr#3460)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 10, 2026
publishFrameMqtt() was defined but never connected to the frameDecoded
signal, so decoded AX.25 frames were never published to MQTT. Both HF
and VHF paths share the same emission point, so one connection covers
both. Introduced in aethersdr#3460.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 10, 2026
…Principle VIII.

Adds a Direwolf-derived AFSK demodulator alongside the existing libmodem
HF 300-baud path. Profile selection is exposed via a VhfMode enum (Off,
A, APlus) with a VHF combo box in the AetherModem dialog. Profile B/B+
are deferred to the follow-on direwolf-profiles branch.

- AetherAFSKDemod: Direwolf Bell 202 AFSK demodulator ported to C++/Qt,
  with AetherSDR lane/confidence interface
- IAfskDemod: abstract demodulator interface allowing libmodem (HF) and
  Direwolf (VHF) lane types to coexist in the same shim lane vector
- VhfMode enum: Off / A / APlus; drives lane construction and lane-count
  calculation in AetherAx25LibmodemShim
- Profile A+: 9-slicer space-gain diversity (kVhf1200SpaceGains) matching
  Direwolf MAX_SUBCHANS=9 geometry
- AetherFMDiscrimDemod excluded from this branch; present only in
  feat/ax25-direwolf-profiles where B/B+ profiles are implemented
- Ax25HfPacketDecodeDialog: VHF profile combo wired to VhfMode; MQTT
  topic constants sourced from MqttSettings.h (merged via aethersdr#3460)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
K5PTB added a commit to K5PTB/AetherSDR that referenced this pull request Jun 10, 2026
publishFrameMqtt() was defined but never connected to the frameDecoded
signal, so decoded AX.25 frames were never published to MQTT. Both HF
and VHF paths share the same emission point, so one connection covers
both. Introduced in aethersdr#3460.

Co-Authored-By: Claude Sonnet 4.6 <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