commander: collapse ArmStateMachine and simplify #21129

dagar · 2023-02-15T03:07:57Z

simplify vehicle_status.arming_state down to just armed and disarmed
- ARMING_STATE_INIT doesn't matter
- ARMING_STATE_STANDBY is effectively pre_flight_checks_pass
- ARMING_STATE_STANDBY_ERROR not needed
- ARMING_STATE_SHUTDOWN effectively not used (all the poweroff/shutdown calls loop forever in place)
- ARMING_STATE_IN_AIR_RESTORE doesn't exist anymore
collapse ArmStateMachine into commander
- all requests already go through Commander::arm() and Commander::dismarm()
other minor changes
- VEHICLE_CMD_DO_FLIGHTTERMINATION undocumented (unused?) test command (param1 > 1.5f) removed
- switching to NAVIGATION_STATE_TERMINATION triggers parachute command centrally (only if armed)

There's still some overlap and duplication between actuator_armed and vehicle_status that we can consolidate.

I don't particularly like all the scattered HIL_STATE_ON checks, these should really be nearly entirely eliminated.

MaEtUgR

In my eyes this is a very valuable simplification. There is a lot of distributed, glanced-over, unnecessary complexity in this logic and after going through I'm reasonably sure you captured all important bits. The cases for which I'm not so sure I made a comment. We have to test these through if it didn't happen already but it's certainly worth it because things get much more deterministic.

Some small suggestions I'll add in a commit.

MaEtUgR · 2023-03-09T18:59:25Z

src/modules/commander/Commander.cpp

@@ -2368,7 +2324,7 @@ void Commander::control_status_leds(bool changed, const uint8_t battery_warning)
 			BOARD_ARMED_LED_ON();
 		}

