Rpm defaults fix for 4.3 #2712

asizon · 2021-12-25T19:43:52Z

-Removed non used rpm defaults from 4.2 to avoid msp complications with outdated and not suported version by this.
-Allows confirm or decline dyn values changes.

asizon · 2021-12-25T19:45:18Z

This feature is added here for 10.8.0, removed it for 4.2 version since it is not in 10.7.0:
#1955

ctzsnooze · 2021-12-26T00:07:28Z

Thanks, Asizon.

With the new dynamic notch defaults, many quads can fly safely without RPM filtering, and without making any changes to dynamic notch settings.

I wonder now if we might just provide a warning note, and not make any changes to the dynamic notch values.

blckmn · 2022-01-03T10:17:19Z

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

asizon · 2022-01-04T06:56:54Z

@ctzsnooze @KarateBrot I will make flight test and logs this week to see graphicaly if we really need this rpm defaults now with the new dynamic notch.

ctzsnooze · 2022-01-05T09:39:30Z

I tested it, and the code works really well.

If the dynamic notch filters are at default, the user gets 'safe' notch filters when turning RPM filtering off, and 'propwash friendly' notch filters when turning RPM on. For both transitions, they get this message:

WARNING: Some dynamic notch values have been changed to default values because the RPM filtering has been activated/deactivated.
Please, check before flying.

However, if the user has either loaded a preset that changed their dynamic notch filter settings, or themselves made a change, they get no warning message when turning RPM filtering off, and will not get the 'safer' filters.

Maybe this is a bit illogical? It seems strange to show a strong warning when we shift to 'safe' filters, but we show no warning, none at all, when the user turns RPM off, and we do not provide the 'safer' filters.

This is a tough one.

The 'unsafe' transition is from RPM to non-RPM. That's when a user needs strong dynamic notch filtering, or they may cook their motors.

It seems to me that we should always show a warning in this case. However the warnings should be different, depending on whether or not we automatically changed the dynamic notches.

For example, when we don't automatically change the dynamic notch filter settings:

WARNING: Turning off RPM filtering may overheat the motors.
Because you have custom gyro dynamic notch settings, we do not automatically change those values. You should check that they are appropriate for a non-RPM filter configuration. Typically, three or more dynamic notches are required.
Please monitor your motor temperatures closely for the next few flights.

Whereas if we do automatically change the dynamic notch filter settings:

WARNING: Turning off RPM filtering may overheat the motors.
Typically three, wider dynamic notches are required. We have automatically made those changes for you.
Please monitor your motor temperatures closely for the next few flights.

If we enable RPM filtering, the downside of not changing dynamic notches is only that you may have stronger filtering than is required, leading to a bit of propwash. I'm not sure we really need a warning. A note should be sufficient.

For example, on enabling RPM filtering, where we do not automatically change dynamic notch settings:

NOTE: Turning on RPM filtering usually means that a single, narrower dynamic notch filter is sufficient.
Because you have custom gyro dynamic notch settings, we have not automatically made that change.
Please review your dynamic notch settings before you fly, and monitor your motor temperatures for the next few flights.

And when we do:

NOTE: Turning on RPM filtering usually means that a single, narrower dynamic notch filter is sufficient.
We have automatically made that change for you.
Please monitor your motor temperatures carefully for the next few flights.

asizon · 2022-01-05T09:55:06Z

@ctzsnooze thanks for your coments! I can easy add a diferent dialog for this nothing change case yes.

asizon · 2022-01-07T17:18:56Z

Here my flight Logs with 3 cases, hope that they can be useful @KarateBrot @ctzsnooze

NO RPM_DefaultDynfilter.zip
RPM_DefaultDynFilter.zip
RPM_DynCount-1_Q-500.zip

ctzsnooze · 2022-01-10T07:42:01Z

In the past, when turning RPM filtering off, all users:

got a warning message,
automatically shifted to safer notch filter settings

With this PR, when we turn RPM tilting off, if we have modified any aspect of the notch filter setup, we DO NOT get safer filters, and we DO NOT get a warning.
I think this is unsafe.

Also, in the past, when enabling RPM filtering, we

got a warning
automatically shifted to notch filter to a configuration that caused less delay.

With this PR, we do not get a warning, and we do not get the filter change.

These automatic behaviours have been existing for a long time, and have worked well. I am very concerned about changing the behaviour.

As I said before, if we are not going to make the automatic change, we need to show a message that warns the user of the bad things that could happen, and what they need to do. I already gave some text that we can use for these messages.

ctzsnooze · 2022-01-10T07:55:52Z

@asizon thanks for your logs.
This comparison:

Left side, RPM filtering + 3 Notches, Q=300 (what you would get with this PR on enabling RPM with no change in DN)
Right side, RPM Filtering + 1 Notch, Q=500 (our current auto configuration when enabling RPM)
As expected, the left panel shows a 35hz resonant peak in the spectrum due to significant propwash problems from filter delay. Both spectrums are from D.

ctzsnooze · 2022-01-10T08:02:25Z

This comparison is:

left side: RPM filter ON, with a single notch Q=500 (our auto setting for RPM enabled)
right side: RPM filter OFF, three notches Q=300 (our auto setting for RPM disabled)
Note more high frequency noise on the right, though less propwash.
It's not hard to imagine how much worse the noise would be if we only had one single dynamic notch at say 450Hz, and no RPM filtering, which could happen without warning if the user just turned RPM filtering off in this PR.

ctzsnooze · 2022-01-10T08:08:09Z

Our automatic changes have, in the past, given users reasonable filter delay when enabling RPM filtering, and safe filters when turning it off. If we remove these automatic changes, we run a risk that people will end up with too much propwash, or unsafe filters, and not really understand how that happened. All I have to do is modify my dynamic notch setting a little bit, and suddenly I lose that protection. To me this isn't safe.

It would be OK if we had a strong warning message as above. It would be even better if that message included an 'apply changes' button, so the user could agree to the automatic change.

asizon · 2022-01-10T08:43:39Z

@ctzsnooze these automatic changes are existing from 4.3,not long time, but this pr still continues changing dyn defaults when rpm enabled/disabled. Only doesnt change if dyn values are already changed manually or with presets.

ctzsnooze · 2022-01-13T00:41:23Z

@asizon Yes, I'm aware of what the PR will do.

I still feel that since this proposal will not change the dynamic notch filters, as we have done for the whole of 4.3, and will not provide a message to warn the user, only because of a customisation to dynamic notch settings, it is potentially unsafe, and unsatisfactory.

Having reflected on it, an approach that maybe balances out the different concerns is that we always off the user a message whenever they switch rpm filtering on or off. Always.

That message warns of the potential hazard and offers to set the dynamic notch filters automatically to suit the change. It has an OK button that, when pressed, automatically sets them. If the user presses Cancel, they can keep their existing dynamic notch filters.

Most naive users will press OK and get the 'safe' option. Experienced users who wanted to retain custom dynamic notch settings, even though they change RPM settings, can go 'Cancel. Everyone is warned.

I think this is the safest solution.

Messages should be different when the RPM filter is enabled vs when it is disabled. Maybe something like these:

WARNING:
This change will disable RPM filtering, and may overheat your motors.
Reset the dynamic notch filters to recommended values?
[Cancel] [OK]

NOTE:
This change will enable RPM filtering, increasing filter delay..
Reset the dynamic notch filters to recommended values?
[Cancel] [OK]

asizon · 2022-01-13T05:34:52Z

Thankyou @ctzsnooze ,i need to take time tomorrow and this weekend to make these changes amd some others.

asizon · 2022-01-16T12:17:36Z

I have made requested changes @ctzsnooze . Thanks @limonspb for the new yes/no dialog and pointed out.
Used generalized i18n texts to avoid more increasses of maintenance.

ctzsnooze · 2022-01-16T20:51:26Z

locales/en/messages.json

-    },
-    "dialogDynFiltersConfirm": {
-        "message": "OK"
+        "message": "<span class=\"message-negative\"><b>WARNING: This change will enable/disable RPM filtering, increasing/decreasing filter delay.</b></span><br>Reset the dynamic notch filters to recommended values?"


One single message is fine.
Maybe add the word /effectiveness, so it reads: increasing/decreasing filter delay/effectiveness.

I will change it :)

ctzsnooze · 2022-01-16T20:59:17Z

Works perfectly apart from one issue that I noticed. The dialog only appears the first time the toggle is moved. So you could, for instance, have this result:

