-
Notifications
You must be signed in to change notification settings - Fork 4.4k
issue-7133: add ntrip support #13361
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
issue-7133: add ntrip support #13361
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds comprehensive NTRIP/RTK support to QGroundControl, enabling connection to NTRIP casters for real-time kinematic (RTK) positioning corrections. The implementation includes authentication, mountpoint selection, periodic GGA transmission, and RTCM/SPARTN data injection via MAVLink.
Key changes:
- Introduces NTRIPManager for caster connection management and RTCM/SPARTN data forwarding
- Adds new Settings → NTRIP/RTK configuration page with connection parameters
- Implements RTCM parser with message filtering and SPARTN pipeline support
Reviewed Changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/GPS/NTRIP.h/.cc | Core NTRIP implementation with TCP/TLS connectivity and RTCM/SPARTN parsing |
| src/Settings/NTRIPSettings.h/.cc | Settings group for NTRIP configuration parameters |
| src/UI/AppSettings/NTRIPSettings.qml | User interface for NTRIP settings configuration |
| src/FactSystem/ | Bitmask overflow fixes and new UI controls |
| src/FirmwarePlugin/PX4/ | Parameter metadata fixes for bit range validation |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| void SettingsManager::registerQmlTypes() | ||
| { | ||
| (void) qmlRegisterUncreatableType<SettingsManager>("QGroundControl.SettingsManager", 1, 0, "SettingsManager", "Reference only"); | ||
| (void) qmlRegisterUncreatableType<NTRIPSettings>("QGroundControl", 1, 0, "NTRIPSettings", "Reference only"); | ||
|
|
||
| } |
Copilot
AI
Sep 6, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The registerQmlTypes() method is added but never called. This suggests the QML type registration may not be functioning as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_ntripManagerRegisterQmlTypes() is actually called automatically by Q_COREAPP_STARTUP_FUNCTION once the Q(Core)Application is constructed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've wanted to use Q_COREAPP_STARTUP_FUNCTION in the past but I believe the doc says not to use it unless you are dynamically linking a library or something right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why not just use the QML_ELEMENT stuff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've wanted to use Q_COREAPP_STARTUP_FUNCTION in the past but I believe the doc says not to use it unless you are dynamically linking a library or something right?
I didn't see this in the doc, can you point me to it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why not just use the QML_ELEMENT stuff?
We need one NTRIPManager for the whole app that both C++ and QML can share.
With QML_ELEMENT/QML_SINGLETON, QML ends up managing its own instance (often one per engine), which doesn’t match that “single, app-wide” requirement.
We also need to control when the object is created so it attaches to the right parent (QGCApplication) and avoids being created during short-lived helper runs.
The current pattern (manual registration with a provider that returns NTRIPManager::instance()) guarantees a single, stable instance and consistent ownership across the app.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, for joystick manager we just used Q_APPLICATION_STATIC, QML_ELEMENT, and qmlEngine->rootContext()->setContextProperty(QStringLiteral("joystickManager"), JoystickManager::instance());.
You don't have to worry about creating multiple objects because the instance isn't created until it's actually referenced within the code. So the QCoreApplication doesn't actually create any of the singletons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't look like you reference ntrip in cpp right? QML_SINGLETON should be fine because we only create the qmlengine once.
|
Would be preferable if the NTRIP stuff was in a separate PR/commit from the metadata, CI, and fact system changes you did. And your added QML file should be part of a qml module in cmake. |
5c964e5 to
b69fa9f
Compare
Add NTRIP/RTK support: caster auth, mountpoint, periodic GGA, RTCM/SPARTN ingest, MAVLink injection
Description
Test Steps
Known Issues / Limitations
Test Environment
Checklist:
Related Issue
Fixes #7133
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.