-	} else if (_arm_state_machine.isStandby()) {
+	} else if (_vehicle_status.pre_flight_checks_pass) {


pre_flight_checks_pass should not pass if we are currently calibrating then.

src/modules/commander/Commander.cpp

MaEtUgR · 2023-03-09T20:57:43Z

src/modules/commander/Commander.cpp


 				// reject if armed or shutting down
 				answer_command(cmd, vehicle_command_ack_s::VEHICLE_CMD_RESULT_TEMPORARILY_REJECTED);

 			} else {

-				/* try to go to INIT/PREFLIGHT arming state */


Temperature calibration is the only case that sets neither calibration_enabled nor rc_calibration_in_progress. Are we sure we can't arm in temperature calibration?

MaEtUgR · 2023-03-09T21:02:38Z

src/modules/commander/Commander.cpp

+		    || (!actuator_armed_prev.manual_lockdown && _actuator_armed.manual_lockdown)
+		   ) {
+			if (isArmed()) {
+				send_parachute_command();


There was previously a guard that did not allow triggering the parachute multiple times except for when resetting for test purposes with the undocumented unterminate command. I don't see why this should be an issue but wanted to note that it's a change in behaviour.

MaEtUgR · 2023-03-09T21:06:26Z

src/modules/commander/Commander.cpp

-			} else {
-				_actuator_armed.force_failsafe = false;
-				_actuator_armed.lockdown = false;
+				if (!isArmed()) {


Shouldn't there be a way to terminate before arming for testing? I'm asking because the DRS parachute mechanism to my knowledge can/should be tested before arming.

Terminate before arming? Doesn't sound intuitive to me, to be honest.

MaEtUgR · 2023-03-09T21:06:29Z

src/modules/commander/Commander.cpp


-				_lockdown_triggered = false;
-				_flight_termination_triggered = false;
+				} else if (_user_mode_intention.change(vehicle_status_s::NAVIGATION_STATE_TERMINATION)) {


Are there additional conditions to enter termination then? Can we then unterminate by switching mode? Could that happen unintentionally? We could chop up the parachute in that case no?

MaEtUgR · 2023-03-10T09:36:16Z

src/modules/commander/Commander.cpp

+		return TRANSITION_NOT_CHANGED;
+	}
+
+	if (_vehicle_status.calibration_enabled


Best we move all of these additional

calibration

Throttle too high

armed by RC

checks into the _health_and_arming_checks next. Probably it needs to know calling reason if that's not the case already.

MaEtUgR · 2023-03-10T09:40:15Z

src/modules/commander/Arming/ArmStateMachine/ArmStateMachine.cpp

-		if (valid_transition
-		    && (new_arming_state == vehicle_status_s::ARMING_STATE_ARMED)
-		    && fRunPreArmChecks
-		    && !(status.hil_state == vehicle_status_s::HIL_STATE_ON)


It could be that HIL now fails certain preflight checks that were skipped in an obfuscated way before. We need to test it and go through these if that didn't happen already.

MaEtUgR · 2023-03-10T09:59:34Z

src/modules/mavlink/streams/HEARTBEAT.hpp

 			}

 			// system_status overrides
 			if (actuator_armed.force_failsafe || actuator_armed.lockdown || actuator_armed.manual_lockdown
 			    || vehicle_status.nav_state == vehicle_status_s::NAVIGATION_STATE_TERMINATION) {
+
 				system_status = MAV_STATE_FLIGHT_TERMINATION;

 			} else if (vehicle_status.calibration_enabled) {


Since we can't calibrate while being armed this "override" doesn't make sense. The valid case is already handled above.

Make message format consistent with the arming denied error messages.

pre_flight_checks_pass can only be true or false the else case cannot be reached.

MaEtUgR · 2023-03-10T10:25:48Z

I rebased the pr without conflict and added my suggestions in additional commits.
The points to take special care of sorted by subjective severty are:

Do HIL, HITL, SIH pass preflight checks?
commander: collapse ArmStateMachine and simplify #21129 (comment)
Can terminate be denied? _user_mode_intention.change(vehicle_status_s::NAVIGATION_STATE_TERMINATION)
commander: collapse ArmStateMachine and simplify #21129 (comment)
Parachute triggerable multiple times, unterminate with (accidental) mode change?
commander: collapse ArmStateMachine and simplify #21129 (comment)
While calibrating pre-flight checks should not pass for consistent reporting to work.
commander: collapse ArmStateMachine and simplify #21129 (comment)
No way to test trigger parachute on the ground, should probably not be easy but somehow possible
commander: collapse ArmStateMachine and simplify #21129 (comment)
Is it possible to arm in temperature calibration because none of the calibration flags are set?
commander: collapse ArmStateMachine and simplify #21129 (comment)

DronecodeBot · 2023-05-16T15:35:04Z

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-maintainers-call-may-16-2023/32137/1

julianoes

Nice! I couldn't spot anything obviously wrong.

julianoes · 2023-05-30T05:59:28Z

src/modules/commander/Commander.cpp

-			} else {
-				_actuator_armed.force_failsafe = false;
-				_actuator_armed.lockdown = false;
+				if (!isArmed()) {


Terminate before arming? Doesn't sound intuitive to me, to be honest.

…e_machine_collapse

dagar force-pushed the pr-commander_arming_state_machine_collapse branch from 69a6b14 to edf4388 Compare February 15, 2023 03:13

dagar requested a review from MaEtUgR February 15, 2023 03:16

dagar added the Admin: Cleanup (housekeeping) 🧹 label Feb 15, 2023

dagar force-pushed the pr-commander_arming_state_machine_collapse branch 6 times, most recently from 33ba4a0 to dd21af4 Compare February 15, 2023 05:47

MaEtUgR reviewed Mar 10, 2023

View reviewed changes

dagar and others added 5 commits March 10, 2023 11:11

commander: collapse ArmStateMachine and simplify

0ebca4e

HEARTBEAT: remove duplicate calibrating state handling

6ee637b

Commander: style change vehicle_status initialization

f76892d

Commander: consistent disarm error message

ef3b6d8

Make message format consistent with the arming denied error messages.

Commander: remove unreachable case in LED feedback

09f41da

pre_flight_checks_pass can only be true or false the else case cannot be reached.

MaEtUgR force-pushed the pr-commander_arming_state_machine_collapse branch from dd21af4 to 09f41da Compare March 10, 2023 10:12

MaEtUgR mentioned this pull request Mar 27, 2023

uORB: Add subscription with single field selection #19775

Open

dagar mentioned this pull request May 8, 2023

Send parachute MAVLink command on disarm in air #21553

Open

dagar mentioned this pull request May 24, 2023

mavlink heartbeat stream: Fix system status always reporting FLIGHT_TERMINATION in HITL #21636

Merged

julianoes reviewed May 30, 2023

View reviewed changes

Merge remote-tracking branch 'px4/main' into pr-commander_arming_stat…

61b8689

…e_machine_collapse

dagar merged commit 88e7452 into main Jul 28, 2023
83 of 84 checks passed

dagar deleted the pr-commander_arming_state_machine_collapse branch July 28, 2023 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

commander: collapse ArmStateMachine and simplify #21129

commander: collapse ArmStateMachine and simplify #21129

dagar commented Feb 15, 2023 •

edited

Loading

MaEtUgR left a comment

MaEtUgR Mar 9, 2023

MaEtUgR Mar 9, 2023

MaEtUgR Mar 9, 2023

MaEtUgR Mar 9, 2023 •

edited

Loading

julianoes May 30, 2023

MaEtUgR Mar 9, 2023

MaEtUgR Mar 10, 2023

MaEtUgR Mar 10, 2023

MaEtUgR Mar 10, 2023

MaEtUgR commented Mar 10, 2023

DronecodeBot commented May 16, 2023

julianoes left a comment

julianoes May 30, 2023

commander: collapse ArmStateMachine and simplify #21129

commander: collapse ArmStateMachine and simplify #21129

Conversation

dagar commented Feb 15, 2023 • edited Loading

MaEtUgR 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

MaEtUgR Mar 9, 2023 • edited Loading

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

MaEtUgR commented Mar 10, 2023

DronecodeBot commented May 16, 2023

julianoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dagar commented Feb 15, 2023 •

edited

Loading

MaEtUgR Mar 9, 2023 •

edited

Loading