Skip to content

Conversation

jnomikos
Copy link
Contributor

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.

@jnomikos
Copy link
Contributor Author

jnomikos commented Sep 15, 2025

Note: I still see GPS GCS flashing between invalid and valid, will investigate further

Edit: was due to logic issue fixed below

@jnomikos
Copy link
Contributor Author

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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@Davidsastresas
Copy link
Member

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants