Skip to content

New PID sliders and changed defaults #2572

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 30, 2021
Merged

New PID sliders and changed defaults #2572

merged 3 commits into from
Sep 30, 2021

Conversation

@haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Aug 20, 2021 •
edited
Loading

Changes:

🏷️ Depends on: betaflight/betaflight#10919

Screenshot from 2021-09-19 01-13-30

Credits to @spatzengr and @ctzsnooze for providing text for the tooltips and slider labels.
I have worked together with @ctzsnooze at some point.

@haslinghuis haslinghuis added this to the 10.8.0 milestone Aug 20, 2021
@haslinghuis haslinghuis self-assigned this Aug 20, 2021
@haslinghuis haslinghuis force-pushed the change_sliders branch 7 times, most recently from ac61cc5 to 59bc56b Compare August 21, 2021 02:27
@haslinghuis haslinghuis marked this pull request as draft August 21, 2021 03:16
@haslinghuis haslinghuis force-pushed the change_sliders branch 2 times, most recently from 9a11a7d to 94bc133 Compare August 21, 2021 05:39
@asizon
Copy link
Member

asizon commented Aug 21, 2021

Sorry @haslinghuis but we loose all backward compatibility here, we loose one of the sliders for old versions and just stick response slider works.

@haslinghuis haslinghuis force-pushed the change_sliders branch 2 times, most recently from 732d994 to 0a1cefb Compare August 21, 2021 11:37
@haslinghuis haslinghuis marked this pull request as ready for review August 21, 2021 11:43
asizon
asizon requested changes Aug 21, 2021
View reviewed changes
src/js/TuningSliders.js Outdated
this.sliderMasterMultiplier = 1;
this.sliderPDRatio = 1;
this.sliderPDGain = 1;
this.sliderFeedforwardGain = 1;
Copy link
Member

Choose a reason for hiding this comment

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

This parametre can go outside this Api version condition because is aplied to all versions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

src/js/TuningSliders.js
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
$('output[name="sliderDGain-number"]').val(this.sliderDGain);
$('output[name="sliderPIGain-number"]').val(this.sliderPIGain);
$('output[name="sliderFeedforwardGain-number"]').val(this.sliderFeedforwardGain);
Copy link
Member

Choose a reason for hiding this comment

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

Same here, is aplied to all versions and can go outside.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@haslinghuis haslinghuis force-pushed the change_sliders branch 3 times, most recently from 022e109 to eb72725 Compare August 21, 2021 21:22
@haslinghuis haslinghuis force-pushed the change_sliders branch 3 times, most recently from b0890c4 to 61533db Compare August 21, 2021 22:08
@haslinghuis haslinghuis force-pushed the change_sliders branch 2 times, most recently from 1578d8c to a7343fb Compare September 28, 2021 00:33
ctzsnooze
ctzsnooze previously approved these changes Sep 28, 2021
View reviewed changes
mikeller
mikeller reviewed Sep 28, 2021
View reviewed changes
src/js/tabs/pid_tuning.js Outdated

MSP.promise(MSPCodes.MSP_SET_RESET_CURR_PID).then(() => {
if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
MSP.promise(MSPCodes.MSP_EEPROM_WRITE).then(() => {
Copy link
Member

Choose a reason for hiding this comment

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

What is the reason that we are executing an immediate save operation after this?

Copy link
Member Author

@haslinghuis haslinghuis Sep 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Now changing a slider go to another tab and come back to pidtuning it's as from the start (unsaved changes are lost).

Saving values here because as long settings are not saved configurator thinks (see next paragraph) they are changed - until the FC reboots (unsaved changes are retained until reboot and then lost - which could lead to surprises in the field).

Doing the refresh resets variables holding user changes to reset the state without saving. So perhaps I could store and retrieve those variables instead but I need to know if the board has the same unique identifier as the user could switch board instead.

With firmware sliders we are writing new sliders values to firmware and retrieve the calculated pids. Also have thought about having a temp profile in firmware instead which will be copied to the active profile at the moment user updates (save) configuration.

Observation: MSP_SET_RESET_CURR_PID is not resetting the Gyro Filter.

Copy link
Member

Choose a reason for hiding this comment

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

The problems that I can see with this are:

  • it is unexpected to the user that we save settings outside of clicking 'Save';
  • the proposed behaviour is inconsistent for different firmware versions;
  • this is even more confusing if a TX is connected at the same time and the user is switching profiles through the TX, triggering saves in configurator.

So I think a better way would be to revert to the old behaviour of just discarding the changes (now probably requiring a restoreInitialSettings` on PID profile changes).

Also, the gyro filter is not part of the PID profile, so there is no reason it should be linked in any way to the PID profile functionality.

Copy link
Member Author

@haslinghuis haslinghuis Sep 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Found a way ;-)
Have removed the tooltip for the button as it's no longer needed. Now it behaves as before.
initialSettings were overwritten on tab reload, but now have added another flag to workaround.
Thanks for letting me solve this puzzle.

@haslinghuis haslinghuis force-pushed the change_sliders branch 9 times, most recently from 3767fc8 to 8e3cfd7 Compare September 29, 2021 12:22
ctzsnooze
ctzsnooze previously approved these changes Sep 29, 2021
View reviewed changes
@haslinghuis haslinghuis force-pushed the change_sliders branch 7 times, most recently from 439dfa3 to f3a1c51 Compare September 29, 2021 22:46
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 9 Code Smells

No Coverage information No Coverage information
1.5% 1.5% Duplication

@haslinghuis
Copy link
Member Author

haslinghuis commented Sep 29, 2021 •
edited
Loading

Finished debugging

mikeller
mikeller approved these changes Sep 30, 2021
View reviewed changes
Copy link
Member

@mikeller mikeller left a comment

Choose a reason for hiding this comment

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

MSP_SET_RESET_CURR_PID looks much better now - thanks.

I think the direct access to document. should be replaced with JQuery to retain consistency - no reason to mix approaches here. And condition should be getting a better name. But these can be done after mergeing, so that we can get wider testing of this.

src/js/TuningSliders.js
$('.tab-pid_tuning .MasterSlider').toggle(this.expertMode);
$('.tab-pid_tuning .DMinRatioSlider').toggle(this.expertMode && dMinShow);
$('.tab-pid_tuning .advancedSlider').toggle(this.expertMode);
document.getElementById('sliderDMaxGain').disabled = !this.expertMode;
Copy link
Member

Choose a reason for hiding this comment

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

Why are we doing direct DOM manipulation here? We are using JQuery everywhere else in the legacy code, so we should be doing this here as well.

src/js/TuningSliders.js
const WARNING_DMAX_GAIN = 60;
const WARNING_DMIN_GAIN = 40;
let WARNING_DMIN_GAIN = 40;
let condition;
Copy link
Member

Choose a reason for hiding this comment

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

A more descriptive name of what the condition means will make this easier to understand.

@mikeller mikeller merged commit 27ad23c into betaflight:master Sep 30, 2021
@haslinghuis haslinghuis deleted the change_sliders branch September 30, 2021 11:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mikeller mikeller mikeller approved these changes

@ctzsnooze ctzsnooze ctzsnooze left review comments

@asizon asizon asizon left review comments

Assignees

@haslinghuis haslinghuis

Labels

Needs coordination with firmware RN: IMPROVEMENT Tested

Projects

None yet

Milestone

10.8.0

Development

Successfully merging this pull request may close these issues.

Implement P and D sliders to match 10919 Slider changes are saved without "save" button can't manually change PIDFs

6 participants

@haslinghuis @asizon @spatzengr @blckmn @ctzsnooze @mikeller