FailureDetector (safe version) #10202
Conversation
bresch
added
Admin: Enhancement (improvement) 💡
Fixed Wing 🛩️
Multirotor 🛸
Hybrid VTOL 🛩️🚁
| * @unit degrees | ||
| * @group Commander | ||
| */ | ||
| PARAM_DEFINE_INT32(COM_FAIL_R, 60); |
There was a problem hiding this comment.
If you foresee the list of FailureDetector parameters growing I'd break it off into a new group (and param file) now.
| @@ -2385,6 +2386,32 @@ Commander::run() | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| /* Check for failure detector status */ | |||
| if (armed.armed) { | |||
There was a problem hiding this comment.
&& vehicle_control_mode.flag_control_attitude_enabled?
There was a problem hiding this comment.
I would like to leave it like this as it is just an absolute attitude check and not an attitude error check (for now).
|
Shouldn't this be based on the error rather than absolute angle? This shouldn't be oblivious to the controller configuration. |
Yes, but he problem is the setpoint jumps... |
I know, but I'm talking about more of a usability concern. The user shouldn't be able to configure this such that it triggers under normal operation. This is one of the reasons I had originally wondered about the controller being responsible for raising the error condition, and commander the actual triggering. |
|
@dagar Absolutely, we might add a check to make sure the parameters are larger than the max roll/pitch values of the controller. Basically, with this PR, I just want to introduce the general skeleton of the failure detector. The logic inside is really simple and will be improved later. |
|
Let me start by saying the PR in its current form looks good and I'm happy to merge it once we get it to fit.
Yes exactly, but my (future) concern is that this piece within commander then becomes dependent on attitude controller internals (particular parameters for each vehicle and how they're used in different contexts). Which is one of the reasons I get hung up on the architecture now for something so simple. |
msg/vehicle_status.msg
Outdated
| @@ -67,7 +74,7 @@ bool high_latency_data_link_active # all low latency datalinks to GCS lost | |||
| uint8 data_link_lost_counter # counts unique data link lost events | |||
| bool engine_failure # Set to true if an engine failure is detected | |||
| bool mission_failure # Set to true if mission could not continue/finish | |||
| bool attitude_failure # Set to true if attitude failure has been detected | |||
| uint8 failure_detector_status # Bitmask containing FailureDetector status [0, 0, 0, 0, 0, FAILURE_ALT, FAILURE_PITCH, FAILURE_ROLL] | |||
There was a problem hiding this comment.
This is fine, but you can feel free to use bools here if you want. The message is low rate so the space savings are negligible. For code though it requires more work to get or set a bit, and then for log analysis it ends up being a mess because the constants are lost.
For example estimator_status bitfields (https://github.com/PX4/Firmware/blob/master/msg/estimator_status.msg#L25) ended up being a poor choice. These constants have been manually duplicated across both ecl and Firmware, the ecl analysis python scripts under Tools, in Flight review, etc. They're also fairly inaccessible with flightplot.
tl;dr - booleans + publish on change is your friend
|
Could you please rebase? Thanks! |
bresch
force-pushed
the
pr-failure-detector-safe
branch
from
c03b6f9 to
f01e816
Compare
|
Rebased on master |
|
@dagar Is it fine to merge now? |
| * @min 0 | ||
| * @max 180 | ||
| * @unit degrees | ||
| * @group FailureDetector |
There was a problem hiding this comment.
This is the group shown in the QGC parameter tree and contain spaces.
eg @group FW L1 Control
|
That's odd, github won't let me "Rebase and merge", but I could still "Squash and merge". If you're fine with this going into master as single merged commit we can merge now, otherwise please rebase on master and we'll try again. |
…puts PX4_ERR messages if roll or pitch is exceeded.
…s. Rename 'result' into 'updated'
…nd io. A Null mixer can now be used to set a fixed value: "disarmed" if the system is operational or "failsafe" in failsafe mode
- use pragma once guard in FailureDetector.hpp - send Commander parent to ModuleParams and remove update_params() method - simplify attitude checks - FailureDetector::get_status() (previously named "get") is now a constant method that returns a constant reference
…protection; remove PX4_WARN and send mavlink msg only once
…atus msg (failure_detector_status) and instead of struct in class
|
@dagar Rebased on master! :) Hope it works now |
bresch
force-pushed
the
pr-failure-detector-safe
branch
from
5ba165b to
1df43ba
Compare
|
Thanks, everything looks good now. |
Save version of #10179
This PR has no "dangerous" changes as it only sets a flag bit in
failure_detector_statusinvehicle_statusbut doesn't activate a failsafe action.This PR needs to go in master for testing and is completely invisible to the user.
The failsafe triggering part will come in a future PR.
FYI @LorenzMeier @dagar @JohannesBrand