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 setting pid denom #13535

Merged
merged 2 commits into from Apr 21, 2024
Merged

Conversation

@haslinghuis haslinghuis added this to the 4.5 milestone Apr 16, 2024
@haslinghuis haslinghuis self-assigned this Apr 16, 2024
Copy link

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #13535 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@haslinghuis
Copy link
Member Author

pid_denom is reset here again:

} else {
const float pidLooptime = samplingTime * pidConfig()->pid_process_denom;
if (motorConfig()->dev.useDshotTelemetry) {
motorUpdateRestriction *= 2;
}
if (pidLooptime < motorUpdateRestriction) {
uint8_t minPidProcessDenom = motorUpdateRestriction / samplingTime;
if (motorUpdateRestriction / samplingTime > minPidProcessDenom) {
// if any fractional part then round up
minPidProcessDenom++;
}
minPidProcessDenom = constrain(minPidProcessDenom, 1, MAX_PID_PROCESS_DENOM);
pidConfigMutable()->pid_process_denom = MAX(pidConfigMutable()->pid_process_denom, minPidProcessDenom);
}

@sanderpuh
Copy link

sanderpuh commented Apr 16, 2024

I've tested it by adding #13535 as commit on flashing Betaflight 4.5 dev and for me it works!

After enabling bidirectional DShot in the configurator, I am now still able to change the esc/motor protocol and I can lower the PID Loop frequency. Changes are persistent after pressing save, thank you! Tested it on 2 builds, both had the same issue and both are now fixed.

@ledvinap
Copy link
Contributor

@haslinghuis : the test for 4k loop is not necessary?

@haslinghuis
Copy link
Member Author

On the test target pid_denom is set to 2 after enabling dshot telemetry.

@ledvinap
Copy link
Contributor

if (motorUpdateRestriction / samplingTime > minPidProcessDenom) {
// if any fractional part then round up
minPidProcessDenom++;
}

strange way to write 'ceil()'

@ledvinap
Copy link
Contributor

On the test target pid_denom is set to 2 after enabling dshot telemetry.

yes, that is a bug. But it will also remove test to prevent denom==1.

MIN(2, pid_denom) will fix the obvious bug. The 4k test may be from old CPUs and not necessary

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.

fixes the bug

@ledvinap
Copy link
Contributor

@haslinghuis : 4000/3600 is pretty arbitrary, maybe you can keep 4000. I mixed it up with 4k PID loop time ..
(#12509)

@haslinghuis haslinghuis merged commit 30415a3 into betaflight:master Apr 21, 2024
46 checks passed
@haslinghuis haslinghuis deleted the fix-pid-denom branch April 21, 2024 09:58
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.

CRAZYBEEF4SX1280 fixed DSHOT300 and 4kHz PID Loop when Bidirectional DShot is enabled on Betaflight 4.5
5 participants