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

Cascaded dynamic notches. #7078

Merged
merged 2 commits into from Dec 4, 2018

Conversation

@kmitchel
Copy link
Contributor

kmitchel commented Nov 11, 2018

This PR improves the dynamic notch by replacing the notch filter with 2 narrow notches offset from the FFT calculated center. Additional changes to defaults include disabling glpf2, and setting dynamic gyro lowpass minimum to 150 Hz bi-quad for an improvement in filter latency while improving motor noise attenuation.

An additional debug mode has been added. This is not critical, but a convenience to prevent unnecessary hacking of existing debug modes while testing.

The before/after static filter option has been removed. An "auto" option has been added to fft range to select a recommended range based on dynamic gyro lowpass max. gyro_lowpass_hz no longer sets the dynamic gyro lowpass minimum, to help make configuration easier for pilots.

@ctzsnooze will provide a summary of testing.

Show resolved Hide resolved src/main/flight/pid.h

@mikeller mikeller added this to the 4.0 milestone Nov 12, 2018

@mikeller

This comment has been minimized.

Copy link
Member

mikeller commented Nov 12, 2018

This is overflowing SPRACINGF3NEO. Will need to prune (in another pull request) once this has been finalised.

@kmitchel kmitchel closed this Nov 13, 2018

@kmitchel kmitchel reopened this Nov 13, 2018

@ctzsnooze

This comment has been minimized.

Copy link
Contributor

ctzsnooze commented Nov 14, 2018

User CLI is now simplified such that the first lowpass filter will remain at the lowpass 1 value until the throttle has been increased enough that the 'throttle derived motor frequency' line has exceeded that minimum value. As throttle increases further, the cutoff frequency rises along the cubic curve to reach the max dynamic value at full throttle. Setting max to 0, or indeed any value below min, disables dynamic lowpass behaviour, and lowpass 1 becomes static, 'like usual'. This behaviour applies independently to gyro and D filtering, and makes it easy for the user to enable of disable the rise.

Typically a user would enable dynamic lowpass filtering to reduce prop wash by reducing delay as throttle increases.

The dynamic notch filtering works best when the first gyro lowpass is configured as a biquad.

The max value should be set around or just above the maximum motor noise frequency (actual peak rpm / 60). This encourages good dynamic notch tracking at high throttle, by allowing sufficient high frequency noise into the FFT from which the optimal position of the dynamic notch can be set. Values of 500 on 5" quads, 600-650 on 4" and upwards to 800Hz on smaller setups are not out of the question. If this filter isn't set high enough, the FFT won't track high properly and will drop down and may worsen delay. The first gyro filter should be configured as a biquad to strongly suppress above its cutoff value. The second, fixed gyro lowpass should be disabled and is not helpful. The end result at full throttle is very little gyro delay at all, since there is only one singly lowpass filter at very high cutoff, and no other filtering except for the dynamic notch itself.

The min dynamic filter value sets the strength of the gyro filtering at low throttle. This encourages good dynamic notch tracking at low throttle values by removing high frequency noise which typically exists as harmonics above the fundamental motor frequency. One those harmonics are removed, the dynamic notch can and will drop quite low. If the min value is set low, noise attenuation at zero throttle will be stronger, at the expense of slower deeper and probably worse prop wash. If the min value is set too high, motors may warm up in gentle flying, and the dynamic notch may track above the intended noise peak.

dynLpfIdle = pidProfile->dyn_lpf_dterm_idle / 100.0f;
dynLpfIdlePoint = (dynLpfIdle - (dynLpfIdle * dynLpfIdle * dynLpfIdle) / 3.0f) * 1.5f;
dynLpfInvIdlePointScaled = 1 / (1 - dynLpfIdlePoint) * (pidProfile->dyn_lpf_dterm_max_hz - dynLpfMin);
dynLpfMin = pidProfile->dterm_lowpass_hz;

This comment has been minimized.

@mikeller

mikeller Nov 14, 2018

Member

I am not sure that reusing pidProfile->dterm_lowpass_hz to set the dynamic LPF minimum frequency is the right thing to do here, in the interest of having a clean and not intermingled configuration, and to allow users to experiment with different settings.
What do others think?

