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

Improve pidTuningIdleMinRpmHelp Tooltip text #1976

Merged
merged 1 commit into from
Apr 25, 2020

Conversation

leocb
Copy link
Contributor

@leocb leocb commented Apr 21, 2020

This phrase is more concise, direct to the point and uses less technical jargon that's better for the end-user that does not care to what happens under the hood. Please correct me if I defined something wrong.

@@ -3663,7 +3663,7 @@
"message": "Dynamic Idle Value [rpm]"
},
"pidTuningIdleMinRpmHelp": {
"message": "Set this parameter to provide an rpm based floor-value as a safety net against motor desync.<br>A racer might prefer very good response and low rates - therefore a low Dynamic Idle[rpm] and a high Dshot Idle value may be useful. Freestylers and LOS pilots will appreciate very low thrust at idle and might want to use less Dshot Idle value while using a bit more Dynamic Idle[rpm] to avoid desyncs at high rates. Note that the Dynamic Idle[rpm] value always must be less than Dshot Idle value.<br><br>Visit this <a href=\"https://github.com/betaflight/betaflight/wiki/Tuning-Dynamic-Idle\"target=\"_blank\" rel=\"noopener noreferrer\">wiki</a> entry for more info."
"message": "Set this parameter to dynamically adjust the minimum DShot value based on the rpm and avoid motor desyncs.<br>Lower Dynamic Idle [rpm] with higher DShot Idle provides more response at low throttle, better for racing.<br>Higher Dynamic Idle [rpm] with lower DShot Idle provides less unwanted thrust at low throttle, better for freestyle.<br>Note that the Dynamic Idle [rpm] value must always be less than the DShot Idle value.<br><br>Visit this <a href="https://github.com/betaflight/betaflight/wiki/Tuning-Dynamic-Idle"target="_blank" rel="noopener noreferrer">wiki</a> entry for more info."
Copy link
Member

Choose a reason for hiding this comment

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

The backslashes in the string need to be escaped: \"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. how the hell did the Travis CI build succeeded with this major flaw?! JS sure is forgiving

Copy link
Member

Choose a reason for hiding this comment

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

@leocb: It's not about being 'forgiving', it's about being lazy and only evaluating most things at runtime (and then happily crashing).
But you are making a good point - it might make sense to add a test for this. And I am halfway surprised that SonarCloud does not do a correctness check for JSON.

@mikeller mikeller added this to the 10.7.0 milestone Apr 21, 2020
@ctzsnooze
Copy link
Member

The change may unintentionally cause confusion.

When dynamic idle is active, the DShot value becomes a minimum throttle value. The actual DShot minimum value is zero.

Therefore it is not correct to say, "Set this parameter to dynamically adjust the minimum DShot value based on the rpm..." because it doesn't do that.

What rpm filtering actually does is to permanently set the minimum DShot value to zero, and then dynamically modifies the values sent to each motor, individually, to prevent its rpm from falling below the specified minimum.

I think my original introductory sentence that says, "Set this parameter to provide an rpm based floor-value as a safety net against motor desync." is technically correct and simple to understand.

@asizon
Copy link
Member

asizon commented Apr 21, 2020

The first phrase is wrong to describe Dynamic Idle. Maybe we can use same like this(from new updated Dynamic Idlea entry):

Dynamic Idle improves how Betaflight controls the motors at the low end of the rpm range based on the rpm data to avoid motor desyncs.

@ctzsnooze
Copy link
Member

ctzsnooze commented Apr 21, 2020

There remain some unknowns about dynamic idle for racing. I am certain that this, below, is true, and am comfortable with this text for 4.2.

Dynamic Idle improves how Betaflight controls the motors at the low end of the rpm range. It uses rpm data to provide a safety net against motor desyncs, and also improves stability in drops and motor braking under positive airflow conditions. For longer inverted hang time, reduce DShot idle value and minimum rpm together, but not so low as to cause desyncs. The Dynamic Idle rpm value should be set to about 20% below the rpm of the currently configured Dshot idle value, as described in the wiki.

@leocb
Copy link
Contributor Author

leocb commented Apr 21, 2020

That's a very good suggestion, may I try to improve it a bit more?
I think it's valid to keep in mind that people that care about performance will read the wiki, so we should keep the tooltip easy for non technical people that want to get the most of their craft without diving too deep into the specifics.

Dynamic Idle improves how Betaflight controls the motors at low rpm. It uses the rpm data to reduce(eliminate?) motor desyncs, improves motor braking on straight lines, stability during drops and the PID performance at zero throttle. For a longer inverted hang time, reduce DShot idle value and minimum rpm together, but not so low as to cause desyncs. The Dynamic Idle rpm value should be set to about 20% below the rpm of your Dshot idle value, for more info, see <this wiki page>(link)

"safety net" may be hard for translators, so it's better to use similar, but different phrasing, just wondering if I should use reduce or eliminate. I believe reduce is more realistic?

@McGiverGim
Copy link
Member

I don't think safety net can be a problem for translations. Some languages will translate it without problems, and others maybe need to "change" a little the phrase, but nothing that our advanced translator can't do :P
But if we find a better word/phrase, perfect.

@ctzsnooze
Copy link
Member

Maybe as a compromise,
Dynamic Idle improves how Betaflight controls the motors at low rpm. It uses the rpm data to reduce the chance of motor desyncs, improves motor braking when airflow into the props is positive, and improves low throttle stability and responsiveness. For longer inverted hang times, the DShot idle and minimum rpm values should be reduced together, but not so low as to cause desyncs. The Dynamic Idle rpm value should be set to about 20% below the rpm of your Dshot idle value, for more info, see <this wiki page>(link)

Rationale:

  • depending on configuration, dynamic idle cannot absolutely eliminate desyncs. If the user tunes it with very low values to optimised inverted hang time, they will run a real risk of desyncs.
  • braking is improved in all kinds of manoeuvres, not just straight, but curves and inverted drops and turns; but ONLY when there is strongly positive airflow into the props, strong enough to keep the rpm above the minimum.

mikeller
mikeller previously approved these changes Apr 21, 2020
@leocb
Copy link
Contributor Author

leocb commented Apr 22, 2020

Here's my suggestion based on ctzsnooze comments on slack:
suggestion n.1:
Dynamic Idle improves how Betaflight controls the motors at low rpm and reduces the chances of motor desync, to achieve this, it responds to the effect of the wind speeding up or slowing down the props, allowing more authority to the PID; This improves the craft stability, motor braking and responsiveness at zero throttle. For racing, higher values are recommended, for freestyle and high-rate setups lower values are better, but not too low as to cause desyncs. In both cases, it's recommended to set the Dynamic Idle rpm to be about 20% below the rpm of what Dshot Idle achieves (get it from the <motors> tab). For a longer inverted hang time, DShot idle value and Minimum rpm should be lowered together. This setting can be tricky to tune, please read <this Wiki page> to learn more.

But this is too much text to cram into a tool tip, suggestion n.2 is:
Dynamic Idle improves the motors control at low rpm and reduces desync, it responds to the effect of the wind speeding up or slowing down the props, giving more authority to the PID; This improves stability, motor braking and responsiveness. For racing, higher values are recommended, for freestyle and high-rate setups lower values are better, but not too low as to cause desyncs. In both cases, it's recommended to set the Dynamic Idle rpm to be about 20% below the rpm of Dshot Idle (see the <motors> tab). For a longer inverted hang time, DShot idle value and Minimum rpm should be lowered together. Please read <this Wiki page> to learn how to tune this properly

@ctzsnooze
Copy link
Member

ctzsnooze commented Apr 22, 2020

We have no experience about optimal settings for racing.
Recommending low idle values for freestyle will lead to many complaints about pitch wobble.
I will revise the wiki soon, but the notes there about racing and freestyle were inherited from an earlier draft and I don't think they are entirely correct.
This text seems about right to me, I think it's all we should be saying in a tooltip. They should refer to the wiki for more info.
Dynamic Idle improves control at low rpm and reduces risk of desyncs. It corrects problems caused by airflow speeding up or slowing down the props, improving PID authority, stability, motor braking and responsiveness. The Dynamic Idle rpm should be set to be about 20% below the rpm of your Dshot Idle value (see the <motors> tab). Usually there is no need to change your DShot idle value from defaults. For longer inverted hang time, DShot idle value and Minimum rpm should be lowered together.

@leocb
Copy link
Contributor Author

leocb commented Apr 22, 2020

I completely agree with ctzsnooze, and thanks for your time! the text on his last comment is perfect IMO.
really glad we could define a good tooltip for this feature.

The PR has been updated accordingly

@asizon
Copy link
Member

asizon commented Apr 22, 2020

Seems that you forgot wiki entry reference :) is one of the most important part of this tooltip, originaly added due to the difficulty of adjusting and understanding this feature, and to take advantage of our current resources.
<br><br>Visit this <a href=\"https://github.com/betaflight/betaflight/wiki/Tuning-Dynamic-Idle\"target=\"_blank\" rel=\"noopener noreferrer\">wiki</a> entry for more info

This phrase is more concise, direct to the point and uses less technical jargon that's better for the end-user that soes not care to what happens under the hood. Please correct me if I defined something wrong.

escaped double quotes chars

better IdleMinRPM tooltip

Thanks to ctzsnooze

Added wiki entry for Dynamic Idle
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!

Bug A 0 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.7% 0.7% Duplication

@leocb
Copy link
Contributor Author

leocb commented Apr 23, 2020

added wiki entry and rebased

@mikeller mikeller merged commit aa98588 into betaflight:master Apr 25, 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.

5 participants