-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
Switch into last mode after landing and disarming after RTL/Land #12494
Conversation
1e41761
to
37688c4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that's all correct except that one comment.
83861c6
to
e3dced9
Compare
How will this effect battery swaps in Qgroundcontrol? When flying a large survey mission, it is common for low battery failsafe to trigger and the vehicle executes RTL (Return Mode). |
80fd2f5
to
48db423
Compare
480db61
to
7942c96
Compare
7942c96
to
eb14b49
Compare
@dakejahl I didn't realize rebasing would dismiss your review. I haven't made any more changes, so this should still be ready to go! |
I have such mixed feelings about rebasing lol |
src/modules/commander/Commander.cpp
Outdated
if (disarmed_and_mission_finished && is_auto_state(internal_state.main_state) | ||
&& last_non_auto_state != commander_state_s::MAIN_STATE_MAX) { | ||
// This branch will only happen once per mission finish because, after transitioning to the non-auto | ||
// state, is_auto_state(internal_state.main_state) will be false. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user switches to mission again within MISSION_FINISH_DISARM_TIMEOUT
, it will be true again, so the comment is not quite right.
Since the timeout is 500ms, the logic should be fine, but I'd be more comfortable if multiple triggering of this is prevented (for example we might have to increase MISSION_FINISH_DISARM_TIMEOUT
for some reason).
How does COM_DISARM_LAND
matter for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does
COM_DISARM_LAND
matter for this?
I think it won't work if COM_DISARM_LAND
is bigger, right? Because too much time has lapsed by the time it disarms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed this to not rely on timing. Now, it just makes sure all of the following conditions are met:
- The vehicle just now disarmed
- The vehicle is currently landed
- The current mode is an auto mode
- The current mission is finished
Here's the code:
bool just_disarmed = !armed.armed && was_armed;
bool just_finished_auto_mission = is_auto_state(internal_state.main_state) && _mission_result_sub.get().finished;
bool last_state_valid = last_non_auto_state != commander_state_s::MAIN_STATE_MAX;
if(just_disarmed && land_detector.landed && just_finished_auto_mission && last_state_valid){
PX4_INFO("Just finished auto mission, transitioning back to last manual mode.");
main_state_transition(status, last_non_auto_state, status_flags, &internal_state);
}
Is this sufficient? Or did I miss some edge case?
When this goes in, can you ping me. Probably should be documented in the land/return mode docs. |
2cad947
to
e05adca
Compare
@@ -375,6 +377,11 @@ main_state_transition(const vehicle_status_s &status, const main_state_t new_mai | |||
|
|||
if (ret == TRANSITION_CHANGED) { | |||
if (internal_state->main_state != new_main_state) { | |||
// If transitioning from non-auto to auto state, save the last non-auto state | |||
if (!is_auto_state(internal_state->main_state)) { | |||
last_non_auto_state = internal_state->main_state; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you do two consecutive RTLs this stays in the first non-auto state and will switch back to the manual mode in the first RTL, even if the second RTL was triggered out of an auto state. You need to reset the state in an else
statement to not have history in your state machine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me make sure I understand correctly: If you have the sequence, for example, Manual -> Mission -> RTL -> Mission -> RTL
, then it will switch back to Manual
after the final RTL
completes. Should it not do this? Under what circumstances exactly should it not switch back to a manual mode after an RTL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess what @LorenzMeier means is that in this case it should switch back to Mission
for this case.
This would mean the check should be for any mode that is not RTL, instead of non-auto mode.
@LorenzMeier can you confirm please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My expectation would be here that it switches back to the mode it had before RTL, not the last manual mode before RTL - otherwise I could have been through a number of mission - RTL - mission - RTL sequences and be left with the manual mode from an hour ago
65628ca
to
c945a02
Compare
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
This is pending a comment by @LorenzMeier. |
What Problem is this PR trying to solve? |
when landed after an RTL, PX4 requires to be switched back to a flight mode manually, before being able to take off. IMO this is confusing. I would expect it to be ready for take off, after the RTL command is finished. |
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
Closing as stale. |
Describe problem solved by the proposed pull request
Closes #12354 .
Test data / coverage
Ran several missions in SITL, starting from every non-auto mode. After landing (Either via RTL or just landing in place), I verified that the mode returned to the most recent non-auto mode.
Describe your preferred solution
After an RTL or Auto Landing, and after disarming, the Commander will now automatically switch to the last mode that was used before the auto mission. For example, if the sequence of modes is:
Position -> Auto Mission -> Auto RTL
After the landing and disarming, it will switch back to Position.