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

Improved tuning sliders calculation #10509

Merged
merged 1 commit into from Feb 21, 2021

Conversation

IllusionFpv
Copy link
Contributor

Refactored calculateNewPidValues function, now save pids once, need testing

@@ -34,59 +34,25 @@ static void calculateNewPidValues(pidProfile_t *pidProfile)
{
const pidf_t pidDefaults[FLIGHT_DYNAMICS_INDEX_COUNT] = {
[PID_ROLL] = PID_ROLL_DEFAULT,
[PID_PITCH] = PID_ROLL_DEFAULT, // using the same defaults as roll
[PID_PITCH] = PID_PITCH_DEFAULT, // using the same defaults as roll
Copy link
Member

Choose a reason for hiding this comment

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

Switching from Roll to Pitch Default here. How about changing the comment too. For reference and understanding what is the difference in this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ops, forgot to remove the comment, in the old code calculated roll and pitch pids were the same(based on the same roll defaults)

Copy link
Member

Choose a reason for hiding this comment

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

Also this seems to include the commit / changes from 10508 but I guess it's to test this PR until squashed / rebased?

Any idea about the disabled filter sliders (cannot reset / enable) in configurator? (Not aware about ongoing work / discussion - already looked at solving but couldn't find any other than there is a new simple* setting not involved in filtering but pid settings only - have to check yet the dterm / gyro filters)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I've done this upon the other branch

@mikeller mikeller added this to the 4.3 milestone Feb 14, 2021
@mikeller
Copy link
Member

@IllusionFpv: Let me know when this has been tested enough.

@spatzengr
Copy link

Will this solve the issue of the Configurator calced PIDs and the FW calced PIDs not aligning? Seems like the Configurator calced PIDs are a little off and when it saves and refreshed the PID grid from MSP, it updates to be what the FW calced???

z1yQSqCcTa

@IllusionFpv
Copy link
Contributor Author

@spatzengr

Will this solve the issue of the Configurator calced PIDs and the FW calced PIDs not aligning?

Yes, in current code pids misaligned after some slider iterations

@IllusionFpv
Copy link
Contributor Author

@mikeller I fixed applying tuning, for me is good to go now

roll pitch ratio now increase pitch

constrain dmin ratio, disable dmin if multiplier is 200

fix apply simplified tuning
@haslinghuis
Copy link
Member

@IllusionFpv please provide test firmware using make unified_zip for easy testing ;-)

@spatzengr
Copy link

@IllusionFpv , can you set simplified_pids_mode = 2 (RPY) by default in your PR? Then it will align with the Configurator when sliders are enabled.

Testing this build on ["Reorder fw sliders" #2402], it looks like all is in order. The only thing that is off yet is it seems like the PID Grid is always one step behind?

TQYyR23T7e

Also, fast movement doesn't register correctly. Seems like in the configurator, we need to create another call/update (call Refresh) of the PID grid in the firmware so the PID forms isn't always one step behind with fast movement or small tweaks. Let me look at that.
pg5Pg2BPCj

@mikeller
Copy link
Member

@spatzengr:

@IllusionFpv , can you set simplified_pids_mode = 2 (RPY) by default in your PR? Then it will align with the Configurator when sliders are enabled.

Not sure that aligning the simplified settings with the legacy configurator sliders should be the goal here - if the new settings and their defaults are better then we should go for them even if they do not match the legacy configurator sliders.

@mikeller mikeller merged commit 893d981 into betaflight:master Feb 21, 2021
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

5 participants