Commander/preflight checks: Add monitoring to ESC failures #16704

cmic0 · 2021-01-31T21:08:23Z

Resurrected version of #14800.

Describe problem solved by this pull request
This PR extends monitoring to failures reported by ESCs with telemetry.

ESC checks are enabled/disabled with the COM_ARM_CHK_ESCS parameter.

Test data / coverage
I've been flying this exact code with smart ESCs since the first PR was opened.

cmic0 · 2021-01-31T21:27:30Z

@dagar this was also a suggestion you had back in the time

Did you consider splitting the esc_status update check and error reporting and separately check for timeouts and missing ESCs? You shouldn't need to copy esc_status (a rather large message with 8 esc_reports) every iteration.

I think would make sense to do it

dagar · 2021-01-31T21:59:15Z

src/modules/commander/Commander.cpp

+
+				if (esc_status.esc[index].failures != _last_esc_failure[index]) {
+
+					if (esc_status.esc[index].failures & esc_report_s::FAILURE_OVER_CURRENT_MASK) {


These aren't really generic failures (not used by UAVCAN, DSHOT, etc), perhaps the error message should be confined to the specific module?

Mh not really, I've seen this as a recurring pattern where people with proprietary ESCs ended up doing the logic in the driver (and the failures ended up to be almost the same). I think that this is a good first step towards supporting such edge cases

Alright, let's see how things continue to evolve.
I'm trying to fine the line where commander is tied in such that it's aware of the components and high level health status, but doesn't have to have specific intimate knowledge unless it's actually going to do something with that information.

dagar · 2021-01-31T22:00:55Z

src/modules/commander/Commander.cpp

+	} else {
+
+		if (_esc_status_was_updated && !_status_flags.condition_escs_error) {
+			mavlink_log_critical(&_mavlink_log_pub, "ESCs telemetry timeout");


Is this user actionable? If it's not going to block arming or trigger a failsafe in flight then I don't think it's worth sending to a user. You could change it to PX4_WARN to catch in the logs for review later.

actually now i remember the use case for this:
if you have ESC that are reporting telemetry but the telemetry line is completely disconnected from the vehicle you would be allowed to arm without the telemetry line.
This check guarantees that if this situation happens user is then warned. What i usually do is publishing an esc_status message right when the driver starts, such that then the logic would kick in if no further statuses are received

dagar · 2021-02-02T14:55:52Z

src/modules/commander/Commander.cpp

-		mavlink_log_critical(&_mavlink_log_pub, "%soffline", esc_fail_msg);
+	} else {
+
+		if (_esc_status_was_updated && !_status_flags.condition_escs_error) {


Isn't _esc_status_was_updated is always going to be true within esc_status_check()?

not completely, IIRC i added that condition to guarantee that the check does not trigger a false positive at boot

dagar · 2021-02-02T14:59:01Z

@dagar this was also a suggestion you had back in the time

Did you consider splitting the esc_status update check and error reporting and separately check for timeouts and missing ESCs? You shouldn't need to copy esc_status (a rather large message with 8 esc_reports) every iteration.

I think would make sense to do it

@cmic0 yes, I think it's simply a matter of only doing the work of esc_status_check() when the topic updates OR if you haven't had an update for the timeout period (700_ms). In the correct code once you receive the first ORB_ID(esc_status) commander is now copying it every single iteration (100 Hz) even when there's no new data.

mrpollo · 2021-02-15T17:49:34Z

@cmic0 thanks for resurrecting your PR, much appreciated, and really cool feature. 🏆

Could you please address @dagar comments above?

@dagar could you advise on how to fix some of the failed tests? I think this pushes FMUv2 out of flash again.

region `flash' overflowed by 445 bytes

dagar · 2021-03-29T14:34:08Z

@cmic0 can you rebase to see if this fits now?

cmic0 · 2021-03-29T14:37:03Z

yep - i am actually trying improve the code to avoid copying the esc_status if the escs are already timing out

cmic0 · 2021-03-30T09:41:52Z

@dagar with c85ca27 i moved out the timeout check from the esc_status_check(), this should avoid that we keep copying the esc_status message for nothing

dagar · 2021-03-30T13:30:13Z

SITL tests are failing.

src/modules/commander/Commander.cpp

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

src/modules/commander/Commander.hpp

src/modules/commander/Commander.cpp

COM_ARM_CHK_ESCS set to 0. The user will need to enable it manually. Co-authored-by: Beat Küng <[email protected]>

Igor-Misic · 2021-04-22T11:37:15Z

FYI: I am not authorized to merge this PR.

This update to this PR: PX4/PX4-Autopilot#16704

cmic0 requested review from MaEtUgR and dagar January 31, 2021 21:08

cmic0 mentioned this pull request Jan 31, 2021

Extend ESC checks to handle failures #14800

Closed

1 task

dagar reviewed Jan 31, 2021

View reviewed changes

dagar added the Admin: Enhancement (improvement) 💡 label Jan 31, 2021

dagar reviewed Feb 2, 2021

View reviewed changes

cmic0 force-pushed the pr-esc-failure-checks branch 2 times, most recently from 69f3266 to c85ca27 Compare March 30, 2021 09:36

cmic0 requested a review from bkueng March 30, 2021 09:42

Igor-Misic force-pushed the pr-esc-failure-checks branch from bd01cd0 to 478cca0 Compare April 15, 2021 19:15

Igor-Misic reviewed Apr 15, 2021

View reviewed changes

src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved

Igor-Misic force-pushed the pr-esc-failure-checks branch 4 times, most recently from 4551bbd to 764c977 Compare April 20, 2021 13:22

cmic0 added 2 commits April 21, 2021 14:07

Commander/preflight checks: Add monitoring to ESC failures

40abac1

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

Commander:esc_checks minor improvements

263c56c

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

Igor-Misic force-pushed the pr-esc-failure-checks branch 2 times, most recently from 419a105 to b8f304f Compare April 21, 2021 12:09

bkueng reviewed Apr 22, 2021

View reviewed changes

src/modules/commander/Commander.hpp Outdated Show resolved Hide resolved

src/modules/commander/Commander.cpp Outdated Show resolved Hide resolved

Commander: checking COM_ARM_CHK_ESCS param for ESCs telemetry

cf2b25d

COM_ARM_CHK_ESCS set to 0. The user will need to enable it manually. Co-authored-by: Beat Küng <[email protected]>

Igor-Misic force-pushed the pr-esc-failure-checks branch from 9c35196 to cf2b25d Compare April 22, 2021 07:34

bkueng approved these changes Apr 22, 2021

View reviewed changes

Igor-Misic added a commit to PX4/PX4-user_guide that referenced this pull request Apr 22, 2021

parameter_reference: updated esc telemetry

ba329fc

This update to this PR: PX4/PX4-Autopilot#16704

Igor-Misic mentioned this pull request Apr 22, 2021

parameter_reference: updated esc telemetry PX4/PX4-user_guide#1217

Closed

TSC21 approved these changes Apr 22, 2021

View reviewed changes

TSC21 merged commit d631a5d into master Apr 22, 2021

TSC21 deleted the pr-esc-failure-checks branch April 22, 2021 11:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commander/preflight checks: Add monitoring to ESC failures #16704

Commander/preflight checks: Add monitoring to ESC failures #16704

cmic0 commented Jan 31, 2021

cmic0 commented Jan 31, 2021

dagar Jan 31, 2021

cmic0 Feb 2, 2021

dagar Feb 2, 2021

dagar Jan 31, 2021

cmic0 Mar 29, 2021

cmic0 Mar 30, 2021

dagar Feb 2, 2021

cmic0 Mar 29, 2021 •

edited

Loading

dagar commented Feb 2, 2021

mrpollo commented Feb 15, 2021

dagar commented Mar 29, 2021

cmic0 commented Mar 29, 2021

cmic0 commented Mar 30, 2021

dagar commented Mar 30, 2021

Igor-Misic commented Apr 22, 2021


		if (esc_status.esc[index].failures != _last_esc_failure[index]) {

		if (esc_status.esc[index].failures & esc_report_s::FAILURE_OVER_CURRENT_MASK) {

Commander/preflight checks: Add monitoring to ESC failures #16704

Commander/preflight checks: Add monitoring to ESC failures #16704

Conversation

cmic0 commented Jan 31, 2021

cmic0 commented Jan 31, 2021

dagar Jan 31, 2021

Choose a reason for hiding this comment

cmic0 Feb 2, 2021

Choose a reason for hiding this comment

dagar Feb 2, 2021

Choose a reason for hiding this comment

dagar Jan 31, 2021

Choose a reason for hiding this comment

cmic0 Mar 29, 2021

Choose a reason for hiding this comment

cmic0 Mar 30, 2021

Choose a reason for hiding this comment

dagar Feb 2, 2021

Choose a reason for hiding this comment

cmic0 Mar 29, 2021 • edited Loading

Choose a reason for hiding this comment

dagar commented Feb 2, 2021

mrpollo commented Feb 15, 2021

dagar commented Mar 29, 2021

cmic0 commented Mar 29, 2021

cmic0 commented Mar 30, 2021

dagar commented Mar 30, 2021

Igor-Misic commented Apr 22, 2021

cmic0 Mar 29, 2021 •

edited

Loading