Skip to content

Conversation

@asizon
Copy link
Member

@asizon asizon commented Oct 2, 2020

To go with betaflight/betaflight#10164
It updates ff_interpolate_sp default value from average 2 to No Average.

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Only one question...

McGiverGim
McGiverGim previously approved these changes Oct 2, 2020
Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

I suppose you want to say change from Average 2 to NO average (and not ON). It's ok to me.

                                        <select id="ffInterpolate">
                                            <option i18n="pidTuningFfInterpolateSpOptionNoAverage"     value="1">
                                            <option i18n="pidTuningFfInterpolateSpOptionAverage2"      value="2"/>
                                            <option i18n="pidTuningFfInterpolateSpOptionAverage3"      value="3"/>
                                            <option i18n="pidTuningFfInterpolateSpOptionAverage4"      value="4"/>
                                        </select>

One suggestion (not mandatory) to make it easier to read:

  const NO_AVERAGE = 1;
  $('select[id="ffInterpolate"]').val(FC.ADVANCED_TUNING.ff_interpolate_sp > 0 ? FC.ADVANCED_TUNING.ff_interpolate_sp : NO_AVERAGE);

@asizon
Copy link
Member Author

asizon commented Oct 2, 2020

@McGiverGim yes sorry, ON is the value in cli for No Average. Make a constant for the default and better reads sound good.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 3, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell E 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Member

@McGiverGim McGiverGim left a comment

Choose a reason for hiding this comment

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

Approved. When the "constant" is only used at one place, usually is good to place it just the line before the use.

@mikeller mikeller merged commit 2f37546 into betaflight:master Oct 6, 2020
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.

3 participants