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

navigator: set acceptance radius even for IDLE waypoint #13965

Merged
merged 3 commits into from Jan 21, 2020

Conversation

RomanBapst
Copy link
Contributor

This fixes the heading drift people have been seeing right after a takeoff.
The problem was that the auto flighttask was creating a yaw setpoint in the direction of the velocity setpoint. There is code implemented which locks the yaw setpoint if the vehicle is within the acceptance radius of the target, however, the navigator published an IDLE setpoint with an acceptance radius of 0 and this is what was used by the flight task.

Still not sure why the acceptance radius was not updated once the takeoff setpoint was sent.

@RomanBapst
Copy link
Contributor Author

@MaEtUgR FYI

@RomanBapst
Copy link
Contributor Author

Before this PR:
image

After this PR:
yaw_good

@dagar
Copy link
Member

dagar commented Jan 17, 2020

This seems kind of weird and out of place. What if instead we used Navigator::reset_triplets() in that section of logic to set appropriate defaults in all the position setpoints, and then set it to position_setpoint_s::SETPOINT_TYPE_IDLE?

@MaEtUgR
Copy link
Member

MaEtUgR commented Jan 17, 2020

@RomanBapst Please mention the original issue when you make a pr for a fix. It was this one: #13675

Your findings explain why the yaw setpoint is not exactly the yaw estimate to begin with. But it doesn't explain why the yaw setpoint is quickly unfiltered jumping to zero and otherwise gets heavily filtered even though it's a reset. At least the jump to zero is a concern to me.

Is there an easy way to reproduce the issue in SITL such that it happens every takeoff? That would help debugging the other issues.

@RomanBapst
Copy link
Contributor Author

@MaEtUgR Have a look at the plot above with the fix. I don't see any yaw clamping anymore.
I investigated why the yaw setpoint was moving around in arbitrary directions in the first place. That was obviously causes by the flighttask doing it's velocity vector heading calculations which was in turn caused by the acceptance radius being publishes as 0 by the navigator.

If you still see any clamping issues let me know, I do not see them.
Any by the way, the problem was easily reproducible in SITL. The only reason why you did not see it 100% of the time was because sometimes by coincidence the yaw setpoint was actually close to the current yaw during takeoff and so the effect was not really visible.

Signed-off-by: RomanBapst <bapstroman@gmail.com>
Signed-off-by: RomanBapst <bapstroman@gmail.com>
@RomanBapst
Copy link
Contributor Author

@dagar I followed your suggestion, please have a look.

Copy link
Member

@bresch bresch left a comment

Choose a reason for hiding this comment

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

Good catch!

Copy link
Member

@MaEtUgR MaEtUgR left a comment

Choose a reason for hiding this comment

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

Sorry that I didn't have time to look at all the details. You should probably get the fix in before we figure out all the questions I added in my comment.

_pos_sp_triplet.previous.acceptance_radius = get_default_acceptance_radius();
_pos_sp_triplet.current.acceptance_radius = get_default_acceptance_radius();
_pos_sp_triplet.next.acceptance_radius = get_default_acceptance_radius();

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change

@dagar dagar merged commit fa83f37 into master Jan 21, 2020
@bresch bresch deleted the pr-fix_heading_drift_takeoff branch January 21, 2020 16:24
@julianoes julianoes mentioned this pull request Jan 23, 2020
@mrpollo mrpollo mentioned this pull request Mar 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants