AP_Arming: check all GPS's for fix and health #18333
Conversation
There was a problem hiding this comment.
Getting a message back of "Bad GPS Position" is actually worse then the previous distance number. The number showed that clearly they two were different, whereas bad position doesn't tell us which is wrong, and it leaves us easily staring at just the primary. The message should be telling you which GPS has a bad status.
I think the check either needs to move into AP_GPS and tell you which GPS has a low status/bad health, or you need to just encode the logic in AP_Arming to do the scan. But the error strings really need to get better for this change to not just create more confusion.
|
Move to only check within AP arming, AP_GPS changes removed, much smaller patch. I still need to give this some testing in SITL. |
libraries/AP_Arming/AP_Arming.cpp
Outdated
| for (uint8_t i = 0; i < gps.num_sensors(); i++) { | ||
| if (gps.get_type(i) == AP_GPS::GPS_Type::GPS_TYPE_NONE) { | ||
| if (gps.primary_sensor() == i) { | ||
| check_failed(ARMING_CHECK_GPS, report, "GPS primary not configured"); |
There was a problem hiding this comment.
Doesn't this block you from disabling GPS if you don't need it? This looks like it will have the indoor positioning people yelling at us...
There was a problem hiding this comment.
Yes it does, but you already have to turn off the gps check or you fail at hdop, see #14661
IMHO we should not be comming into the gps check at all in that case
There was a problem hiding this comment.
That sorta breaks the template here: https://github.com/ArduPilot/ardupilot/blob/master/libraries/AP_Arming/AP_Arming.cpp#L1215
If we want to not run GPS checks then we should be bailing in the start of GPS checks. (How to reliably ID that is a different discussion)
libraries/AP_Arming/AP_Arming.cpp
Outdated
| check_failed(ARMING_CHECK_GPS, report, "GPS is not healthy"); | ||
| return false; | ||
| //GPS OK? | ||
| if (!AP::ahrs().home_is_set() || |
There was a problem hiding this comment.
I do think it is a little odd checking home as part of the GPS checks. Maybe we should move this, although nowhere really jumps out as better.
There was a problem hiding this comment.
AP_AHRS::pre_arm_checks ?
It's gated on the GPS checks being engaged, which is a bit silly, but we can continue to use that in the pre_arm_checks.
There was a problem hiding this comment.
checking home which is not GPS specific can lead to the wrong GPS being identified, is primary is set to 2nd GPS and no home it will say 1st GPS is bad
There was a problem hiding this comment.
moved home check to after per-gps checks.
IamPete1
force-pushed
the
GPS_arming
branch
2 times, most recently
from
0ef5f41
to
bf10a2f
Compare
|
rebased and stopped trying to check the type of the blended instance. |
IamPete1
changed the title
|
seem fair to me. all tests pass. |
| #endif | ||
| (gps.get_type(i) == AP_GPS::GPS_Type::GPS_TYPE_NONE)) { | ||
| if (gps.primary_sensor() == i) { | ||
| check_failed(ARMING_CHECK_GPS, report, "GPS %i: primary but not configured", i+1); |
There was a problem hiding this comment.
"configured" is a terrible word to use in relation to GPSs here :-)
|
... also, +1 for grocer's apostrophe. |
This prevents comparing the distance between two GPS where one has a fix and the other does not. If you have two GPS's setup they must now both be healthy and both have a 3D fix. Currently we will do
all_consistentcheck if only the primary is healthy and has a fix, this results in exciting errors such a the one I hit in the wild where the GPS was reporting a position inconsistency of 5731458m (the distance between my true location and 0 lat 0 long)