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.

Sign up for GitHub

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

Jump to bottom

Raise upper limit of dyn_idle_min_rpm #2985

Merged
merged 1 commit into from Sep 18, 2022
Merged

Raise upper limit of dyn_idle_min_rpm #2985

merged 1 commit into from Sep 18, 2022

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Jul 30, 2022 •
edited

Update after betaflight/betaflight/pull/11769

Thanks to @KarateBrot for raising the idea.

@haslinghuis haslinghuis added this to the 10.9.0 milestone Jul 30, 2022
@haslinghuis haslinghuis self-assigned this Jul 30, 2022
@github-actions

This comment has been minimized.

Sign in to view
@KarateBrot
Copy link
Member

The original idea is from @sugaarK, I just made the PR for it.

@blckmn
Copy link
Member

blckmn commented Jul 31, 2022 •
edited

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

@sugaarK
Copy link
Member

sugaarK commented Jul 31, 2022

Works good too!

McGiverGim
McGiverGim requested changes Jul 31, 2022
View reviewed changes
src/tabs/pid_tuning.html Outdated
@@ -762,7 +762,7 @@
</tr>

<tr class="idleMinRpm">
<td><input type="number" name="idleMinRpm-number" step="1" min="0" max="100"/></td>
<td><input type="number" name="idleMinRpm-number" step="1" min="0" max="200"/></td>
Copy link
Member

Choose a reason for hiding this comment

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

This does not depend of the Betaflight version?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Updated. As the PG version in firmware has not been raised we should keep it designate this for 4.4 anyway?

Copy link
Member

Choose a reason for hiding this comment

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

In this case the CLI command dyn_idle_min_rpm is backwards compatible cause we're only extending the range, not the underlying data type. So we could add betaflight/betaflight#11769 to 4.3.2.

The configurator part still needs to go into 4.4 release (I guess? Or do we have sub-versions for BFC?). But for the 4.3.2 firmware you would at least have access to the greater dyn idle range via CLI.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks. Changed to 4.3

@github-actions

This comment has been minimized.

Sign in to view
@sonarcloud
Copy link

sonarcloud bot commented Jul 31, 2022

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

@github-actions

This comment has been minimized.

Sign in to view
asizon
asizon requested changes Aug 30, 2022
View reviewed changes
src/js/tabs/pid_tuning.js Outdated
@@ -492,6 +492,10 @@ pid_tuning.initialize = function (callback) {
$('.rpmFilter').hide();
}

if (semver.gte(FC.CONFIG.apiVersion, API_VERSION_1_44)) {
Copy link
Member

Choose a reason for hiding this comment

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

It should be 1.45??

Copy link
Member Author

Choose a reason for hiding this comment

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

Firmware PR is tagged as 4.3.2 not 4.4

Copy link
Member

Choose a reason for hiding this comment

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

Ok so, we need to fix it first, it is taged after merge and is merged on ly in master

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@sonarcloud
Copy link

sonarcloud bot commented Aug 31, 2022

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

@haslinghuis haslinghuis moved this from Configurator to Ready to merge in Finalizing Firmware 4.4 Release Sep 18, 2022
@blckmn blckmn merged commit 185db5b into betaflight:master Sep 18, 2022
Finalizing Firmware 4.4 Release automation moved this from Ready to merge to Closed Sep 18, 2022
@haslinghuis haslinghuis deleted the dynIdleMin branch September 18, 2022 23:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@KarateBrot KarateBrot KarateBrot left review comments

@asizon asizon asizon requested changes

@McGiverGim McGiverGim McGiverGim requested changes

@blckmn blckmn blckmn approved these changes

Assignees

@haslinghuis haslinghuis

Labels
RN: IMPROVEMENT
Projects
Finalizing Firmware 4.4 Release
  
Done
Milestone
10.9.0
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@haslinghuis @KarateBrot @blckmn @sugaarK @McGiverGim @asizon