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 integrated yaw usage #2651

Merged
merged 2 commits into from Dec 10, 2021
Merged

Conversation

asizon
Copy link
Member

@asizon asizon commented Nov 7, 2021

Allow manual pid change when integrated yaw is enabled.

@asizon asizon added this to the 10.8.0 milestone Nov 7, 2021
@asizon asizon self-assigned this Nov 7, 2021
haslinghuis
haslinghuis previously approved these changes Nov 8, 2021
@blckmn
Copy link
Member

blckmn commented Nov 8, 2021

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • 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 -> FAIL

@haslinghuis
Copy link
Member

haslinghuis commented Nov 8, 2021

After testing I found we have to press the Enable button twice.

Moving the code to pid_tuning.js solves it:

        $('input[id="useIntegratedYaw"]').change(function() {
            const checked = $(this).is(':checked');
            if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
                this.sliderPidsMode = 0;
            }
            $('#pid_main .pid_data input').prop('disabled', !checked);
            $('#pidTuningIntegratedYawCaution').toggle(checked);
        }).change();

limonspb
limonspb previously approved these changes Nov 25, 2021
@asizon asizon dismissed stale reviews from limonspb and haslinghuis via e734c11 November 25, 2021 06:36
Finalizing Firmware 4.3 Release automation moved this from To do to Review in progress Nov 25, 2021
move code to pidtuning.js

fix tuningsliders
limonspb
limonspb previously approved these changes Nov 25, 2021
@haslinghuis haslinghuis moved this from Review in progress to Reviewer approved in Finalizing Firmware 4.3 Release Nov 25, 2021
@haslinghuis haslinghuis moved this from Reviewer approved to In progress in Finalizing Firmware 4.3 Release Nov 28, 2021
@ctzsnooze
Copy link
Member

In my testing, the PR allows me to edit manually edit PID values, when integrated yaw is enabled, but if I save the Profile, then my changes are lost and I cannot edit the values anymore.

@ctzsnooze ctzsnooze moved this from Approval Needed to Review Needed in Finalizing Firmware 4.3 Release Nov 30, 2021
@haslinghuis
Copy link
Member

@ctzsnooze Can't reproduce as I've tested

@haslinghuis haslinghuis moved this from Review Needed to Approval Needed in Finalizing Firmware 4.3 Release Nov 30, 2021
@asizon
Copy link
Member Author

asizon commented Nov 30, 2021

Yes, we need to take a look into this issue. Ithink sliders are not changing correctly from RPY to off.

src/js/tabs/pid_tuning.js Outdated Show resolved Hide resolved
@haslinghuis
Copy link
Member

Sorry @asizon I made a typo in my first suggestion. Now I have more time I had a fresh look and found you should move the fix to around line 1936 as this only applies to sliders. We can leave the code around 550 untouched:

$('input[id="useIntegratedYaw"]').change(() => {
    if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
        TuningSliders.sliderPidsMode = 0;
    }
    TuningSliders.updatePidSlidersDisplay();
}

The typo was this.sliderPidsMode instead of TuningSliders.sliderPidsMode.

@haslinghuis haslinghuis moved this from Approval Needed to Review Needed in Finalizing Firmware 4.3 Release Nov 30, 2021
@klutvott123
Copy link
Member

So enabling integrated yaw should disable sliders for RPY, not just Y?

@haslinghuis
Copy link
Member

Hmm you suggest setting slider to RP mode instead to OFF? Makes sense to me. Testing now.

@klutvott123
Copy link
Member

I suggest setting it to RP IF it's currently set to RPY

@haslinghuis
Copy link
Member

@asizon

Worked out the changes needed @klutvott123 suggested:

diff2.txt

@ctzsnooze
Copy link
Member

@asizon - do you have time to update this PR and test it? We need to resolve this for 10.8 RC1 as soon as possible.

@sonarcloud
Copy link

sonarcloud bot commented Dec 8, 2021

SonarCloud Quality Gate failed.    Quality Gate failed

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@ctzsnooze ctzsnooze self-requested a review December 9, 2021 23:31
@ctzsnooze
Copy link
Member

ctzsnooze commented Dec 9, 2021

Tested, works by pushing slider mode to RP mode.

@ctzsnooze ctzsnooze requested review from mikeller, blckmn and McGiverGim and removed request for mikeller December 9, 2021 23:32
@haslinghuis haslinghuis moved this from Review Needed to Configurator in Finalizing Firmware 4.3 Release Dec 10, 2021
@limonspb limonspb merged commit 79e081f into betaflight:master Dec 10, 2021
Finalizing Firmware 4.3 Release automation moved this from Configurator to Finished (Merged) Dec 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants