commander: do not reboot on USB disconnect when armed

[korigod]

This commit fixes in-flight reboot on USB disconnect. Flying with USB can be discussed in issue #9663, but whether it's recommended or not it is possible on the default firmware so it looks reasonable to fix this unnecessary risk of crash because of in-flight reboot.

[dagar]

Isn't the core problem that we aren't respecting the arming state machine for reboot?

[korigod]

Using arming_state_transition to reboot can be a solution but reboot is not implemented in state_machine_helper.cpp. Is it intentional or just not done yet?

Of course it's possible to call arming_state_transition and then call px4_shutdown_request if that call haven't returned TRANSITION_DENIED, but it's significantly more complicated than direct !armed.armed check.

[dagar]

It's a bit messy, but not that complicated.

	if (TRANSITION_DENIED != arming_state_transition(&status, battery, safety, vehicle_status_s::ARMING_STATE_REBOOT,
		&armed, false, &mavlink_log_pub, &status_flags, arm_requirements, hrt_elapsed_time(&commander_boot_timestamp))
	) {
		// reboot
	}

You could wrap that mouthful in a bool reboot() helper or similar and we could start addressing the other questionable areas.

The point is that no one other than the state machine should have ever been deciding this. As easy as it is to do the quick fix, at some point we have to start taking the first steps towards a proper safe fix before commander complexity gets completely out of hand.

[korigod]

Ok, but in commander.cpp there are not only reboot requests (px4_shutdown_request(true, false)), but also requests for shutdown (px4_shutdown_request(false, false)). If the former are controlled by a state machine, shouldn't the latter be controlled by it too? Or maybe it should be only ARMING_STATE_SHUTDOWN as it is more generic? Then we request transition to ARMING_STATE_SHUTDOWN in cases of both reboot and shutdown.

[korigod]

@dagar, please review. If you believe ARMING_STATE_REBOOT renaming is questionable, I can remove these commits and open another pull request for them.

[julianoes]

Looks correct to me, thanks!

[julianoes]

@korigod could you rebase this and fix the conflicts so we can get it in? Thanks.

[korigod]

@julianoes, my colleagues will perform test flights in a few days to make sure everything is all right and then we can merge. Thanks!

[mcsauder]

@korigod , I discussed this recently with @dagar , and the good point he brought up was the additional CPU load for each running mavlink instance, roughly 6%-8% per instance. Be sure to post a log that the CPU load can be scrutinized. For the general population, it won't matter as this only impacts post-arm, but that is also when it will be most important for CPU loads.

You PR looks good to me.

[sfalexrog]

As per @korigod's request, I've performed some bench tests on a Pixracer-based drone. In all cases I've armed the drone manually, increased the throttle slightly, disconnected and reconnected the USB cable. In order to get the second MAVLink stream I've used the mavlink_shell.py script from PX4.

The results are as follows:

• Master branch:
  • Single MAVLink stream over USB: https://logs.px4.io/plot_app?log=4729de21-f9d0-4878-8cb2-cef1a2a8de4c
  • Two MAVLink streams (one over USB, another over TELEM1 UART): https://logs.px4.io/plot_app?log=c2db7247-1c74-49d1-9e98-e690dfaa9fdd
• This P/R:
  • Single MAVLink stream over USB: https://logs.px4.io/plot_app?log=67f60de3-1117-41f6-bcb9-cb054bc7545c
  • Two MAVLink streams (one over USB, another over TELEM1 UART): https://logs.px4.io/plot_app?log=8fc224db-45d3-4741-8622-257244bf5548

[mcsauder]

@korigod , would you rebase again? (It looks like it will pass CI once you rebase it.)

[korigod]

@mcsauder, indeed, it passes the checks now.

[julianoes]

Thanks everyone.