-
-
Notifications
You must be signed in to change notification settings - Fork 898
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
Add rates type selection #1909
Add rates type selection #1909
Conversation
Looks like it's still not working. Working on it. |
Ok, now the Sonar checks are working. Sonar got confused by the branch the pull request is on being identical to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-added my comments from #1906.
} | ||
} | ||
|
||
TABS.pid_tuning.changeRatesTypeLogo = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this looks great, I am not sure how much value it is, especially for users with small screens. So we might drop the rates logos at some point in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe they are too big. I like the logos but maybe a little smaller will fit in big and small screens.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logos occupy an unused space in the rates type selection tab, and scale accordingly, so no space wasted. They are a late addition to the code. If you see the screenshots you will clearly see that they are in empty and wasted space
If then there are problems they can be removed, but i think it's better this way
(And i personally tested the app all the time in a 1366x768 screen with the smallest possible windows size without problems)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I say is that it does not need to occupy all the space. Maybe smaller looks cleaner. But it always depends on the taste of the developer 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do a little smaller then
src/js/tabs/pid_tuning.js
Outdated
@@ -779,6 +841,7 @@ TABS.pid_tuning.initialize = function (callback) { | |||
ADVANCED_TUNING.motorOutputLimit = parseInt($('.pid_tuning input[name="motorLimit"]').val()); | |||
ADVANCED_TUNING.autoProfileCellCount = parseInt($('.pid_tuning input[name="cellCount"]').val()); | |||
ADVANCED_TUNING.idleMinRpm = parseInt($('input[name="idleMinRpm-number"]').val()); | |||
RC_tuning.rates_type = $('select[id="ratesType"]').val(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One important aspect of making this configurable in the UI is that this allows us to track how many users select each rates type by adding analytics tracking for this.
Can you please add something like this:
let value = parseInt($('select[id="ratesType"]').val());
let newValue = undefined;
if (value !== RC_tuning.rates_type) {
newValue = $('select[id="ratesType"]').find('option:selected').text();
}
self.analyticsChanges['RatesType'] = newValue;
RC_tuning.rates_type = value;
This will cause the selected rates type to be sent as analytics data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to add anything beside this?
Thanks @mikeller. Will fix all asap |
@fgiudice98: I think the 'Actual Rates' type needs a a conversion of the values entered in the UI as well - please make sure you get feedback from @ctzsnooze on this. |
} | ||
} | ||
|
||
TABS.pid_tuning.changeRatesTypeLogo = function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think maybe they are too big. I like the logos but maybe a little smaller will fit in big and small screens.
Hi @fgiudice98 this looks great. I have built it for mac and will check later when I am home. Asizon has shown me screen dumps that look correct for Actual mode. |
For Quickrates this is done into the betaflight code, and i think it's better this way just to prevent errors when setting up from cli
Yeah, just tell me what scale and min, max, step values i need to put in the code
If you have the rate in deg/s the step is always 10deg/s, even in quickrates |
There are other places where the scale of parameters set in CLI is different from what is displayed in the UI - for example in cases where the actual value has got fractions, which are not supported in CLI. Here the issue is that in the firmware the parameters for all rates types are stored in the same parameters, and CLI does not support scaling of parameters based on other parameter values. In the UI we can do this, and we should. |
Yes, i know, i even do it in my code. But i was talking about the hard limit of the 2000deg/s of the angleRate, i think i missread. |
@mikeller @McGiverGim apart from the const var that i sure need to change. What to do with the other sonar issues? Need to make all the literals in variables? Is it really needed even if they are in completely different functions? By the way, here the latest build for testing: |
Not all. The literals we transform in variables if it is clearer, but sometimes it is worst than the literal form. At least for me I let you decide this part. |
@McGiverGim just what i was thinking about the literals |
Good progress @fgiudice98! Looking at the remaining items in the Sonar report:
|
It looks really good now from the user perspective with current firmware. |
@mikeller thanks for the advice! will fix and test all in a while @ctzsnooze I tested earlier when working on it and all seems good, but as i said in the description other tests are always welcome |
Works fine with previous MSP versions 1.42. Good job! Also smallers logos looks better, warning message cosmetic also now is perfect, looking forward to see it in 4.2 and 10.7.0 |
tested now all the latest fixes. @fgiudice98 can you check this |
@TheIsotopes will be fixed in the next commit, thanks for the report |
yes i can reproduce it, not efect at all when changing type, only changes rates type, no efect in values,graphics,image, all tab |
Don't know if it's a .change problem @fgiudice98 , when you restart configurator, it loads everything perfectly, except the values that were previously saved |
ok tested, only one problem, I cant goes with previoues saved values and rate type, i get a warning message also for change to my previous saved settings |
Guys, me and @asizon were talking about the change rates type warning. For now it shows when changing to a rates type that is not the one saved in the eeprom. This warning has two buttons, one to proceed with the change and one to return to the saved configuration. What do you thing about it? Is better this way to prevent configuration error by the user or it needs some changes? |
@fgiudice98: I just had a look at the warning popup in the latest version, and to me this looks good. |
@mikeller Perfect! Tell me when i need to make the rebase and squash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only two minor "cosmetic" changes...
Good work!!
For now the KISS logo is a placeholder text, i will update it in the future, after i have talked with the guys from KISS Don't know why travis failed building, seems like a travis error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, Travis seems to be failing...
For the rest, the code seems good enough to me!
Add rates type selection with working graphs and max angular speed Fix deadband MSP changes and fix actual rates limit Small style changes and fixes Fixes Fixed the code pointed out in the review Various sonar fixes Updated the values of actual rates setting Reduced logo size Added analytics to rates type Minor fix + rounding by step value Now the calculation of the angle rate takes into account the values rounded by the step value (especially visible with values that are in deg/s) Various fixes Angle rate calculation in different functions Fix for value step validation Sonar fixes (mostly variables) Force refresh to prevent errors ... ... when changing rates type after saving to eeprom Logos refinement + minor changes Touch to trigger travis Fix Raceflight rate
Looks mostly good now - there are still a lot of constant expressions left that are defined with |
@mikeller the |
@fgiudice98: Building code 'for future use' is a fallacy, as it implies that we are any good at predicting the future - unfortunately, the reality in software engineering is that we are not. |
Kudos, SonarCloud Quality Gate passed! 3 Bugs No Coverage information |
This is still missing the analytics, but I can add these in a second pull request. |
Discussion / screen shots in: #1906