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

Dynamic Idle configurable #1905

Merged
merged 3 commits into from
Mar 5, 2020
Merged

Dynamic Idle configurable #1905

merged 3 commits into from
Mar 5, 2020

Conversation

asizon
Copy link
Member

@asizon asizon commented Mar 4, 2020

I am seeing a great effort in joelucid to improve the explanation in the wiki and social networks of how this feature works and I think it now looks better. Adding Dynamic Idle feature configurable parametre. It is a feature that depends on the pidprofile so I have linked this tab with MSP_PID_ADVANCED. My first idea is to show in some way that this feature belongs to the pid profile, happy to hear ideas. Also modify the function of hiding values that depend on rpm and unify it. The tooltip needs updating by @joelucid suggestions.
DynamicIdle

@mikeller
Copy link
Member

mikeller commented Mar 4, 2020

@asizon: Ignore the Sonar complaints - they are caused by me accidentally disabling, and then re-enabling some detection rules. 😖

@mikeller
Copy link
Member

mikeller commented Mar 4, 2020

I think this is on the wrong tab - since it is dependent on the currently selected PID profile it should go onto the PID Tuning tab, otherwise it will be confusing and hard for users to determine which PID profile they are changing this value for, as the PID profile indicator is not shown:

image

@mikeller mikeller added this to the 10.7.0 milestone Mar 4, 2020
@joelucid
Copy link

joelucid commented Mar 4, 2020

I think this is on the wrong tab - since it is dependent on the currently selected PID profile it should go onto the PID Tuning tab, otherwise it will be confusing and hard for users to determine which PID profile they are changing this value for, as the PID profile indicator is not shown

Yeah this is a bit of a problem: logically it belongs next to dshot_idle_value, but that is on a non profile page. On the other hand dshot_idle_value would really be better suited in a profile so it can be changed according to cell count.

Maybe dshot_idle_value should become a profile setting and both be moved to the pid page?

@mikeller
Copy link
Member

mikeller commented Mar 5, 2020

@joelucid: Agreed, moving dshot_idle_value into the 'PID" profile might be useful for some use cases. The problem is that the whole profile business is a tangle, and should be re-worked to maximise usefulness. Moving individual parameters around in a piecemeal fashion will not fix this, and will make it very hard to support the different versions in configurator.

Hence I think moving the dynamic idle value into the PID tuning tab is what can be done to address this at the moment.

@asizon
Copy link
Member Author

asizon commented Mar 5, 2020

ok then I have done it, moved to pid tuning tab :) , this is the result
dynamicidlefinal

@sonarcloud
Copy link

sonarcloud bot commented Mar 5, 2020

Kudos, SonarCloud Quality Gate passed!

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

No Coverage information No Coverage information
0.0% 0.0% Duplication

@asizon
Copy link
Member Author

asizon commented Mar 5, 2020

I also changed the name of the function of the tab configuration to make it more generic, it didn't occur to me when I did it a while ago. Maybe we should listen to Sonarcloud error

@mikeller mikeller merged commit 94bd817 into betaflight:master Mar 5, 2020
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.

3 participants