GPS preflight check: improve reporting #10586

bkueng · 2018-09-27T15:05:32Z

Previously it was possible to get a Position Control rejected message without further advice what was actually wrong (the drone could even have a valid GPS position). So now we report warnings even if gps is not required for arming (which could be annoying too...).
The GPS failure message was very generic, making it hard to debug the cause. Now we check every bit and send an appropriate warning. EKF2 has threshold parameters for each of those checks.

All strings were checked not to exceed the maximum length of 50 characters.

Additional changes:

ekf2: only report gps failure flags that are enabled
PreflightChecks: improve labels in general by not capitalizing everything
mavlink_receiver: reduce s_variance_m_s for HIL GPS message from 1 to 0.1. This fixes position control for HIL not working.

… 0.1 1 was too high to pass the EKF2 test (EKF2_REQ_SACC which is 0.5 by default) and thus switching into position mode in HIL was not possible.

So that they can be used for reporting errors.

estimator status init is not required.

mhkabir · 2018-09-27T15:29:03Z

src/modules/commander/PreflightCheck.cpp

@@ -113,7 +113,7 @@ static bool magnometerCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &sta

 		if (!mag_valid) {
 			if (report_fail) {
-				mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: MAG #%u invalid", instance);
+				mavlink_log_critical(mavlink_log_pub, "Preflight Fail: Mag #%u invalid", instance);


What does "Mag invalid" mean?

Changed to Preflight Fail: no valid Data for Mag. Is that better?

Much better! Can you do it for accel/gyro too?

mhkabir · 2018-09-27T15:31:26Z

src/modules/commander/PreflightCheck.cpp

+			if (report_fail) {
+				// Only report the first failure to avoid spamming
+				if (status.gps_check_fail_flags & (1 << estimator_status_s::GPS_CHECK_FAIL_GPS_FIX)) {
+					mavlink_log_critical(mavlink_log_pub, "Preflight %s: GPS fix not too low", fail_str);


What does this mean?

Ha, my fail. Fixed.

dagar · 2018-09-27T15:42:17Z

We should double check the QGC side. There was discussion of special handling for "PREFLIGHT" and "PREARM" messages previously.

@DonLakeFlyer

- previously it was possible to get a Position Control rejected message without further advice what was actually wrong. So now we report warnings even if gps is not required for arming (which could be annoying too...). - the GPS failure message was very generic, making it hard to debug the cause. Now we check every bit and send an appropriate warning All strings were checked not to exceed the maximum length of 50 characters.

bkueng · 2018-09-27T15:55:47Z

A quick QGC grep for PREFLIGHT did not reveal anything.

dagar · 2018-09-27T16:01:03Z

Two more points to think about for later.

There are usability and safety issues in and around these requirements we should discuss at some point. For example if RTL is configured anywhere (failsafes for loss of RC, low battery, etc) perhaps we should require valid home and valid position for arming. Obviously we don't want to burden a user that intends to jump straight into a stabilized or altitude flight only, but the desired use case and implications need to be carefully thought through.
The way these GPS checks are implemented makes the system dependant on EKF2 in a way that seems easily avoidable. Nearly all of these "estimator" GPS check bits are simply against the raw GPS data. https://github.com/PX4/ecl/blob/master/EKF/gps_checks.cpp#L110-L126 @priseborough Thoughts?

dagar · 2018-09-27T16:04:53Z

A quick QGC grep for PREFLIGHT did not reveal anything.

Looks like it's only "PreArm:" currently.
https://github.com/mavlink/qgroundcontrol/blob/master/src/QGCApplication.cc#L677-L680

@DonLakeFlyer @bkueng can we agree on what to do here? The way QGC handles "PreArm:" looks much nicer.

mavlink/qgroundcontrol#6436
mavlink/qgroundcontrol#3460

dagar · 2018-09-27T16:13:29Z

src/modules/commander/PreflightCheck.cpp


-			success = false;
-			goto out;
+		const char *fail_str = enforce_gps_required ? "Fail" : "Warn";


Let's use the existing mavlink severity levels instead.
https://mavlink.io/en/messages/common.html#MAV_SEVERITY

https://mavlink.io/en/messages/common.html#STATUSTEXT

DonLakeFlyer · 2018-09-27T23:57:39Z

FYI: The original pull which support the PreArm QGC thing was lost in the fray. The new one is here: mavlink/qgroundcontrol#6898 and could use some testing.

Antiheavy · 2018-09-28T03:14:04Z

This is a great PR! Much needed improvements. I'll humbly submit some wording suggestions as comments in-line with the code. thanks!

Antiheavy · 2018-09-28T03:19:45Z

src/modules/commander/PreflightCheck.cpp

@@ -113,7 +113,7 @@ static bool magnometerCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &sta

 		if (!mag_valid) {
 			if (report_fail) {
-				mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: MAG #%u invalid", instance);
+				mavlink_log_critical(mavlink_log_pub, "Preflight Fail: no valid Data for Mag #%u", instance);


Suggest: "Preflight Fail: no valid data from Mag #%u", or "Preflight Fail: invalid data from Mag #%u"

Antiheavy · 2018-09-28T03:25:14Z

src/modules/commander/PreflightCheck.cpp

@@ -123,13 +123,13 @@ static bool magnometerCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &sta

 		if (!calibration_valid) {
 			if (report_fail) {
-				mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: MAG #%u UNCALIBRATED", instance);
+				mavlink_log_critical(mavlink_log_pub, "Preflight Fail: Mag #%u uncalibrated", instance);


The inconsistent use of the terms "Mag" and "Compass" can be confusing for users. In QGC you must go to the "Compass" calibration page to calibrate the Mag(s). This is a larger issue, so I guess I'm just making a comment more than suggesting a change here.

Antiheavy · 2018-09-28T03:33:07Z

src/modules/commander/PreflightCheck.cpp

@@ -249,7 +249,7 @@ static bool accelerometerCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &

 		if (!accel_valid) {
 			if (report_fail) {
-				mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: ACCEL #%u invalid", instance);
+				mavlink_log_critical(mavlink_log_pub, "Preflight Fail: no valid Data for Accel #%u", instance);


suggest: "Preflight Fail: no valid data from Accel #%u" or "Preflight Fail: invalid data from Accel #%u"

Antiheavy · 2018-09-28T03:35:36Z

src/modules/commander/PreflightCheck.cpp

@@ -313,7 +313,7 @@ static bool gyroCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &status, u

 		if (!gyro_valid) {
 			if (report_fail) {
-				mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: GYRO #%u invalid", instance);
+				mavlink_log_critical(mavlink_log_pub, "Preflight Fail: no valid Data for Gyro #%u", instance);


suggest: "Preflight Fail: no valid data from Gyro #%u" or "Preflight Fail: invalid data from Gyro #%u"

Antiheavy · 2018-09-28T03:37:30Z

src/modules/commander/PreflightCheck.cpp

@@ -356,14 +356,14 @@ static bool baroCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &status, u

 		if (!baro_valid) {
 			if (report_fail) {
-				mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: BARO #%u invalid", instance);
+				mavlink_log_critical(mavlink_log_pub, "Preflight Fail: no valid Data for Baro #%u", instance);


suggest: "Preflight Fail: no valid data from Baro #%u" or "Preflight Fail: invalid data from Baro #%u"

Antiheavy · 2018-09-28T03:44:08Z

src/modules/commander/PreflightCheck.cpp

@@ -389,7 +389,7 @@ static bool airspeedCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &statu
 	if ((orb_copy(ORB_ID(differential_pressure), fd_diffpres, &differential_pressure) != PX4_OK) ||
 	    (hrt_elapsed_time(&differential_pressure.timestamp) > 1_s)) {
 		if (report_fail && !optional) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: AIRSPEED SENSOR MISSING");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: Airspeed Sensor missing");


Couple comments here:

this is the first use of "< > sensor missing". The other sensors print out "no < > sensor". From a user standpoint I like the "< > sensor missing" message better because it implies that the sensor is both expected to be there and also that it is missing. The meaning for "no < > sensor" message is a bit more vague. Either way, I think we should aim for consistency.

can this be extended to support multiple diffpres/airspeed sensors?

Good point, I switched to the 'sensor missing' version.

I don't know, it's probably a bigger thing.

Yes it can and yes it's a slightly bigger thing.

Antiheavy · 2018-09-28T03:45:15Z

src/modules/commander/PreflightCheck.cpp

@@ -400,7 +400,7 @@ static bool airspeedCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s &statu
 	if ((orb_copy(ORB_ID(airspeed), fd_airspeed, &airspeed) != PX4_OK) ||
 	    (hrt_elapsed_time(&airspeed.timestamp) > 1_s)) {
 		if (report_fail && !optional) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: AIRSPEED SENSOR MISSING");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: Airspeed Sensor missing");


same comment as I made in the "diffpres" section above. I'm also unclear as to why there are two of these, but I'll assume there is a good reason.

It goes back to the change that allowed airspeed to be < 0. The code is checking |differential_pressure|, but could now be updated to check airspeed only. I think we already have this in a branch somewhere...

Antiheavy · 2018-09-28T03:50:38Z

src/modules/commander/PreflightCheck.cpp

@@ -518,7 +518,7 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, vehicle_status_s &vehicle_s
 	// Check if preflight check performed by estimator has failed
 	if (status.pre_flt_fail) {
 		if (report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: EKF INTERNAL CHECKS");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: EKF internal checks");


what does this even mean? Can this message be written more specifically and in a way to be understood by the average user?

It's one that I would like to get rid of as well. @priseborough ?

Let's remove or re-write the language.

"Preflight fail: Position unknown" or something like that.

I changed it to Position unknown.

Antiheavy · 2018-09-28T03:54:32Z

src/modules/commander/PreflightCheck.cpp

@@ -530,7 +530,7 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, vehicle_status_s &vehicle_s

 	if (status.hgt_test_ratio > test_limit) {
 		if (report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: EKF HGT ERROR");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: EKF Height Error");


This comment applies to all the messages that contain "EKF":

the average user has no clue what an EKF is. They are very confused by these messages.

the term "EKF" doesn't add any helpful information to any of these messages anyway.

Suggest: "Preflight Fail: Height estimate error"

Antiheavy · 2018-09-28T03:55:10Z

src/modules/commander/PreflightCheck.cpp

@@ -542,7 +542,7 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, vehicle_status_s &vehicle_s

 	if (status.vel_test_ratio > test_limit) {
 		if (report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: EKF VEL ERROR");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: EKF Velocity Error");


suggest: "Preflight Fail: Velocity estimate error" or "Preflight Fail: Velocity error"

Antiheavy · 2018-09-28T03:56:22Z

src/modules/commander/PreflightCheck.cpp

@@ -554,7 +554,7 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, vehicle_status_s &vehicle_s

 	if (status.pos_test_ratio > test_limit) {
 		if (report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: EKF HORIZ POS ERROR");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: EKF Horizontal Position Error");


suggest: "Preflight Fail: Horizontal position error"

Antiheavy · 2018-09-28T03:56:38Z

src/modules/commander/PreflightCheck.cpp

@@ -566,7 +566,7 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, vehicle_status_s &vehicle_s

 	if (status.mag_test_ratio > test_limit) {
 		if (report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: EKF YAW ERROR");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: EKF Yaw Error");


suggest: "Preflight Fail: Yaw error"

Antiheavy · 2018-09-28T03:57:36Z

src/modules/commander/PreflightCheck.cpp

@@ -579,7 +579,7 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, vehicle_status_s &vehicle_s
 	if (fabsf(status.states[13]) > test_limit || fabsf(status.states[14]) > test_limit
 	    || fabsf(status.states[15]) > test_limit) {
 		if (report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: EKF HIGH IMU ACCEL BIAS");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: EKF High IMU Accel Bias");


suggest: "Preflight Fail: High Accelerometer Bias"

Antiheavy · 2018-09-28T03:57:57Z

src/modules/commander/PreflightCheck.cpp

@@ -592,55 +592,59 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, vehicle_status_s &vehicle_s
 	if (fabsf(status.states[10]) > test_limit || fabsf(status.states[11]) > test_limit
 	    || fabsf(status.states[12]) > test_limit) {
 		if (report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: EKF HIGH IMU GYRO BIAS");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: EKF High IMU Gyro Bias");


suggest: "Preflight Fail: High Gyro Bias"

Antiheavy · 2018-09-28T03:59:39Z

src/modules/commander/PreflightCheck.cpp

+				} else {
+					if (!ekf_gps_fusion) {
+						// Likely cause unknown
+						mavlink_log_critical(mavlink_log_pub, "Preflight %s: EKF not using GPS", fail_str);


same comment that most users are confused by the term "EKF".
Suggest: "Preflight %s: System not using GPS"

bkueng · 2018-09-28T09:43:16Z

Thanks @Antiheavy, great feedback. I think I incorperated all of it.

I updated the PR to use the correct log level, and I think it's good now.

Long-term the goal is to use the events interface for that.

dagar · 2018-09-28T12:47:16Z

src/modules/commander/PreflightCheck.cpp

@@ -223,7 +223,7 @@ static bool magConsistencyCheck(orb_advert_t *mavlink_log_pub, vehicle_status_s

 	if (sensors.mag_inconsistency_ga > test_limit) {
 		if (report_status) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: MAG SENSORS INCONSISTENT");
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: Compass Sensors inconsistent");


Compass Sensors?

From a user perspective I think "Compass Sensors inconsistent" is more easily understood and more consistent with the user interface in QGC. However, "Mag" and "Mag Sensor(s)" is still used in some of the other warnings.

On a side note: I think it is still okay if "MAG" is used in the parameters and other developer and integrator focused areas. In user focused areas (UX and non-developer documentation) "compass" is more easily understood.

Compass instead of Mag seems better to me as long as we're consistent everywhere. Compass Sensors is what I was questioning that seemed inconsistent with other messages.

So which is better: "Preflight Fail: Compass inconsistent" or "Preflight Fail: Compasses inconsistent"?

Antiheavy · 2018-09-28T21:04:06Z

src/modules/commander/PreflightCheck.cpp

 			}
 		}

 	} else {
 		if (!optional && report_fail) {
-			mavlink_log_critical(mavlink_log_pub, "PREFLIGHT FAIL: NO MAG SENSOR #%u", instance);
+			mavlink_log_critical(mavlink_log_pub, "Preflight Fail: Compass Sensor #%u missing", instance);


Edit: nevermind ~~for consistency with other messages suggest using: "Preflight Fail: Compass #%u missing"~~

dagar · 2018-09-29T13:17:31Z

I'll merge this now and we can continue with any additional suggestions in another pull request or issue.

bkueng added 3 commits September 27, 2018 16:47

mavlink_receiver: reduce s_variance_m_s for HIL GPS message from 1 to…

98d0b3e

… 0.1 1 was too high to pass the EKF2 test (EKF2_REQ_SACC which is 0.5 by default) and thus switching into position mode in HIL was not possible.

ekf2: only report gps failure flags that are enabled

383e4f9

So that they can be used for reporting errors.

PreflightChecks: improve labels by not capitalizing everything

c4c5df4

estimator status init is not required.

LorenzMeier previously approved these changes Sep 27, 2018

View reviewed changes

mhkabir reviewed Sep 27, 2018

View reviewed changes

bkueng dismissed LorenzMeier’s stale review via e600e7c September 27, 2018 15:38

bkueng force-pushed the gps_preflight_cleanup branch from 45f1c5a to e600e7c Compare September 27, 2018 15:38

bkueng added 2 commits September 27, 2018 17:50

PreflightCheck: improve error message in case there is no sensor data

fd7dd79

bkueng force-pushed the gps_preflight_cleanup branch from e600e7c to fd7dd79 Compare September 27, 2018 15:50

dagar reviewed Sep 27, 2018

View reviewed changes

dagar mentioned this pull request Sep 28, 2018

Move ArduPilot preflight check handling to generic QGC mavlink/qgroundcontrol#6898

Merged

Antiheavy reviewed Sep 28, 2018

View reviewed changes

bkueng added 2 commits September 28, 2018 11:29

mavlink_log: add mavlink_log_warning

0ab720d

PreflightCheck: update messages & use proper log level

56dfd55

PreflightCheck: rename 'Estimator internal checks' to 'Position unknown'

02722f9

LorenzMeier approved these changes Sep 28, 2018

View reviewed changes

dagar reviewed Sep 28, 2018

View reviewed changes

Antiheavy reviewed Sep 28, 2018

View reviewed changes

dagar approved these changes Sep 29, 2018

View reviewed changes

dagar merged commit df9a09c into master Sep 29, 2018

dagar deleted the gps_preflight_cleanup branch September 29, 2018 13:17

bkueng mentioned this pull request Oct 25, 2018

HIL no position lock #10752

Closed

Antiheavy mentioned this pull request Jan 20, 2019

Pr commander msg output cleanup #8175

Closed

GPS preflight check: improve reporting #10586

GPS preflight check: improve reporting #10586

Conversation

bkueng commented Sep 27, 2018 • edited by AuterionWrikeBot Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar commented Sep 27, 2018

bkueng commented Sep 27, 2018

dagar commented Sep 27, 2018 • edited Loading

dagar commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonLakeFlyer commented Sep 27, 2018

Antiheavy commented Sep 28, 2018

Choose a reason for hiding this comment

Antiheavy Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Antiheavy Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Antiheavy Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkueng commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Antiheavy Sep 28, 2018 • edited Loading

Choose a reason for hiding this comment

dagar commented Sep 29, 2018

bkueng commented Sep 27, 2018 •

edited by AuterionWrikeBot

Loading

dagar commented Sep 27, 2018 •

edited

Loading

Antiheavy Sep 28, 2018 •

edited

Loading

Antiheavy Sep 28, 2018 •

edited

Loading

Antiheavy Sep 28, 2018 •

edited

Loading

dagar Sep 28, 2018 •

edited

Loading

Antiheavy Sep 28, 2018 •

edited

Loading