This comment has been minimized.

@McGiverGim

McGiverGim Nov 14, 2018

Member

I see it correct because enabling the dyn lpf it disables the static lpf. If we use two different variables maybe the user can think he has two different filters and that is not true.

This comment has been minimized.

@mikeller

mikeller Nov 22, 2018

Member

Ok. But the way one parameter is now used for two different things is very confusing. It sould be used as the dynamic lowpass mid frequency (i.e. the equivalent of the static frequency), or renamed to something like dterm_lowpass_static_mid_or_dyn_min_hz to make it indicative of the different ways it is used.

This comment has been minimized.

@McGiverGim

McGiverGim Nov 22, 2018

Member

I don't know too much about filters, but I think it will be the mid frequency if it was a notch. It is a lpf, so it will be the lower cutoff.
So to me, this parameter is always the lower cutoff, the only different is that if it the user configures it dynamic, this lower cutoff will go upper with the throttle, if not, it will remain static, but in both cases the min cutoff will remain the same.

This comment has been minimized.

@mikeller

mikeller Nov 25, 2018

Member

@McGiverGim: Common sense would suggest that if you take a static setting (like the lowpass cutoff) and switch it to dynamic, if you were to reuse the old parameter that had been used to indicate the static cutoff, this would be used for the middle of the the range of values that the dynamic setting can take on, not at one arbitrary end of the value range.

This comment has been minimized.

@fujin

fujin Nov 28, 2018

Member

I stand by my previous comments. Sorry this is a little long and a bit of a "poop and swoop".

It doesn't matter what we perceive pilots think about the "level of perfection" prescribed in code review nor that we think "pilots NEVER complained" - pilots do not have to care - they don't maintain the projects. Similarly.. none of us are omniscient - it's indefensible and not constructive.

