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

Remove TPA from rate control #13208

Merged
merged 1 commit into from
Oct 18, 2019
Merged

Remove TPA from rate control #13208

merged 1 commit into from
Oct 18, 2019

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Oct 16, 2019

Describe problem solved by this pull request
Throttle PID Attenuation (TPA) is a concept used by racing autopilots to get rid of oscillations that occur with different thrust levels than hover thrust. These oscillations are usually because ESCs do not control thrust but rather voltage duty cycle (or in some cases RPM) of the motor and the effective thrust the propeller generates is not just linearly dependent on the actuator output command of the autopilot. TPA is in my eyes a hacky way of working around this problem which is certainly better than not compensating at all but worse than correcting the actual thrust curve because it's hard to tune. And it's overhead like @dagar wrote here: #13173 (comment)

Describe your solution
I suggest to remove TPA from the rate controller since the thrust curve compensation https://docs.px4.io/v1.9.0/en/config_mc/pid_tuning_guide_multicopter.html#thrust_curve is the prefered method of compensation.

This is a proposal and I don't want to step on anyones toes, that's also why I didn't suggest this in #13173 but suggest it now separately.

Describe possible alternatives
A clear and concise description of alternative solutions or features you've considered.

Test data / coverage
SITL gazebo tested.

Additional context
Original feature pr: #5861
Fix for when it was broken for a while and no one noticed: #9166

bkueng
bkueng previously approved these changes Oct 16, 2019
Copy link
Member

@bkueng bkueng left a comment

Choose a reason for hiding this comment

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

Fine with me. I never had any vehicle where I had to use it - the thrust curve solves it cleanly.

Will you update the docs as well?

dagar
dagar previously approved these changes Oct 16, 2019
Copy link
Member

@dagar dagar left a comment

Choose a reason for hiding this comment

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

Sounds good to me, but some people will initially be unhappy. Can we prepare a brief line or two to include in the release notes and when this inevitably comes up in issues and discuss?

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Oct 16, 2019

Will you update the docs as well?

Sure

Can we prepare a brief line or two to include in the release notes and when this inevitably comes up in issues and discuss?

How about the explanation I posted in the description? Someone who can write better English than me can reword it 😇

@dagar
Copy link
Member

dagar commented Oct 16, 2019

Can we prepare a brief line or two to include in the release notes and when this inevitably comes up in issues and discuss?

How about the explanation I posted in the description? Someone who can write better English than me can reword it

Yes that's fine, it's more that I want to make sure it's highlighted at release time.

bresch
bresch previously approved these changes Oct 17, 2019
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.

Nice! Less parameters, less code, less problems.

@julianoes
Copy link
Contributor

Something is not happy:

/RateControl/CMakeFiles/unit-RateControl.dir/RateControlTest.cpp.o -MF src/modules/mc_att_control/RateControl/CMakeFiles/unit-RateControl.dir/RateControlTest.cpp.o.d -o src/modules/mc_att_control/RateControl/CMakeFiles/unit-RateControl.dir/RateControlTest.cpp.o -c ../../src/modules/mc_att_control/RateControl/RateControlTest.cpp

../../src/modules/mc_att_control/RateControl/RateControlTest.cpp:42:76: fatal error: too many arguments to function call, expected 4, have 5

        Vector3f torque = rate_control.update(Vector3f(), Vector3f(), 0.f, false, 0.f);

                          ~~~~~~~~~~~~~~~~~~~                                     ^~~

../../src/modules/mc_att_control/RateControl/RateControl.hpp:96:2: note: 'update' declared here

        matrix::Vector3f update(const matrix::Vector3f rate, const matrix::Vector3f rate_sp, const float dt, const bool landed);

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Oct 17, 2019

On it.

because it's mostly unused and we have thrust curve correction
see parameter THR_MDL_FAC
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Oct 17, 2019

Rebased and corrected the unit test.

@dagar dagar merged commit 08ec6a2 into master Oct 18, 2019
@dagar dagar deleted the remove-tpa branch October 18, 2019 19:54
@mhkabir
Copy link
Member

mhkabir commented Oct 18, 2019

Need a docs update, FYI @hamishwillee

@MaEtUgR
Copy link
Member Author

MaEtUgR commented Oct 21, 2019

Drafted doc change here: PX4/PX4-user_guide#592

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants