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

TPA optimisations #12721

Merged
merged 5 commits into from
May 11, 2023
Merged

Conversation

ctzsnooze
Copy link
Member

@ctzsnooze ctzsnooze commented Apr 24, 2023

Code optimisation for the TPA calculation.

Master code requires 26 cycles while throttle < TPA breakpoint, and 63 cycles while sticks are above breakpoint.

This PR reduces that to only 8 cycles, regardless of the throttle position, and returns the same result. This is achieved by doing most of the computation in pid_init.c and avoiding conditional statements.

I'm not sure that throttle can be > 1.0 at this point in the code, but kept that check in place, just in case, since if it was > 1.0 then P and D could be pushed higher than intended.

@ctzsnooze ctzsnooze requested review from KarateBrot, haslinghuis, blckmn and ledvinap and removed request for KarateBrot and haslinghuis April 24, 2023 06:05
@ctzsnooze ctzsnooze self-assigned this Apr 24, 2023
@github-actions

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Apr 24, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> FAIL
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> FAIL
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> PASS

Copy link
Contributor

@ledvinap ledvinap left a comment

Choose a reason for hiding this comment

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

Is it really worth touching this code? It's some obscure heuristic that probably works.
This PR does only shave few cycles (at the cost of more state variables), but code is still poorly documented and hard to read.

src/main/flight/pid.c Outdated Show resolved Hide resolved
src/main/flight/pid.c Outdated Show resolved Hide resolved
src/main/flight/pid_init.c Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ctzsnooze
Copy link
Member Author

ctzsnooze commented Apr 24, 2023

I do think that saving those cycles is important. Future work for more precise attitude control will consume more cycles, and an F411 overclocked to 120Mhz with a 4k PID loop is using 71% CPU in Angle mode already.

I measured the whole primary PID loop in Angle mode as consuming at baseline 1,657 cycles. I'm not sure where the remaining 19,000 cycles get consumed :-)

This PR saves 55 of those cycles, a non-trivial proportion of the total, and the maths is no more difficult to follow than before.

This graphic shows the measured cycleCount for the main pidController() function, over about ⅓ of a second.
The 'minimum' is 1657 cycles, with regular spikes up to about 2400 cycles. The spikes occur approximately every third PID loop. I'd love to know what causes them.

Screen Shot 2023-04-24 at 23 56 03

@ledvinap
Copy link
Contributor

Sorry, I thought that TPA runs at RC update rate, not PID one. In that case saving is more important.

@haslinghuis
Copy link
Member

@ledvinap moved TPA in #11779 with a patch #11969

src/main/cli/settings.c Outdated Show resolved Hide resolved
src/main/flight/pid_init.c Outdated Show resolved Hide resolved
src/main/flight/pid.c Outdated Show resolved Hide resolved
@ctzsnooze
Copy link
Member Author

@KarateBrot @ledvinap would appreciate review again... I hope I addressed the comments appropriately?
Keen to get this merged.

@ctzsnooze ctzsnooze requested a review from KarateBrot May 10, 2023 00:59
@github-actions

This comment has been minimized.

src/main/flight/pid_init.c Outdated Show resolved Hide resolved
@github-actions
Copy link

Do you want to test this code? Here you have an automated build:
Assets
WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

@haslinghuis haslinghuis merged commit 10067ad into betaflight:master May 11, 2023
AkankshaJjw pushed a commit to AkankshaJjw/betaflight that referenced this pull request May 29, 2023
* TPA optimisations

* improvement, thanks @ledvinap

* update following review comments, thanks karatebrot and ledvinap

* include rx.h in pid_init.c to get PWM_RANGE_MIN

* review suggestion
obeny pushed a commit to obeny/betaflight that referenced this pull request Oct 11, 2023
* TPA optimisations

* improvement, thanks @ledvinap

* update following review comments, thanks karatebrot and ledvinap

* include rx.h in pid_init.c to get PWM_RANGE_MIN

* review suggestion
obeny pushed a commit to obeny/betaflight that referenced this pull request Oct 23, 2023
* TPA optimisations

* improvement, thanks @ledvinap

* update following review comments, thanks karatebrot and ledvinap

* include rx.h in pid_init.c to get PWM_RANGE_MIN

* review suggestion
obeny pushed a commit to obeny/betaflight that referenced this pull request Nov 15, 2023
* TPA optimisations

* improvement, thanks @ledvinap

* update following review comments, thanks karatebrot and ledvinap

* include rx.h in pid_init.c to get PWM_RANGE_MIN

* review suggestion
davidbitton pushed a commit to davidbitton/betaflight that referenced this pull request Feb 5, 2024
* TPA optimisations

* improvement, thanks @ledvinap

* update following review comments, thanks karatebrot and ledvinap

* include rx.h in pid_init.c to get PWM_RANGE_MIN

* review suggestion
@ctzsnooze ctzsnooze deleted the PR-TPA-optimisations branch September 5, 2024 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

7 participants