Skip to content

Conversation

@KarateBrot
Copy link
Member

@KarateBrot KarateBrot commented Nov 8, 2021

General

This is the counterpart to PR betaflight/betaflight#10673

For BF 4.3 / API 1.44 onwards I got rid of the "DYNAMIC_FILTER" switch in the configuration tab. Instead I introduced an on/off switch in the filters tab, so finally all filters can be enabled and disabled from inside the filters tab.

Thanks to @haslinghuis for giving me code that was nearly ready to use. He basically made half of this PR.

Todo

  • Wait for rfc filter sliders #2638 to get merged
  • Rebase
  • Update dyn notch settings layout according to new rfc slider layout style
  • Bugfixes
  • Mark PR as ready for review

Preview

features

dynNotchSwitch

@haslinghuis
Copy link
Member

@KarateBrot this needs a rebase now 😃

@ctzsnooze
Copy link
Member

ctzsnooze commented Nov 26, 2021

One small comment about the Dynamic Notch switch behaviour.

If Dynamic Notch count is > 0, say '3', and I turn the switch to 'off' (left position), the switch changes colour to grey, but the value for notch count remains unchanged.

Usually 'zero' count = 'off', so I would expect the notch count value to go to '0' the moment I move the slider to the left.

It might be more intuitive to do it like that. If the user decides against turning it off, it would be nice to restore their previous value (unless they saved it at zero).

The value does go to zero after save, but I guess I am suggesting that it should go to zero before the save.

Otherwise it does what it says :-)

@ctzsnooze
Copy link
Member

ctzsnooze commented Nov 26, 2021

Also, it's a small point, but in other elements, the values are with their labels in the right column, but here the numbers are on the left, squashed in with the switch.

We should be consistent = numbers AND labels to the right (like the rpm filters, immediately above).

This image is with this PR applied over current master.

Screen Shot 2021-11-26 at 16 31 55
.

@KarateBrot
Copy link
Member Author

KarateBrot commented Nov 26, 2021

@ctzsnooze Hold onto your horse, lol. But thanks for the feedback anyways. I need to completely rework this now that #2638 is merged (actually haslinghuis has already worked something out for me). I'll have it ready tonight, I think.

@KarateBrot KarateBrot force-pushed the dynNotchSwitch branch 2 times, most recently from b83a65a to d93b6bf Compare November 27, 2021 15:42
@KarateBrot KarateBrot closed this Nov 27, 2021
@KarateBrot KarateBrot reopened this Nov 27, 2021
@KarateBrot KarateBrot force-pushed the dynNotchSwitch branch 2 times, most recently from 716f430 to d936160 Compare November 27, 2021 16:40
haslinghuis
haslinghuis previously approved these changes Nov 28, 2021
@KarateBrot KarateBrot force-pushed the dynNotchSwitch branch 2 times, most recently from 25e89cb to b969d36 Compare November 30, 2021 16:35
@KarateBrot KarateBrot marked this pull request as ready for review November 30, 2021 16:35
haslinghuis
haslinghuis previously approved these changes Nov 30, 2021
Copy link
Member

@haslinghuis haslinghuis left a comment

Choose a reason for hiding this comment

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

Just tested on 4.2.9 and 4.3

@blckmn
Copy link
Member

blckmn commented Nov 30, 2021

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 -> PASS
  • assigned to an approver -> FAIL
  • approver count at least three -> FAIL

haslinghuis
haslinghuis previously approved these changes Nov 30, 2021
@haslinghuis haslinghuis requested a review from a team November 30, 2021 20:31
klutvott123
klutvott123 previously approved these changes Nov 30, 2021
Copy link
Member

@klutvott123 klutvott123 left a comment

Choose a reason for hiding this comment

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

Tested. Works 👍

@KarateBrot
Copy link
Member Author

KarateBrot commented Nov 30, 2021

I tested it on 4.1, 4.2 and 4.3.

Edit:
Had to push again because I somehow forgot to hide the dynamic notch note on line 3104 in pid_tuning.js when you are using BF >= 4.3

@KarateBrot KarateBrot closed this Dec 1, 2021
@KarateBrot KarateBrot reopened this Dec 1, 2021
@KarateBrot KarateBrot dismissed stale reviews from klutvott123 and haslinghuis via 595e3b3 December 1, 2021 02:01
@KarateBrot KarateBrot requested review from haslinghuis and klutvott123 and removed request for a team December 1, 2021 02:01
haslinghuis
haslinghuis previously approved these changes Dec 1, 2021
haslinghuis
haslinghuis previously approved these changes Dec 1, 2021
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 1, 2021

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 7 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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