Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commander: complete thread cycle before emergency shutdown #9563

Merged

Conversation

potaito
Copy link
Contributor

@potaito potaito commented May 30, 2018

Problem: When an emergency landing occurs as part of the battery failsafe, the vehicle will shutdown immediately after landing.
https://github.com/PX4/Firmware/blob/1ee08da9c4182f7e30c2b39462bdead069c9c588/src/modules/commander/commander.cpp#L1964-L1972

However, many state updates like vehicle_status.msg are published at the very end of the cycle:
https://github.com/PX4/Firmware/blob/1ee08da9c4182f7e30c2b39462bdead069c9c588/src/modules/commander/commander.cpp#L2632-L2677

In case of an emergency landing, the code in the second snippet for the updates is not executed, once disarmed. This means that QGC does not register the landed state and last seen state will be the landing in progress. In our case, this appears as a communication loss mid-flight, because armed, which our app treats differently compared to a communication loss when disarmed and landed.

In regard of the new DroneSDK, I would imagine that other apps might run into similar problems.

The proposed solution in this PR is to finish the thread cycle and execute the shutdown command at the very end. It would probably make sense to do the same thing for all calls of px4_shutdown_request() in the commander to avoid similar problems in the future.

@bkueng May I request your input? :)

@potaito potaito requested a review from bkueng May 30, 2018 09:53
@potaito potaito self-assigned this May 30, 2018
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with the change, although adding another flag to commander is not ideal.

int ret_val = px4_shutdown_request(false, false);

if (ret_val) {
mavlink_log_critical(&mavlink_log_pub, "SYSTEM DOES NOT SUPPORT SHUTDOWN");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here we need to reset the flag dangerous_battery_level_requests_poweroff

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks 👍

@@ -2747,6 +2740,20 @@ Commander::run()

arm_auth_update(now, params_updated || param_init_forced);

// Handle shutdown request from emergency battery action
if(dangerous_battery_level_requests_poweroff){
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a minimal chance that the system is armed now, so that should be handled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ooh good catch!! I will add it.

@potaito
Copy link
Contributor Author

potaito commented May 30, 2018

I'm ok with the change, although adding another flag to commander is not ideal.

I agree, but could not think of something better. Hence why this is just a proposal, open for discussion :)

Actually, we could remove the flag emergency_battery_voltage_actions_done as it does not do anything. In the current state it would prevent that the action is performed more than once:
https://github.com/PX4/Firmware/blob/1ee08da9c4182f7e30c2b39462bdead069c9c588/src/modules/commander/commander.cpp#L1964-L1967

But since the action is "shut the system down", we are never leaving that if-statement anyway. Correct me if I'm wrong of course.

Edit: Just saw that the flag is needed in case when the shutdown is not supported. Never mind!

@potaito
Copy link
Contributor Author

potaito commented May 30, 2018

Alternatively we could also move the whole battery check to the end of the run() function, in which case no extra flag would be necessary. But then something else might break 🙈

@potaito potaito force-pushed the pr-commander-finish-cycle-after-emergency-landing branch from d2d0c29 to e4c110e Compare May 30, 2018 14:12
@LorenzMeier LorenzMeier merged commit 15a44a0 into master Jun 2, 2018
@LorenzMeier LorenzMeier deleted the pr-commander-finish-cycle-after-emergency-landing branch June 2, 2018 06:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants