Skip to content

feat(tci): Network dialog TCI tab — connected clients + live traffic monitor#2879

Open
nigelfenton wants to merge 2 commits into
aethersdr:mainfrom
nigelfenton:feat/tci-client-list
Open

feat(tci): Network dialog TCI tab — connected clients + live traffic monitor#2879
nigelfenton wants to merge 2 commits into
aethersdr:mainfrom
nigelfenton:feat/tci-client-list

Conversation

@nigelfenton
Copy link
Copy Markdown
Contributor

@nigelfenton nigelfenton commented May 19, 2026

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, ::1127.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 ''
    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

  • Builds clean on Windows (MSVC, Ninja); app launches/stable
  • Clients table populates; live connect/leave; role inference correct
  • Name alias persists by IP across restart — verified on rig
  • 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)
  • Build clean on macOS (Darwin 25 arm64, brew Qt6 — 651/651, no
    PR-introduced warnings)
  • Build clean on Linux (Ubuntu 24.04 noble, GCC, Qt6 6.4.2 — 638/638,
    no PR-introduced warnings)
  • ctest --output-on-failure on Linux: 15/15 passed, no
    regressions from the new signals/accessor
  • !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.

@nigelfenton nigelfenton changed the title feat(tci): Radio Setup -> TCI tab listing connected clients feat(tci): Network dialog -> TCI tab listing connected clients May 19, 2026
@nigelfenton nigelfenton changed the title feat(tci): Network dialog -> TCI tab listing connected clients feat(tci): Network dialog TCI tab — connected clients + live traffic monitor May 19, 2026
@nigelfenton nigelfenton force-pushed the feat/tci-client-list branch 2 times, most recently from d161edb to acd7369 Compare May 20, 2026 01:24
@nigelfenton nigelfenton marked this pull request as ready for review May 20, 2026 01:32
Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

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

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_tciServer member in MainWindow.h:377 is already #ifdef HAVE_WEBSOCKETS-guarded, so the new call site in MainWindow::showNetworkDiagnosticsDialog is safe.
  • IPv4-mapped IPv6 / ::1 normalisation in connectedClients() gives a stable alias key — good catch.
  • QSignalBlocker around the programmatic repopulation so itemChanged only 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_tci outlives the dialog (owned by MainWindow), 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 (each tciAliasSet). For typical use this is a non-issue, but if you wanted to be defensive a QTimer::singleShot(0, ...) coalesce would batch a flurry of edits into one disk write. Skip if not bothered.
  • tciSuppress serialises a QSet<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.

Copy link
Copy Markdown
Contributor

@aethersdr-agent aethersdr-agent Bot left a comment

Choose a reason for hiding this comment

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

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 under TciMonitorSuppressed) 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, ::1127.0.0.1) makes the alias key stable across socket-level oddities. Good detail.
  • QSignalBlocker in refreshTciClientTable correctly distinguishes programmatic repopulation from genuine user edits to the Name cell.
  • Rolling 20k row cap on the monitor table is appropriate and bounded.
  • !HAVE_WEBSOCKETS stubs at the bottom of the .cpp keep the symbol surface stable without leaking the type into the compiled-out path.
  • Thread safety is fine — TciServer is created with this parent on the main thread (MainWindow.cpp:4264), so signal delivery to the dialog is same-thread/direct.

Suggestions (non-blocking)

  1. tciMessage docstring is slightly misleading. The header comment says "high-rate per-client telemetry sends are intentionally not emitted," but broadcast() is also used for the ~5 Hz rx_smeter broadcast (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 like rx_smeter are emitted but easily suppressed." Small thing, but it'll prevent future confusion about why the monitor lights up so much by default.

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

  3. QSet iteration order in tciSuppress produces non-deterministic JSON ordering on each save. Harmless functionally, but if you want stable settings diffs, a QStringList sorted before serialisation would do it. Minor.

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

nigelfenton added a commit to nigelfenton/AetherSDR that referenced this pull request May 20, 2026
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>
nigelfenton and others added 2 commits May 21, 2026 22:17
…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>
@nigelfenton nigelfenton force-pushed the feat/tci-client-list branch from 4bb59d8 to 3e06204 Compare May 22, 2026 02:18
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.

1 participant