Skip to content

Prevent rotation pitch calculations from running post-rotation #29834

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

Conversation

Georacer
Copy link
Contributor

@Georacer Georacer commented Apr 20, 2025

Summary

This PR prevents the nav_pitch calculations related to takeoff rotation (Plane::takeoff_calc_pitch()) from taking place after the plane has completed the rotation.

It addresses an issue reported for ArduPlane 4.6beta.

Details

The nav pitch during rotation is set to scale linearly based on the groundspeed/airspeed_cruise ratio.
After that, the rest of the pitch calculations (e.g. calc_nav_pitch() when airspeed is enabled) can resume.
Also, at some point, if enabled, the level-off calculations will take over and reduce the demanded pitch.

But, if due to misconfiguration or a sudden strong headwind the groundspeed falls below cruise airspeed, the rotation calculations will take effect again and override the rest.

One result can be that pitch will be held high and the level-off calculations will be ignored.
In the following SITL test, the red cursor is set on the point where a strong headwind is turned on:
Screenshot from 2025-04-20 17-57-26

Another user has reported that there's also a chance to lead to a ping-pong effect, when the groundspeed oscillates around cruise airspeed:
image

The fix declares a new state flag, which gets set when the rotation is first complete.
Then, it will never let the rotation calculations to run again in the same takeoff.

Screenshot from 2025-04-20 17-54-41

An autotest has been created to prevent future regression.

Additional test fix

In my local testing the autotest Plane.TakeoffGround() would fail locally, even though it has recently passed in CI.

I have found that this is related to the difference between VHR_HUD.groundspeed and gps().groundspeed(), both in value and publication rate.

The autotest waits for the VFR_HUD.groundspeed to check the nav_pitch angle, which comes from AHRS.grounspeed.
But Plane::takeoff_calc_pitch() uses gps().groundspeed(), which is both lower and has lower refresh rate.
As a result, at the time of the check, the nav_pitch angle was outside of the test interval (14.69<14.9) and fail the test:
image

The test shifts the check for a bit later.
It was also necessary to push back the loiter part of the takeoff, to remove the effects of level-off and stall prevention (during banks) on nav_pitch.

Testing

Testing in SITL has been done:
takeoff_rotation_vs_leveloff_tests.zip

@IamPete1
Copy link
Member

This PR is against 4.6? Do we have the same issue in master?

tridge
tridge previously requested changes Apr 21, 2025
Copy link
Contributor

@tridge tridge left a comment

Choose a reason for hiding this comment

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

thanks for this! just need to make sure we reset for takeoff mode

@rmackay9 rmackay9 mentioned this pull request Apr 21, 2025
97 tasks
@tridge
Copy link
Contributor

tridge commented Apr 21, 2025

@Georacer we also need a PR against master, should go into master first

@Georacer
Copy link
Contributor Author

@Georacer we also need a PR against master, should go into master first

Here have it! #29860

@tridge tridge dismissed their stale review April 24, 2025 02:03

fixed

@rmackay9
Copy link
Contributor

@Georacer,

I've re-run the failing test but it doesn't seem to be happy for some reason. Maybe you could have a peek and see if you can figure out why it won't pass?

@Georacer
Copy link
Contributor Author

@Georacer,

I've re-run the failing test but it doesn't seem to be happy for some reason. Maybe you could have a peek and see if you can figure out why it won't pass?

I will try locally. Thanks Randy!

@tridge tridge force-pushed the pr/takeoff_rotation_vs_leveloff branch from 17896a5 to d47bc49 Compare April 28, 2025 09:22
@tridge
Copy link
Contributor

tridge commented Apr 28, 2025

@Georacer forced push change to try to make this pass CI

@Georacer
Copy link
Contributor Author

@tridge it still fails two builds, in what I think is not related to my change.
Could you take another look?

@rmackay9
Copy link
Contributor

This has been included in 4.6.0-beta via PR #29888

@rmackay9 rmackay9 closed this Apr 29, 2025
@rmackay9 rmackay9 moved this from Pending to 4.6.0-beta6 in 4.6 Backports Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 4.6.0-beta6
Development

Successfully merging this pull request may close these issues.

4 participants