feat(tci): Network dialog TCI tab — connected clients + live traffic monitor#2879
Conversation
d161edb to
acd7369
Compare
There was a problem hiding this comment.
Thanks @nigelfenton — this is a really well-built PR. Conventions look right, comments explain the why (the IP-as-key/AppSettings dotted-path note in particular is exactly the kind of context that prevents this getting "fixed" wrong later), and the cross-platform verification matrix in the description is appreciated.
A few small notes, none blocking:
Looks good
- AppSettings used throughout (not QSettings); JSON-blob-under-one-valid-key is the right workaround for the dotted-path constraint.
m_tciServermember inMainWindow.h:377is already#ifdef HAVE_WEBSOCKETS-guarded, so the new call site inMainWindow::showNetworkDiagnosticsDialogis safe.- IPv4-mapped IPv6 /
::1normalisation inconnectedClients()gives a stable alias key — good catch. QSignalBlockeraround the programmatic repopulation soitemChangedonly ever fires on real user edits — correct.connect(m_tci, ..., this, ...)uses the 5-arg form, so the connections tear down automatically when the dialog dies.m_tcioutlives the dialog (owned byMainWindow), so the raw pointer is fine.- 20k-row rolling cap on the monitor — good bound on memory.
Minor nits (optional, not blocking)
tciAliasMap()re-parses the JSON on every refresh and every edit. Tiny data, fine in practice — flagging only because if alias counts ever grew, caching on the dialog would be trivial.AppSettings::instance().save()fires after every keystroke-completion on the Name column (eachtciAliasSet). For typical use this is a non-issue, but if you wanted to be defensive aQTimer::singleShot(0, ...)coalesce would batch a flurry of edits into one disk write. Skip if not bothered.tciSuppressserialises aQSet<QString>to a JSON array — order is non-deterministic, which means the on-disk file will churn on every save even if logically unchanged. Sorting before serialise (QStringList s(m_tciSuppressed); s.sort();) would make the settings file diff-friendly.
On your open question (offline clients with curated names): I'd defer it. Showing live-only is the right default for v1 — it keeps the UI honest about what's actually connected right now, which is the primary use case (diagnose who's hammering the radio). The "curate names anytime" workflow can wait until a user actually asks for it; deferring also avoids you having to design a forget/expire UX in the same PR. The JSON map already preserves names across disconnect/reconnect, which is the part that matters.
The !HAVE_WEBSOCKETS upstream-link-failure flag at MainWindow.cpp:13993 is a fair callout — definitely a separate fix, agree it shouldn't be bundled here.
Nice work.
There was a problem hiding this comment.
Thanks @nigelfenton — nice contribution. The design is well thought through and the PR description does a great job explaining the non-obvious choices.
What looks good
- AppSettings usage is correct. Storing the alias map as a single JSON value under
TciClientAliases(and the suppression list underTciMonitorSuppressed) is the right call — the comment explaining why per-IP keys can't be used with the XML/dotted-path backing is exactly the kind of WHY-comment that's worth keeping. - Peer address normalisation in
connectedClients()(IPv4-mapped IPv6 → IPv4,::1→127.0.0.1) makes the alias key stable across socket-level oddities. Good detail. QSignalBlockerinrefreshTciClientTablecorrectly distinguishes programmatic repopulation from genuine user edits to the Name cell.- Rolling 20k row cap on the monitor table is appropriate and bounded.
!HAVE_WEBSOCKETSstubs at the bottom of the .cpp keep the symbol surface stable without leaking the type into the compiled-out path.- Thread safety is fine —
TciServeris created withthisparent on the main thread (MainWindow.cpp:4264), so signal delivery to the dialog is same-thread/direct.
Suggestions (non-blocking)
-
tciMessagedocstring is slightly misleading. The header comment says "high-rate per-client telemetry sends are intentionally not emitted," butbroadcast()is also used for the ~5 Hzrx_smeterbroadcast (TciServer.cpp:1407), so that does emit. The user-facing escape hatch is the per-command suppression — which is exactly what your test plan verifies. Consider tightening the doc to "binary audio/IQ frames are not emitted; high-rate text broadcasts likerx_smeterare emitted but easily suppressed." Small thing, but it'll prevent future confusion about why the monitor lights up so much by default. -
Open question on offline clients with saved names: I'd defer to a follow-up. Showing greyed-offline entries is friendly but pulls in lifecycle/expiry UI that doesn't fit this PR's scope. Live-only is the right default for v1 — if users ask for offline curation later, it can land standalone.
-
QSetiteration order intciSuppressproduces non-deterministic JSON ordering on each save. Harmless functionally, but if you want stable settings diffs, aQStringListsorted before serialisation would do it. Minor. -
Mid-edit refresh: if
clientsChanged()fires while the user is mid-edit of a Name cell, the in-progress text is replaced. Unlikely in practice (clientsChanged fires on connect/disconnect/stream toggles, not on a timer), and not a regression — just noting.
No correctness or AppSettings/RAII issues found. Cross-platform testing matrix is thorough. The Open Question and the MainWindow.cpp:13993 upstream finding are both well-flagged and not yours to fix in this PR.
Nice work.
Addresses two non-blocking review nits on PR aethersdr#2879: - TciServer.h: tighten the tciMessage signal docstring so it is clear that binary audio/IQ frames are never emitted, but high- rate text broadcasts (e.g. rx_smeter) ARE emitted and are intended to be muted via the monitor's per-command suppression list. - NetworkDiagnosticsDialog.cpp::tciSuppress: serialise the suppression set via a sorted QStringList instead of iterating the QSet directly. QSet iteration order is non-deterministic, which was churning the TciMonitorSuppressed JSON value on every save even when the contents were logically unchanged. No runtime behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…monitor
Adds a "TCI" tab to Settings → Network (NetworkDiagnosticsDialog) with:
- A compact connected-clients table (~6 rows then scroll): endpoint,
inferred role, audio/IQ/RX+TX-sensor subscriptions, and an editable
Name column. Aliases are stored under one valid AppSettings key as a
JSON {ip:name} map (an IP cannot be an AppSettings/XML-dotted key —
segments start with a digit and IPv6 has ':'). Peer addresses are
normalised (IPv4-mapped IPv6 → IPv4, ::1 → 127.0.0.1) so the alias
key is stable.
- A live TCI traffic monitor below: 3-column colour-coded table
(Time / Dir+Cmd / Message), bounded to a rolling 20k rows. Pause,
Clear, Save log…, and a right-click context menu offering
"Suppress all '<cmd>' messages" (drops future and removes existing
rows of that command) and Remove selected row(s). Suppressed
commands persist (JSON array under one valid AppSettings key) with
a "Suppressed: …" indicator + Clear-suppressions button. Mirrors
the interaction model and palette of the standalone TCI Monitor.
TciServer additions (no behavioural change to existing handling):
- connectedClients() snapshot accessor.
- clientsChanged() signal — connect / disconnect / audio start-stop.
- tciMessage(direction,text) signal — one emission per inbound
command and per outbound broadcast (high-rate per-client telemetry
sends are intentionally not emitted to keep the stream readable).
MainWindow::showNetworkDiagnosticsDialog() passes the TciServer to
the dialog, guarded for !HAVE_WEBSOCKETS builds.
No TCI protocol change; no change to any client. Out of scope for
this PR (deferred): routing TCI traffic into the Network dialog's
Logs tab.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Addresses two non-blocking review nits on PR aethersdr#2879: - TciServer.h: tighten the tciMessage signal docstring so it is clear that binary audio/IQ frames are never emitted, but high- rate text broadcasts (e.g. rx_smeter) ARE emitted and are intended to be muted via the monitor's per-command suppression list. - NetworkDiagnosticsDialog.cpp::tciSuppress: serialise the suppression set via a sorted QStringList instead of iterating the QSet directly. QSet iteration order is non-deterministic, which was churning the TciMonitorSuppressed JSON value on every save even when the contents were logically unchanged. No runtime behaviour change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4bb59d8 to
3e06204
Compare
|
Claude here — merged. Thanks @nigelfenton, this is a substantial What stood out on review: The AppSettings key-constraint workaround was specifically called The architectural boundary is clean: read-only accessor + The second commit (`3e062044` "clarify tciMessage doc + sort On the open question: ship-as-is is the right call. Names already Ships in v26.5.3. 73, |
… + @G6PWY-Chris (#3067) Two attribution errors in the v26.5.3 release CHANGELOG: 1. @M7HNF-Ian (Ian, separate GitHub account) → @nigelfenton (Nigel Fenton, G0JKN) — Nigel authored all 5 of the commits I bucketed under that line: TCI Network dialog tab (#2879), tx_gain + ALC (#2950), use-after-free fix (#2946), vfo emit on band-change (#2828), mic-capture suppress during TCI feed (#2982). 2. @chrisb1964 → @G6PWY-Chris (Chris G6PWY) — single commit author in the cycle was the latter; chrisb1964 is unrelated. Reported by Nigel directly. All other handles verified against gh api repos/.../commits/<sha>. GitHub release notes already updated; this lands the same fix in the in-repo CHANGELOG. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…monitor (aethersdr#2879) ## Summary Adds a **TCI** tab to **Settings → Network** (`NetworkDiagnosticsDialog`). One page: 1. **Connected clients** — compact table (~6 rows then scroll): endpoint, inferred role, stream subscriptions, editable **Name** alias (local JSON `{ip:name}` map; per-IP keys would be invalid in AppSettings' XML/dotted path — IPs start with a digit and IPv6 has `:`). Peer address normalised (IPv4-mapped IPv6 → IPv4, `::1` → `127.0.0.1`) so the alias key is stable. 2. **Live TCI traffic monitor** — colour-coded table of bidirectional traffic. **Pause**, then **right-click a row → "Suppress all '<cmd>' messages"** (drops future + removes existing of that command); also Remove row(s) / Clear / **Save log…**. Suppressions persist (JSON array under one valid AppSettings key) with a "Suppressed: …" indicator + a Clear-suppressions button. Mirrors the standalone TCI Monitor's interaction model and palette. **No TCI protocol change.** A read accessor + two signals drive the UI; existing TCI handling untouched. > Squashed to one clean commit before requesting review. ## Open question for review A client with a saved Name but currently **offline** is not shown (live connections only). Show greyed "offline" so names can be curated anytime? Trade-off: list lifecycle (expiry/forget UI). Flagged for a steer. ## Implementation - `TciServer`: `connectedClients()`; `clientsChanged()` (connect/disconnect, audio start-stop, **IQ start-stop, RX/TX sensor enable-disable**) drives the client table — so every column live-refreshes; `tciMessage(dir,text)` for each inbound command and each outbound broadcast (one per logical message; high-rate per-client telemetry sends not emitted) drives the monitor. - `NetworkDiagnosticsDialog`: `TCI` tab (only when a `TciServer` exists). Monitor table bounded to a rolling 20k rows. - `MainWindow::showNetworkDiagnosticsDialog()` passes the `TciServer`, guarded for `!HAVE_WEBSOCKETS`. Out of scope (deferred): TCI traffic into the Network Logs tab. ## Test plan - [x] Builds clean on Windows (MSVC, Ninja); app launches/stable - [x] Clients table populates; live connect/leave; role inference correct - [x] Name alias persists by IP across restart — verified on rig - [x] Monitor: live colour-coded rx/tx rows; Pause; right-click suppress drops+removes that command; suppressions persist; Save log — verified on rig (rx_smeter suppressed live, indicator shown) - [x] Build clean on macOS (Darwin 25 arm64, brew Qt6 — 651/651, no PR-introduced warnings) - [x] Build clean on Linux (Ubuntu 24.04 noble, GCC, Qt6 6.4.2 — 638/638, no PR-introduced warnings) - [x] `ctest --output-on-failure` on Linux: **15/15 passed**, no regressions from the new signals/accessor - [x] `!HAVE_WEBSOCKETS` build verified for this PR: all of TciServer.cpp, NetworkDiagnosticsDialog.cpp and RadioSetupDialog.cpp compile clean; full link blocked solely by a pre-existing upstream unguarded `m_tciServer` use at `MainWindow.cpp:13993` on `main` (identical to ours) — flagging for separate upstream fix, not bundled here ### Known limitations honestly noted - macOS / Linux **runtime** not exercised — both verified at build level only (no GUI on the headless Linux build host, no Mac in front of me). Windows runtime is where the feature path was driven end-to-end. > Verified on live hardware: Windows 11 (full feature path on the radio), > macOS arm64 build, Linux build + ctest. All cross-platform gates closed. > Ready for review. --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… + @G6PWY-Chris (aethersdr#3067) Two attribution errors in the v26.5.3 release CHANGELOG: 1. @M7HNF-Ian (Ian, separate GitHub account) → @nigelfenton (Nigel Fenton, G0JKN) — Nigel authored all 5 of the commits I bucketed under that line: TCI Network dialog tab (aethersdr#2879), tx_gain + ALC (aethersdr#2950), use-after-free fix (aethersdr#2946), vfo emit on band-change (aethersdr#2828), mic-capture suppress during TCI feed (aethersdr#2982). 2. @chrisb1964 → @G6PWY-Chris (Chris G6PWY) — single commit author in the cycle was the latter; chrisb1964 is unrelated. Reported by Nigel directly. All other handles verified against gh api repos/.../commits/<sha>. GitHub release notes already updated; this lands the same fix in the in-repo CHANGELOG. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Adds a TCI tab to Settings → Network (
NetworkDiagnosticsDialog).One page:
inferred role, stream subscriptions, editable Name alias (local JSON
{ip:name}map; per-IP keys would be invalid in AppSettings' XML/dottedpath — IPs start with a digit and IPv6 has
:). Peer address normalised(IPv4-mapped IPv6 → IPv4,
::1→127.0.0.1) so the alias key is stable.traffic. Pause, then right-click a row → "Suppress all ''
messages" (drops future + removes existing of that command); also
Remove row(s) / Clear / Save log…. Suppressions persist (JSON array
under one valid AppSettings key) with a "Suppressed: …" indicator + a
Clear-suppressions button. Mirrors the standalone TCI Monitor's
interaction model and palette.
No TCI protocol change. A read accessor + two signals drive the UI;
existing TCI handling untouched.
Open question for review
A client with a saved Name but currently offline is not shown (live
connections only). Show greyed "offline" so names can be curated anytime?
Trade-off: list lifecycle (expiry/forget UI). Flagged for a steer.
Implementation
TciServer:connectedClients();clientsChanged()(connect/disconnect,audio start-stop, IQ start-stop, RX/TX sensor enable-disable) drives
the client table — so every column live-refreshes;
tciMessage(dir,text)for each inbound command and each outbound broadcast (one per logical
message; high-rate per-client telemetry sends not emitted) drives the
monitor.
NetworkDiagnosticsDialog:TCItab (only when aTciServerexists).Monitor table bounded to a rolling 20k rows.
MainWindow::showNetworkDiagnosticsDialog()passes theTciServer,guarded for
!HAVE_WEBSOCKETS.Out of scope (deferred): TCI traffic into the Network Logs tab.
Test plan
drops+removes that command; suppressions persist; Save log — verified
on rig (rx_smeter suppressed live, indicator shown)
PR-introduced warnings)
no PR-introduced warnings)
ctest --output-on-failureon Linux: 15/15 passed, noregressions from the new signals/accessor
!HAVE_WEBSOCKETSbuild verified for this PR: all of TciServer.cpp,NetworkDiagnosticsDialog.cpp and RadioSetupDialog.cpp compile clean;
full link blocked solely by a pre-existing upstream unguarded
m_tciServeruse atMainWindow.cpp:13993onmain(identical toours) — flagging for separate upstream fix, not bundled here
Known limitations honestly noted
only (no GUI on the headless Linux build host, no Mac in front of me).
Windows runtime is where the feature path was driven end-to-end.