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

Motor Output Limit and Profile Cell Count #1901

Merged
merged 1 commit into from
Mar 2, 2020

Conversation

asizon
Copy link
Member

@asizon asizon commented Feb 28, 2020

Added motor_output_limit and auto_profile_cell_count configurable parameters. I know there is no place at all appropriate to add it, happy to hear opinions and discuss this :)

motoroutputlimitfinal

@dridsfpv
Copy link

LIMIT: limits the maximum output signal for the motors. In order to use batteries with more cells than those the motor theoretically supports (make sure the electronics supports the voltage). Less than 100% limit causes a linear reduction in the motor drive signal.
LIMIT =desired KV / real KV * 100
Example: LIMIT=1800/2700*100=67

CELL COUNT: it allows to assign profiles for different type batteries (with different number of cells). Betaflight automatically detects de voltage of the battery when plugged in the quad and it will select the according profile.

@mikeller
Copy link
Member

Just a cosmetic remark: Wouldn't it be better to add the tooltips to the fields themselves:

image

This will also make it easier to maintain when fields are moved around.

@mikeller mikeller added this to the 10.7.0 milestone Feb 28, 2020
@asizon
Copy link
Member Author

asizon commented Feb 29, 2020

Yes @mikeller ,much better like that, thanks!
tooltipmotoroutput

mikeller
mikeller previously approved these changes Feb 29, 2020
@mikeller
Copy link
Member

@McGiverGim: What do you think about the Sonar 'bugs' reported on this pull request?

@McGiverGim
Copy link
Member

Until now we have ignored this HTML bugs but we can try to fix them. @asizon can you look at the Sonar link and try to fix them? If you click on the issue there is an explanation about the problem and how to fix it.

@asizon
Copy link
Member Author

asizon commented Mar 1, 2020

I will try it now! And need to revise the tootltip, what do you think about the above @dridsfpv proposal?

@mikeller
Copy link
Member

mikeller commented Mar 1, 2020

@McGiverGim: Not 100% sure this is the right approach. Pretty much all of these 'bugs' seem to be aiming at making content and applications available to vision impaired users through screen readers. While this is a good and important thing for general purpose users, I am reasonably sure that there is only very little overlap between users who can not read text on a screen and users who fly racing drones, therefore I am not sure it is good use of time to fix these 'bugs' - maybe we should just ignore them in the sonar config?

@McGiverGim
Copy link
Member

True. We can disable them in Sonar to limit this kind of errors.

@asizon
Copy link
Member Author

asizon commented Mar 1, 2020

@mikeller what do you think about these ctzsnooze suggested names??

suggestedmotoroutput

@mikeller
Copy link
Member

mikeller commented Mar 1, 2020

@asizon: Looks good to me - I think for these settings it is important that the help texts are good, as the things that are controlled by them are relatively complex.

@ctzsnooze
Copy link
Member

This is really great!
Although it's called motor_output_limit in the CLI, it's a linear attenuation of motor outputs, and at max there is a limit. But it's important to know that it attenuates below the limit. Hence suggesting that we write 'Attenuation %' to indicate that it isn't a simple 'hard cutoff' limit. Maybe that will convey a bit better what it actually achieves.

@mikeller
Copy link
Member

mikeller commented Mar 2, 2020

@ctzsnooze: The CLI name is not very good in reality - we should flog they guy who put it in in the first place in betaflight/betaflight#7482 - oh wait. 🤔
'Attenuation' is much better indeed. In CLI I'd have concerns about this being a term that isn't necessarily familiar with non-English speakers, whereas in the UI we can address this with translations.

@ctzsnooze
Copy link
Member

ctzsnooze commented Mar 2, 2020

For the info messages, maybe something like:
LIMIT: Attenuates motor commands by the set percentage. Reduces ESC current and motor heat when using higher cell count batteries, e.g. for similar performance from a 6S battery on a 4S build, try 66%; for a 4S battery on a 3S build, try 75%.
CELL COUNT: Automatically activates the first profile that has a cell count equal to the connected battery.

@mikeller
Copy link
Member

mikeller commented Mar 2, 2020

@ctzsnooze: Looks good - we probably don't need the 'LIMIT' / 'CELL COUNT' headers in the tooltip - it is clear enough what they belong to.
@asizon: Do you want to change the texts to @ctzsnooze's proposals?

@sonarcloud
Copy link

sonarcloud bot commented Mar 2, 2020

SonarCloud Quality Gate failed.

Bug C 8 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@asizon
Copy link
Member Author

asizon commented Mar 2, 2020

Yes @mikeller i have updated to ctzsnooze suggested texts :) and squashed. Thanks for your text suggestions @ctzsnooze and @dridsfpv

mikeller
mikeller previously approved these changes Mar 2, 2020
Fix cell count min

Move tootltip`icons

Cosmetic changes
@mikeller mikeller merged commit aef32bb into betaflight:master Mar 2, 2020
mathiasvr added a commit to mathiasvr/betaflight-deciphered that referenced this pull request Mar 3, 2020
@asizon asizon mentioned this pull request Mar 3, 2020
2 tasks
@Juzzle1
Copy link
Contributor

Juzzle1 commented May 6, 2020

The new tool tip states:
LIMIT: Attenuates motor commands by the set percentage.

Surely this should read
LIMIT: Attenuates motor commands to the set percentage.

In my mind the current wording implies a value of 100% will attenuate the output by 100% and therefore be zero?

@mikeller
Copy link
Member

mikeller commented May 6, 2020

@Juzzle1: You are correct - calling this 'attenuation' is actually not a good term for it anyway, as this is 'the amount of reduction'.

@Juzzle1
Copy link
Contributor

Juzzle1 commented May 6, 2020

I think the term 'limit' is also very confusing, as this suggests a cap on the peak output power, rather than linear scaling.

I'd suggest:

Frame title:
Motor Output Scaling

Setting label:
Scale Factor (%)

Tip text:
Motor output linear scale factor (as percentage). Reduces ESC current and motor heat when using higher cell count batteries, e.g. When using a 6S battery on a craft that has motors, props and tuning designed for 4S, try setting the value at 66%; when using a 4S battery on a craft intended for 3S, try 75%.
Always make sure that all of your components can support the voltage of the battery you are using.

@mikeller
Copy link
Member

mikeller commented May 7, 2020

@Juzzle1: Yes, that sounds better - can I get you to open a pull request for these changes?

@asizon
Copy link
Member Author

asizon commented May 7, 2020

Sure that we want to change now all feature name?maybe we should keep the feature name and change only the setting name. Also @Juzzle1 take acount that the current tooltip is on #1977

@mikeller
Copy link
Member

mikeller commented May 7, 2020

For what I can see this is only used in 2 places, pidTuningMotorLimit and pidTuningMotorLimitHelp, so the fallout should be benign. And @Juzzle1 is right, 'attenuation' is the wrong term for this - e.g. if you attenuate the output by 20% you are left with 80% of the output.

@asizon
Copy link
Member Author

asizon commented May 7, 2020

Yes, i agree that attenuation is wrong and we must change it. But I see that @Juzzle1 also want to change Motor Output Limit to Motor Output Scaling? .

@Juzzle1
Copy link
Contributor

Juzzle1 commented May 7, 2020

'Limit' is also used in the CLI (motor_output_limit) , so don't want to make things more confusing, however I don't think this is the correct terminology for what the feature actually does.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants