-
Notifications
You must be signed in to change notification settings - Fork 17.4k
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
AC_Circle: Always restore the radius from parameters #16757
AC_Circle: Always restore the radius from parameters #16757
Conversation
nice work. I'd like to hear from other devs but I think the circle radius should always be restored by re-entering circle mode. This would avoid the need for the new option or at least it should be defaulted to 1. The equivalent in Auto mode is the WPNAV_SPEED. If the user exits Auto and enters again the target speed will be reset to the parameter value. So if we go with this then we probably need to change AC_CIRCLE so it doesn't directly use the _radius variable but instead has another radius parameter which is used. set_radius() should only update this temporary value leaving the _radius parameter value unchanged. |
Thanks @rmackay9 . Let's wait for other opinions. In the meanwhile i've flight tested this and it works as expected. I'm not quite sure we should reset the radius every time, since if you start from the perimeter then it might be actually good to keep the user set radius and just continue the circle from where it was stopped. I guess a good compromise could be to reset the radius by default when the user choose to start the circle from the centre of it since if you start at the perimeter you expect the moves of the machine (going CW or CCW), while if you start from the center of the circle, the machine has to reposition itself and it might head very quickly toward and obstacle. What do you think? |
d07e6d8
to
10d8031
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.
Just need to make sure were still using the right radius and not the param value for circles via auto and guided.
10d8031
to
88ef200
Compare
Tested with SITL, LGTM 👍 |
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.
This breaks auto circles, they are done at the param radius rather than the radius given in the mission. I suspect your not testing it correctly because they also seem to be fine on master for me.
To fix I think you just need to set the param value not the internal variable in the set rad call.
ardupilot/libraries/AC_WPNav/AC_Circle.cpp
Line 150 in f7da853
_radius = constrain_float(radius_cm, 0, AC_CIRCLE_RADIUS_MAX); |
Actually my suggestion will break updating the radius from sticks in a mission. You probably need a Of course this change also means that you cant just change the radius param and have it update in real time. You would have to exit and re-enter circle mode. Not a complaint, just a observation. |
88ef200
to
1ba69f5
Compare
Thanks for the review @IamPete1 |
@giacomo892 Yeah, Loiter turns, you need to give it a number of turns and a radius. |
Tested on SITL, working as expected beside #17126 |
I thought there was an issue with the get_radius() needing to check if init() had been called or not because there is a code path that calls get_radius() before init is called. I think it's when the Loiter-turns command has a lat,lon specified. |
Thanks for the review. Looks like it is not the case, it seems all good to me. |
1ba69f5
to
d200e77
Compare
d200e77
to
244946b
Compare
244946b
to
fbd2caa
Compare
add _last_radius? @IamPete1 will do a new PR |
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.
Re-tested in SITL, works great, Thank you
Allow the pilot to reload the circle radius from the parameters every time CIRCLE mode is enabled.
It is nice from a safety point of view. Had my copter heading towards a tree because I supposed the circle to be 10m in radius, instead it was much higher due to me having adjusted it with the pitch stick in a previous use of CIRCLE mode.