Skip to content
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

inav_use_gps_no_baro testing and code review indicates erroneous description? #10040

Closed
sensei-hacker opened this issue May 15, 2024 · 6 comments · Fixed by #10041
Closed

inav_use_gps_no_baro testing and code review indicates erroneous description? #10040

sensei-hacker opened this issue May 15, 2024 · 6 comments · Fixed by #10041

Comments

@sensei-hacker
Copy link
Collaborator

sensei-hacker commented May 15, 2024

I was looking at #9966 and I'm not sure the description added is accurate.
I wonder if I'm missing something in the code?

Looking at the code:

if ((ctx->newFlags & EST_BARO_VALID) && (!positionEstimationConfig()->use_gps_no_baro) && (wBaro > 0.01f)) {

It appears to me the setting actually disables using baro altitude at all.

From my reading, baro is used only if:
if ((ctx->newFlags & EST_BARO_VALID) && (!positionEstimationConfig()->use_gps_no_baro) && (wBaro > 0.01f)) {

I then ran a test to confirm, increasing baro altitude (reduced pressure) while watching the altitude.
This is the result with inav_use_gps_no_baro = OFF, which was the default a few weeks ago:

no_baro_off.mp4

The altitude estimate correctly responds to the baro, with inav_use_gps_no_baro = OFF.

However, with inav_use_gps_no_baro = ON, changing the baro results in no change to the altitude estimate:
Caution - loud.

no_baro_on.mp4

This test result is consistent with my reading of the code.

Am I missing something in the code, or should that description be updated, and perhaps the default restored to what it was a few weeks ago?

@breadoven
Copy link
Collaborator

breadoven commented May 15, 2024

Ah well done, you're completely right. inav_use_gps_no_baro doesn't only use the GPS if the Baro isn't available, it in fact doesn't use the Baro at all even if available.

This explains the hopeless Althold I had on the quad the other day. With the Baro disabled it was just using GPS altitude which when you've only just got a fix is usually inaccurate and drifts a lot. Althold was terrible initially but got better as the number of satellites increased which would have improved the GPS altitude measurement.

I just tested with inav_use_gps_no_baro = ON and altitude pos and vel are zeroed when disarmed until the GPS gets a lock then they're all over the place, constant vertical rates of almost 1m/s when the quad is just sat on the ground + an altitude of several meters when it should be 0 when disarmed. Setting inav_use_gps_no_baro to OFF and everything works as it should again, when the GPS gets a lock altitude pos and vel barely change because I'm guessing altitude estimation will be much more biased to using the Baro than the GPS.

So the problem isn't so much enabling inav_use_gps_no_baro by default but the fact this setting doesn't do what is intended, i.e. only use GPS alone if a Baro isn't available. Seems it was always wrong with the code the way it is.

@breadoven
Copy link
Collaborator

Actually looking at the history of navigation_pos_estimator.c, inav_use_gps_no_baro was originally only relevant for a quad and only if no Baro was available so the setting description made more sense:

else if ((STATE(FIXED_WING_LEGACY) || positionEstimationConfig()->use_gps_no_baro) && (ctx->newFlags & EST_GPS_Z_VALID)) {

At least there's an easy fix, just turn that setting OFF if you have a working Baro.

@sensei-hacker
Copy link
Collaborator Author

sensei-hacker commented May 15, 2024

Yeah longer term the issue is the code doesn't do what we probably want. We'd perhaps like the code to do what the description says.

At the moment, what we actually have is a "disable the baro" setting. That setting we actually have, the setting that disables the baro, should probably default to off, IMHO. Until we have a setting that doesn't disable the baro.

That's assuming we actually want to implement the setting as described. In approximately 100,000 Discord messages, the setting never came up; no user has ever needed to change it that I've seen. (Until 7.1.1 when the default was changed and it did the wrong thing). It may be the setting isn't needed - as evidenced by the fact we haven't actually had it, and nobody has complained about not having it.

@breadoven
Copy link
Collaborator

With the code as it is now it will use Baro and GPS as available so I think the setting is redundant and should be removed. If you want to disable the Baro just do it from the sensors config.

@RomanLut
Copy link
Contributor

This option was working as intended till 7.0. In 7.1 it disables baro usage. Should either be renamed or better removed completety.

With the code as it is now it will use Baro and GPS as available so I think the setting is redundant and should be removed. If you want to disable the Baro just do it from the sensors config.

With code is it now, althold modes are not shown in configurator with baro sensor disabled in sensor config. The only way to show them is to set inav_use_gps_no_baro = ON. This is something which should be fixed too.

@sensei-hacker
Copy link
Collaborator Author

I think #10041 takes care of it, but if anyone has a need to know, it looks like the bug appeared in 7.1.

#9387

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

Successfully merging a pull request may close this issue.

3 participants