I should also mention the guidance given is not by any means a novel approach. Software quality metrics regarding coupling and cohesion exist from the early 60's, and Dijkstra later commented on the separation of concerns in On the role of scientific thought in '74 (published in '82). It was widely accepted by '89, and expanded on in Elements of Functional Programming (Reade). This went on to be revolutionary for the entire software industry and computer science fields.

There (obviously) exist numerous individuals, projects and organisations who can not, or do not yet hold themselves to such standards or patterns and principles, in turn suffering the quality problems associated, with case studies and data wherever one looks. As we are dealing with flying, Hard Real-time critical death machines, we must strictly hold contributions to computer science and software engineering standards, principles and patterns in order to provide stability, certainty and promises around existing code quality levels, and safety.

To explain further based on my previous comment, from an software engineering perspective, the current parameters all had single concern / responsibility. As follows:

gyro_lowpass_type. Controls the type of lowpass.
gyro_lowpass_hz. Controls the lowpass cutoff frequency regardless of type.
gyro_lowpass2_type. Controls the type of secondary lowpass.
gyro_lowpass2_hz. Controls the secondary lowpass cutoff frequency regardless of type.

In this abstraction, it is not important that the cutoff frequency of a biquad lowpass provides a different result to a pt1 lowpass, as the concerns are modular and decoupled, that is: a gyro lowpass filter can be initialized of varying types (see previous existing implementations of: fake KF, higher order butterworth, biquad RC+FIR2, FIR) without impacting the cutoff, a gyro lowpass filter has a matching apply function-pointer, and a gyro lowpass filter's coefficients should be calculated from a given cutoff frequency. The functions have low coupling, and high cohesion.

If we were to introduce a filter type that could not be configured for a given cutoff frequency, I would not expect us to shoehorn it into that existing abstraction. We would either have to special case this different behavior (coupling) which would then have to be reasoned about by anyone working on the abstraction itself, or the gyro lowpass filter initialization abstraction completely removed, re-introducing duplication, and reducing cohesion.

Similarly, where one previously existing filter type implementation had a specific detail (concern) (Butterworth higher order, or fake KF "q" and "r", or ) a new parameter was introduced explicitly for that concern alone.

This existing abstraction only fits the dynamic lowpass partially - a dynamic lowpass can be one of multiple types, except it special cases biquad to use DF1, and also requires additional parameters to specify other details of the implementation.

This all firmly reinforces not overloading existing parameters: the concern is not shared, the intent should be explicit, as well as the interfaces should reveal the intent.

I hope this helps explain the guidance, although I fear we are doomed to repeat or introduce many well-understood fundamental computer science problems "re-learning" solutions to these quality problems, as opposed to trusting established and respected principles offered by those of us with or from professional engineering, computer science or distributed systems backgrounds - most of whom had these fundamentals drilled into us by our professors, tutors, senior engineering staff or mentors. We are trained to spot such problems early on in the software development life-cycle and address them before they escape.

Finally, I will concede that it can be beneficial to learn such principles by safely attempting, and failing, then being guided through the "why", however as I mentioned above due to our Hard Real-time critical concerns, we have much less room to "go fast & break things" compared to normal Soft real-time or non real-time systems and software development.

Where this seems to break down is we often recommend a specific practice to mitigate a known fundamental problem, then get into a debate about how convenient it is for pilots to use (or similar), which is to put it bluntly: a complete waste of time and energy and puts quality and safety at risk for little gain.

That's all I got.. Hope you guys can come to a good resolution.

This comment has been minimized.

@mikeller

mikeller Nov 28, 2018

Member

Thanks @fujin. One small point:

Finally, I will concede that it can be beneficial to learn such principles by safely attempting, and failing, then being guided through the "why"

Unfortunately, where this discussion is concerned, we are talking about a range of options that impacts on how a published API will be defined - an API that includes provisions for retaining backwards compatibility, in order to allow pilots a certain amount of leniency when updating the firmware on their craft. As a consequence, this means that the cost of 'failing' includes having the failure set in stone for a long time after the API has been released, even after the failure been recognised and fixed, by having to support the failed version of the API with workarounds / special cases in every client that uses the API.

Betaflight has failed in this respect in the past, and learned this the hard way (if you want to see proof of this look into the special cases that are handled in https://github.com/betaflight/betaflight-configurator/blob/master/src/js/msp/MSPHelper.js), and one of the learnings is that it is worthwhile to use scrutiny, caution, and best practices for everything that affects the MSP API, as any error there will be costly for a long time after it was made.

This comment has been minimized.

@ctzsnooze

ctzsnooze Nov 28, 2018

Contributor

The general logic of presets is well explained above, and I thank you for it. The response does not however identify what safety or quality risk exists in the simpler proposal that is being addressed by the more complex proposal.

Without any explicit detail as to what you are actually concerned about in this specific instance, the generalities are guidelines only, and the case should be determined on the merits or otherwise of the alternate suggestions.

In my estimation, the selection between "PT1, BIQUAD, or DYNAMIC" for lowpass is a "single concern" choice. All influence filter behaviour and how the quad behaves. That the cutoff remains the same actually is a minor point; the user changes them to change behaviour of the quad, to counter noise; each choice results in some different behaviour. So I think when we introduced FKF we put it here in the same way. I don't see any issue with putting DYNAMIC here, nor do I think anyone does.

The issue at hand is whether using the gyro_lowpass_hz value for the static value which applies as the lowpass for a fair bit of the lower end of the throttle curve is a hazard.

If the previous PT1 value was not hazardous, and the user selected DYNAMIC, it would not be the lowpass value that would present a hazard, any more than if they selected BIQUAD.

So I fail to understand what the safety hazard is that you are concerned by, and to date NO-ONE has explained what they think the safety hazard is.

Of course there IS a safety hazard with choosing DYNAMIC, but that arises from the MAX value, which could make the motors hot. Not the starting value, which either would be the default (fine, no dangerous than choosing biquad), or what the user had previously customised it to (also safe since cutoff does not change).

So please explain the safety hazard, because I just don't get it.

This for me is a practical matter, not a theoretical or emotional matter. The question is practical: what exactly do you perceive to be the actual safety hazard. What exactly, in this specific case, do you think people might do that would achieve something bad?

Because for anything to happen, the user has to select 'DYNAMIC', they have to think about a default MAX; if they do this without reading anything at all it will be perfectly safe (as far as the concept is safe and the max is safe; there is no problem at the low end, since the cutoff is the same in dynamic at the low end).

I am unable to understand why you cannot identify a safety hazard and in the absence of ANY single clear identifiable concern I restate that I do not agree with what you propose.

None the less it is apparent that I must concede; I give up.

Just bear in mind that, consequently, no-one can easily change their initial cutoff value in dynamic lowpass mode via their OSD or LUA, until LUA and OSD get updated. For a parameter that we would value field feedback on, this is a significant inconvenience. Lua already has issues with out of memory for number of parameters, just adding one more won't help.

This comment has been minimized.

@fujin

fujin Nov 28, 2018

Member

Hey, I'm sorry - you kind of honed in on what I said regarding safety as "hazardous" which is not where I was going at all. I was referring to safety from (our) the maintainer's perspective regarding the software's behavior - software backwards compatibility, and understanding for future changes during routine work such as refactoring, or testing. I agree though, the user must be safe while operating the system, including motor heat and such. I believe you have picked an adequate maximum hz (whatever the lowest level interface name becomes), it worked well when I tested it. I have no objection there.

Regarding outer layer interfaces and naming, if a new parameter is introduced it won't be added to public MSP protocol for a little while. As I mentioned earlier, this decoupling allows for simplification of work on a given layer's module or function.

I do not know that any potential MSP protocol changes should be included in 4.0.0 release, although Semantic Versioning 2.0 would allow it as part of the major breaking change release - @mikeller? We would need time to agree on naming to stabilize the proposed MSP protocol changes first.

That means you would have to use custom Lua scripts from a branch if you wanted to work with new MSP protocol (even with 4.0.0), and potentially a custom configurator from a branch, until 4.1.0 like we would normally do. This gives us time to experiment with the settings and CMS interfaces without having to port all of those changes to the MSP based interfaces which is strictly versioned which is the primary reasoning for the decoupling. During development, the CLI interface settings (and parameter groups) and the CMS (OSD) interface are fastest to change, with minimal dependencies (maybe Blackbox?)

I think the case for re-using temporarily gyro_lowpass_hz is an interesting but fundamental problem from a maintenance perspective. I will agree it worked well in my exposure to it during your team's development of the dynamic lowpass and cascaded notches which I commend.

Doing version-specific behavior in the outer interfaces (MSP, Lua, Configurator) is very bad (should any arise), and we would be stuck with it (as well as supporting the old behavior), if that is where the control for the dynamic lowpass is done.

So, I still believe that dynamic specific parameters should be introduced. I would also do the OSD CMS menu work for the new parameter, I can help with this. I can also show you how to do the MSP part and a custom Lua script which can be distributed and changed more frequently, separate from the normal Betaflight release life-cycle.

This comment has been minimized.

@mikeller

mikeller Nov 28, 2018

Member

@fujin: The Betaflight release version number and the MSP API version number are pretty much independent, and have been treated like this in the past. This means that Betaflight 4.0.0 does not give us free reign to introduce breaking change into MSP. I do not think that it would be advisable for us to do so at any time, as this would either be massively disruptive for our users, or else require us to implement and maintain two independent MSP frontends in configurator, putting a huge strain on our resources for little benefit.

As for @ctzsnooze's concern about making the parameters available for users to tune: For rapid testing / feedback this should be done in CMS - this is the one interface where we can move quickly without being encumbered in API / backwards compatibility constraints.
Doing it in lua won't work well either way - if we use the approach of hacking gyro_lowpass_hz to apply to gyro_lowpass_min_hz then users will only be able to change the minimum frequency in the field, but not disable the dynamic filter or set the maximum frequency. To me this sounds even more incomplete and confusing than not having any of the settings available in lua for now.

Re @ctzsnooze's concern about lua memory overflow: Since each page of the script is loaded independently, and unloaded before the next page is loaded, adding more settings to a page will only cause memory issues if there are too many settings on this page already, and in this case it can be easily mitigated by splitting this page into two separate pages - which is normally needed anyway for QX7 / X9D, since the display size cannot really hold more elements than will fit into memory.

Re lua and versioning, I have tinkered some, and come up with betaflight/betaflight-tx-lua-scripts#188 that allows lua pages to be loaded dependent on the MSP API version.

@kmitchel kmitchel force-pushed the kmitchel:cascaded_dynamic_notch branch from 0124bf2 to 838e6c4 Nov 16, 2018

@kmitchel kmitchel force-pushed the kmitchel:cascaded_dynamic_notch branch from 838e6c4 to 09b3910 Nov 25, 2018

@ctzsnooze

This comment has been minimized.

Copy link
Contributor

ctzsnooze commented Nov 26, 2018

The development of this branch has now reached a really good point. It gives a big improvement in dynamic notch filter performance over the classic single notch.

The amount of general lowpass filtering required becomes less, and because the new notches are narrower, there is less delay and better prop wash handling for any given amount of noise suppression.

The Q and width factors default to values that preferentially do best with noise control. Higher Q (narrower notch), and to a lesser extent narrower width, will reduce delay further at the expense of allowing motor noise to escape around the side of the notch. Q and width parameters are therefore exposed into the CLI for hardcore tuners, but since in the past the dynamic notch Q factor wasn't exposed, we could lock them into #defines later, perhaps once we get more confirmation that the defaults are reasonable.

The main / only issue is CPU load on F411 and F3 boards; there are now two notches to move around dynamically. We would appreciate any advice on how to optimise code performance to minimise CPU hit.

pidProfile->dterm_lowpass_hz = 120;
pidProfile->dterm_lowpass2_hz = 180;
pidProfile->dterm_lowpass_hz = 150;
pidProfile->dterm_lowpass2_hz = 150;

This comment has been minimized.

@etracer65

etracer65 Nov 27, 2018

Member

I really don't like this. Changing the defaults based on whether a feature is included in the code or not is a bad idea. Particularly since the value takes on different function with/without the feature. So assume that this code can't be included for F3, then those users will have different filtering defaults without knowing why. They might try to use some settings from somebody else's tuning guide (that has the feature) and unknowingly set bad values. I think we need to separate the settings for normal lowpass and dynamic (min/max). Then allow the user to select between the filter types explicitly - not implicitly.

This comment has been minimized.

@kmitchel

kmitchel Nov 28, 2018

Author Contributor

USE_DYN_LPF is intended for troubleshooting/debug. Dynamic lowpass is being built in the F3 targets. I'm trying to be as non-intrusive as practical, so the feature can easily be removed if necessary. This filter strategy requires reconfiguring the 2nd gyro and dterm filters, but I'm trying to preserve the old configuration in case it's built without dynamic lowpass.

This comment has been minimized.

@mikeller

mikeller Nov 28, 2018

Member

USE_DYN_LPF is intended for troubleshooting/debug.

Ok, in this case this is wrong and has to be changed - we need to be able to build targets without features enabled if they don't fit into flash, or else we won't be able to build them at all.

Please fix this so USE_DYN_LPF becomes useable for disabling the feature for release builds, and then let us know so we can review.

This comment has been minimized.

@fujin

fujin Nov 28, 2018

Member

Agreed with both @etracer65 and @mikeller. Explicit choices and match existing behavior of USE_ define(s) throughout codebase.

This comment has been minimized.

@kmitchel

kmitchel Nov 28, 2018

Author Contributor

It does disable it. Almost everything is wrapped in defines and maintains the previous filter behavior when undefined.

@spatzengr

This comment has been minimized.

Copy link

spatzengr commented Nov 30, 2018

@ctzsnooze , has anyone completed very thorough comparisons of the single notch (with dynamic LPFs) vs. the dual notch (with the same dynamic LPF) settings? If all is as anticipated, the dual should show less noise in the Plasma graphs vs. the single, and if that is the case the dynamic LPFs minimum can be raised to reduce latency.

Basically, from what I saw a couple weeks ago (below test results), we needed dyn notch Q factors more around 2.0+ to get the same latency. Loosely following, I think that is where the defaults are landing; with a reduced separation between the dual dynamic notch, centers to compensate for the sharper (narrower) notches. I don't want to piss in the cheerios, but strive to be very confident, with clear data backup, this change is resulting in more attenuation for the same or less latency.

This should be a comp. of current Master defaults vs. the PR defaults. To make sure a simple flash up (since most just fly stock nowadays -- as they should expect those to be the best) will be an improvement and not get worse. With the dynamic notch improvements PR and the dynamic LPFs PR, we were not adding a filter, so this was not a concern. Here we are adding a filter so being varied cautious that it is indeed more attenuation and less latency comp is critical.

My results from Q = 1.2 and Q = 3.0. Seemed like 3.0 was too high, or the width between the notches needed to be reduced. I know folks have been testing, but I have not seen a graphic yet like the below making the direct comparison so it is very clearly a winner.

I'm sorry I have not been more active recently to muster up the testing needed, I have my excuses but won't waste your time with excuses. I'll try to get in there to getter done, but if someone beats me to it or already has the data, great even better!

2018 11 12-comp of mashup vs teal_nickel_1

kmitchel added some commits Oct 27, 2018

@kmitchel kmitchel force-pushed the kmitchel:cascaded_dynamic_notch branch from 09b3910 to 233ac7f Dec 2, 2018

@spatzengr

This comment has been minimized.

Copy link

spatzengr commented Dec 3, 2018

Comparisons against Master build 1263 vs. the PR build with a 5" quad.

I should note the pilot felt prop wash was better on build #1263. That said, looking at the logs, I'm not seeing a distinct betterment by running spectrographs on PID Error. More detailed review of the logs is needed. That said, my understand with this PR is that of dual notch gap is set to 0 (W=0) then it is back to a single notch so users can have either way. Single Notch or Dual Notch.

wes s logs comparisons_1

Show resolved Hide resolved src/main/flight/pid.h
@spatzengr

This comment has been minimized.

Copy link

spatzengr commented Dec 4, 2018

Complements of @kmitchel :

The below show the dual dynamic notch with a Q=1.2 (120) and W=8%
q 120 w 8

The below show the dual dynamic notch with a Q=2.4 (240) and W=8%
q 240 w 8

The below show the dual dynamic notch with a Q=1.2 (120) and W=16%
q 120 w 16

The gap between the notches straddles the motor noise peak as detected by the FFT. W is a percentage of the FFT detected peak frequency. Therefore, at a 400hz FFT motor peak, the notch gap with a W=10% is 80hz (40hz + 40hz). This is done because the notches naturally get wider with a fixed Q approach. So as a result, they can be made wider to attenuate more motor noise with the same amplitude reduction.

@mikeller mikeller merged commit c400186 into betaflight:master Dec 4, 2018

@ctzsnooze

This comment has been minimized.

Copy link
Contributor

ctzsnooze commented Dec 5, 2018

This diagram shows the two notches at default values (Q of 1.2, width 8%) when FFT is active at 200Hz, and when the dynamic lowpass also is a biquad and also at 200Hz.
q120_w8percent_center200

@ctzsnooze

This comment has been minimized.

Copy link
Contributor

ctzsnooze commented Dec 5, 2018

This shows the same settings but at 450Hz, typical full throttle FFT peak for a 5"
q120_w8percent_center450

@ctzsnooze

This comment has been minimized.

Copy link
Contributor

ctzsnooze commented Dec 5, 2018

This compares the old dynamic notch, centred at 200hz, Q of .7, with lowpass 1 as a PT1 at 100hz and lowpass 2 also a PT1 at 300hz, to the new code, also centred 200hz, with defaults
classicnotch_200_q707_pt1s_100_300
q120_w8percent_center200

@spatzengr

This comment has been minimized.

Copy link

spatzengr commented Dec 20, 2018

For posterity and the poor Mark Spatz of the future who looks through PRs to figure out what the heck debut mode values do:

image

@McGiverGim

This comment has been minimized.

Copy link
Member

McGiverGim commented Dec 20, 2018

@spatzengr I can try to add this debug modes to the Blackbox, to show a tag more legible.

But I'm not too sure about the units of each field. Some of them seem degrees/second, but others seem ms, etc. If you have time, open an issue/feature in the Blackbox github with this image and the units of each field and I can try to add it.

@kmitchel kmitchel deleted the kmitchel:cascaded_dynamic_notch branch Dec 28, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.