-
Notifications
You must be signed in to change notification settings - Fork 13.7k
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
RTL: Perform transition towards home location #14780
Conversation
065001b
to
7757c1e
Compare
This issue has been automatically marked as stale because it has not had recent activity. Thank you for your contributions. |
@dayjaby there were a lot of VTOL RTL changes coming in with #16377, resulting in conflicts here that would need to be addressed first. |
@dayjaby Do you have time to rebase this? |
Sure! Did not notice Silvans message. |
7757c1e
to
b487c39
Compare
b487c39
to
742b4d2
Compare
Log file: https://logs.px4.io/plot_app?log=455021db-8971-4254-a61f-51a95cbfb88f The transition overshoots a little, which can be compensated by setting larger loiter radius, for example 70m instead of 50m: https://logs.px4.io/plot_app?log=a8c69cac-4c30-4546-9579-ea48f7886047 |
@sfuhrer can you check this? |
@dayjaby It needs another rebase. Otherwise I think it's a nice improvement with rather minimal changes. I wonder though if we shouldn't just use NAV_LOITER_RAD for the loiter radius (I know, previously I commented otherwise, but we really only should introduce new params if we're really convinced) - I guess you introduced a new one to specifically be able to tune if for the optimal distance required to finish the transition right over home? |
19173da
to
78b4071
Compare
@sfuhrer "Right over home" not so much, but rather having not too high turning angles, especially close to home. In the future I would like to perform the back transition against the wind. Are the values from the wind_estimator reliable enough for that? |
Makes sense. But then I guess it's okay to use NAV_LOITER_RAD, or do you see a reason why the RTL loiter should have different radius than the Hold loiter? Both could e.g. be set to 75 for a 2m wingspan VTOL.
I would say so yes, especially as during the loiter down you can estimate the wind quite well |
I think the descend loiter radius should in general be higher than the normal loiter radius. While descending you gain more speed, so having less sharp turns might be beneficial. This also means that you can descend faster with a larger radius. If this benefit can be neglected, we can go for the NAV_LOITER_RAD approach. |
@RomanBapst what's your opinion here? I still feel like having just one loiter radius param for now would be better, and add a separate one if flight testing shows the clear need for it. |
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.
@dayjaby I'm not quite following how this should work. Where are you now triggering the transition once the home waypoint is reached?
I tested in SITL and I had a flyaway so I think something is not quite right.
Could you please re-test?
Hey @dayjaby can you please help us address some of the reviewer comments above? we would love to get this merged soon |
78b4071
to
0e80aac
Compare
0e80aac
to
2910a70
Compare
@RomanBapst I am sorry; a slice of code did not survive one of the rebases. Thanks for testing it! This time I tested it properly: https://logs.px4.io/plot_app?log=aa3bf100-4cb5-453b-9dcb-a4738ab04126 The backtransition is done if the default acceptance radius is reached. |
@julianoes Regarding the failed SITL test: I am pretty sure this PR does not influence any iris flights. I also tried your failure injection and it lands successfully.
Do you have any idea what's wrong here? |
* @increment 0.5 | ||
* @group Return Mode | ||
*/ | ||
PARAM_DEFINE_FLOAT(RTL_LOITER_RAD, 50.0f); |
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.
@dayjaby This is now the same default as NAV_LOITER_RAD. Does this really makes sense?
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 not, but I would like some test flights first before setting it to a more sane value.
@dayjaby I have one last comment regarding the parameter default. Please advise on that. Other than that I think this PR is good to go. |
@RomanBapst Comparing the two previous log files (https://logs.px4.io/plot_app?log=a8c69cac-4c30-4546-9579-ea48f7886047 and https://logs.px4.io/plot_app?log=455021db-8971-4254-a61f-51a95cbfb88f), by just increasing the descend loiter radius from 50m to 70m, the roll angle when backtransition commences decreases from 42 degrees to 15 degrees. Let's go for this value? |
Describe problem solved by this pull request
Followup on #14728. To prevent performing a transition while still loitering around the home location, this PR makes sure that the drone heads towards the home location before triggering the transition. For this purpose, it is reasonable to add a new parameter RTL_LOITER_RAD to allow the user to set a loiter radius for the altitude descending that still allows the drone to turn towards the home location before initiating a transition.
Test data / coverage
Sitl gazebo log: https://review.px4.io/plot_app?log=b71abace-d7ba-4f9d-a2ef-71ac005d09e1