Add network diagnostics trend graphs#2309
Conversation
There was a problem hiding this comment.
Pull request overview
This PR upgrades the existing Network Diagnostics dialog into a multi-tab UI with lightweight, Qt-painted trend graphs and introduces a background history sampler so recent metrics are available immediately when the dialog opens.
Changes:
- Add a
NetworkDiagnosticsHistorybackground sampler that records 1s diagnostics samples and prunes/compacts history up to 1 week. - Rework
NetworkDiagnosticsDialoginto Overview/Details plus dedicated metric graph tabs, with timeframe selection and clickable legends. - Refresh the main window’s network tooltip while hovered and wire the dialog to use the shared diagnostics history.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/gui/NetworkDiagnosticsDialog.h | Adds time-series sample/history types and extends the dialog to consume shared history plus graph widgets. |
| src/gui/NetworkDiagnosticsDialog.cpp | Implements Qt-painted time-series graphs, dialog UI rework, background sampling, and chart bucketing logic. |
| src/gui/MainWindow.h | Adds a persistent NetworkDiagnosticsHistory* and a tooltip refresh timer member. |
| src/gui/MainWindow.cpp | Instantiates/cleans up diagnostics history, passes it into the dialog, and adds live tooltip refresh behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for (PanadapterStream::StreamCategory cat : cats) { | ||
| PanadapterStream::CategoryStats cs = m_model->categoryStats(cat); | ||
| const qint64 delta = cs.bytes - m_lastCatBytes[cat]; | ||
| m_lastCatBytes[cat] = cs.bytes; | ||
| const double rateKbps = kbpsFromBytes(delta); |
| NetworkDiagnosticsSample& bucket = compacted.last(); | ||
| bucket.rttMs = std::max(bucket.rttMs, sample.rttMs); | ||
| bucket.audioGapMs = std::max(bucket.audioGapMs, sample.audioGapMs); | ||
| bucket.audioJitterMs = std::max(bucket.audioJitterMs, sample.audioJitterMs); | ||
| bucket.packetLossPct = std::max(bucket.packetLossPct, sample.packetLossPct); | ||
| bucket.audioLossPct = std::max(bucket.audioLossPct, sample.audioLossPct); | ||
| bucket.fftLossPct = std::max(bucket.fftLossPct, sample.fftLossPct); |
| const qint64 curRx = m_model->rxBytes(); | ||
| sample.rxKbps = kbpsFromBytes(curRx - m_lastRxBytes); | ||
| m_lastRxBytes = curRx; | ||
| const qint64 curTx = m_model->txBytes(); | ||
| sample.txKbps = kbpsFromBytes(curTx - m_lastTxBytes); | ||
| m_lastTxBytes = curTx; |
| return QString("Last %1 minutes").arg(m_rangeSeconds / 60); | ||
| } | ||
| if (m_rangeSeconds < 86400) { | ||
| return QString("Last %1 hours").arg(m_rangeSeconds / 3600); | ||
| } | ||
| return QString("Last %1 days").arg(m_rangeSeconds / 86400); |
|
Hi @rfoust — thanks for the contribution! Quick heads-up on the red ❌ you're seeing: the only failing check is The cleanup step occasionally fails due to transient API/permission issues on GitHub's side and is safe to ignore for the purposes of reviewing your patch. No action needed from you on that front. While you're waiting on the build, Copilot left some review comments worth a look — none of them block merge but a couple are real bugs that would be nice to address in this PR rather than as follow-ups:
#2 in particular is worth doing before this lands — TSAN/UBSAN runs would eventually flag it and it's much cheaper to fix now than to debug a flaky data corruption later. Really nice feature overall — the historical view is going to be super useful for debugging stream issues. Thanks for taking the time on it! 🙌 |
|
Addressed the actionable review feedback in
Verification:
|
![aethersdr-agent[bot]](https://avatars.githubusercontent.com/in/3286087?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks @rfoust — this is a substantial, well-structured contribution. Continuous background sampling, bounded history, clock-aligned bucketing, and the thread-safety hardening on PanadapterStream are all nicely done. The scope is consistent with the PR description.
Copilot findings — verification
Tracing the actual code in this revision, all four Copilot comments appear to be false positives:
-
"Rate calculations assume a perfect 1s interval" — incorrect.
kbpsFromBytes()atsrc/gui/NetworkDiagnosticsDialog.cpp:721already takeselapsedSeconds, andsampleNow()computes it (src/gui/NetworkDiagnosticsDialog.cpp:784) and divides every byte-delta by it. Negative deltas are clamped viastd::max<qint64>(0, ...). Looks like Copilot was reading an earlier diff revision. -
"pruneSamples bucket merge only updates latency/loss" — incorrect. Lines
881–887explicitly averagerxKbps,txKbps,audioKbps,fftKbps,waterfallKbps,meterKbps, anddaxKbpsviamergeAverage. (Minor nit:audioBufferMsis overwritten with the latest sample rather than averaged, but that seems intentional since it's an instantaneous level, not a rate.) -
"Data race on PanadapterStream counters" — addressed in this PR.
m_totalRxBytes/m_totalTxBytesare nowstd::atomic<qint64>,m_statsMutexguardsm_catStatsandm_streamStats, andcategoryStats()returns a value under lock. The signature change fromconst CategoryStats&toCategoryStatsis safe — all existing callers either copy or read fields immediately. -
"
rangeLabel()always plural" — incorrect. The function atsrc/gui/NetworkDiagnosticsDialog.cpp:236-248already special-cases== 1for minute/hour/day.
A few things worth a look
-
AudioEngine cross-thread access in
NetworkDiagnosticsHistory::sampleNow(): I checked andrxBufferBytes(),rxBufferUnderrunCount(),rxBufferSampleRate()are allstd::atomicreads inAudioEngine.h:660-663, so polling from the GUI thread is safe. No change needed — just confirming. -
Destructor ordering in
MainWindow::~MainWindow(src/gui/MainWindow.cpp:3793-3801): the comment and explicit deletes ahead of audio teardown are correct and necessary becauseWA_DeleteOnCloseonly fires on close, and Qt parent-driven destruction would otherwise occur after the body of the destructor runs. Good catch. -
Tooltip refresh timer: minor — the
Enter/Leave/ToolTipfilter handlers don'treturn trueforEnter/Leave, so the events still propagate. That's fine here, but ifm_networkLabelever installs a tooltip via stylesheet/eventfilter on a child, the redundantsetToolTipfrom both the timer lambda and theToolTipevent could double up. Not a bug, just something to be aware of.
Build clean, scope appropriate, conventions look consistent (RAII, atomics, mutex-locked snapshots, no QSettings usage). Nice work.
Summary
Testing
cmake --build build -j8git diff --check