Remote ID: Fix zero and negative GCS GPS altitude being invalid#13378
Remote ID: Fix zero and negative GCS GPS altitude being invalid#13378jnomikos wants to merge 3 commits intomavlink:masterfrom
Conversation
|
Note: I still see GPS GCS flashing between invalid and valid, will investigate further Edit: was due to logic issue fixed below |
|
There was a logic issue in my fix. qIsNan logic was reversed, oops. |
Was invalidating if altitude WASN'T NaN. Should be other way around
| if (geoPositionInfo.isValid()) { | ||
| // If we dont have altitude for FAA then the GPS data is no good | ||
| if ((_settings->region()->rawValue().toInt() == Region::FAA) && !(gcsPosition.altitude() >= 0) && _gcsGPSGood) { | ||
| if ((_settings->region()->rawValue().toInt() == Region::FAA) && qIsNaN(gcsPosition.altitude()) && _gcsGPSGood) { |
There was a problem hiding this comment.
The NaN check makes sense except for the fact that PositionManager will never set altitude to NaN. The reason your code remove the flicker is because of this fact. Or in other words qIsNaN(gcsPosition.altitude() will always be false.
The PositionManager code needs to be updated to use NaN for value not available for lat/lon/altitude/heading. And then all the code that references PositionManager gcs position/heading needs to be checked to handle unavailable values.
Can you take a look at doing that as well?
There was a problem hiding this comment.
Thanks for the info, will look into it. Might be in a few weeks because I have a lot on my plate and will be moving all next week
|
Thanks for the review Don. Maybe we are better using a feasible range for GCS altitude? like (-1000) - 10000 or something like that to be on the safe side, in case for some reason we were receiving odd data from the GPS? |
Yeah, right now we simply debug when there's an error from QGeoPositionInfoSource. I think we should instead handle the error and base validity on that. |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug in Remote ID Manager where GCS GPS validity incorrectly flagged zero or negative altitude values as invalid, causing the GPS validity status to flash. The fix correctly distinguishes between missing altitude data (NaN) and valid zero/negative altitudes (e.g., locations below sea level).
Key Changes:
- Changed altitude validation from checking
>= 0to checking for NaN usingqIsNaN() - Only affects FAA region GPS validation where altitude is mandatory
- Allows valid zero and negative altitude values while still detecting missing altitude data
3 participants
When the GCS GPS altitude is 0 or negative, GCS GPS validity will flash between valid and invalid. This is not correct. Altitude can be 0 or negative since it is measured as altitude above sea level (I think). It should be a NAN check instead.
Made GCS GPS validity not check if altitude is <= 0. Instead, check if it's not NAN.
Test Steps
Run normal QGC with GCS that has negative altitude or 0 and witness GCS GPS validity flash between valid and invalid. With this PR, witness that not happen.
Checklist:
Related Issue
#13376
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
--
Note that on my PC which doesn't have GPS, I had lat, lon, and alt show up as zero and GPS was still valid, so we might want to add some other way of detecting if alt is emitted or not.