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.

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

Fixed Anti Gravity Gain value #2005

Merged
merged 1 commit into from May 6, 2020
Merged

Fixed Anti Gravity Gain value #2005

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions src/js/tabs/pid_tuning.js
View file
Open in desktop
Expand Up @@ -181,9 +181,13 @@ TABS.pid_tuning.initialize = function (callback) {
antiGravitySwitch.change(function() {
var checked = $(this).is(':checked');
if (checked) {
const MAX_ACCELERATOR_GAIN = semver.gte(CONFIG.apiVersion, API_VERSION_1_43) ? 3.5 : 1.1;
const itermAcceleratorGain = Math.max(ADVANCED_TUNING.itermAcceleratorGain / 1000, MAX_ACCELERATOR_GAIN);
$('.antigravity input[name="itermAcceleratorGain"]').val(itermAcceleratorGain);
if (ADVANCED_TUNING.itermAcceleratorGain === 1000) {
Copy link
Member

@McGiverGim McGiverGim May 5, 2020 •
edited

Choose a reason for hiding this comment

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

Sorry to not see this in the first review ;)
After removing the Math.max, the DEFAULT_ACCELERATOR_GAIN is only used inside the if, so better to move inside it:

 if (ADVANCED_TUNING.itermAcceleratorGain === 1000) {
   const DEFAULT_ACCELERATOR_GAIN = semver.gte(CONFIG.apiVersion, API_VERSION_1_43) ? 3.5 : 1.1;
   $('.antigravity input[name="itermAcceleratorGain"]').val(DEFAULT_ACCELERATOR_GAIN);
} else {

Another solution is to change the complete if-else by a:

$('.antigravity input[name="itermAcceleratorGain"]').val(ADVANCED_TUNING.itermAcceleratorGain === 1000 ?  DEFAULT_ACCELERATOR_GAIN : itermAcceleratorGain);

But maybe in this case is clearer with the if-else.

Take the form that you prefer...

Copy link
Member Author

Choose a reason for hiding this comment

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

dont worry,fixed now, i think that I prefer if/else in this case,as you said looks more clearer.

Copy link
Member

@McGiverGim McGiverGim May 5, 2020 •
edited

Choose a reason for hiding this comment

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

It happens the same with the itermAcceleratorGain :( It is used only in the else part...

I think this confinement has made me slower in reactions...

Copy link
Member Author

Choose a reason for hiding this comment

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

I was thinking exactly that for itermAcceleratorGain

Copy link
Member

Choose a reason for hiding this comment

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

Yes, and it is modified too some lines up:

betaflight-configurator/src/js/tabs/pid_tuning.js

Line 172 in 12dfc4f

$('.antigravity input[name="itermAcceleratorGain"]').val(ADVANCED_TUNING.itermAcceleratorGain / 1000);

so we end assigning it twice. I think this line can be removed but is outside the scope of this PR. To me is OK now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@McGiverGim all is done now!

const DEFAULT_ACCELERATOR_GAIN = semver.gte(CONFIG.apiVersion, API_VERSION_1_43) ? 3.5 : 1.1;
$('.antigravity input[name="itermAcceleratorGain"]').val(DEFAULT_ACCELERATOR_GAIN);
} else {
const itermAcceleratorGain = (ADVANCED_TUNING.itermAcceleratorGain / 1000);
$('.antigravity input[name="itermAcceleratorGain"]').val(itermAcceleratorGain);
}
$('.antigravity .suboption').show();
if (ADVANCED_TUNING.antiGravityMode == 0) {
$('.antigravity .antiGravityThres').hide();
Expand Down