UAVCAN pre-arm checks on escs_status (cleanup of #12153) #12641

cmic0 · 2019-08-06T08:07:10Z

This is a cleaned-up version of #12153.
Addressed @RomanBapst review comments:

Fixed SITL failing for esc that do no support esc_status reporting UAVCAN ESCs pre-arm checks #12153 (comment)
Removed optional message UAVCAN ESCs pre-arm checks #12153 (comment)
Code cosmetics UAVCAN ESCs pre-arm checks #12153 (comment)
Inverted pre-arm check logic so that CI tests does not fail (integrated in this commit, the message is now called condition_escs_error)

Test log:
https://logs.px4.io/plot_app?log=5b7f4b1c-9c1a-4f52-bdb8-25d6bb5e13e1

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

bkueng · 2019-08-06T08:21:42Z

src/modules/commander/state_machine_helper.cpp

@@ -1010,6 +1010,12 @@ bool prearm_check(orb_advert_t *mavlink_log_pub, const vehicle_status_flags_s &s

 	}

+	if (status_flags.condition_escs_error && !status_flags.circuit_breaker_engaged_escs_check) {
+		mavlink_log_critical(mavlink_log_pub, "Arming denied! One or more ESCs are offline");


if (prearm_ok && reportFailures) {

bkueng · 2019-08-06T08:24:32Z

src/lib/mixer/mixer_group.cpp

+		if (rotor_count > 0) {
+			break;
+
+		} else { mixer = mixer -> _next;}


Replace with:

} mixer = mixer->_next;

Uh, In the original PR was like that but in the review I was suggested to use break for exiting the while loop.
What is the best practice for exiting the while loop?
The reference says that breaks can be used for exiting while loops, so i guess is just a madder of best choice?

It's not about the break, you can leave it in. But if you break you don't need the else, and removing it makes the code more readable.
As for best practices, it depends: exiting a larger loop from within many nested cases is prone to errors and harder to understand. Adding a break in a simple loop on the other hand can make the code simpler.

bkueng · 2019-08-06T08:26:17Z

src/drivers/uavcan/uavcan_main.cpp

@@ -1123,6 +1123,11 @@ UavcanNode::ioctl(file *filp, int cmd, unsigned long arg)
 				} else {

 					_mixers->groups_required(_groups_required);
+					PX4_INFO("Groups required %d", _groups_required);
+
+					_rotor_count = _mixers->get_multirotor_count();


Keep this locally:

Suggested change

_rotor_count = _mixers->get_multirotor_count();

int rotor_count = _mixers->get_multirotor_count();

bkueng · 2019-08-06T08:26:49Z

msg/vehicle_status_flags.msg

@@ -14,14 +14,15 @@ bool condition_local_velocity_valid		# set to true by the commander app if the q
 bool condition_local_altitude_valid
 bool condition_power_input_valid                # set if input power is valid
 bool condition_battery_healthy                # set if battery is available and not low
-
+bool condition_escs_error		      # set to true in case of standard ESCs that do not provide status OR if all the ESCs are online


Comment looks incorrect.

bkueng · 2019-08-06T08:27:32Z

src/lib/mixer/mixer_group.cpp

+
+		rotor_count = mixer->get_multirotor_count();
+
+		if (rotor_count > 0) {


Why are you not summing all up? Would IMO be more robust.

bkueng · 2019-08-06T08:36:07Z

src/drivers/uavcan/actuators/esc.cpp

+
+uint8_t UavcanEscController::check_escs_status()
+{
+	int esc_status_flags = 255;


Can you only set the bits for the ones that are really online?

Actually yes, I think in an early implementation I used this logic to fix something but after reviewing makes completely sense to re-invert the logic :)!

bkueng · 2019-08-06T08:36:46Z

src/lib/circuit_breaker/circuit_breaker_params.c

+ * Circuit breaker for ESCs status check
+ *
+ * Setting this parameter to 271828 will disable offline ESCs detection.
+ * If this check is enabled, then the ESCs check will fail is not all the ESCs are reporting their status.


Suggested change

* If this check is enabled, then the ESCs check will fail is not all the ESCs are reporting their status.

* If this check is enabled, then the ESCs check will fail if not all the ESCs are reporting their status.

bkueng · 2019-08-06T08:38:38Z

src/modules/commander/Commander.cpp

@@ -1610,6 +1614,18 @@ Commander::run()
 			}
 		}

+		if (orb_exists(ORB_ID(esc_status), 0) == PX4_OK) {


Do you really need this? Is it not enough to check if it's updated and the timestamp is valid?

You are right, this is a left over that I introduced then the logic was still reasoning on "is valid" and thus i needed to initialize that to true in case of escs not reporting status. Changing the message to condition_escs_error (and thus inverting the logic) automatically solved this corner case. Will get rid of that

src/modules/commander/Commander.cpp

bkueng · 2019-08-06T08:40:58Z

src/modules/commander/Commander.cpp

+	else if (esc_status.esc_online_flags < _last_esc_online_flags) {
+
+		for (int index = 0; index < esc_status.esc_count; index++) {
+			if ((bool)!(esc_status.esc_online_flags & (1 << index))) {


Suggested change

if ((bool)!(esc_status.esc_online_flags & (1 << index))) {

if ((esc_status.esc_online_flags & (1 << index)) == 0) {

dagar · 2019-08-06T15:07:14Z

How about a simple boolean parameter like COM_ARM_MIS_REQ rather than a circuit breaker and magic number? At some point I was going to try rolling all the pre-arming checks into a single bitfield parameter.

cmic0 · 2019-08-07T12:36:57Z

@dagar thanks for the hint will have a look

ndepal · 2019-08-07T12:52:07Z

src/drivers/uavcan/actuators/esc.cpp

+
+	for (int index = 0; index < esc_status_s::CONNECTED_ESC_MAX; index++) {
+
+		if (hrt_elapsed_time(&_esc_status.esc[index].timestamp) < 800000.0f && _esc_status.esc[index].timestamp > 0) {


if you reverse the order of the two statements you can avoid the (presumably more expensive) hrt_elapsed_time() call when the timestamp is invalid.

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

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

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

cmic0 · 2019-08-08T10:23:38Z

@bkueng thanks for the review, all your comments have been addressed; I think the PR is now ready to go.
FYI: I cleaned up the commit history with the last push :)

src/modules/commander/Commander.cpp

bkueng · 2019-08-08T11:10:03Z

msg/vehicle_status_flags.msg

 bool circuit_breaker_engaged_power_check
 bool circuit_breaker_engaged_airspd_check
 bool circuit_breaker_engaged_enginefailure_check
 bool circuit_breaker_engaged_gpsfailure_check
 bool circuit_breaker_flight_termination_disabled
 bool circuit_breaker_engaged_usb_check
 bool circuit_breaker_engaged_posfailure_check	# set to true when the position valid checks have been disabled
+bool circuit_breaker_engaged_escs_check


Since it's not a circuit breaker anymore, can you rename this as well?

Correct this can actually be removed.

bkueng · 2019-08-08T11:16:09Z

src/drivers/uavcan/actuators/esc.cpp

+
+	for (int index = 0; index < esc_status_s::CONNECTED_ESC_MAX; index++) {
+
+		if (_esc_status.esc[index].timestamp > 0 && hrt_elapsed_time(&_esc_status.esc[index].timestamp) < 800000.0f) {


2 more things you can do here: pull the hrt_absolute_time call out of the loop: hrt_abstime now = hrt_absolute_time();, and use the time literals: now - _esc_status.esc[index].timestamp < 800_ms) {
(add using namespace time_literals; at the top of the file).

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

cmic0 added 2 commits August 5, 2019 19:19

Added topics to esc_status and vehicle_status_flags.

f1d64b0

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

uavcan esc: added timeout checks to escs.

e5300f0

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

cmic0 mentioned this pull request Aug 6, 2019

UAVCAN ESCs pre-arm checks #12153

Closed

bkueng reviewed Aug 6, 2019

View reviewed changes

cmic0 mentioned this pull request Aug 6, 2019

[WIP] Voltage inconsistency checks on ESCs reporting esc_status #12647

Closed

ndepal reviewed Aug 7, 2019

View reviewed changes

cmic0 added 4 commits August 8, 2019 11:44

Commander: added esc_status prearm checks

989091a

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

uavcan escs: Modified esc_status reporting to have _rotor_count publi…

f30e196

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

uavcan_main: replaced printf messages with PX4_INFO

9729403

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

Improved code comment for condition_escs_error topic.

9cde428

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

cmic0 force-pushed the escstatus_checks_clean branch from c54e76a to e7e0769 Compare August 8, 2019 10:16

bkueng reviewed Aug 8, 2019

View reviewed changes

cmic0 added 4 commits August 8, 2019 17:41

Simplified esc_status healthiness logic.

b779c94

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

mixer_group: improved get_multirotor_count () code readbility.

fd22e30

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

Commander: move esc_status as local variable.

78a4047

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

uavcan esc: use time literals for timeout checks.

b7d489e

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

cmic0 force-pushed the escstatus_checks_clean branch from 1add79e to b7d489e Compare August 8, 2019 15:42

bkueng approved these changes Aug 9, 2019

View reviewed changes

bkueng merged commit aae16cc into PX4:master Aug 9, 2019

weekly-digest bot mentioned this pull request Aug 11, 2019

Weekly Digest (4 August, 2019 - 11 August, 2019) #12682

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UAVCAN pre-arm checks on escs_status (cleanup of #12153) #12641

UAVCAN pre-arm checks on escs_status (cleanup of #12153) #12641

cmic0 commented Aug 6, 2019

bkueng Aug 6, 2019

bkueng Aug 6, 2019

cmic0 Aug 6, 2019

bkueng Aug 6, 2019

bkueng Aug 6, 2019

bkueng Aug 6, 2019

bkueng Aug 6, 2019

bkueng Aug 6, 2019

cmic0 Aug 7, 2019

bkueng Aug 6, 2019

bkueng Aug 6, 2019

cmic0 Aug 7, 2019

bkueng Aug 6, 2019

dagar commented Aug 6, 2019

cmic0 commented Aug 7, 2019

ndepal Aug 7, 2019

cmic0 commented Aug 8, 2019

bkueng Aug 8, 2019

cmic0 Aug 8, 2019

bkueng Aug 8, 2019

	_rotor_count = _mixers->get_multirotor_count();
	int rotor_count = _mixers->get_multirotor_count();


		rotor_count = mixer->get_multirotor_count();

		if (rotor_count > 0) {

	* If this check is enabled, then the ESCs check will fail is not all the ESCs are reporting their status.
	* If this check is enabled, then the ESCs check will fail if not all the ESCs are reporting their status.

	if ((bool)!(esc_status.esc_online_flags & (1 << index))) {
	if ((esc_status.esc_online_flags & (1 << index)) == 0) {


		for (int index = 0; index < esc_status_s::CONNECTED_ESC_MAX; index++) {

		if (hrt_elapsed_time(&_esc_status.esc[index].timestamp) < 800000.0f && _esc_status.esc[index].timestamp > 0) {


		for (int index = 0; index < esc_status_s::CONNECTED_ESC_MAX; index++) {

		if (_esc_status.esc[index].timestamp > 0 && hrt_elapsed_time(&_esc_status.esc[index].timestamp) < 800000.0f) {

UAVCAN pre-arm checks on escs_status (cleanup of #12153) #12641

UAVCAN pre-arm checks on escs_status (cleanup of #12153) #12641

Conversation

cmic0 commented Aug 6, 2019

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

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

dagar commented Aug 6, 2019

cmic0 commented Aug 7, 2019

Choose a reason for hiding this comment

cmic0 commented Aug 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment