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

AP_GPS: Ignore Z-axis difference while dual GPS difference check #28236

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

tomashmyh
Copy link

@tomashmyh tomashmyh commented Sep 26, 2024

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

@rmackay9 why is this marked "safety"? AFAICS this can only make it easier to arm, meaning it makes things less safe?

libraries/AP_GPS/AP_GPS_MAV.cpp Outdated Show resolved Hide resolved
@peterbarker
Copy link
Contributor

This definitely looks like it would fix the problem mentioned in the issue.

Please outline what testing this has had.

I've marked this as DevCall for potential merge, just need to know how it's been tested. Have you tested this on non-mavlink GPSs, for example?

@tomashmyh
Copy link
Author

This definitely looks like it would fix the problem mentioned in the issue.

Please outline what testing this has had.

I've marked this as DevCall for potential merge, just need to know how it's been tested. Have you tested this on non-mavlink GPSs, for example?

I have tested this change only with mavlink GPS.
So the test looked like this:

  1. First GPS - 'physical' GPS DroneCAN type; second GPS - mavlink GPS type(this GPS data was sent via companion computer)
  2. Mavlink GPS type sent data without altiutude included using GPS_INPUT (232) mavlink message
  3. ARMING_CHECK=670206 - so GPS lock check was enabled
  4. Verification that fix is working as expected was done by approving that with that set up copter was able to arm
    I will try to find some screenshots from tests, but unfortunately cannot guarantee that will find some, because I did those tests that around a month ago)

Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

what is the use case?
accepting this message could result in the EKF consuming an invalid altitude. We can't have 3D fix and no altitude

@rmackay9
Copy link
Contributor

Hi @tomashmyh,

Could you clarify the use-case for this? There are a lot of places in the AP code where we check that the GPS provides 3D fix (which implies the altitude is available as well).

No precisely we're wondering if perhaps you're trying to provide a location from an external system (like a vision based system) in which case we have better ways to provide the location/position rather than using the GPS_INPUT message. VISION_POSITION_ESTIMATE and VISION_SPEED_ESTIMATE or ODOMETRY messages might be better.

@tomashmyh
Copy link
Author

Hi @tomashmyh,

Could you clarify the use-case for this? There are a lot of places in the AP code where we check that the GPS provides 3D fix (which implies the altitude is available as well).

No precisely we're wondering if perhaps you're trying to provide a location from an external system (like a vision based system) in which case we have better ways to provide the location/position rather than using the GPS_INPUT message. VISION_POSITION_ESTIMATE and VISION_SPEED_ESTIMATE or ODOMETRY messages might be better.

Hi @rmackay9
I have no a special use-case for this scenario. I know that there are a much better mavlink messages for providing a location from an external system but this fix is not for those cases. The reason why I thought this fix should be there and why I did it is because I think that for now processing GPS_INPUT is not properly done in AP_GPS_MAV.cpp. User can send this message with fix_type=3(3D fix) and with set GPS_INPUT_IGNORE_FLAG_ALT in ignore_flags bitmap. In this case AP will not allow to arm a vehicle even if the main GPS source and all system in general is healthy and vehicle is ready for use.
Also there is a possibility that user send fix_type=2(2D fix) and with GPS_INPUT_IGNORE_FLAG_ALT but the system does not allow a vehicle to be armed because for the same reason.

@tomashmyh
Copy link
Author

what is the use case? accepting this message could result in the EKF consuming an invalid altitude. We can't have 3D fix and no altitude

but if mavlink GPS source is not the main one so EKF is not consuming its data. So in this case vehicle can not be armed and used. Also there is a possibility that user send fix_type=2(2D fix) and with GPS_INPUT_IGNORE_FLAG_ALT

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 this pull request may close these issues.

6 participants