feat(gps): Comprehensive GPS/RTK/NTRIP subsystem improvements#14112
feat(gps): Comprehensive GPS/RTK/NTRIP subsystem improvements#14112HTRamsey wants to merge 1 commit intomavlink:masterfrom
Conversation
HTRamsey
force-pushed
the
feature/gps-updates
branch
2 times, most recently
from
bd106e6 to
ace132e
Compare
Build ResultsPlatform Status
Some builds failed. Pre-commit
Pre-commit hooks: 4 passed, 36 failed, 7 skipped. Artifact Sizes
Updated: 2026-04-17 01:30:49 UTC • Triggered by: Android |
HTRamsey
force-pushed
the
feature/gps-updates
branch
17 times, most recently
from
10839e3 to
3c34872
Compare
There was a problem hiding this comment.
Pull request overview
This PR substantially refactors and expands QGroundControl’s GPS subsystem, splitting responsibilities into RTK and NTRIP components, adding new abstractions for transports and routing, and integrating new status/controls into the UI and QML globals.
Changes:
- Introduces new RTK/NTRIP architecture components (RTCM routing, MAVLink RTCM fragmentation + stats, reconnect policy, connection stats, UDP forwarder) and adds extensive unit/integration/E2E test coverage.
- Extends vehicle GPS FactGroups and metadata (altitudes, accuracies, speeds, and GNSS integrity enums) and updates toolbar indicator behavior/badges.
- Adds/updates QML settings pages and supporting reusable QML controls, plus new RTK base station map overlay.
Reviewed changes
Copilot reviewed 149 out of 152 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
src/QmlControls/QGroundControlQmlGlobal.h |
Exposes new GPS-related objects to QML (gpsManager, gpsRtk fact group, models). |
src/QmlControls/QGroundControlQmlGlobal.cc |
Wires QML globals to GPSManager singletons and models in ctor. |
src/GPS/GPSManager.h/.cc |
Implements multi-device RTK management, RTCM router/mavlink, and event log plumbing. |
src/GPS/NTRIP/NTRIPReconnectPolicy.cc |
Adds exponential-backoff reconnect logic for NTRIP. |
test/GPS/NTRIP/NTRIPHttpTransportTest.cc |
Updates whitelist tests and adds transport config validation tests. |
src/GPS/RTK/RTKBaseStationMapItem.qml |
Adds map overlay for RTK base station marker/line. |
src/UI/toolbar/GPSIndicator.qml |
Enhances GPS indicator coloring, badges, and click handling. |
Comments suppressed due to low confidence (1)
test/GPS/NTRIP/NTRIPHttpTransportTest.cc:78
- This assertion is logically equivalent to
QVERIFY(t._rtcmParser.isWhitelisted(9999));but the current!x == falseform is hard to read and easy to misinterpret. Simplifying it will make the intent (empty whitelist accepts all IDs) clearer.
You can also share your feedback on Copilot code review. Take the survey.
| QGCLabel { | ||
| anchors.centerIn: parent | ||
| text: "!" | ||
| color: "white" | ||
| font.bold: true | ||
| font.pixelSize: parent.height * 0.7 | ||
| } |
There was a problem hiding this comment.
The interference badge label uses a hardcoded "white" text color. To keep theming consistent (light/dark palettes, custom themes), prefer using QGCPalette colors instead of literal color strings.
| if (_deviceMap.isEmpty()) { | ||
| return nullptr; | ||
| } | ||
| return _deviceMap.cbegin().value(); |
There was a problem hiding this comment.
_primaryDevice() selects the primary RTK device via QHash::cbegin(), but QHash iteration order is not deterministic. With multiple RTK devices connected, the chosen primary (and thus the fact group / satellite model / position source exposed to UI) can change unpredictably. Consider explicitly tracking a primary device (first-connected, user-selected, etc.) or using an ordered container.
| return _deviceMap.cbegin().value(); | |
| // Select primary device deterministically by choosing the device with the | |
| // lexicographically smallest key, instead of relying on QHash iteration order. | |
| auto bestIt = _deviceMap.cbegin(); | |
| for (auto it = _deviceMap.cbegin(); it != _deviceMap.cend(); ++it) { | |
| if (it.key() < bestIt.key()) { | |
| bestIt = it; | |
| } | |
| } | |
| return bestIt.value(); |
| int NTRIPReconnectPolicy::nextBackoffMs() const | ||
| { | ||
| return qMin(kMinReconnectMs * (1 << qMin(_attempts, 4)), kMaxReconnectMs); | ||
| } |
There was a problem hiding this comment.
nextBackoffMs() caps the exponent with qMin(_attempts, 4), so the backoff tops out at 16s (1s * 2^4) and will never reach kMaxReconnectMs (30s). If the intended max is 30s, allow attempts to grow until the computed backoff exceeds kMaxReconnectMs (then clamp), or adjust the cap accordingly.
HTRamsey
force-pushed
the
feature/gps-updates
branch
2 times, most recently
from
be8e7f1 to
8492da8
Compare
HTRamsey
force-pushed
the
feature/gps-updates
branch
from
8490048 to
6b29db1
Compare
HTRamsey
force-pushed
the
feature/gps-updates
branch
7 times, most recently
from
c2693db to
cfa8369
Compare
HTRamsey
force-pushed
the
feature/gps-updates
branch
2 times, most recently
from
949fd32 to
1e08787
Compare
HTRamsey
force-pushed
the
feature/gps-updates
branch
2 times, most recently
from
41c6bf3 to
95d7bbe
Compare
| }; | ||
|
|
||
| // Rebuild _constellationSummary from used-satellite counts. | ||
| // counts maps full constellation name → used satellite count. | ||
| void _buildSummary(const QMap<QString, int> &counts); | ||
|
|
||
| // Map QGeoSatelliteInfo::SatelliteSystem to the canonical constellation name | ||
| // used throughout this class ("GPS", "GLONASS", "Galileo", "BeiDou", "QZSS", …). | ||
| static QString _qtSystemName(QGeoSatelliteInfo::SatelliteSystem system); |
There was a problem hiding this comment.
SatelliteModel.h uses QMap in the _buildSummary signature, but the header doesn't include <QtCore/QMap>. This can break compilation in any translation unit that includes SatelliteModel.h without already including QMap. Add the missing include in the header to keep it self-contained.
| qint64 NmeaPipeDevice::writeData(const char *data, qint64 len) | ||
| { | ||
| _buffer.append(data, len); | ||
| emit readyRead(); | ||
| return len; | ||
| } |
There was a problem hiding this comment.
NmeaPipeDevice::writeData appends directly to _buffer without enforcing kMaxBufferSize, so callers using QIODevice::write can grow the buffer unbounded (feedData has overflow protection but writeData bypasses it). Apply the same overflow handling in writeData (or make the device read-only and route all writes through feedData).
| void NTRIPConnectionStats::stop() | ||
| { | ||
| _rateTimer.stop(); | ||
| if (_rateTracker.bytesPerSec() != 0.0) { | ||
| _rateTracker.reset(); | ||
| emit dataRateChanged(); | ||
| } | ||
| } |
There was a problem hiding this comment.
NTRIPConnectionStats::stop() may call _rateTracker.reset(), which also resets totalBytes (bytesReceived), but the code only emits dataRateChanged(). This leaves the bytesReceived Q_PROPERTY potentially out of sync with QML bindings. If stop() resets the tracker, also emit bytesReceivedChanged (and consider whether stop() should reset totals at all vs. only clearing the rate).
HTRamsey
force-pushed
the
feature/gps-updates
branch
6 times, most recently
from
93d439e to
cbf9949
Compare
HTRamsey
force-pushed
the
feature/gps-updates
branch
4 times, most recently
from
1cfa68f to
683bbe0
Compare
HTRamsey
force-pushed
the
feature/gps-updates
branch
from
683bbe0 to
dc8932b
Compare
Summary
Restructures and enhances the GPS subsystem with improved architecture, testability, and UI integration.
Architecture
src/GPS/intoNTRIP/andRTK/subdirectories with clean separation of concernsNTRIPTransportabstract base class with factory injection pattern for testabilityRTCMRouterfor multi-device RTCM distribution (vehicles via MAVLink + base stations via serial)GPSTransportabstraction decoupling GPS provider from serial implementationGPSEventModelfor structured, queryable event loggingNTRIP
NTRIPReconnectPolicy)NTRIPConnectionStats)NTRIPGgaProvider)NTRIPUdpForwarder)NTRIPError::SslError)NTRIPTransportConfig::isValid())stopNTRIP()canceled pending reconnect timerstopNTRIP()not canceling reconnect when called while idle with pending timerRTK
RTCMMavlinkMAVLink fragmentation with bandwidth trackingRTKSatelliteModelfor per-satellite signal strength displayGPSRTKFactGroupwith survey-in progress, baseline, and heading factsVehicle Integration
UI
QGCListView)RepeaterCleanup
_MSC_VER < 1900compatibility code_WIN32toQ_OS_WINfor Qt convention consistencyqCCriticalreplacingQ_ASSERT_X)Tests
NTRIPEndToEndTest)MockNTRIPTransport,MockGPSRtkNTRIPIntegrationTest,RTKIntegrationTest,SettingsLifecycleTestRTCMTestHelperfor building valid RTCM3 frames with correct CRCTest plan
-Werror)