AP_AHRS: give clearer prearm messages #24243

Hwurzburg · 2023-07-07T15:32:22Z

this comes up over and over....
two alternatives for improvement....the latter commit being suggested by Tim T. and I think I agree....at least it gives the user directions on how to resolve it...

but this is about as clear as you can make it unfortunately since the inconsistency can be from many sources

whichever the group decides on I will remove the other/squash

@peterbarker we need to discuss TIm's idea of automating this page also : https://ardupilot.org/plane/docs/common-prearm-safety-checks.html....I sent you a metadata format proposal

edit: Peter and I have talked at length....there are 3 options:

I try to improve the wiki page on prearms....which I have done
we try to add metadata to selected cryptic prearms like this and parse to auto update the wiki docs in some manner...currently there are over ~400 potential messages although many don't need explanation
we add a mechanism to provide the GCS with json base information on selected prearms that allow clicking in the messages screen to provide detailed resolution data

#3 is a very large effort
#2 is many days of python work and testing plus the C++ effort (which Peter is unable to do with his current load)
#1 is done, but any other nagging messages can be added and expanded...

inflight error messages are another topic and beyond scope here

timtuxworth · 2023-07-07T16:04:24Z

So there are 2 completely different use cases here.

regular user who just needs to take the plane outside to get a satellite fix - this user will never set AHRS_TYPE, probably doesn't even know or care what AHRS, EKF3, EKF2 etc are.
more advanced or experimental user who is messing with AHRS_TYPE or other low level configuration. This user is probably comfortable looking in the log post flight and trying to jam meaningful information into 50 characters for them is really a wast of time - the data is in the log.

Hwurzburg · 2023-07-07T16:08:32Z

there are many more ways to get the inconsistency than just sufficient GPS....

and its easy for a user upgrading from older firmware to mess up AHRS_TYPE when moving to EKF3....so AHRS_TYPE issues have come mainly from normal users...so the not using AHRS type message is 100% needed and its exactly clear that you are not using the selected AHRS type....anyway this PR only deals with the filter inconsistency messages

timtuxworth · 2023-07-07T16:19:27Z

#24243 is much better (I've added comments), but users will still be confused by:

DCM/EKF2/EKF3 - they should not need to care about this. IMO this bleeding of developer interests into the UI for end users is very unfortunate.

As a user/pilot I am still very confused about what it actually means if EKF is complaining about Roll/Pitch or Yaw.

timtuxworth

In essence adding "Wait or check sensors" is great, but "R/P" not so great. By looking at the code that triggers this
const float rp_diff_rad = primary_quat.roll_pitch_difference(ekf2_quat);
I figured using "difference" vs "inconsistent" - 2 characters would be saved which gives us the full word "roll/pitch" back

timtuxworth · 2023-07-07T15:59:17Z

libraries/AP_AHRS/AP_AHRS.cpp

@@ -2277,7 +2277,7 @@ bool AP_AHRS::attitudes_consistent(char *failure_msg, const uint8_t failure_msg_
            // check roll and pitch difference
            const float rp_diff_rad = primary_quat.roll_pitch_difference(ekf2_quat);
            if (rp_diff_rad > ATTITUDE_CHECK_THRESH_ROLL_PITCH_RAD) {
-                hal.util->snprintf(failure_msg, failure_msg_len, "EKF2 Roll/Pitch inconsistent by %d deg", (int)degrees(rp_diff_rad));
+                hal.util->snprintf(failure_msg, failure_msg_len, "EKF2: R/P inconsistent. Wait or check sensors");


The second part is great. Sorry to say but R/P doesn't help a non developer pilot. My reaction would be "WTF is R/P" if I get this message while on the field getting ready to fly.

Also - does a pilot/user really know or care that it is EKF2 (or 3 or DCM) - answer no and it only confuses them.

How about: "EKF2: Roll/Pitch difference. Wait or check sensors"
using "difference" instead of "inconsistent" saves 2 characters and gets it into 50 characters".

its not a roll to pitch difference which gets implied.....inconsistent is more accurate....alternatively... Atittude inconsistent instead of Roll/Pitch although yaw is considered attitude also...I think the commit language is best as is

If you used "EKF3: Attitude off by 10 deg. Wait/check sensors" you could also use the same message for Yaw (which is also attitude) - so you could save about 45 bytes of flash by reusing the string.

Actually if you used "%s: Attitude off by %d deg. Wait/check sensors" and pass "EKF3"/"EKF2"/"DCM" as a parameter you save even more, by replacing all the messages with this one string.

Even for me as someone who somehow knows what happens inside the EKF would be confused by R/P, Roll/Pitch would be way better.

timtuxworth · 2023-07-07T16:12:21Z

libraries/AP_AHRS/AP_AHRS.cpp

@@ -2286,7 +2286,7 @@ bool AP_AHRS::attitudes_consistent(char *failure_msg, const uint8_t failure_msg_
            primary_quat.angular_difference(ekf2_quat).to_axis_angle(angle_diff);
            const float yaw_diff = fabsf(angle_diff.z);
            if (check_yaw && (yaw_diff > ATTITUDE_CHECK_THRESH_YAW_RAD)) {
-                hal.util->snprintf(failure_msg, failure_msg_len, "EKF2 Yaw differs from current AHRS by %d deg", (int)degrees(yaw_diff));
+                hal.util->snprintf(failure_msg, failure_msg_len, "EKF2: Yaw inconsistent, Wait or check sensors");


To be consistent with Roll/Pitch - "EKF2: Yaw difference. Wait or check sensors"

timtuxworth · 2023-07-07T16:13:12Z

libraries/AP_AHRS/AP_AHRS.cpp

@@ -2313,7 +2313,7 @@ bool AP_AHRS::attitudes_consistent(char *failure_msg, const uint8_t failure_msg_
            primary_quat.angular_difference(ekf3_quat).to_axis_angle(angle_diff);
            const float yaw_diff = fabsf(angle_diff.z);
            if (check_yaw && (yaw_diff > ATTITUDE_CHECK_THRESH_YAW_RAD)) {
-                hal.util->snprintf(failure_msg, failure_msg_len, "EKF3 Yaw differs from current AHRS by %d deg", (int)degrees(yaw_diff));
+                hal.util->snprintf(failure_msg, failure_msg_len, "EKF3: Yaw inconsistent, Wait or check sensors");


"EKF3: Yaw difference. Wait or check sensors"

timtuxworth · 2023-07-07T16:15:21Z

libraries/AP_AHRS/AP_AHRS.cpp

@@ -2329,7 +2329,7 @@ bool AP_AHRS::attitudes_consistent(char *failure_msg, const uint8_t failure_msg_
        // check roll and pitch difference
        const float rp_diff_rad = primary_quat.roll_pitch_difference(dcm_quat);
        if (rp_diff_rad > ATTITUDE_CHECK_THRESH_ROLL_PITCH_RAD) {
-            hal.util->snprintf(failure_msg, failure_msg_len, "DCM R/P differs from current AHRS by %d deg", (int)degrees(rp_diff_rad));
+            hal.util->snprintf(failure_msg, failure_msg_len, "DCM: R/P inconsistent. Wait or check sensors");


"DCM: Roll/Pitch difference. Wait or check sensors"
for consistency with EKF2 and EKF3

again, disagree with your suggestion

timtuxworth · 2023-07-07T16:15:45Z

libraries/AP_AHRS/AP_AHRS.cpp

@@ -2344,7 +2344,7 @@ bool AP_AHRS::attitudes_consistent(char *failure_msg, const uint8_t failure_msg_
            primary_quat.angular_difference(dcm_quat).to_axis_angle(angle_diff);
            const float yaw_diff = fabsf(angle_diff.z);
            if (check_yaw && (yaw_diff > ATTITUDE_CHECK_THRESH_YAW_RAD)) {
-                hal.util->snprintf(failure_msg, failure_msg_len, "DCM Yaw differs from current AHRS by %d deg", (int)degrees(yaw_diff));
+                hal.util->snprintf(failure_msg, failure_msg_len, "DCM: Yaw inconsistent, Wait or check sensors");


DCM: Yaw difference. Wait or check sensors.

IamPete1 · 2023-07-08T02:19:34Z

DCM/EKF2/EKF3 - they should not need to care about this. IMO this bleeding of developer interests into the UI for end users is very unfortunate.

We should aspire to bring users to a level of higher understanding not dumb down the interface to the lowest level. I agree these messages could be better, but we can make them better by adding more information not by removing what is already there.

Think of these messages like the engine low temperature light in your car. Most people don't care or pay much attention. The blue light goes out after a bit once the engine has got up to temperature, all good. However some engines will break if you run them too hard when there cold, some drivers like to look after there engines and let them warm up before going for a drive. Unlike car manufactures we don't have the benefit of knowing what sort of car our warning light will be fitted in. So we have to provide as much information as possible (as I say we could do better here) and leave it to the user to ignore or not based on there use case.

rmackay9 · 2023-07-08T02:52:11Z

I think we (including me) should work on a big "errors and warnings" wiki page that gives instructions to the user on what they can do in these situtations. I think that it's going to be very difficult to provide enough advice to the user in just 54 characters no matter how hard we try.

timtuxworth · 2023-07-08T03:00:26Z

We should aspire to bring users to a level of higher understanding not dumb down the interface to the lowest level. I agree these messages could be better, but we can make them better by adding more information not by removing what is already there.

In 50 characters?

How about user friendly messages, with an "ADVANCED_MESSAGES_ENABLE" setting that displays these detailed messages, in addition to the user message?

timtuxworth · 2023-07-08T03:02:54Z

Think of these messages like the engine low temperature light in your car. Most people don't care or pay much attention. The blue light goes out after a bit once the engine has got up to temperature, all good. However some engines will break if you run them too hard when they're cold, some drivers like to look after there engines and let them warm up before going for a drive. Unlike car manufactures we don't have the benefit of knowing what sort of car our warning light will be fitted in. So we have to provide as much information as possible (as I say we could do better here) and leave it to the user to ignore or not based on their use case.

Great example. The mechanic connects the engine diagnostics to a computer in the workshop to get the full picture. (just like a developer reviewing a log file after a flight) The "light" is the dumbed down UI for the user. That is EXACTLY what we need! Specifically: "Check engine" - there is a check engine light.

The ArduPilot messages need to tell the pilot/user

what is wrong
2. what they should do about it

Hwurzburg · 2023-07-08T03:04:42Z

@rmackay9 > I think we (including me) should work on a big "errors and warnings" wiki page that gives instructions to the user on what they can do in these situtations. I think that it's going to be very difficult to provide enough advice to the user in just 54 characters no matter how hard we try.

there already exists that page....it needs updating...and perhaps automation...https://ardupilot.org/plane/docs/common-prearm-safety-checks.html

timtuxworth · 2023-07-08T03:24:48Z

That page only refers to pre-arms @Hwurzburg - are you suggesting to make it more general?

Hwurzburg · 2023-07-08T12:12:10Z

prearms are the only thing that prevents a user from flying ...in flight messages are almost always just informational and most the user can do nothing about while flying....yaw resets, lane switches, etc......and the normal feedback messages like transitions being complete and waypoints being crossed....if there are any that need user intervention and are unclear, we could add a page for them...aside from watchdogs, which we have a page for, I cant think of user actionable inflight messages off hand that are not clear
making GCS messages totally dumbed down is not a good solution in my opinion....helping a user resolve arming issues is a good goal however

IamPete1 · 2023-07-08T12:29:13Z

Great example. The mechanic connects the engine diagnostics to a computer in the workshop to get the full picture. (just like a developer reviewing a log file after a flight) The "light" is the dumbed down UI for the user. That is EXACTLY what we need! Specifically: "Check engine" - there is a check engine light.

Maybe I didn't think my analogy through enough. This message is not the warning light, it is a hint about extra information the mechanic needs. The warning light is the vehicle refusing to arm.

The ArduPilot messages need to tell the pilot/user

what is wrong
2. what they should do about it

In most cases I disagree that we should tell users that to do, the action is often not clear to ArduPilot, if it was then we would do it automatically. There are some exceptions, like the pre-arm that you need to reboot after compass calibration, where there is a single action that will resolve. However this EKF stuff is a prime example of a issue that could be caused by a number of things. We should provide as much information about the issue as possible. The advice on how to resolve should be elsewhere, we probably could do a better job at documenting such things, maybe some flow charts or similar. I guess in that context the what they should do becomes "go and have a look at this flow chart".

Maybe some sort of metadata could be provided to the GCS as we do for params and logs. There could be a regex for the pre-arm and a link to a page giving more information.

timtuxworth · 2023-07-08T15:30:24Z

prearms are the only thing that prevents a user from flying ...in flight messages are almost always just informational and most the user can do nothing about while flying....yaw resets, lane switches, etc......and the normal feedback messages like transitions being complete and waypoints being crossed....if there are any that need user intervention and are unclear, we could add a page for them...aside from watchdogs, which we have a page for, I cant think of user actionable inflight messages off hand that are not clear
making GCS messages totally dumbed down is not a good solution in my opinion....helping a user resolve arming issues is a good goal however

In my mind I've linked this to the "Lane Switch" message PR #24181 - it's basically the same thing - the user experience for a non-developer is being muddied with information designed to help developers - generally causing confusion.

timtuxworth · 2023-07-08T15:37:53Z

Maybe some sort of metadata could be provided to the GCS as we do for params and logs. There could be a regex for the pre-arm and a link to a page giving more information.

Excellent idea! The metadata could also be harvested directly into he wiki - right @Hwurzburg ? If you get something started I would be happy to contribute.

Hwurzburg · 2023-07-08T16:03:50Z

I am discussing with Peter already...see the description box of the PR

Hwurzburg · 2023-07-08T16:29:09Z

fyi...there are over 280 prearm/arming failure messages..just in the ARMING modules ...most are not cryptic....only the cryptic ones need be metadata tagged and searchable in the wiki
failuremessages.txt

there are probably a hundred more in the various other modules

timtuxworth · 2023-07-11T01:21:29Z

fyi...there are over 280 prearm/arming failure messages..just in the ARMING modules ...most are not cryptic....only the cryptic ones need be metadata tagged and searchable in the wiki failuremessages.txt

there are probably a hundred more in the various other modules

Who defines "cryptic"? I'm thinking there are messages you (or even I) might think are obvious that others might need more explanation.

Hwurzburg · 2023-07-11T01:29:26Z

Who defines "cryptic"? I'm thinking there are messages you (or even I) might think are obvious that others might need more explanation.

anyone who provides end user support as support engineers or admins the RCG or Facebook groups or monitors the help channels would have exposure to users complaining: "what do I do when I get this?"

Hwurzburg · 2023-07-17T15:05:34Z

Flash reduction via refactor is good but scope creep...I will leave that to someone else...
after all the discussion inputs, I have chosen a slightly different message and this I what I will go with for the devcall today...it informs precisely why the prearm occurs, although the possible causes cannot be delineated in a one line message, nor a precise solution so I think this is the best that we can do at this point...

timtuxworth · 2023-07-17T19:49:20Z

I don't see how adding "AHRS" helps at all. For those that don't know what that is, it's just babble. For those who do know, they will already know this is about AHRS. I'd suggest dropping "with current AHRS", but either way I think it's better than what we get now.

Hwurzburg · 2023-07-17T20:10:04Z

then EKF or DCM will be similarly lost...but if they get these messages and they don't resolve themselves they will have to learn what AHRS and EKF mean....to relay the issue at the least to more knowledgeable people...any search of the wiki for EKF
explains what they are

rmackay9 · 2023-07-17T23:33:21Z

I agree that adding "AHRS" won't help most users. They will just wonder, "What's an EKF vs an AHRS?". We also need to be careful that the mesasges don't become too long because then the message will wrap and won't be clearly visible on the MP HUD.

peterbarker · 2023-07-18T00:01:13Z

libraries/AP_AHRS/AP_AHRS.cpp

@@ -2277,7 +2277,7 @@ bool AP_AHRS::attitudes_consistent(char *failure_msg, const uint8_t failure_msg_
            // check roll and pitch difference
            const float rp_diff_rad = primary_quat.roll_pitch_difference(ekf2_quat);
            if (rp_diff_rad > ATTITUDE_CHECK_THRESH_ROLL_PITCH_RAD) {
-                hal.util->snprintf(failure_msg, failure_msg_len, "EKF2 Roll/Pitch inconsistent by %d deg", (int)degrees(rp_diff_rad));
+                hal.util->snprintf(failure_msg, failure_msg_len, "EKF2 Roll/Pitch inconsistent with current AHRS. Wait or check sensors");


Suggested change

hal.util->snprintf(failure_msg, failure_msg_len, "EKF2 Roll/Pitch inconsistent with current AHRS. Wait or check sensors");

hal.util->snprintf(failure_msg, failure_msg_len, "%s Roll/Pitch inconsistent with current AHRS. Wait or check sensors", "EKF2");

etc elsewhere.

This will let the compiler re-use the format string, actually saving bytes on this PR.

.... could potentially be

Suggested change

hal.util->snprintf(failure_msg, failure_msg_len, "EKF2 Roll/Pitch inconsistent with current AHRS. Wait or check sensors");

hal.util->snprintf(failure_msg, failure_msg_len, "%s %s inconsistent with current AHRS. Wait or check sensors", "EKF2", "Roll/Pitch");

Doesn't look like it was done in this PR.

This branch shows what I meant; feel free to cherry-pick the patch :-)

https://github.com/ArduPilot/ardupilot/compare/master...peterbarker:ardupilot:pr/save-bytes?expand=1

That branch saves 224 bytes vs master, whereas this one costs 80-odd bytes,

okay done....credit given...thanks

libraries/AP_AHRS/AP_AHRS.cpp

rmackay9

Looks good, thanks!

Co-authored-by: Peter Barker <[email protected]>

peterbarker · 2023-07-24T01:51:58Z

libraries/AP_AHRS/AP_AHRS.cpp

@@ -2257,6 +2257,11 @@ void AP_AHRS::getCorrectedDeltaVelocityNED(Vector3f& ret, float& dt) const
    ret.z += GRAVITY_MSS*dt;
 }

+void AP_AHRS::set_failure_inconsistent_message(const char *estimator, const char *axis, float diff_rad, char *failure_msg, const uint8_t failure_msg_len) const


This is my patch, but I could've used snprintf_failure_inconsistent_message here

rmackay9 · 2023-07-31T23:58:14Z

We agreed on the dev call that we can merge once Henry has confirmed he's completed testing.

rmackay9 · 2023-08-01T00:08:52Z

Tested and above is a screen shot from SITL.

Hwurzburg force-pushed the messages branch from 5807cde to 8212d1c Compare July 7, 2023 15:55

Hwurzburg requested review from tridge, rmackay9 and peterbarker July 7, 2023 16:20

timtuxworth suggested changes Jul 7, 2023

View reviewed changes

Hwurzburg added the DevCallTopic label Jul 7, 2023

timtuxworth mentioned this pull request Jul 8, 2023

EKF: suppress lane switch warning #24181

Open

Hwurzburg force-pushed the messages branch 2 times, most recently from 36a52c8 to 16e0286 Compare July 17, 2023 15:00

Hwurzburg changed the title ~~AP_AHRS: give clearer prearm messages~~ AP_AHRS: give clearer prearm messagesAP_AHRS:Improve prearm messages for filter inconsistencies Jul 17, 2023

peterbarker requested changes Jul 18, 2023

View reviewed changes

CraigElder removed the DevCallTopic label Jul 18, 2023

tridge requested changes Jul 18, 2023

View reviewed changes

libraries/AP_AHRS/AP_AHRS.cpp Outdated Show resolved Hide resolved

Hwurzburg force-pushed the messages branch from 16e0286 to 28e2f55 Compare July 18, 2023 00:45

rmackay9 reviewed Jul 18, 2023

View reviewed changes

libraries/AP_AHRS/AP_AHRS.cpp Show resolved Hide resolved

Hwurzburg force-pushed the messages branch from 28e2f55 to 45716b5 Compare July 18, 2023 01:29

Hwurzburg changed the title ~~AP_AHRS: give clearer prearm messagesAP_AHRS:Improve prearm messages for filter inconsistencies~~ AP_AHRS: give clearer prearm messages Jul 18, 2023

rmackay9 approved these changes Jul 18, 2023

View reviewed changes

rmackay9 added the MergeOnCIPass label Jul 18, 2023

Hwurzburg force-pushed the messages branch from 45716b5 to 97328d5 Compare July 22, 2023 11:37

Hwurzburg removed the MergeOnCIPass label Jul 22, 2023

Hwurzburg requested review from peterbarker and tridge July 22, 2023 11:40

AP_AHRS:improve filter inconsistent messages

9733a9d

Co-authored-by: Peter Barker <[email protected]>

Hwurzburg force-pushed the messages branch from 97328d5 to 9733a9d Compare July 24, 2023 00:25

Hwurzburg requested a review from rmackay9 July 24, 2023 00:26

Hwurzburg added the DevCallTopic label Jul 24, 2023

peterbarker approved these changes Jul 24, 2023

View reviewed changes

CraigElder removed the DevCallTopic label Jul 31, 2023

rmackay9 merged commit 0aef2cc into ArduPilot:master Aug 1, 2023

Hwurzburg deleted the messages branch August 1, 2023 00:11

	hal.util->snprintf(failure_msg, failure_msg_len, "EKF2 Roll/Pitch inconsistent with current AHRS. Wait or check sensors");
	hal.util->snprintf(failure_msg, failure_msg_len, "%s Roll/Pitch inconsistent with current AHRS. Wait or check sensors", "EKF2");

AP_AHRS: give clearer prearm messages #24243

AP_AHRS: give clearer prearm messages #24243

Conversation

Hwurzburg commented Jul 7, 2023 • edited Loading

timtuxworth commented Jul 7, 2023

Hwurzburg commented Jul 7, 2023

timtuxworth commented Jul 7, 2023

timtuxworth left a 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

Hwurzburg Jul 7, 2023 • edited Loading

Choose a reason for hiding this comment

timtuxworth Jul 8, 2023 • 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

IamPete1 commented Jul 8, 2023

rmackay9 commented Jul 8, 2023

timtuxworth commented Jul 8, 2023

timtuxworth commented Jul 8, 2023 • edited Loading

Hwurzburg commented Jul 8, 2023

timtuxworth commented Jul 8, 2023

Hwurzburg commented Jul 8, 2023 • edited Loading

IamPete1 commented Jul 8, 2023

timtuxworth commented Jul 8, 2023 • edited Loading

timtuxworth commented Jul 8, 2023 • edited Loading

Hwurzburg commented Jul 8, 2023

Hwurzburg commented Jul 8, 2023 • edited Loading

timtuxworth commented Jul 11, 2023

Hwurzburg commented Jul 11, 2023 • edited Loading

Hwurzburg commented Jul 17, 2023

timtuxworth commented Jul 17, 2023

Hwurzburg commented Jul 17, 2023 • edited Loading

rmackay9 commented Jul 17, 2023

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

rmackay9 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmackay9 commented Jul 31, 2023

rmackay9 commented Aug 1, 2023

Hwurzburg commented Jul 7, 2023 •

edited

Loading

Hwurzburg Jul 7, 2023 •

edited

Loading

timtuxworth Jul 8, 2023 •

edited

Loading

timtuxworth commented Jul 8, 2023 •

edited

Loading

Hwurzburg commented Jul 8, 2023 •

edited

Loading

timtuxworth commented Jul 8, 2023 •

edited

Loading

timtuxworth commented Jul 8, 2023 •

edited

Loading

Hwurzburg commented Jul 8, 2023 •

edited

Loading

Hwurzburg commented Jul 11, 2023 •

edited

Loading

Hwurzburg commented Jul 17, 2023 •

edited

Loading