ardupilot icon indicating copy to clipboard operation
ardupilot copied to clipboard

Plane: Fix MAV_CMD_NAV_LOITER_TURNS with zero turns skipping waypoint location

Open joshanne opened this issue 1 year ago • 4 comments
trafficstars

In the case where MAV_CMD_NAV_LOITER_TURNS waypoint is used the AV can skip travelling to the waypoint and jump directly to the next waypoint (if the conditions are correct).

This ensures loiter turns cannot be verified as complete until both loiter target is reached and turn count is met (the latter half is unchanged).

First commit shows the issue occurring, and is an example use case where you might use 0-turn loiters. Second commit shows the fix to the issue.

Before After
image image

joshanne avatar Nov 14 '24 01:11 joshanne

@peterbarker @tridge I suspect a similar issue of short circuiting a waypoint can occur with LOITER_TO_ALT, given it is not checking for reaching of the loiter target either.

joshanne avatar Nov 14 '24 01:11 joshanne

@peterbarker @tridge I suspect a similar issue of short circuiting a waypoint can occur with LOITER_TO_ALT, given it is not checking for reaching of the loiter target either.

Although, i haven't been able to prove this through a separate autotest.

joshanne avatar Nov 14 '24 22:11 joshanne

@joshanne I'd like to look at this a bit more

tridge avatar Nov 19 '24 00:11 tridge

@tridge have you had a chance to look at this yet?

I think this PR is good. It's not addressing the underlying issue, but it's applying the same fix that other places in the code have employed and is the simplest correction with the lowest chance to hurt anything. And this is, I think, a pretty critical bug.

The root of the issue seems to stem from here. In loiter_angle_update, we specifically try to stop the angle sum from accumulating until we reach the loiter target, but between clearing the angle and calling update_loiter (where the reached_loiter_target finally switches back to false), this update occurs, so this protection doesn't always work. https://github.com/ArduPilot/ardupilot/blob/b722ea8f2f88719f7c1ee92d5257f215fee0b68e/ArduPlane/navigation.cpp#L26-L36

This snippet in loiter_angle_update would work correctly if loiter_angle_reset also reset the flag in the navigation controller that tracks if we have reached the loiter target. I made a quick patch that added a function to clear the _WPcircle flag and that does work to fix this bug, but it's also less clear to me that it doesn't come with other unintended consequences.

robertlong13 avatar Jan 20 '25 03:01 robertlong13

@joshanne @robertlong13 I've rebased this PR on master

tridge avatar Jun 09 '25 22:06 tridge

No worries, what's your thoughts on it being an actual issue?

joshanne avatar Jun 09 '25 22:06 joshanne

@joshanne my apologies for forgetting about this one!

tridge avatar Jun 09 '25 22:06 tridge

No worries, what's your thoughts on it being an actual issue?

yes, I think it is, we'll merge, thanks!

tridge avatar Jun 09 '25 23:06 tridge

Merged, thanks!

peterbarker avatar Jun 10 '25 04:06 peterbarker

I've pulled the code change into 4.6.2-beta1 but was unable to pull in the autotest change due to a merge conflict

rmackay9 avatar Jun 24 '25 10:06 rmackay9

This is included in 4.6.2-beta1 via https://github.com/ArduPilot/ardupilot/pull/30466

rmackay9 avatar Jun 25 '25 07:06 rmackay9