Fix infinite climb bug when using EK3_OGN_HGT_MASK #26917

peterbarker · 2024-04-29T05:00:15Z

This bug-finding and fix are not mine. The autotest is mine, however :-)

@pavloblindnology reported them here: #24039

@pavloblindnology was this the fix you were recommending?

This PR is based on #26916 which adds some infrastructure changes to make autotesting with baro drift easier.

rmackay9 · 2024-04-30T04:02:23Z

@peterbarker,

I guess this is a bug that can happen on a real vehicle as well? It's not just a SITL issue is it?

peterbarker · 2024-04-30T07:10:04Z

Some pictures from a log where we never install the check_altitude message hook (pre-fix):

peterbarker · 2024-04-30T07:12:17Z

I guess this is a bug that can happen on a real vehicle as well? It's not just a SITL issue is it?

I have no idea.

I'm not even sure I'm replicating what @pavloblindnology is seeing.

pavloblindnology · 2024-05-15T14:02:02Z

@peterbarker Sorry for late response. Yes, the fix is right.

I guess this is a bug that can happen on a real vehicle as well? It's not just a SITL issue is it?

@rmackay9 I tested this in SITL only, but I don't see any reason why it can't happen on a real vehicle. I don't remember all the details of my tests as it was quite a while ago. But the main idea is that with EK3_OGN_HGT_MASK=5, Ardupilot doesn't use previously computed value of ekfGpsRefHgt to convert new GPS measurements. And as barometer altitude usually rises faster than GPS altitude, this results in constant ekfGpsRefHgt monotonical correction.

tridge · 2024-05-20T23:27:18Z

libraries/AP_NavEKF3/AP_NavEKF3_Measurements.cpp

-        } else {
-            gpsDataNew.hgt = 0.01 * (gpsloc.alt - EKF_origin.alt);
-        }
+        gpsDataNew.hgt = (ftype)((double)0.01 * (double)gpsloc.alt - ekfGpsRefHgt);


this removes the (1<<2) bit check on the input side, do we need to change the two on the output side?

This fixes the divergence, but leaves the feature broken. If a hot fix is required, remove the option to set bit 2. Looking at the code I think the underlying problem is that the offset beteween the origin height and ekfGpsRefHgt is is not being added tot he baro alt if bit 0 is set or to the range finder if bit 1 is set when in combination with bit 2. I will look at the code this week and try to come up with a longer term fix.

I've forked this PR branch here https://github.com/priseborough/ardupilot/commits/PR26917/ and pushed an alternative set of patches for this issue that preserve the intent of the EK3_OGN_HGT_MASK bit 2 option.

priseborough@a38c01a
priseborough@f6a56d4

It passes the new autotests on my PC.

davidbuzz · 2024-05-20T23:29:50Z

@priseborough

peterbarker · 2024-05-20T23:32:16Z

Note that my autotest only reproduces the issue - I didn't point any fail condition in for the specific bug

priseborough

I've forked this PR branch here https://github.com/priseborough/ardupilot/commits/PR26917/ and pushed an alternative set of patches for this issue that preserve the intent of the EK3_OGN_HGT_MASK bit 2 option.

priseborough/ardupilot@a38c01a
priseborough/ardupilot@f6a56d4

It passes the new autotests on my PC.

priseborough · 2024-05-21T07:53:55Z

libraries/AP_NavEKF3/AP_NavEKF3_Measurements.cpp

-        } else {
-            gpsDataNew.hgt = 0.01 * (gpsloc.alt - EKF_origin.alt);
-        }
+        gpsDataNew.hgt = (ftype)((double)0.01 * (double)gpsloc.alt - ekfGpsRefHgt);


This fixes the divergence, but leaves the feature broken. If a hot fix is required, remove the option to set bit 2. Looking at the code I think the underlying problem is that the offset beteween the origin height and ekfGpsRefHgt is is not being added tot he baro alt if bit 0 is set or to the range finder if bit 1 is set when in combination with bit 2. I will look at the code this week and try to come up with a longer term fix.

priseborough · 2024-05-21T10:59:48Z

libraries/AP_NavEKF3/AP_NavEKF3_Measurements.cpp

-        } else {
-            gpsDataNew.hgt = 0.01 * (gpsloc.alt - EKF_origin.alt);
-        }
+        gpsDataNew.hgt = (ftype)((double)0.01 * (double)gpsloc.alt - ekfGpsRefHgt);


I've forked this PR branch here https://github.com/priseborough/ardupilot/commits/PR26917/ and pushed an alternative set of patches for this issue that preserve the intent of the EK3_OGN_HGT_MASK bit 2 option.

priseborough@a38c01a
priseborough@f6a56d4

It passes the new autotests on my PC.

peterbarker · 2024-05-22T04:17:28Z

I've reset this to Paul's branch and rebased this PR on master and removed the commit/revert pair.

So some less-than-fortunate news. Somehow tridge's PR to use a better atmospheric model (#26915) also fixed the autotest.

I bisected the code to work that out:

IOW the autotest passes before the fixes in this branch are applied, which is Less Than Good. OTOH, since I've no idea how tridge's patch influences things, I'm quite happy to just carry on.

@priseborough are you OK with merging this even 'though the problem doesn't reproduce on master post-tridge-patches?

priseborough · 2024-05-22T05:02:19Z

Will check log and get back to you

priseborough · 2024-05-22T10:19:25Z

The better atmospheric model changes how the offset between baro height and GPs height varies so it makes sense the autotest would be sensitive to this. I'm happy to merge on the basis the old model shows it failing before. It should be possible to make it fail on the new model by fudging the baro scale factor or similar.

The offset generated by the EK3_OGN_HGT_MASK parameter bit 2 option is applied to the baro or range finder sensor so it does not have to be applied to the local position height.

peterbarker · 2024-05-22T10:22:15Z

The better atmospheric model changes how the offset between baro height and GPs height varies so it makes sense the autotest would be sensitive to this. I'm happy to merge on the basis the old model shows it failing before. It should be possible to make it fail on the new model by fudging the baro scale factor or similar.

Thanks Paul. Marking as MergeOnCIPass.

peterbarker · 2024-05-23T00:36:06Z

Thanks @pavloblindnology - we've merged a fix for the bug you found.

ddomit · 2024-06-10T06:34:08Z

Any idea to which version of arducopter is going to include this?

Does ":Apply corrections to local position" mean that it will update the home altitude accordingly? i have tried using it set to 1, but when it comes back to home its sometimes 10 meters off...

Log:
https://drive.google.com/file/d/1oEntDXzGjo2kmv54ihtVaRBwAyEa0qKx/view?usp=drive_link

rmackay9 · 2024-06-12T23:01:45Z

Hi @ddomit,

This PR's fix will most likely be included in 4.5.5. We would normally have included it in the next release (4.5.4) but that's likely to include only a single small fix

ddomit · 2024-06-13T05:07:20Z

Hi @ddomit,

This PR's fix will most likely be included in 4.5.5. We would normally have included it in the next release (4.5.4) but that's likely to include only a single small fix

Thanks!! Do you think this fix and update will solve the issue im having? Will it update the home location with the altitude corrections it does?

rmackay9 · 2024-06-13T06:28:53Z

@ddomit,

I think it's best to move this to the support forums. Support within a PR is difficult and the issue may very well be unrelated. Could you create a new topic in the support forum?

ddomit · 2024-06-14T02:45:00Z

@ddomit,

I think it's best to move this to the support forums. Support within a PR is difficult and the issue may very well be unrelated. Could you create a new topic in the support forum?

Thanks @rmackay9 it is okay i'll wait for the release, i dont need to create an issue.

I just have one doubt, i havent found anywhere, does "Apply corrections to local position" mean that it will update home altitude as well?

rmackay9 · 2024-06-14T03:54:37Z

Hi @ddomit,

If bit2 ("Apply corrections to local position") is set then the vehicle's current altitude estimate is adjusted (instead of adjusting the EKF origin's altitude). The EKF doesn't have access to the AHRS home location (including its altitude) so it is not directly adjusting it. To put it another way, if bit2 is set and the EKF adjusts the vehicle's current altitude, before and after a flight, if you asked the AHRS what the home lat, lon and alt are it should show that they have not changed.

postelzhuk · 2024-06-25T09:06:22Z

Hi everyone. I made some SITL-tests from master, Plane-4.4, Plane-4.5. Almost sure this changes also fixes problem with baro drift:
https://discuss.ardupilot.org/t/ekf3-use-barometer-and-gps-for-altitude-posz/119540/2
EK3_OGN_HGT_MASK, 5 does help with SLOW baro drifts after this changes.

peterbarker added the DevCallEU label Apr 29, 2024

peterbarker force-pushed the pr/ogn-bug branch 2 times, most recently from 04f24e7 to c57c5c0 Compare April 29, 2024 07:52

rmackay9 added BUG EKF labels Apr 30, 2024

peterbarker force-pushed the pr/ogn-bug branch from c57c5c0 to f33aaa4 Compare April 30, 2024 06:56

tridge requested a review from priseborough May 1, 2024 07:12

tridge removed the DevCallEU label May 1, 2024

tridge mentioned this pull request May 1, 2024

Plane 4.5.x issue tracking #26202

Open

52 tasks

rmackay9 added the DevCallTopic label May 16, 2024

tridge reviewed May 20, 2024

View reviewed changes

rmackay9 mentioned this pull request May 20, 2024

Copter/Rover-4.5.0 issues list #26103

Open

92 tasks

priseborough reviewed May 21, 2024

View reviewed changes

peterbarker force-pushed the pr/ogn-bug branch from f33aaa4 to 6a2300a Compare May 22, 2024 04:14

peterbarker added the DevCallEU label May 22, 2024

tridge removed the DevCallEU label May 22, 2024

priseborough approved these changes May 22, 2024

View reviewed changes

peterbarker and others added 3 commits May 22, 2024 20:21

autotest: add test for EK3_OGN_HGT_MASK bug

660ab61

AP_NavEKF3: Adjust sensor height when EK3_OGN_HGT_MASK bit 2 is set

d3713e5

AP_NavEKF3: Remove unncessary local position height reporting offset

e8b6304

The offset generated by the EK3_OGN_HGT_MASK parameter bit 2 option is applied to the baro or range finder sensor so it does not have to be applied to the local position height.

peterbarker force-pushed the pr/ogn-bug branch from 6a2300a to e8b6304 Compare May 22, 2024 10:21

peterbarker added MergeOnCIPass and removed DevCallTopic labels May 22, 2024

peterbarker merged commit 91423d4 into ArduPilot:master May 23, 2024
91 checks passed

peterbarker deleted the pr/ogn-bug branch May 25, 2024 00:33

andyp1per mentioned this pull request Jun 28, 2024

Copter, Rover, Plane, Tracker: 4.5.5-beta1 release #27416

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix infinite climb bug when using EK3_OGN_HGT_MASK #26917

Fix infinite climb bug when using EK3_OGN_HGT_MASK #26917

peterbarker commented Apr 29, 2024

rmackay9 commented Apr 30, 2024

peterbarker commented Apr 30, 2024

peterbarker commented Apr 30, 2024

pavloblindnology commented May 15, 2024

tridge May 20, 2024

priseborough May 21, 2024

priseborough May 21, 2024

davidbuzz commented May 20, 2024

peterbarker commented May 20, 2024

priseborough left a comment

priseborough May 21, 2024

priseborough May 21, 2024

peterbarker commented May 22, 2024 •

edited

Loading

priseborough commented May 22, 2024

priseborough commented May 22, 2024

peterbarker commented May 22, 2024

peterbarker commented May 23, 2024

ddomit commented Jun 10, 2024 •

edited

Loading

rmackay9 commented Jun 12, 2024 •

edited

Loading

ddomit commented Jun 13, 2024

rmackay9 commented Jun 13, 2024

ddomit commented Jun 14, 2024 •

edited

Loading

rmackay9 commented Jun 14, 2024

postelzhuk commented Jun 25, 2024

Fix infinite climb bug when using EK3_OGN_HGT_MASK #26917

Fix infinite climb bug when using EK3_OGN_HGT_MASK #26917

Conversation

peterbarker commented Apr 29, 2024

rmackay9 commented Apr 30, 2024

peterbarker commented Apr 30, 2024

peterbarker commented Apr 30, 2024

pavloblindnology commented May 15, 2024

tridge May 20, 2024

Choose a reason for hiding this comment

priseborough May 21, 2024

Choose a reason for hiding this comment

priseborough May 21, 2024

Choose a reason for hiding this comment

davidbuzz commented May 20, 2024

peterbarker commented May 20, 2024

priseborough left a comment

Choose a reason for hiding this comment

priseborough May 21, 2024

Choose a reason for hiding this comment

priseborough May 21, 2024

Choose a reason for hiding this comment

peterbarker commented May 22, 2024 • edited Loading

priseborough commented May 22, 2024

priseborough commented May 22, 2024

peterbarker commented May 22, 2024

peterbarker commented May 23, 2024

ddomit commented Jun 10, 2024 • edited Loading

rmackay9 commented Jun 12, 2024 • edited Loading

ddomit commented Jun 13, 2024

rmackay9 commented Jun 13, 2024

ddomit commented Jun 14, 2024 • edited Loading

rmackay9 commented Jun 14, 2024

postelzhuk commented Jun 25, 2024

peterbarker commented May 22, 2024 •

edited

Loading

ddomit commented Jun 10, 2024 •

edited

Loading

rmackay9 commented Jun 12, 2024 •

edited

Loading

ddomit commented Jun 14, 2024 •

edited

Loading