Add a function to check the health of the Barometer

[JulioCesarMatias]

This PR tries to fix #8224

If the pressure or temperature does not change after 2 seconds, the Barometer is marked as unhealthy. When the barometer was forced (through a minor code change) to not update the pressure and temperature, Baro health was marked as bad in iNav Configurator.

[JulioCesarMatias]

@robustini We need to test this without the modifications made by Konstantin (#8201), because then your Baro will work incorrectly, and these modifications made by me will take effect and make the Baro unusable (which was not done before). What target can I compile for you to test this?

[robustini]

@robustini We need to test this without the modifications made by Konstantin (#8201), because then your Baro will work incorrectly, and these modifications made by me will take effect and make the Baro unusable (which was not done before). What target can I compile for you to test this?

Thanks mate, it was absolutely necessary!
Don't worry, I compile the code without the PR #8201 as well, so the barometer median filter must be there.
So since I use the master "git revert -m 1 4d4e279", right?
So if during an RTH the barometer should fail iNav use either the gps altitude or the rage finder for height corrections, right?
I mean while during the flight RTH activated, not when the software was booted.
Hope the code reasons that way.

[robustini]

[master 11f6d976a] Revert "Merge pull request #8201 from iNavFlight/de_remove_baro_median"
Date: Sat Jul 16 07:42:18 2022 +0200
4 files changed, 66 insertions(+), 1 deletion(-)
From https://github.com/iNavFlight/inav

• [new ref] refs/pull/8083/head -> pr1
  remote: Enumerating objects: 32, done.
  remote: Counting objects: 100% (32/32), done.
  remote: Compressing objects: 100% (17/17), done.
  remote: Total 32 (delta 21), reused 26 (delta 15), pack-reused 0
  Unpacking objects: 100% (32/32), 8.04 KiB | 216.00 KiB/s, done.
  From https://github.com/iNavFlight/inav
• [new ref] refs/pull/8233/head -> pr4
  Auto-merging src/main/sensors/barometer.h
  Auto-merging src/main/sensors/barometer.c
  CONFLICT (content): Merge conflict in src/main/sensors/barometer.c
  Automatic merge failed; fix conflicts and then commit the result.

It seems that after the revert it doesn't want to compile.

[breadoven]

How likely is it that a healthy Baro in a static stable position doesn't show any pressure/temperature changes over 2 seconds ? Looking at Baro sensor logging it seems very unlikely given the amount of random variation but is that true in all cases ?

[robustini]

How likely is it that a healthy Baro in a static stable position doesn't show any pressure/temperature changes over 2 seconds ? Looking at Baro sensor logging it seems very unlikely given the amount of random variation.

Look at the barometer data in my second log in the issue, the barometer after a few seconds from the takeoff went to 0 for the whole flight.
This prevented iNav from switching to gps for the altitude control, and shot my the racer in orbit.

[JulioCesarMatias]

@robustini We need to test this without the modifications made by Konstantin (#8201), because then your Baro will work incorrectly, and these modifications made by me will take effect and make the Baro unusable (which was not done before). What target can I compile for you to test this?

Thanks mate, it was absolutely necessary! Don't worry, I compile the code without the PR #8201 as well, so the barometer median filter must be there. So since I use the master "git revert -m 1 4d4e279", right? So if during an RTH the barometer should fail iNav use either the gps altitude or the rage finder for height corrections, right? I mean while during the flight RTH activated, not when the software was booted. Hope the code reasons that way.

Actually not yet, for AutoPilot to use GPS altitude when Baro health is poor, it is necessary to change the sanity check in the fc_msp_box.c extension.

[JulioCesarMatias]

[master 11f6d976a] Revert "Merge pull request #8201 from iNavFlight/de_remove_baro_median" Date: Sat Jul 16 07:42:18 2022 +0200 4 files changed, 66 insertions(+), 1 deletion(-) From https://github.com/iNavFlight/inav

• [new ref] refs/pull/8083/head -> pr1
  remote: Enumerating objects: 32, done.
  remote: Counting objects: 100% (32/32), done.
  remote: Compressing objects: 100% (17/17), done.
  remote: Total 32 (delta 21), reused 26 (delta 15), pack-reused 0
  Unpacking objects: 100% (32/32), 8.04 KiB | 216.00 KiB/s, done.
  From https://github.com/iNavFlight/inav
• [new ref] refs/pull/8233/head -> pr4
  Auto-merging src/main/sensors/barometer.h
  Auto-merging src/main/sensors/barometer.c
  CONFLICT (content): Merge conflict in src/main/sensors/barometer.c
  Automatic merge failed; fix conflicts and then commit the result.

It seems that after the revert it doesn't want to compile.

you have to add uint8_t use_median_filtering; inside the barometerConfig_t structure, and also add .use_median_filtering = SETTING_BARO_MEDIAN_FILTER_DEFAULT, inside PG_RESET_TEMPLATE(barometerConfig_t, barometerConfig,

[JulioCesarMatias]

How likely is it that a healthy Baro in a static stable position doesn't show any pressure/temperature changes over 2 seconds ? Looking at Baro sensor logging it seems very unlikely given the amount of random variation but is that true in all cases ?

No, no false negative will happen, the barometer is noisy, its previous value will always be different from the current one.

[robustini]

you have to add uint8_t use_median_filtering; inside the barometerConfig_t structure, and also add .use_median_filtering = SETTING_BARO_MEDIAN_FILTER_DEFAULT, inside PG_RESET_TEMPLATE(barometerConfig_t, barometerConfig,

If I have to do this then the revert won't work.
One thing: this PR of yours performs a baro sanity check and it's ok, but the other PR is already merged in the master.
Why not adapt your code to the current situation?
If we disable the median filter from the parameters, won't the result be identical?
Otherwise a merge of this PR in the future won't be possible given the conflict, right?

[JulioCesarMatias]

you have to add uint8_t use_median_filtering; inside the barometerConfig_t structure, and also add .use_median_filtering = SETTING_BARO_MEDIAN_FILTER_DEFAULT, inside PG_RESET_TEMPLATE(barometerConfig_t, barometerConfig,

If I have to do this then the revert won't work. One thing: this PR of yours performs a baro sanity check and it's ok, but the other PR is already merged in the master. Why not adapt your code to the current situation? If we disable the median filter from the parameters, won't the result be identical? Otherwise a merge of this PR in the future won't be possible given the conflict, right?

What I want with this PR is for the system to make sure the Baro reads are good to use, and that the algorithm doesn't just get stuck on validating whether or not the baro was found in I2C. Previously, the baroIsHealthy function always returned true, now it returns true or false, and now this function is safe to use elsewhere in the algorithm, especially if baro fails, for AutoPilot to use GPS Altitude

[ricardodeazambuja]

I was comparing your solution for baroIsHealthy with what I am proposing (#8534), and I think yours would need to test all the drivers to check if those timeout values make sense to all sensors. Maybe just adding BARO_TIMEOUT_MS and BARO_DATA_CHANGE_TIMEOUT_MS to the settings with super conservative default values would already be enough, but in this scenario it would almost be the same as keeping the old implementation for the average user that doesn't use the CLI.
To be honest, imho the drivers are the ones that should check their own health and make use of sensorsSet(SENSOR_BARO) and sensorsClear(SENSOR_BARO) every time they pass a new baro reading along.

[JulioCesarMatias]

I was comparing your solution for baroIsHealthy with what I am proposing (#8534), and I think yours would need to test all the drivers to check if those timeout values make sense to all sensors. Maybe just adding BARO_TIMEOUT_MS and BARO_DATA_CHANGE_TIMEOUT_MS to the settings with super conservative default values would already be enough, but in this scenario it would almost be the same as keeping the old implementation for the average user that doesn't use the CLI. To be honest, imho the drivers are the ones that should check their own health and make use of sensorsSet(SENSOR_BARO) and sensorsClear(SENSOR_BARO) every time they pass a new baro reading along.

I don't think I agree with that...

Just check if the sensor is available on the bus to validate health? I don't think so... For me, it's more convenient to check if the sensor is responding to each machine cycle.

Using the method described by you may be flawed, when the sensor is recognized but does not deliver good readings, or even no readings at all.

[ricardodeazambuja]

I was comparing your solution for baroIsHealthy with what I am proposing (#8534), and I think yours would need to test all the drivers to check if those timeout values make sense to all sensors. Maybe just adding BARO_TIMEOUT_MS and BARO_DATA_CHANGE_TIMEOUT_MS to the settings with super conservative default values would already be enough, but in this scenario it would almost be the same as keeping the old implementation for the average user that doesn't use the CLI. To be honest, imho the drivers are the ones that should check their own health and make use of sensorsSet(SENSOR_BARO) and sensorsClear(SENSOR_BARO) every time they pass a new baro reading along.

I don't think I agree with that...

Just check if the sensor is available on the bus to validate health? I don't think so... For me, it's more convenient to check if the sensor is responding to each machine cycle.

Using the method described by you may be flawed, when the sensor is recognized but does not deliver good readings, or even no readings at all.

I totally agree with you that it should have a way to report more than only true or false, but baroIsHealthy can't return anything else. Moreover, getHwBarometerStatus doesn't improve on that either.
The solution I proposed for baroIsHealthy is literally doing nothing to prevent a sensor that doesn't work because I would expect the driver to do that.
Maybe a better place would be within navigation. There's already a INAV_BARO_TIMEOUT_MS defined in navigation_pos_estimator_private.h and EST_BARO_VALID takes that into account.
One problem I noticed is the default value for positionEstimationConfig()->max_eph_epv. It takes a while until an estimation uncertainty falls below that default value and the navigation will use that estimation until its eph or epv grows bigger than the default.

[b14ckyy]

I added this to a test build and found that it breaks functionality with HITL simulation. AS the simulated baro will not have any drift, it is flagged as unhealthy.

[ricardodeazambuja]

I added this to a test build and found that it breaks functionality with HITL simulation. AS the simulated baro will not have any drift, it is flagged as unhealthy.
@b14ckyy Could you test my pull request #8534 to see if the HITL simulation still works? Our lab is in need to get the problem with the MSP2_SENSOR_BARO message (and, therefore, the sensor) fixed so we can use the main master branch instead of our fork. We need the baro working because it's necessary to allow us to use the Intel Realsense T265 with INAV 6 (see discussion here #8516)

[b14ckyy]

@ricardodeazambuja yep, no problem with this PR. Baro is seen as valid in INAV when HITL is connected.

[ricardodeazambuja]

@JulioCesarMatias, I am also focused on using MSP_BARO from other sources, like @b14ckyy, and it would be weird to be forced to make sure the baro is not perfect to make it work. I still think this should be in the driver side of the things.