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

Fix for unhandled TKOFF_THR_MAX #27693

Merged
merged 3 commits into from
Jul 31, 2024

Conversation

Georacer
Copy link
Contributor

@Georacer Georacer commented Jul 29, 2024

This patches a bug that was introduced via #27174.

It fixes an unhandled case of TKOFF_THR_MAX=0, where THR_MAX would not be sourced as a value for the minimum throttle limit during takeoff.
The relevant conditions are:

  • TKOFF_OPTIONS=0 (default)
  • TKOFF_THR_MAX=0 (default)

This would result in incorrect backing-off of the minimum throttle during the climb, which @Hwurzburg spotted:
image

Now it behaves as it should:
image
Log: https://www.dropbox.com/scl/fi/akdqr9o5niv3qleexh3yd/122221_TakeoffTakeoff4.bin?rlkey=o9uykk6cm7785oxctkj17txxq&dl=0


Side-note:

I notice that there was significant pitch and throttle oscillation in Henry's log.
I don't think this is related to #27174, as the oscillation persists even after TAKEOFF mode has finished climbing, where TECS operates in a regime which I haven't edited.
I believe that a sudden pitch-down and throttle-cut was triggered (because of this bug) during the climb and the default tuning and model physics would yield bad control behaviour.

@Hwurzburg
Copy link
Collaborator

This makes it safe from my sitl testing...
in another PR we need to get the model fixed so as not to go unstable
and , more importantly, get the takeoff leveling working again to prevent the overshoots in altitude on auto takeoffs

@peterbarker
Copy link
Contributor

Pretty sure this is breaking several Plane autotests.

@Georacer
Copy link
Contributor Author

@peterbarker I believe the latest commit fixed it.
It was a case of the minimum throttle during a transition being set to TAKEOFF_THR_MAX. This is because in servos.cpp the transition throttle limits were historically being treated the same as the takeoff throttle limits. And this resulted in a back-transition getting a minimum throttle allowance of TAKEOFF_THR_MAX.

For now I have reverted to the behaviour before #27174.
@tridge I'll have to find another way to introduce the minimum throttle during a transition (and find a way to tell between a forward and backward transition).

The failing autotests passed locally, but let's see what the CI server says about it.

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.

i think we can make min takeoff throttle work for fwd transition

ArduPlane/servos.cpp Show resolved Hide resolved
ArduPlane/servos.cpp Show resolved Hide resolved
@tridge tridge merged commit 85bb4ad into ArduPilot:master Jul 31, 2024
63 checks passed
@IamPete1 IamPete1 removed the DevCallEU label Aug 7, 2024
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.

5 participants