-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Add prearm check surrounding presence of crashdump data #26710
Add prearm check surrounding presence of crashdump data #26710
Conversation
296ae40
to
79d5860
Compare
79d5860
to
2c1693b
Compare
I've retested this after applying @tridge 's suggestions. |
2c1693b
to
6ce1028
Compare
Just one suggestion, you could add a few words to the parameter description explaining what a watchdog means - eg "the firmware rebooted, which may happen in-flight, causing the vehicle to crash" or something if you want. Otherwise people might just set the parameter anyway. The other thing I'm wondering, is what happens with a watchdog on a Plane, for example - would this mean that when it comes back up you can't arm with the arm switch/rudder arming (eg people who don't have a GCS), or are there usually some pre-arms anyway? (I remember something about a there being a "fast boot" after a watchdog that skips a lot of the checks?) |
Yep, you're quite right, you can now force-arm past this. |
ARMING_CRASHACK ? ARMING_CRASHDUMP? ARMING_CRASH_IGN ? |
#if AP_CRASHDUMP_ACK_ENABLED | ||
// @Param: CRSHDMP_ACK | ||
// @DisplayName: CrashDump Acknowledgement Required | ||
// @Description: Must have value "1" if a crashdump is present on the system, or prearm failure will be raised. Do not set this parameter unless the risks of doing so are fully understood. The presence of a crash dump means that the firmware currently installed has crashed with a hardfault or watchdog. The crash dump file gives diagnostic information which can help in finding the issue. Please check the ArduPilot documentation for more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arming check enable/disable when CrashDumps files are found on the SD Card. CrashDumps are saved after critical software failures resulting in the autopilot immediately rebooting. If these are present the vehicle is likely unsafe to fly. Please report to the ArduPilot support team.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Arming check enable/disable when CrashDumps files are found on the SD Card. CrashDumps are saved after critical software failures resulting in the autopilot immediately rebooting. If these are present the vehicle is likely unsafe to fly. Please report to the ArduPilot support team.
We do put crashdump data onto SD cards - but that's not the data this PR is checking. CrashDump flashes it into the STM32 flash area. We copy it from there onto the SD card on subsequent boots.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've adjusted the wording here. It's turning into a bit of an essay.
I'm not sure what you mean? I mentioned the example of people who don't always have a groundstation connected. They won't be able to force-arm at all, right? So my question was whether this check would mean that they now can't arm at all, or whether there's usually other pre-arms active anyway, meaning that people without a GCS will already not be able to re-arm? |
Yeah, lots of people fly without GCS using yaapu. This feels like it could be a usability impediment with people unable to arm but unsure why and lots of complaints on the forums. I'm not convinced making this the default is the right thing to do... |
Given how frequently people just miss the current notification about there being a crash dump, I think turning it into a pre-arm is necessary. But if we can make it not be a pre-arm on the first bootup after the watchdog (when they might still be in the air) then that would really help... |
CrashDumps should be very rare and I'm not sure we want users just going off and flying again after one of these. |
ArduPilot/WebTools#137 adds a alert to the Hardware Report WebTool. We just need to decide where were going to send users to make a report (both that and this arming check should send users to the same place) |
Why a new parameter? Why not use ARMING_OPTIONS? |
If we use a bit in |
Yep, good call. We bypass this check - Plane Just Does That when we've detected a watchdog-when-armed event. My initial formulation PR'd didn't have this behaviour, I changed it to not be a mandatory check to allow for mid-air-rearm. |
6ce1028
to
4e9e50a
Compare
I've force-pushed this branch with my re-tested changes. It is now wholly contained within AP_Arming which is nice. I think I've addressed all comments here, with the exception of Andy's musings on people that don't have a GCS being unable to get themselves out of this situation. Randy's rebutted that saying they probably shouldn't be flying anyway - but OTOH we are offering this parameter to anybody with a GCS! Fundamentally we need the people running yaapu to acknowledge the problem. I've often thought it would be nice to be able to set an arbitrary parameter with yaapu, even if it is a little painful interface-wise (less so with touch-screen input!). I don't think we should delay merging this based on an inability of non-GCS-users to work around it in the field. I do acknowledge there's a UI problem there, but at least with yaapu GCS there should be a way forward. We've seen direct evidence that people are flying very large, heavy, expensive, and dangerous vehicles being armed with firmware which has created crash-dumps, and we need something in to stop that happening. |
I did not add this to 4.5.2-beta1 due to a merge conflict |
Under certain circumstances ArduPilot writes "CrashDump" data to flash memory, past where the ArduPilot code resides. It is cleared when the flash is zeroed (e.g. when flashing a new firmware).
When a log file is opened, we log this data into that log file and warn the user that we've had to do that.
At boot time we attempt to save the dump to the SD card, and warn the user we've done that.
Those two things were insufficient to make users aware that the firmware present on the board had crashed at some point in the past. This PR makes the presence of crashdump data impossible to miss by adding a pre-arm check which fails if the crashdump data is present.
It is possible to set a new parameter
BRD_CRSHDMP_ACK
to 1 to avoid the pre-arm failure. In that case we still emit a warning as part of the prearm logic. This parameter is set to zero any time the board boots and no crash dump data is present, so the user must acknowledge the presence of any new crashdump data which gets written.In the following screenshot the "acknowledgement" parameter was written, allowing the vehicle to then arm:
I have checked that re-flashing the firmware clears the ack flag, forcing re-ack of further crashes.