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

Plane: Prevent failsafe from returning to DO_LAND_START if already past #13065

Merged
merged 2 commits into from
Feb 18, 2020

Conversation

MattKear
Copy link
Contributor

This PR is commissioned by DUALRC

This is to fix this issue #12264

Problem:
The aircraft will return to the DO_LAND_START marker when a failsafe initiates an RTL or Land command, despite having already passed the DO_LAND_START flag in the mission. This causes the aircraft to extend its flight time and power drain when a battery failsafe has been triggered. This behaviour is demonstrated in the image below:

Problem

This PR fixes the problem by setting the flight state to FLIGHT_LAND once the DO_LAND_START flag has been passed. The resulting behaviour is illustrated below:

Fix

Testing
This has been tested in SITL/Real Flight 8 for both land and RTL failsafe actions. Quadplanes and regular fixed wings have been tested. It has also been tested with multiple DO_LAND_START flags as I have heard of some users that do this. In all of the aforementioned cases the aircraft continues along the intended auto mission after the DO_LAND_START as is intended. RTL mode has been initiated from non-auto modes to ensure that the intended behaviour remains unchanged.

I have tried to think of all of the potential Gotchyas with this approach. I am keen to discuss on Devcall as there maybe some subtleties that I have missed.

Copy link
Contributor

@WickedShell WickedShell left a comment

Choose a reason for hiding this comment

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

Unfortunately this doesn't work as presented, you need to actual track when you enter/exit a landing sequence (which as far as I can tell actually means a DO_LAND_STOP addition). To explain why this is needed it's important to realize that we can plan multiple DO_LAND_START items in the mission and the aircraft will select the nearest geographic one when you ask it to jump to the landing. This can lead to the aircraft flying items after one of the planned landing items (as they can be anywhere in the mission, particularly when jumps get involved) and it would not actually be landing.

A simpler example of this is simply consider the case where the landing was planned as the first thing in the mission, you'd never jump to the landing in this case. (This is not a hypothetical I've seen people plan this before).

Your change will also result in the plane always being in a FLIGHT_LAND stage as soon as the vehicle is beyond the first landing item, I believe.

@MattKear
Copy link
Contributor Author

Having had a chat with @WickedShell I understand some of the different ways that users are typically using the DO_LAND_START flag. I would like to discuss it on the dev call to get agreement on the best way forward.

@MattKear MattKear force-pushed the DO_LAND_START_restart-master branch from a4528b1 to b8dfd73 Compare January 9, 2020 22:34
@IamPete1 IamPete1 force-pushed the DO_LAND_START_restart-master branch 2 times, most recently from b51a712 to 952d69d Compare January 18, 2020 15:55
@IamPete1 IamPete1 dismissed WickedShell’s stale review January 18, 2020 19:15

changed significantly

libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
ArduPlane/events.cpp Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
@MattKear MattKear force-pushed the DO_LAND_START_restart-master branch 3 times, most recently from f40386d to 43bdfe2 Compare February 6, 2020 10:00
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
libraries/AP_Mission/AP_Mission.cpp Outdated Show resolved Hide resolved
@MattKear MattKear force-pushed the DO_LAND_START_restart-master branch 3 times, most recently from cee81a7 to 2acf691 Compare February 9, 2020 12:24
@tridge
Copy link
Contributor

tridge commented Feb 17, 2020

An interesting scenario just came up with quadplanes. I was asked by an operator of a large quadplane how they would force the landing sequence to restart if the plane is coming in too high for a landing and it won't have enough battery to safely vertically land. I would normally say to just switch to RTL and the landing sequence would restart, and the plane will go around, losing height. This PR would seem to defeat that.
So what will be our recommended way to restart landing sequence?

@MattKear
Copy link
Contributor Author

Yes, switching to RTL will still work with this PR. This only effects the behaviour when the battery failsafe is first actioned. Therefore, even if the aircraft is in failsafe, changing mode to RTL will ultimatly clear the in_landing_sequence flag and the aircraft will return to the nearest DO_LAND_START marker to attempt another landing.

To be sure I have tested this scenario in SITL and I can confirm that setting the mode to RTL will give the behaviour you expect.

Another option is for the operator to trigger an abort landing. Again, I have tested this in SITL and it intiates a go around and attempts another landing.

@MattKear
Copy link
Contributor Author

White space issues resolved. Ready for merge.

@peterbarker peterbarker merged commit e2f3cb7 into ArduPilot:master Feb 18, 2020
@peterbarker
Copy link
Contributor

I didn't review this one, but it was decided it could be merged after @Gone4Dirt 's whitespace fixes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants