inav_use_gps_no_baro testing and code review indicates erroneous description?

[sensei-hacker]

trafficstars

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:

https://github.com/iNavFlight/inav/blob/b96c3d4c1f89b23d217ecf1757637392c44ad6e2/src/main/navigation/navigation_pos_estimator.c#L564

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:

https://github.com/iNavFlight/inav/assets/1971284/eb89b4e2-9e63-430f-8152-a8c3b180324a

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.

https://github.com/iNavFlight/inav/assets/1971284/c74ace0f-d6f2-4649-9035-56c6d4f1cf0d

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]

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]

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: https://github.com/iNavFlight/inav/blob/c411b500b2cec7964a9e465a73f3ac76c84edab9/src/main/navigation/navigation_pos_estimator.c#L619

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

[sensei-hacker]

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]

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]

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]

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

https://github.com/iNavFlight/inav/pull/9387