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: Mode LoiterAltQLand to reuse same loiter point if available #25318

Merged
merged 2 commits into from
Nov 27, 2023

Conversation

magicrub
Copy link
Contributor

Current behavior:

  • When switching to mode LoiterAltQLand we set the current location as the center of the loiter.

Problem:

  • If we were already performing a loiter then our circle center changes

New/Proposed behavior in this PR:

  • When switching to mode LoiterAltQLand, if we were already in a loiter then continue using that same center location point

@magicrub
Copy link
Contributor Author

Along with PR #25319 this will enable this flight mode to have accurate landings

IamPete1
IamPete1 previously approved these changes Oct 23, 2023
Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

LGTM, had a little play in SITL, a nice improvement.

@@ -10,17 +10,17 @@ bool ModeLoiterAltQLand::_enter()
return true;
}

const Location previous_wp = plane.next_WP_loc;
const bool already_in_a_loiter = plane.reached_loiter_target();
Copy link
Contributor

Choose a reason for hiding this comment

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

this could be true in modes like FBWA, cruise etc, would go to the wrong position

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tridge changed to
const bool already_in_a_loiter = plane.previous_mode->does_auto_navigation() && plane.reached_loiter_target();

Which I think should handle all the cases.
Note, I CAN NOT use the most accurate and direct check plane.loiter.start_time_ms > 0 because it gets set to 0 in set_mode() before _enter() is called. I looked through the code and it looks like moving that set to 0 is probably OK to move to after the _enter() is successful but that has the potential to create some gremlins that aren't worth it. At this point I feel like reached_loiter_target() not being set to zero in the same place is a bug fixing that by setting to zero would break this PR. To globally fix that we would change reached_loiter_target() to return control_mode->does_auto_navigation() && nav_controller->reached_loiter_target(); but that would need some additional testing.

@tridge tridge added WikiNeeded needs wiki update and removed DevCallTopic labels Oct 24, 2023
@magicrub magicrub force-pushed the pr/vtol_land_here branch 2 times, most recently from acb6515 to e0e6751 Compare October 24, 2023 19:37
@magicrub
Copy link
Contributor Author

@Hwurzburg wiki update here ArduPilot/ardupilot_wiki#5499

@magicrub
Copy link
Contributor Author

This is ready to merge with suggestion(s) from the DevCall applied.

@magicrub
Copy link
Contributor Author

I wrote this to do millis() checks and it just looked way too cumbersome for what we're wanting out of this. I realized a more straight-forward solution is to just invalidate the state AFTER a mode change so that enter() and _enter() will still get the existing state but after that it's correctly cleared. That's full-proof. If we want something MORE full full-proof that is time based then we should re-work loiter.start_time_ms to be updated appropriately in all modes which would warrant a new/different PR and lots of testing.

@magicrub
Copy link
Contributor Author

@IamPete1 changes suggested by you from the devcall have been applied. Please re-review

Copy link
Member

@IamPete1 IamPete1 left a comment

Choose a reason for hiding this comment

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

I have tested switching into LoiterAltQLand from a mode which is loitering and the point is not moved. I also tested changing to intermediate mode and the point does move.

@tridge tridge merged commit d1b2c6e into ArduPilot:master Nov 27, 2023
56 checks passed
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.

None yet

4 participants