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

rc_update: throttle trim centering fix for reverse channel #21682

Merged
merged 2 commits into from
Jun 6, 2023

Conversation

MaEtUgR
Copy link
Member

@MaEtUgR MaEtUgR commented Jun 5, 2023

Solved Problem

When integrating an airframe I found that if the throttle channel is reversed the logic that corrects for QGC calibrating the throttle to half the range (see #15949 (comment))

Solution

Changelog Entry

For release notes:

Bugfix: Get full throttle range for QGC RC calibration with reverse throttle channel

Alternatives

This can be removed again once the calibration in QGC is revised and available in a stable release for long enough.

Test coverage

  • I tested this on the real vehicle with SBUS and a reversed throttle.
  • I also double-checked that the unreversed normal case still works as expected on the same setup by changing the parameters to a non-reversed throttle case.

The entire logic did not work for the case when the throttle channel is
reversed because then QGC sets trim = max for that channel and
the result is only half the throttle range.
@MaEtUgR MaEtUgR requested a review from junwoo091400 June 5, 2023 12:36
@MaEtUgR MaEtUgR self-assigned this Jun 5, 2023
Copy link
Contributor

@junwoo091400 junwoo091400 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 cleaned up fix!

Comment on lines +220 to +221
const bool normal_case = !throttle_rev && (throttle_trim == throttle_min);
const bool reversed_case = throttle_rev && (throttle_trim == throttle_max);
Copy link
Contributor

Choose a reason for hiding this comment

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

These cases are relevant only because of the hotfix in the QGC right? If so, should we note it in the code?

Codewise this is a nice way to wrap up #21576!

Copy link
Member Author

Choose a reason for hiding this comment

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

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/px4-maintainers-call-june-06-2023/32473/1

Copy link
Member Author

@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.

Thanks for reviewing! Then I'll merge when everything passes.

Comment on lines +220 to +221
const bool normal_case = !throttle_rev && (throttle_trim == throttle_min);
const bool reversed_case = throttle_rev && (throttle_trim == throttle_max);
Copy link
Member Author

Choose a reason for hiding this comment

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

@MaEtUgR MaEtUgR merged commit f4de43a into main Jun 6, 2023
81 of 84 checks passed
@MaEtUgR MaEtUgR deleted the maetugr/rc-throttle-trim-reverse-fix branch June 6, 2023 15:22
@MaEtUgR
Copy link
Member Author

MaEtUgR commented Jun 6, 2023

Needs to also go on 1.14 👀

@DronecodeBot
Copy link

This pull request has been mentioned on Discussion Forum for PX4, Pixhawk, QGroundControl, MAVSDK, MAVLink. There might be relevant details there:

https://discuss.px4.io/t/driving-backwards-with-a-boat-rover-in-manual-mode/25714/9

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

Successfully merging this pull request may close these issues.

None yet

3 participants