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

EKF: don't reset EKx_GPS_TYPE when GPS has no vertical velocity #15815

Merged
merged 9 commits into from
Nov 30, 2020

Conversation

tridge
Copy link
Contributor

@tridge tridge commented Nov 15, 2020

setting the parameter to 1 causes the following issues:

 - the GPS may not have vertical velocity at the time the parameter
   set happens, but may get it later when the GPS is fully configured

 - we may switch between GPS modules which do/don't have vertical
   velocity

 - the user may download parameters after the set(1), and end up with
   incorrect parameters they may later load onto the vehicle,
   permanently disabling use of vertical velocity

I've now extended this PR to also remove the inhibitGPS and inhibitGpsvertVelUse options which were completely unused and just complicated the code. With the EK3 sources PR now merged the recommended way to change the use of GPS is via the sources system.
Log flying this code is here:
http://uav.tridgell.net/DartXL/2020-11-22/00000024.BIN

@rmackay9
Copy link
Contributor

I'd really like it if we could hold off on EKF3 fixes until my PR has gone in: #14803

@tridge
Copy link
Contributor Author

tridge commented Nov 16, 2020

I'd really like it if we could hold off on EKF3 fixes until my PR has gone in: #14803

yep, no problem

@rmackay9
Copy link
Contributor

Great to see this. We should be able to completely remove the AP_NavEKF_Source::setVelZSource() call as part of this PR (or as a follow-up). I only added this call because it was needed and I'd rather it didn't exist.

I see you didn't use the method of adding an extra boolean to the buffer and then use this to decide whether to fuse or not. I started looking at this a bit yesterday and I also found that there were multiple places where other logic was depending upon whether we had vertical velocities or not so maybe you've decided that going directly to the GPS is the most consistent way.

Copy link
Contributor

@priseborough priseborough left a comment

Choose a reason for hiding this comment

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

Do you have a log showing how it handles a change in gps.have_vertical_velocity() when the EKF is already using GPS?

@tridge
Copy link
Contributor Author

tridge commented Nov 24, 2020

Do you have a log showing how it handles a change in gps.have_vertical_velocity() when the EKF is already using GPS?

no, but I can create one in SITL. I'll add a flag to our NMEA GPS driver to enable/disable GPS vertical velocity

@rmackay9
Copy link
Contributor

No pressure but I'm still keen on seeing this one go in..

setting the parameter to 1 causes the following issues:

 - the GPS may not have vertical velocity at the time the parameter
   set happens, but may get it later when the GPS is fully configured

 - we may switch between GPS modules which do/don't have vertical
   velocity

 - the user may download parameters after the set(1), and end up with
   incorrect parameters they may later load onto the vehicle,
   permanently disabling use of vertical velocity
setting the parameter to 1 causes the following issues:

 - the GPS may not have vertical velocity at the time the parameter
   set happens, but may get it later when the GPS is fully configured

 - we may switch between GPS modules which do/don't have vertical
   velocity

 - the user may download parameters after the set(1), and end up with
   incorrect parameters they may later load onto the vehicle,
   permanently disabling use of vertical velocity
this adds vertical velocity support
this ensures that we record GPS vertical velocity status for every
sample correctly
@tridge
Copy link
Contributor Author

tridge commented Nov 30, 2020

@priseborough and @rmackay9 I've now added the have_vz flag to the GPS buffer data so we get this right on a per sample basis
I've put a SITL log here:
http://uav.tridgell.net/EKF/gps-velz-switch-32.bin
it has one ublox GPS with vertical velocity and one NMEA GPS without vertical velocity. I then failover between the two GPS modules in the flight.
The vertical innovations are fine with the change. We don't actually have a log flag to say if the EKF was fusing vertical velocity, so it is a bit hard to know if vz fusion is happening, but I did attach a debugger and checked that useGpsVertVel is changing when we switch between a GPS that has vertical velocity and one that doesn't.

@rmackay9
Copy link
Contributor

@tridge,

Is this commit included by mistake? It seems mostly unrelated to the rest of the PR.

AP_GPS: support $PHD message for AllyStay NMEA GPS

@tridge tridge merged commit d242339 into ArduPilot:master Nov 30, 2020
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.

None yet

5 participants