User changes DShot Telemetry from ON to OFF -> dialog opens (good).
User Agrees -> Filters change (good)
Then, without saving, user move DShot Telemetry from OFF to ON -> dialog DOES NOT open (bad).
Result = dynamic idle settings for OFF situation, but RPM filter is on.

Otherwise this is really good - simple and intuitive.

haslinghuis · 2022-01-16T22:57:56Z

locales/en/messages.json

-    },
-    "dialogDynFiltersConfirm": {
-        "message": "OK"
+        "message": "<span class=\"message-negative\"><b>WARNING: This change will enable/disable RPM filtering, increasing/decreasing filter delay.</b></span><br>Reset the dynamic notch filters to recommended values?"


Suggested change

"message": "WARNING: This change will enable/disable RPM filtering, increasing/decreasing filter delay. Reset the dynamic notch filters to recommended values?"

"message": "WARNING: This change will enable/disable RPM filtering, increasing/decreasing filter delay delay/effectiveness. Reset the dynamic notch filters to recommended values?"

Please add second  

asizon · 2022-01-17T11:23:19Z

@ctzsnooze Understanding your suspicions, this happens only in the filters tab and I don't know what can be the best way to approach it. The motors tab works fine since the save button is disabled preventing changes from being saved

ctzsnooze · 2022-01-17T23:56:17Z

We do need a fix in the filters tab, because otherwise, a user with RPM on, and who turns it off in filters tab, will get 'stuck' with the wrong (rpm 'off') values even if they switch it back on - and most people won't realised that the values are wrong.

asizon · 2022-01-18T06:32:00Z

Yes @ctzsnooze i will check how we can fix that behabiour.

ctzsnooze · 2022-01-24T20:19:14Z

@asizon it would be great to have this fixed soon, if at all possible.

asizon · 2022-01-25T07:22:29Z

@ctzsnooze i will store dyn notch values and push them if no dialog showsup

Change dialog sonar fixes sonar fix fix text no values change if not dialog shows typo

asizon · 2022-01-26T06:42:30Z

Done @ctzsnooze this is ready and finished now.

ctzsnooze · 2022-01-26T11:47:10Z

Works really well now - thank you!

Tested and approved.

Not sure what to do about the SonarCloud complaints.

asizon · 2022-01-26T13:28:33Z

@ctzsnooze sonarcloud issues are arround the file. They can be fixed in future PRs, not really important and not related to this pr changes.

sonarcloud · 2022-01-26T17:44:40Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
19 Code Smells

No Coverage information
0.0% Duplication

asizon added the RN: BUGFIX label Dec 25, 2021

asizon added this to the 10.8.0 milestone Dec 25, 2021

asizon self-assigned this Dec 25, 2021

asizon requested review from haslinghuis, limonspb and KarateBrot December 25, 2021 19:46

haslinghuis previously approved these changes Dec 30, 2021

View reviewed changes

blckmn previously approved these changes Jan 10, 2022

View reviewed changes

asizon dismissed stale reviews from blckmn and haslinghuis via 6e95233 January 16, 2022 12:16

asizon force-pushed the rpm_defaults_fix branch from 449a95e to e2e7dfd Compare January 16, 2022 12:28

ctzsnooze reviewed Jan 16, 2022

View reviewed changes

haslinghuis reviewed Jan 16, 2022

View reviewed changes

Rpm defaults fix

b7085a5

Change dialog sonar fixes sonar fix fix text no values change if not dialog shows typo

asizon force-pushed the rpm_defaults_fix branch from 92831ff to b7085a5 Compare January 26, 2022 06:41

asizon requested review from haslinghuis, ctzsnooze and blckmn January 26, 2022 06:42

ctzsnooze previously approved these changes Jan 26, 2022

View reviewed changes

KarateBrot previously approved these changes Jan 26, 2022

View reviewed changes

stetic fixes

d72d608

asizon dismissed stale reviews from KarateBrot and ctzsnooze via d72d608 January 26, 2022 17:42

KarateBrot approved these changes Jan 26, 2022

View reviewed changes

haslinghuis approved these changes Jan 26, 2022

View reviewed changes

ctzsnooze approved these changes Jan 26, 2022

View reviewed changes

limonspb approved these changes Jan 28, 2022

View reviewed changes

haslinghuis merged commit f3f8df6 into betaflight:master Jan 28, 2022

ctzsnooze mentioned this pull request Feb 16, 2022

Fix dshot switch #2816

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rpm defaults fix for 4.3 #2712

Rpm defaults fix for 4.3 #2712

asizon commented Dec 25, 2021 •

edited

Loading

asizon commented Dec 25, 2021

ctzsnooze commented Dec 26, 2021

blckmn commented Jan 3, 2022

asizon commented Jan 4, 2022

ctzsnooze commented Jan 5, 2022 •

edited

Loading

asizon commented Jan 5, 2022

asizon commented Jan 7, 2022

ctzsnooze commented Jan 10, 2022 •

edited

Loading

ctzsnooze commented Jan 10, 2022 •

edited

Loading

ctzsnooze commented Jan 10, 2022

ctzsnooze commented Jan 10, 2022

asizon commented Jan 10, 2022

ctzsnooze commented Jan 13, 2022 •

edited

Loading

asizon commented Jan 13, 2022

asizon commented Jan 16, 2022 •

edited

Loading

ctzsnooze Jan 16, 2022

asizon Jan 16, 2022

ctzsnooze commented Jan 16, 2022

haslinghuis Jan 16, 2022 •

edited

Loading

asizon commented Jan 17, 2022

ctzsnooze commented Jan 17, 2022

asizon commented Jan 18, 2022

ctzsnooze commented Jan 24, 2022

asizon commented Jan 25, 2022 •

edited

Loading

asizon commented Jan 26, 2022

ctzsnooze commented Jan 26, 2022 •

edited

Loading

asizon commented Jan 26, 2022 •

edited

Loading

sonarcloud bot commented Jan 26, 2022

	"message": "<span class=\"message-negative\"><b>WARNING: This change will enable/disable RPM filtering, increasing/decreasing filter delay.</b></span><br>Reset the dynamic notch filters to recommended values?"
	"message": "<span class=\"message-negative\"><b>WARNING: This change will enable/disable RPM filtering, increasing/decreasing filter delay delay/effectiveness.</b></span><br><br>Reset the dynamic notch filters to recommended values?"

Rpm defaults fix for 4.3 #2712

Rpm defaults fix for 4.3 #2712

Conversation

asizon commented Dec 25, 2021 • edited Loading

asizon commented Dec 25, 2021

ctzsnooze commented Dec 26, 2021

blckmn commented Jan 3, 2022

asizon commented Jan 4, 2022

ctzsnooze commented Jan 5, 2022 • edited Loading

asizon commented Jan 5, 2022

asizon commented Jan 7, 2022

ctzsnooze commented Jan 10, 2022 • edited Loading

ctzsnooze commented Jan 10, 2022 • edited Loading

ctzsnooze commented Jan 10, 2022

ctzsnooze commented Jan 10, 2022

asizon commented Jan 10, 2022

ctzsnooze commented Jan 13, 2022 • edited Loading

asizon commented Jan 13, 2022

asizon commented Jan 16, 2022 • edited Loading

ctzsnooze Jan 16, 2022

Choose a reason for hiding this comment

asizon Jan 16, 2022

Choose a reason for hiding this comment

ctzsnooze commented Jan 16, 2022

haslinghuis Jan 16, 2022 • edited Loading

Choose a reason for hiding this comment

asizon commented Jan 17, 2022

ctzsnooze commented Jan 17, 2022

asizon commented Jan 18, 2022

ctzsnooze commented Jan 24, 2022

asizon commented Jan 25, 2022 • edited Loading

asizon commented Jan 26, 2022

ctzsnooze commented Jan 26, 2022 • edited Loading

asizon commented Jan 26, 2022 • edited Loading

sonarcloud bot commented Jan 26, 2022

asizon commented Dec 25, 2021 •

edited

Loading

ctzsnooze commented Jan 5, 2022 •

edited

Loading

ctzsnooze commented Jan 10, 2022 •

edited

Loading

ctzsnooze commented Jan 10, 2022 •

edited

Loading

ctzsnooze commented Jan 13, 2022 •

edited

Loading

asizon commented Jan 16, 2022 •

edited

Loading

haslinghuis Jan 16, 2022 •

edited

Loading

asizon commented Jan 25, 2022 •

edited

Loading

ctzsnooze commented Jan 26, 2022 •

edited

Loading

asizon commented Jan 26, 2022 •

edited

Loading