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

Show idleMinRpm on motor tab (active profile) #3625

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Nov 3, 2023

This PR adds the ability to configure either idleMinRpm (enabled and for active PID profile only) or digitalIdlePercent (disabled) depending on Bidirectional Dshot setting. This way it makes more sense to users which setting is used.

image

image

When idleMinRpm is disabled and Bidirectional Dshot is enabled we get:

image

@haslinghuis haslinghuis added this to the 10.10.0 milestone Nov 3, 2023
@haslinghuis haslinghuis self-assigned this Nov 3, 2023

This comment has been minimized.

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented Nov 3, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> FAIL
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

Copy link
Member

@asizon asizon left a comment

Choose a reason for hiding this comment

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

Aproved, but im not sure if this is a good approach using dynamic idle repeated in 2 diferent tabs.

@nerdCopter
Copy link
Member

sort of makes sense to have it in motors-tab, but may lead to confusion since it is a pid-profile item. 🤷‍♂️

@McGiverGim
Copy link
Member

The idea is great, but if it's modifiable by PID profile then it has not sense to have it here.

The only solution can be to have here a "general" value, that can be overwrited for the PID profile one. If the value of the PID profile is zero, then use the general. But this needs modification in the firmware part.

If not, the best solution is to have here a "simple" text message pointing the user where he can find the idle value (in the PID profile TAB).

@nerdCopter
Copy link
Member

nerdCopter commented Nov 8, 2023

adding to @McGiverGim's comment about "simple" text message, potentially could also make that text show the current profile value, but not be modifiable. i.e turn the PR's implementation to a label+explanation, not a field.

@haslinghuis haslinghuis changed the title Make idleMinRpm configurable from motor tab (active profile) Show idleMinRpm on motor tab (active profile) Nov 9, 2023

This comment has been minimized.

@nerdCopter
Copy link
Member

nerdCopter commented Nov 10, 2023

@haslinghuis , i tried to PR to your repo/branch, but PR's are disabled.
please consider potential changes:
image

diff --git a/locales/en/messages.json b/locales/en/messages.json
index b86741b9..ab1748a8 100755
--- a/locales/en/messages.json
+++ b/locales/en/messages.json
@@ -1356,7 +1356,7 @@
         "description": "Help text for the Motor poles field of the ESC/Motor configuration"
     },
     "configurationMotorIdleRpmHelp" : {
-        "message": "You will be able to edit Dynamic Idle setting on the PID tuning tab as this setting is profile dependant. When Dynamic Idle is set to zero static motor idle will apply",
+        "message": "This is the dynamic idle value from the active PID profile. When Dynamic Idle is set to zero, only static motor idle will apply. Change the Dynamic Idle settings on the PID tuning tab.",
         "description": "Help text for dynamic idle setting" 
     },
     "configurationThrottleMinimum": {
diff --git a/src/css/main.less b/src/css/main.less
index f0e1f4b0..aff27d41 100644
--- a/src/css/main.less
+++ b/src/css/main.less
@@ -103,6 +103,13 @@ input[type="number"] {
         margin-left: 5px;
     }
 }
+.noarrows {
+    input::-webkit-outer-spin-button,
+    input::-webkit-inner-spin-button {
+      -webkit-appearance: none;
+/*      margin: 0; */
+    }
+}
 .clear-both {
     clear: both;
 }
diff --git a/src/tabs/motors.html b/src/tabs/motors.html
index 8f009171..5fcf6812 100644
--- a/src/tabs/motors.html
+++ b/src/tabs/motors.html
@@ -104,7 +104,7 @@
                                 </div>
                                 <div class="number idleMinRpm">
                                     <label>
-                                        <div class="numberspacer">
+                                        <div class="numberspacer noarrows">
                                             <input type="number" name="idleMinRpm" min="0" max="100" step="1" readonly/>
                                         </div>
                                         <span i18n="pidTuningIdleMinRpm"></span>

edit: the /* margin: 0; */ line can be removed, i forgot to do so before the diff

Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Contributor

Do you want to test this code? Here you have an automated build:
Betaflight-Configurator-Android
Betaflight-Configurator-Linux
Betaflight-Configurator-macOS
Betaflight-Configurator-Windows
WARNING: It may be unstable and result in corrupted configurations or data loss. Use only for testing!

@haslinghuis haslinghuis merged commit cd80be7 into betaflight:master Nov 10, 2023
9 checks passed
@haslinghuis haslinghuis deleted the motor-idleRpm branch November 10, 2023 18:57
chmelevskij pushed a commit to chmelevskij/betaflight-configurator that referenced this pull request Apr 27, 2024
* Make idleMinRpm configurable from motor tab (active profile)

* Reorder idleMinRpm and digitalIdlePercent

* Fixes per review

* Co-authored-by: nerdCopter <56646290+nerdCopter@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

5 participants