-
Notifications
You must be signed in to change notification settings - Fork 13.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
navigator: fix acceptance radius for multicopter #17661
Conversation
|
||
// We use the acceptance radius of the mission item if it has been set (not NAN) | ||
// but only for multicopter. | ||
if (_navigator->get_vstatus()->vehicle_type == vehicle_status_s::VEHICLE_TYPE_ROTARY_WING |
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.
Can't this logic live inside of get_acceptance_radius()
? We could again pass _mission_item.acceptance_radius
as an argument.
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 actually found the way it was before quite confusing. So get_acceptance_radius()
had this overload but the overload was only used from this location, right?
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.
Right. If one anyway only wants to take the acceptance radius from the param then using get_default_acceptance_radius() would be cleaner though no? And always pass _mission_item.acceptance_radius
as argument of get_acceptance_radius()
?
3/4 SITL tests failed in "Fly VTOL mission". They all seem like they completed, but never disarmed. Restarting to get more data. |
I'll see if I can reproduce. |
|
||
// We use the acceptance radius of the mission item if it has been set (not NAN) | ||
// but only for multicopter. | ||
if (_navigator->get_vstatus()->vehicle_type == vehicle_status_s::VEHICLE_TYPE_ROTARY_WING |
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.
Right. If one anyway only wants to take the acceptance radius from the param then using get_default_acceptance_radius() would be cleaner though no? And always pass _mission_item.acceptance_radius
as argument of get_acceptance_radius()
?
// We use the acceptance radius of the mission item if it has been set (not NAN) | ||
// but only for multicopter. | ||
if (_navigator->get_vstatus()->vehicle_type == vehicle_status_s::VEHICLE_TYPE_ROTARY_WING | ||
&& PX4_ISFINITE(_mission_item.acceptance_radius) && _mission_item.acceptance_radius > 0.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.
Is a comparison >0.0f safer than >FLT_EPSILON?
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.
Right, let me change that.
@julianoes SITL tests continue to fail - is this a release blocker? It is not a core safety issue as we default to the param in many other places, but some use cases might rely on setting the acceptance radius manually. |
@LorenzMeier it makes sense to get it in before the release and I think the CI error is unrelated:
|
a8d90ab
to
cdaa4cb
Compare
This fixes a regression introduced in #16646 which meant that the acceptance radius was no longer used at all for multicopter, and instead only the NAV_ACC_RAD param was used. With this change we use the acceptance radius of the mission item again if it is actually set (and not NAN) which we did not do before, and we only do that for multicopter.
This seems to slip in e.g. as part of the VTOL_LAND command.
cdaa4cb
to
27f4fc7
Compare
Only my suggestion |
This fixes a regression introduced in #16646 which meant that the acceptance radius was no longer used at all for multicopter, and instead only the NAV_ACC_RAD param was used.
With this change we use the acceptance radius of the mission item again if it is actually set (and not NAN) which we did not do before, and we only do that for multicopter.
Found while working on mavlink/MAVSDK#1443.