PositionManager/LinkManager: fall back to internal GPS when NMEA disabled#14588
PositionManager/LinkManager: fall back to internal GPS when NMEA disabled#14588julianoes wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (30.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #14588 +/- ##
==========================================
+ Coverage 25.47% 31.72% +6.25%
==========================================
Files 769 785 +16
Lines 65912 67107 +1195
Branches 30495 31083 +588
==========================================
+ Hits 16788 21292 +4504
+ Misses 37285 31394 -5891
- Partials 11839 14421 +2582
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 459 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Build ResultsPlatform Status
All builds passed. Pre-commit
Pre-commit hooks: 0 passed, 0 failed, 0 skipped. Test Resultslinux-coverage-integration: 34 passed, 0 skipped Code CoverageCoverage: 68.0% No baseline available for comparison Artifact Sizes
Updated: 2026-07-22 05:19:33 UTC • Commit: 253bb41 • Triggered by: Android |
|
@julianoes Can you address the review. I believe both of these are correct from a bug standpoint |
6da033d to
4034403
Compare
|
@DonLakeFlyer done. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Comms/LinkManager.cc:540
- When switching NMEA to "Disabled", this deletes
_nmeaPortbefore tellingQGCPositionManagerto tear down the activeQNmeaPositionInfoSource. Since the position source may still be reading from thatQSerialPortuntilresetNmeaSourceDevice()runs, this ordering risks a use-after-free/crash via queued events.
if (_nmeaPort) {
_nmeaPort->close();
delete _nmeaPort;
_nmeaPort = nullptr;
_nmeaDeviceName = "";
|
@julianoes Can you look at new code review. It tends to be whack-a-mole unfortunately but it also tends to be correct. |
|
Ha ok. |
Disabling the NMEA GPS input (setting the Comm Links NMEA device back to Disabled) closed the NMEA UDP socket but left QGCPositionManager stuck on the now-dead NMEA source, freezing the displayed GCS position. Tear the NMEA source down and fall back to the integrated GPS. Addresses review: - LinkManager only tears down _nmeaPort / resets the position source when NMEA is Disabled, not for a selected serial NMEA port (which is set up by _addSerialAutoConnectLink() and would otherwise break every tick). - resetNmeaSourceDevice() switches away while the NMEA source is still valid so _setPositionSource() runs its normal cleanup (stop, disconnect, reset stale position/accuracy) before the source is deleted. - Use source->disconnect(this) instead of disconnect(source). The latter picks the overload that drops this object's own signals to the source, so the source->this connections survived and were duplicated every time a source was re-selected, running _positionUpdated once per duplicate. - NmeaGpsSettings.qml stores the untranslated device value instead of the translated combo box label, so the LinkManager string comparisons hold in localized builds. Settings written by older builds are migrated by matching the displayed text once.
4034403 to
253bb41
Compare
|
If you've dealt with the code review then hide it marked as resolved. Then I'll run another code review |
Switching NMEA off (e.g. Disabled in Comm Links) closed the UDP socket but left
QGCPositionManageron the dead NMEA source, freezing the GCS position with no way back to the built-in GPS.Add
QGCPositionManager::resetNmeaSourceDevice()to tear down the NMEA source and, if active, revert to the platform default (e.g. integrated Android GPS).LinkManagercalls it when NMEA is no longer configured, and also tears down a lingering serial NMEA port there.