commander state machine fix preflight and prearm error #9193

dagar · 2018-03-30T00:21:01Z

fixes System is arming (or thinks it is arming) even with Critical warnings (specifically "MAG SENSORS INCONSISTENT") #9155

There were many issues. The most important immediate issue is that preflight checks haven't been blocking arming. Additionally many little annoying edge cases I've experienced finally make sense to me where you might not be provided appropriate (or any) feedback trying to arm.

There's still a lot more to do here, but this restores a level of sanity. The next steps from my perspective would be removing the actual sensor check calls from the state machine and centralizing all of it on the commander side. Then commander would simple pass the appropriate flags into a much simpler generated (and documented) state machine.

dagar · 2018-03-30T02:54:25Z

@PX4/testflights please bench test on a couple different systems.

Verify things like...

safety button works as expected for arming or preventing arming
low battery prevents arming
test the COM_ARM_WO_GPS parameter
test the COM_ARM_MIS_REQ parameter
try to arm with a bogus compass setup (uncalibrated, external rotated 90 degrees, etc)
try to arm with USB connected

dagar · 2018-04-03T18:00:41Z

TODO (possibly after this PR):

review preflight, prearm, and all the state machines in general with respect to calibration. I believe there are currently some holes.
review HIL and try to extract and centralize all HIL specific behaviour changes. The state machines themselves (and eventually all of commander) could be completely oblivious to HIL.

Avysuarez · 2018-04-03T23:17:55Z

Test with pixhawk 2.1:
1; safety button works as expected for arming or preventing arming.
Not arming.
2;low battery prevents arming.
Not arming, low battery message.
3; test the COM_ARM_WO_GPS parameter.
Not arming. position reject message
4;test the COM_ARM_MIS_REQ parameter.
Not arming
5; try to arm with a bogus compass setup (uncalibrated, external rotated 90 degrees, etc)
Not arming.
6; try to arm with USB connected.
Not arming.

julianoes

Nice cleanup! I made some inline comments.

julianoes · 2018-04-04T07:31:22Z

src/modules/commander/PreflightCheck.cpp

+
+		system_power_s system_power = {};
+
+		if (orb_copy(ORB_ID(system_power), system_power_sub, &system_power) == PX4_OK) {


If there is no system power update, this check will just pass. As I understand it, there will always be one orb_copy and therefore we would copy an avionics_power_rail_voltage of 0 and fail anyway.

I added a time elapsed check as a workaround, which should be fine for now, but still isn't ideal. After this PR I was thinking about centralizing the arming requirements checks. We can make system_power explicitly required or not.

julianoes · 2018-04-04T07:35:35Z

src/modules/commander/PreflightCheck.cpp

+{
+	bool success = true; // start with a pass and change to a fail if any test fails
+
+	if (prearm) {


I'd prefer the structure like this:

if (!prearm) { // Ignore power check after arming. return true; } ...

Sure, that works for me.

julianoes · 2018-04-04T07:36:34Z

src/modules/commander/PreflightCheck.cpp

@@ -570,12 +612,12 @@ static bool ekf2Check(orb_advert_t *mavlink_log_pub, bool optional, bool report_
 }

 bool preflightCheck(orb_advert_t *mavlink_log_pub, bool checkSensors, bool checkAirspeed, bool checkRC, bool checkGNSS,
-		    bool checkDynamic, bool isVTOL, bool reportFailures, bool prearm, hrt_abstime time_since_boot)
+		    bool checkDynamic, bool checkPower, bool isVTOL, bool reportFailures, bool prearm, const hrt_abstime &time_since_boot)


This list of bools needs to go away in the future, that's super error-prone. A struct containing the bools would already be better.

Completely agree. I was going to start using vehicle_status_flags and vehicle_status after this PR.

julianoes · 2018-04-04T07:44:26Z

src/modules/commander/PreflightCheck.cpp

 				failed = true;
+				mag_fail_reported = true;


The mag_fail_reported flag doesn't do anything as I understand it. Since max_mandatory_mag_count is 1, only the first check is required. After that (reportFailures && !mag_fail_reported)) && required evaluates to false anyway.

It should prevent the repetition of

PREFLIGHT FAIL: MAG 0 UNCALIBRATED PREFLIGHT FAIL: MAG 1 UNCALIBRATED PREFLIGHT FAIL: MAG 2 UNCALIBRATED

Since you can only calibrate them in bulk, I didn't see the point in reporting more than a single line.

But if you only report for mandatory devices then you don't need the reported flag, required should be enough.

It still reports uncalibrated for optional devices. I find the entire thing more convoluted than necessary. It could almost be refactored into a template for all types.

julianoes · 2018-04-04T07:49:55Z

src/modules/commander/state_machine_helper.cpp

-		 * Perform an atomic state update
-		 */
-#ifdef __PX4_NUTTX
-		irqstate_t flags = px4_enter_critical_section();


What's the reason that all this is removed?

Why is it needed? The state machine isn't grabbing new system data in this section. If it was important we'd also need an equivalent on Linux and QuRT.

I don't know. But ok makes sense then.

julianoes · 2018-04-04T07:52:43Z

src/modules/commander/state_machine_helper.cpp

-	if (reportFailures && !prearm_ok) {
-		status_flags->condition_system_prearm_error_reported = true;
+	// Arm Requirements: authorization
+	// check last, and only if everything else has passed


Makes sense.

dagar · 2018-04-05T00:23:37Z

Thanks @Avysuarez. Did you see any situations where arming was blocked and no explanation was given? Did you see any situations where there was a mavlink critical message (pop up in the yellow box), but arming was allowed to complete?

AlexisTM · 2018-04-05T07:06:33Z

NOTE: With the 1.7.2 release, I sometimes cannot arm because of the following error: PREFLIGHT FAIL: ACCEL SENSORS INCONSISTENT - CHECK CALIBRATION.

This is on a Pixhawk 1, with only one IMU.

This does not happen on 1.6.5.

EDIT: It actually has the MPU6000 as main accel and gyro and a ST Micro 14-bit accelerometer/compass.

Avysuarez · 2018-04-05T16:52:54Z

@dagar

Did you see any situations where arming was blocked and no explanation was given?

No, I didn't see any, in all tests when there were no conditions to arm I received messages with the explanation.

Did you see any situations where there was a mavlink critical message (pop up in the yellow box), but arming was allowed to complete?

No, I didn't see any. When I got mavlink critical messages such as low battery, gps not ready to takeoff, etc, arming was not allowed.

AlexisTM · 2018-04-06T15:22:52Z

@dagar

With this PR, we should as well check if the target_local_ned module is initialized and publishing before allowing the OFFBOARD mode (or any other mode?). Aka the PX4 publisher for the following MAVLink handler:

handle_position_target_local_ned(const mavlink::mavlink_message_t *msg, mavlink::common::msg::POSITION_TARGET_LOCAL_NED &tgt)

NOTE: I don't know where this is located in the PX4 Firmware.

- only call prearm if preflight passes - prearm always provide feedback

- this flag often results in hiding useful information, or adding useless information to the mavlink console

- add preflight_check helper

- this is needed for the unit tests

…remove atomic

dagar · 2018-04-16T20:38:52Z

Rebased on master again. Last call for testing.

dagar · 2018-04-17T00:43:47Z

Thanks everyone.

potaito · 2018-05-04T09:39:09Z

@dagar
I am a bit late to the party, but can you clarify something? Before, the power check would only run if the 3.3V and 5.0V power rails were above 0.0V:
https://github.com/PX4/Firmware/blob/c679703da49528cedbbf31766dfffdcb0f5c2863/src/modules/commander/state_machine_helper.cpp#L227-L244

In the new preflight check, this >0.0 condition has been removed:
https://github.com/PX4/Firmware/blob/df042ab20e5ab1df43a322eacdb0e189c1b01652/src/modules/commander/PreflightCheck.cpp#L496-L513

Is there a specific reason for that?

I am asking because I have a system here that is always showing 0.0V on both rails and now the preflight check fails obviously.

That >0.0 check was probably a hack in the first place. So I understand if it should be removed.

dagar · 2018-05-04T13:31:46Z

The structure is a bit different now because the avionics power rail voltage is no longer passed into the arming state machine directly as a float. Now it's only checked after the system_power message publishes, so we don't need the > 0.0 hack.

There's also the power circuit breaker that can be used to explicitly ignore these checks.

dagar added bug priority-critical labels Mar 30, 2018

dagar requested review from bkueng and thomasgubler March 30, 2018 00:21

dagar mentioned this pull request Mar 30, 2018

System is arming (or thinks it is arming) even with Critical warnings (specifically "MAG SENSORS INCONSISTENT") #9155

Closed

dagar force-pushed the pr-commander_state_machine_preflight branch 2 times, most recently from 830d7b1 to 7d75ccd Compare March 30, 2018 00:56

dagar mentioned this pull request Mar 30, 2018

commander preflight check fix #9176

Closed

dagar requested review from julianoes and a team March 30, 2018 02:40

dagar force-pushed the pr-commander_state_machine_preflight branch 3 times, most recently from 7164ae2 to 7c6c0ca Compare March 30, 2018 15:31

dagar mentioned this pull request Mar 30, 2018

commander state machine helper shorten names #9192

Closed

dagar requested a review from LorenzMeier March 30, 2018 15:31

dagar force-pushed the pr-commander_state_machine_preflight branch 2 times, most recently from b6452f6 to e81f370 Compare April 2, 2018 15:11

dagar mentioned this pull request Apr 2, 2018

PX4 allows arming while the control is not fully initialized. (TARGET_LOCAL_NED not yet published) (workaround proposed) #9200

Closed

julianoes reviewed Apr 4, 2018

View reviewed changes

dagar mentioned this pull request Apr 4, 2018

No EKF2 init without Vision Position, arming allowing without attitude estimate #9020

Closed

dagar force-pushed the pr-commander_state_machine_preflight branch 2 times, most recently from 281e595 to b89e407 Compare April 5, 2018 01:25

dagar added 19 commits April 16, 2018 16:33

commander prearm_check is always prearm

5a5ca03

commander arming_state_transition cleanup preflight and prearm calls

beb0636

- only call prearm if preflight passes - prearm always provide feedback

commander preflight sensor failure report one at a time

c90e4a6

commander COMPONENT_ARM_DISARM remove useless arm error

1a8f7cd

commander status flags delete condition_system_prearm_error_reported

0bf7884

- this flag often results in hiding useful information, or adding useless information to the mavlink console

commander move arm authorization to prearm_check

45c97ad

commander move power check to prearm_check

1472760

commander set status_flags.usb_connected directly and delete static

5de463d

commander move avionics rail voltage check out of state machine

567bc2e

- add preflight_check helper

commander state machine add global position check

7538a35

commander arming state machine pass checks by default

287c4b8

- this is needed for the unit tests

commander arming state transition delete useless errors

9b2b458

commander arming state transition consolidate HIL special checks and …

7aa6a44

…remove atomic

commander move safety check to prearm

b4fbd9e

commander prearm_check limit simultaneous error messages

33f814f

commander arm_auth check last

45b985e

commander preflight checks pass status and status_flags messages

b98de09

commander preflightCheck don't report failures during calibration

206d497

commander move telemetry checks to new preflightcheck helper

43160d3

dagar force-pushed the pr-commander_state_machine_preflight branch from e4b050b to 43160d3 Compare April 16, 2018 20:35

dagar merged commit 7bc3a12 into master Apr 17, 2018

dagar deleted the pr-commander_state_machine_preflight branch April 17, 2018 00:43

dagar mentioned this pull request Apr 21, 2018

**Feature Request**: block arming after sensor calibration, require system reboot (or graceful re-init) prior to flight #9030

Closed

dakejahl mentioned this pull request May 4, 2018

Avionics power check type error (int to float) #9412

Merged

mrpollo mentioned this pull request Jun 5, 2018

v1.8 Changelog scratchpad #9593

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commander state machine fix preflight and prearm error #9193

commander state machine fix preflight and prearm error #9193

dagar commented Mar 30, 2018 •

edited by AuterionWrikeBot

Loading

dagar commented Mar 30, 2018 •

edited

Loading

dagar commented Apr 3, 2018

Avysuarez commented Apr 3, 2018

julianoes left a comment

julianoes Apr 4, 2018

dagar Apr 5, 2018

julianoes Apr 4, 2018

dagar Apr 5, 2018

julianoes Apr 4, 2018

dagar Apr 5, 2018

julianoes Apr 4, 2018

dagar Apr 5, 2018

julianoes Apr 5, 2018

dagar Apr 7, 2018

julianoes Apr 4, 2018

dagar Apr 5, 2018

julianoes Apr 5, 2018

julianoes Apr 4, 2018

dagar commented Apr 5, 2018

AlexisTM commented Apr 5, 2018 •

edited

Loading

Avysuarez commented Apr 5, 2018

AlexisTM commented Apr 6, 2018

dagar commented Apr 16, 2018

dagar commented Apr 17, 2018

potaito commented May 4, 2018 •

edited

Loading

dagar commented May 4, 2018


		system_power_s system_power = {};

		if (orb_copy(ORB_ID(system_power), system_power_sub, &system_power) == PX4_OK) {

commander state machine fix preflight and prearm error #9193

commander state machine fix preflight and prearm error #9193

Conversation

dagar commented Mar 30, 2018 • edited by AuterionWrikeBot Loading

dagar commented Mar 30, 2018 • edited Loading

dagar commented Apr 3, 2018

Avysuarez commented Apr 3, 2018

julianoes 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

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 Apr 5, 2018

AlexisTM commented Apr 5, 2018 • edited Loading

Avysuarez commented Apr 5, 2018

AlexisTM commented Apr 6, 2018

dagar commented Apr 16, 2018

dagar commented Apr 17, 2018

potaito commented May 4, 2018 • edited Loading

dagar commented May 4, 2018

dagar commented Mar 30, 2018 •

edited by AuterionWrikeBot

Loading

dagar commented Mar 30, 2018 •

edited

Loading

AlexisTM commented Apr 5, 2018 •

edited

Loading

potaito commented May 4, 2018 •

edited

Loading