Extend Failure Detector functionalities #14766

cmic0 · 2020-04-26T17:31:20Z

Describe problem solved by this pull request
Main goal of this PR is to extend Failure Detector logic to handle ESC arming state.

Some types of ESC require that you send them an arm command before they can actually spin.
If they succeed transitioning to the armed state, they will send back an acknowledgment to PX4.

This PR extends Failure Detector logic to handle this kind of information:
after the vehicle is armed Failure Detector will wait 300_ms to get an answer from the ESCs.
If within this time window all the ESCs do not report to be armed Failure Detector will auto disarm the vehicle.

Checks are performed using esc_status.esc_armed_flags .

During the development of this PR i've also addressed the following points:

Added autodisarm if system is in lockdown. Having the vehicle in lockdown but still armed can be really misleading from a UX standpoint.
Avoid triggering flight termination if vehicle is already in a lockdown state
Introduced a parameter COM_LKDOWN_TKO to extend/disable the "lockdown window".
In some development cases it might be super useful having this logic disabled.

bresch

Ok, we tested that internally for quite some time so it should be safe to merge. However, if we start to put all kinds of failure detection in this class, we might need to review the architecture (and also replace the bitmask with booleans).

bresch · 2020-05-26T07:04:52Z

I rebased on top of master to re-run the CI checks

bkueng · 2020-06-08T08:38:37Z

src/modules/commander/Commander.cpp

+						mavlink_log_critical(&mavlink_log_pub, "ESCs did not respond to arm request");
+					}
+
+				} else if (hrt_elapsed_time(&_time_at_takeoff) < (1_s * _param_com_lkdown_tko.get())) {


Why is this an else? It means during the first 500ms you don't check this.

I originally made it like this because if you use PWM outputs in the first 500ms the signal is still in the spoolup phase, thus the vehicle has not yet taken off.

This probably does not hold if you use UAVCAN, right?

No, and I'd like to avoid these kind of dependencies and implicit assumptions.

src/modules/commander/commander_params.c

bkueng · 2020-06-08T08:45:03Z

src/modules/commander/commander_params.c

+ * Time-out for detecting a failure after takeoff
+ *
+ * A non-zero, positive value specifies the timeframe in seconds within Failure Detector is allowed to put the vehicle into
+ * a lockdown state if attitude exceeds the defined limits.


This does not hold for all flight modes. Can you either clarify or better reference where it is clearly described?

bkueng · 2020-06-08T08:46:49Z

src/modules/commander/failure_detector/FailureDetector.cpp

-	bool status_changed = _status != FAILURE_NONE;
-	_status = FAILURE_NONE;
+
+	int attitude_fields_bitmask = _status & (FAILURE_ARM_ESCS - 1);


This is not very robust. I'd rather enumerate all attitude bits.

bkueng · 2020-06-08T08:49:05Z

src/modules/commander/failure_detector/FailureDetector.cpp

+		updated |= resetAttitudeStatus();
+	}
+
+	if (areEscSmart()) {


This does not do what I would expect from the method name.
Rather use _sub_esc_status.updated() directly.

bkueng · 2020-06-08T08:52:34Z

src/modules/commander/failure_detector/failure_detector_params.c

+
+/**
+ * Enable checks on ESCs that report their arming state.
+ *


Might be worth describing which checks and the consequences of a failed check

hamishwillee · 2020-06-09T00:34:21Z

Would I be correct in thinking that this has no docs implication? - current failure detector docs are here: http://docs.px4.io/master/en/config/safety.html#failure_detector

bresch · 2020-06-25T13:10:27Z

@cmic0 Could you please address Beat's comments please?

cmic0 · 2020-06-29T13:27:10Z

src/modules/commander/failure_detector/FailureDetector.cpp

-	bool status_changed = _status != FAILURE_NONE;
-	_status = FAILURE_NONE;
+
+	int attitude_fields_bitmask = _status & (FAILURE_ROLL | FAILURE_PITCH | FAILURE_ALT | FAILURE_EXT);


@bresch / @bkueng I could not come up with a better naming for this variable.. given that contains FAILURE_ALT and FAILURE_EXT is not really an attitude_fields_bitmask .. any suggestion?

non_esc_bitmask? Otherwise this is fine.

cmic0 · 2020-06-29T15:47:50Z

Would I be correct in thinking that this has no docs implication? - current failure detector docs are here: http://docs.px4.io/master/en/config/safety.html#failure_detector

@hamishwillee I'd say we would need to add something to this sentence:
The failure detector is active in all vehicle types and modes, except for those where the vehicle is expected to do flips (i.e. Acro mode (MC), Acro mode (FW), Rattitude and Manual (FW)).

as now can be disabled with COM_LKDOWN_TKO

src/modules/commander/commander_params.c

bkueng · 2020-06-30T08:02:53Z

src/modules/commander/failure_detector/FailureDetector.cpp

-	bool status_changed = _status != FAILURE_NONE;
-	_status = FAILURE_NONE;
+
+	int attitude_fields_bitmask = _status & (FAILURE_ROLL | FAILURE_PITCH | FAILURE_ALT | FAILURE_EXT);


non_esc_bitmask? Otherwise this is fine.

src/modules/commander/failure_detector/failure_detector_params.c

cmic0 · 2020-06-30T09:12:07Z

@bkueng addressed the final comments. Let me know if you want me to clean the commit history up or if this is already good to go

bkueng

Yes please clean up the history.
And one more detail, otherwise this looks good.

bkueng · 2020-06-30T09:19:12Z

src/modules/commander/failure_detector/FailureDetector.cpp

+		_esc_failure_hysteresis.set_state_and_update(false, time_now);
+		_status &= ~FAILURE_ARM_ESCS;
+		updated = true;


This should not always return true.

fixed in here https://github.com/PX4/Firmware/pull/14766/files#diff-84f12e22ed352e6c0b6b55e22b6f0628R212

Signed-off-by: Claudio Micheli <[email protected]>

- Introduced COM_LKDOWN_TKO parameter - Introduced auto disarm for lockdown state - Do not trigger flight termiantion if system is in lockdown Signed-off-by: Claudio Micheli <[email protected]>

Signed-off-by: Claudio Micheli <[email protected]>

cmic0 · 2020-06-30T11:47:53Z

src/modules/commander/failure_detector/FailureDetector.cpp

+		// reset ESC bitfield
+		_esc_failure_hysteresis.set_state_and_update(false, time_now);
+
+		if (_status & FAILURE_ARM_ESCS) {


@bkueng this should guarantee that the status is updated only if the bit is reset.

- auto disarm when locked down was added in #14766 to prevent user confusion in regular usage, but also breaks HITL where lockdown is enabled for safety - fixes #15686

cmic0 added the Admin: Enhancement (improvement) 💡 label Apr 26, 2020

cmic0 requested a review from bresch April 26, 2020 17:31

weekly-digest bot mentioned this pull request May 3, 2020

Weekly Digest (26 April, 2020 - 3 May, 2020) #14814

Closed

bresch previously approved these changes May 26, 2020

View reviewed changes

bresch dismissed their stale review via b3c10ea May 26, 2020 07:04

bresch force-pushed the pr-failure-detector branch from 1913a23 to b3c10ea Compare May 26, 2020 07:04

bresch self-requested a review May 26, 2020 07:04

bresch previously approved these changes May 26, 2020

View reviewed changes

bkueng reviewed Jun 8, 2020

View reviewed changes

bresch dismissed their stale review via 264f123 June 25, 2020 13:08

cmic0 commented Jun 29, 2020

View reviewed changes

bkueng reviewed Jun 30, 2020

View reviewed changes

cmic0 added 3 commits June 30, 2020 13:35

failure detector: added esc failures detection

7b81292

Signed-off-by: Claudio Micheli <[email protected]>

Commander: make optional tilt-check after takeoff (failure detector).

59ee4ff

- Introduced COM_LKDOWN_TKO parameter - Introduced auto disarm for lockdown state - Do not trigger flight termiantion if system is in lockdown Signed-off-by: Claudio Micheli <[email protected]>

Commander: failure detector, automatically disarm on a ESC failure

0fab8ff

Signed-off-by: Claudio Micheli <[email protected]>

cmic0 force-pushed the pr-failure-detector branch from 2f7e5cf to 0fab8ff Compare June 30, 2020 11:45

cmic0 commented Jun 30, 2020

View reviewed changes

bkueng approved these changes Jun 30, 2020

View reviewed changes

bresch merged commit 743898d into master Jul 1, 2020

bresch deleted the pr-failure-detector branch July 1, 2020 06:55

weekly-digest bot mentioned this pull request Jul 5, 2020

Weekly Digest (28 June, 2020 - 5 July, 2020) #15258

Closed

dagar mentioned this pull request Sep 6, 2020

In HIL mode, motor will be disarmed after 10s even if disable the auto disarmed function on the QGC #15686

Closed

dagar mentioned this pull request Sep 6, 2020

commander: HITL skip auto disarm if lockdown #15690

Merged

dagar added a commit that referenced this pull request Sep 6, 2020

commander: HITL skip auto disarm if lockdown

99b8d42

- auto disarm when locked down was added in #14766 to prevent user confusion in regular usage, but also breaks HITL where lockdown is enabled for safety - fixes #15686

dagar added a commit that referenced this pull request Sep 6, 2020

commander: HITL skip auto disarm if lockdown

1ac3110

- auto disarm when locked down was added in #14766 to prevent user confusion in regular usage, but also breaks HITL where lockdown is enabled for safety - fixes #15686

dagar added a commit that referenced this pull request Sep 6, 2020

commander: HITL skip auto disarm if lockdown

2bc4a5a

- auto disarm when locked down was added in #14766 to prevent user confusion in regular usage, but also breaks HITL where lockdown is enabled for safety - fixes #15686

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend Failure Detector functionalities #14766

Extend Failure Detector functionalities #14766

cmic0 commented Apr 26, 2020

bresch left a comment

bresch commented May 26, 2020

bkueng Jun 8, 2020

cmic0 Jun 29, 2020

bkueng Jun 30, 2020

bkueng Jun 8, 2020

bkueng Jun 8, 2020

bkueng Jun 8, 2020

cmic0 Jun 29, 2020

bkueng Jun 8, 2020

hamishwillee commented Jun 9, 2020

bresch commented Jun 25, 2020

cmic0 Jun 29, 2020

bkueng Jun 30, 2020

cmic0 commented Jun 29, 2020

bkueng Jun 30, 2020

cmic0 commented Jun 30, 2020

bkueng left a comment

bkueng Jun 30, 2020

cmic0 Jun 30, 2020

cmic0 Jun 30, 2020

Extend Failure Detector functionalities #14766

Extend Failure Detector functionalities #14766

Conversation

cmic0 commented Apr 26, 2020

bresch left a comment

Choose a reason for hiding this comment

bresch commented May 26, 2020

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

hamishwillee commented Jun 9, 2020

bresch commented Jun 25, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmic0 commented Jun 29, 2020

Choose a reason for hiding this comment

cmic0 commented Jun 30, 2020

bkueng 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