Fix auto_profile_cell_count range #2993
Conversation
This comment has been minimized.
This comment has been minimized.
|
AUTOMERGE: (FAIL)
|
There was a problem hiding this comment.
To me is complicated to know what is 0 and 1. Maybe change this to a select? With -1 and 0 with some text like "Switch" and "Stay" and the 1 to 8 elements with something like 1S to 8S? With this and some tooltip describing the values will be great: betaflight/betaflight#7516
haslinghuis
force-pushed
the
fix-auto-profile-cell-count-range
branch
3 times, most recently
from
2eccf71
to
b821527
Compare
This comment has been minimized.
This comment has been minimized.
|
Updated with suggestion from @McGiverGim |
locales/en/messages.json
Outdated
| }, | ||
| "pidTuningCellCountStay": { | ||
| "message": "Disable", | ||
| "desciption": "Disable cell count for this profile" |
There was a problem hiding this comment.
| "desciption": "Disable cell count for this profile" | |
| "description": "Disable cell count for this profile" |
There was a problem hiding this comment.
Copy pasta error. Fixed.
locales/en/messages.json
Outdated
| }, | ||
| "pidTuningCellCountChange": { | ||
| "message": "Switch", | ||
| "desciption": "Use this profile if there no profiles matching cell count" |
There was a problem hiding this comment.
| "desciption": "Use this profile if there no profiles matching cell count" | |
| "description": "Use this profile if there no profiles matching cell count" |
There was a problem hiding this comment.
The description is used to give context to translators. I'm not too sure if this description can help. Is more a good tooltip than a context for the word.
haslinghuis
force-pushed
the
fix-auto-profile-cell-count-range
branch
from
b821527
to
bea9d16
Compare
|
Kudos, SonarCloud Quality Gate passed!
|
|
Do you want to test this code? Here you have an automated build: |
haslinghuis
moved this from Configurator
to Ready to merge
in Finalizing Firmware 4.4 Release
|
@McGiverGim Great addition, didn't even think so far. Perfect clarity now 👍❤️. Is it backwards compatible also, e.g. when users had set the switch to - 1 for 'default for all other cases where battery S is not covered by a distinct profile'? This would be of great value... |
|
@RipperDrone in GUI |
|
@haslinghuis Maybe - I even remember I DID set it up in BFC (though I might be wrong). Anyway, it was a well documented CLI setting which then DID show up in the GUI - and which I have now preset on my 50+ quads... PITA to change 'em all :-(. Why not just keep the numbering scheme of designators whilest displaying a more descriptive text in BFC which definitely is a good thing to do? |
|
You don't understand how this works. The description has nothing to do with the internal value. Check nightly and you will see |
sry, I'm not an experienced coder to be fully able to interpret the code. However, looking at your assignments in the code snippet below, I'm assuming there is a NEW meaning of -1 as a value now (?), therefore my interpretation was that the code now is NOT backwards compatible anymore to quads that have had a '-1' setting stored in their diffs (= fallback profile to be used in case there was no distinct matching cell count found in any other profile). Can you pls confirm whether backwards compatibility to previous CLI values/enum is preserved or broken?
|
|
This PR only fixes the bug that -1 value in UI was not selectable and added a switch and meaningful labels for the available range. Cli is using it's own interface to communicate with firmware and has not been changed. |
|
Thank you for explaining 👍😍. I must have been misreading the code assuming the -1 setting got a new meaning... |
Fixes: #2992