Move MQTT configuration into settings dialog#3051
Conversation
d6e27c4 to
abfbeec
Compare
The applet was doing broker credentials, topic editing, publish button editing, and live operation in one cramped surface. This moves durable settings into a persistent MQTT dialog while keeping the applet focused on connect/status/log/publish actions. Shared helper functions preserve legacy AppSettings keys and parse the existing *topic overlay convention into row data for the dialog. Desired subscriptions are now explicit state so automatic antenna alias topics are always subscribed and removed user topics do not survive reconnects. The applet On/Off state is also persisted as MqttEnabled so an operator who left MQTT On gets an automatic reconnect on the next AetherSDR start, after the keychain password load completes. Constraint: Existing MqttHost/MqttPort/MqttUser/MqttTopics/MqttTls/MqttCaFile/MqttButtons keys must keep loading. Constraint: Keychain remains the MQTT password store when available. Rejected: Store topics under a new MqttTopicsJson key | would add migration risk and break the goal to preserve current settings keys. Rejected: Keep publish-button editing in the applet | leaves durable configuration in the cramped live UI. Confidence: high Scope-risk: moderate Directive: Keep AetherSDR-owned MQTT topics out of operator-removable subscription state; append them through mqttSubscriptionTopics(). Tested: cmake --build build --target AetherSDR -j4 Tested: cmake --build build --target mqtt_settings_test mqtt_antenna_alias_test antenna_alias_test -j4 Tested: ctest --test-dir build -R 'mqtt_antenna_alias_test|antenna_alias_test|mqtt_settings_test' --output-on-failure Tested: git diff --check Not-tested: Manual GUI restart smoke test against a live MQTT broker after adding MqttEnabled restore.
abfbeec to
48af020
Compare
There was a problem hiding this comment.
Thanks for this @s53zo — clean separation of durable config from live operation, good test coverage for the topic parse/serialize round-trips, and the legacy-key preservation story is well thought out.
A few observations rather than blockers:
Stale base: the branch was forked from c44f1c5d and is two commits behind main (#3035, #3036). The PR's own commit only touches MQTT files, so Git merge will apply cleanly without reverting those — but reviewers comparing main..head locally will see what look like reverts in src/core/AudioEngine.* and src/gui/SpectrumWidget.*. A rebase would make the diff less alarming at a glance.
Duplicated keychain migration: MqttApplet::loadPasswordFromKeychain() and MqttSettingsDialog::loadPasswordFromKeychain() both perform the legacy-plaintext → keychain migration independently. Functionally fine (whichever fires first wins and removes the legacy key; the other no-ops), but the code is now duplicated between two TUs. Worth extracting into MqttSettings.{h,cpp} as a single migrateLegacyMqttPasswordIfPresent() helper if you'd like to consolidate.
MqttClient::setSubscriptions always re-subscribes: when called on an already-connected client, it unsubscribes removed topics (good), but then re-issues mosquitto_subscribe for the entire desired set including topics already subscribed (src/core/MqttClient.cpp:171-185). Mosquitto handles this fine, but you could diff against previous to skip the no-op subscribes — minor.
Settings-dialog Apply while connected: if the user changes host/port/user/TLS via the dialog while currently connected, MainWindow::showMqttSettingsDialog updates subscriptions on the live client but the broker connection isn't torn down — so credential/host changes need a manual Off/On to take effect. Subscriptions changing live is great; consider documenting (or surfacing in the dialog) that broker-connection fields require Off/On.
Minor — keychain async race in the dialog: MqttSettingsDialog::loadPasswordFromKeychain() populates m_passEdit from the async read callback. If a user opens the dialog and types into the password field before the callback fires, the typed value gets overwritten. Guard with if (!m_passEdit->isModified()) or similar.
The overall structure (Broker / Subscriptions / Publish Buttons tabs, automatic antenna-alias topics kept out of operator-removable state, MqttEnabled persistence + keychain-aware deferred restore) is solid. Help doc updates accurately reflect the new flow.
🤖 aethersdr-agent · cost: $15.0004 · model: claude-opus-4-7
setSubscriptions now subscribes only to topics in `desired` that were not in `previous`, instead of subscribing to every desired topic on every settings change. Behaviour-equivalent (mosquitto_subscribe is idempotent broker-side), but pinning the diff turns a 20-topic stable list with one added topic into 1 SUBSCRIBE packet + 1 SUBACK round-trip instead of 20. Matters most for users with large topic catalogues on slow links (e.g. SmartLink WAN sessions where every round-trip is a real cost). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…(Principle V)
Replaces the per-field flat-key persistence pattern with a single nested
JSON object stored under the new "MqttSettings" key:
{
"host": "localhost",
"port": 1883,
"user": "...",
"tls": false,
"caFile": "...",
"topics": [{"topic": "...", "displayOnPan": ...}, ...],
"buttons": [{"label": "...", "topic": "...", "payload": "..."}, ...],
"enabled": false
}
Matches the Principle V direction the recently-shipped FlexControl
wheel migration (#3029) established for related settings — keep the
related fields in one nested JSON block rather than scattered top-
level keys.
The public API is unchanged. Callers (MqttSettingsDialog, MqttApplet)
continue to use the same per-field load/save functions; the
consolidation happens entirely inside MqttSettings.cpp behind two new
internal helpers `mqttSettingsObject()` and `saveMqttSettingsObject()`.
Migration: one-shot. `mqttSettingsObject()` falls back to reading
the 8 legacy flat keys (`MqttHost`, `MqttPort`, `MqttUser`, `MqttTls`,
`MqttCaFile`, `MqttTopics`, `MqttButtons`, `MqttEnabled`) when the new
"MqttSettings" key is absent. On the next save, the new JSON key is
written and the legacy keys are removed from AppSettings. Existing
users' settings carry over transparently.
The password is *not* part of this object — it lives in the system
keychain with a legacy fallback at "MqttPass", and stays untouched by
this consolidation.
All 13 existing mqtt_settings_test cases still pass (parsing /
serialization / subscription dedup / button JSON round-trip are
unchanged — only the AppSettings backing layer moved). Full AetherSDR
target build clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@s53zo — thanks for this work, the architecture is solid and the independent-review / self-found-fix on the subscription-state bug was exactly the right shape. Per project review I pushed two small maintainer-followup commits on top of your branch (`maintainerCanModify` was on, so they land directly here): `89210ab9` — `perf(mqtt): only subscribe to the diff in setSubscriptions` `setSubscriptions` was re-subscribing to every desired topic on every settings change. Mosquitto is idempotent broker-side so it's correct, but it's wasteful — a 20-topic stable list with one added topic emitted 20 SUBSCRIBE packets instead of 1. The fix is 3 lines: skip topics already in `previous`. Matters most for slow links (SmartLink WAN sessions where every round-trip is a real cost). `576cab8d` — `refactor(mqtt): consolidate 8 flat AppSettings keys into nested JSON (Principle V)` Per AetherSDR's Principle V (related settings nest under one JSON key), the eight MQTT flat keys (`MqttHost`, `MqttPort`, `MqttUser`, `MqttTls`, `MqttCaFile`, `MqttTopics`, `MqttButtons`, `MqttEnabled`) are now consolidated under a single `MqttSettings` key holding a JSON object. The recent FlexControl wheel migration (#3029) established this pattern. Important: I kept your public API identical — every existing `loadXxx`/`saveXxx` function still works exactly the same from the dialog's and applet's perspective. The consolidation happens entirely inside `MqttSettings.cpp` behind two new internal helpers (`mqttSettingsObject()` and `saveMqttSettingsObject()`). MqttSettingsDialog and MqttApplet didn't need any changes. Migration: one-shot transparent upgrade. `mqttSettingsObject()` falls back to reading the 8 legacy flat keys when the new `MqttSettings` key is absent. On the next save, the JSON key is written and the legacy keys are removed from AppSettings. Existing users' settings carry over with no visible change. The password stays untouched — it's in the keychain (with the legacy `MqttPass` fallback you preserved), and that's the right place for it. All 13 of your `mqtt_settings_test` cases still pass against the new backing layer. Full AetherSDR target builds clean. The live-broker smoke test you flagged in the PR description is still pending — once the maintainer runs it (configure, connect, restart, add/remove topics, confirm broker side via `mosquitto_sub -v -t '#'`), the PR is ready to merge. 73, Jeremy KK7GWY & Claude (AI dev partner) |
|
Thanks Jeremy. I reviewed both follow-up commits and the aethersdr-agent review notes.
For the live-broker smoke test, I would run:
|
## Summary Release prep for v26.5.3 — Aetherial Audio TX completion + security hardening + 100-commit reliability sweep. - **CHANGELOG.md** — new v26.5.3 section: 138 commits across 14 contributors since v26.5.2.1 - **README.md** — version line bump 26.5.2.1 → 26.5.3 - **CMakeLists.txt** — already at 26.5.3 (set in #3024) ## Highlights - **Aetherial Audio TX completion** — PAPR all-pass cascade + split-band DESS, full per-stage CHAIN UI - **Security advisories** — GHSA-wfx7-w6p8-4jr2 (WAN cert-pin Phase 2, #3026) + GHSA-qxhr-cwrc-pvrm (CAT PTY symlink, #3027) - **MQTT settings dialog refactor** (#3051, @s53zo) — JSON-array topics + button-event lists, nested-JSON consolidation - **Audio reliability sweep** — WASAPI silent-open watchdog, mono-only USB PnP mic recovery, MQTT subscribe diff - **Native Hamlib NET rigctl** — eliminates external rigctld dependency - **Stream Deck integration** — Elgato SDK plugin (Win/Mac/Linux-via-OpenDeck) ## Contributors thanked @jensenpat (16), @aethersdr-agent (22 bot), @NF0T (12), @rfoust (9), @Ozy311 (5), @M7HNF-Ian (5), @chibondking (5), @M8WLO (4), @s53zo (2), @pepefrog1234 (2), @K5PTB (2), @chrisb1964 (1) ## Test plan - [ ] CI green (build + check-paths + check-windows) - [ ] CHANGELOG renders correctly on GitHub - [ ] README version banner shows 26.5.3 - [ ] After merge: tag v26.5.3 once CI green on main 73, Jeremy KK7GWY & Claude (AI dev partner) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## Summary - Add a persistent `Settings -> MQTT...` dialog for broker credentials, TLS/CA config, subscription rows, display-on-panadapter flags, and publish button definitions. - Keep the MQTT applet as the live surface for connect/disconnect, status, message log, publish buttons, and a shortcut back to settings. - Auto-subscribe AetherSDR antenna alias API topics on every MQTT connection while preserving legacy `MqttTopics` and `MqttButtons` settings. - Persist the MQTT applet On/Off state as `MqttEnabled` so MQTT reconnects automatically on startup when it was left On. - Replace stale pending-topic behavior with explicit desired subscriptions so removed user topics do not survive reconnects. ## Verification - `cmake --build build --target AetherSDR -j4` - `cmake --build build --target mqtt_settings_test mqtt_antenna_alias_test antenna_alias_test -j4` - `ctest --test-dir build -R 'mqtt_antenna_alias_test|antenna_alias_test|mqtt_settings_test' --output-on-failure` - `git diff --check` ## Independent Review - Separate review pass found one blocking subscription-state issue: removed topics could remain in `MqttClient` pending subscriptions across reconnects. - Follow-up runtime report found MQTT did not auto-connect on application start. - Fixed by adding explicit desired subscriptions plus startup restoration that waits until the keychain password load is complete. ## Remaining Risk - Manual GUI startup was reported before this follow-up fix; the auto-connect fix has local build/test coverage but still needs a live-broker restart smoke test from the app. --------- Co-authored-by: Jeremy Fielder <kk7gwy@aethersdr.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
## Summary Release prep for v26.5.3 — Aetherial Audio TX completion + security hardening + 100-commit reliability sweep. - **CHANGELOG.md** — new v26.5.3 section: 138 commits across 14 contributors since v26.5.2.1 - **README.md** — version line bump 26.5.2.1 → 26.5.3 - **CMakeLists.txt** — already at 26.5.3 (set in aethersdr#3024) ## Highlights - **Aetherial Audio TX completion** — PAPR all-pass cascade + split-band DESS, full per-stage CHAIN UI - **Security advisories** — GHSA-wfx7-w6p8-4jr2 (WAN cert-pin Phase 2, aethersdr#3026) + GHSA-qxhr-cwrc-pvrm (CAT PTY symlink, aethersdr#3027) - **MQTT settings dialog refactor** (aethersdr#3051, @s53zo) — JSON-array topics + button-event lists, nested-JSON consolidation - **Audio reliability sweep** — WASAPI silent-open watchdog, mono-only USB PnP mic recovery, MQTT subscribe diff - **Native Hamlib NET rigctl** — eliminates external rigctld dependency - **Stream Deck integration** — Elgato SDK plugin (Win/Mac/Linux-via-OpenDeck) ## Contributors thanked @jensenpat (16), @aethersdr-agent (22 bot), @NF0T (12), @rfoust (9), @Ozy311 (5), @M7HNF-Ian (5), @chibondking (5), @M8WLO (4), @s53zo (2), @pepefrog1234 (2), @K5PTB (2), @chrisb1964 (1) ## Test plan - [ ] CI green (build + check-paths + check-windows) - [ ] CHANGELOG renders correctly on GitHub - [ ] README version banner shows 26.5.3 - [ ] After merge: tag v26.5.3 once CI green on main 73, Jeremy KK7GWY & Claude (AI dev partner) 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Settings -> MQTT...dialog for broker credentials, TLS/CA config, subscription rows, display-on-panadapter flags, and publish button definitions.MqttTopicsandMqttButtonssettings.MqttEnabledso MQTT reconnects automatically on startup when it was left On.Verification
cmake --build build --target AetherSDR -j4cmake --build build --target mqtt_settings_test mqtt_antenna_alias_test antenna_alias_test -j4ctest --test-dir build -R 'mqtt_antenna_alias_test|antenna_alias_test|mqtt_settings_test' --output-on-failuregit diff --checkIndependent Review
MqttClientpending subscriptions across reconnects.Remaining Risk