Notch filter df1 #14755
Notch filter df1 #14755
Conversation
new version mar 2019
Added Direct Form I implementation that support dynamic change of frequencies. Added update method to update frequency on an existing filter. Added setCoefficients method to easily and efficiently create clones of a filter.
LowSide, HighSide, Onnotch and array tests testing the applyDF1 method. Update method test setCoefficients method test Minor fix in update method definition All unit tests are passing.
| void NotchFilter<T>::update(float sample_freq, float notch_freq, float bandwidth) | ||
| { | ||
| // backup state | ||
| T delay_element_1_bkup = _delay_element_1; |
There was a problem hiding this comment.
Maybe we could gain a bit of efficiency if we move the zeroing of the delay elements out of setParameters so we don't need to backup and restore them. Then you can directly use setParameters and we call the new reset() function when needed.
There was a problem hiding this comment.
That sounds good, then the update() method is redundant and can be deleted. We would be using the setParameters() method directly to update the filters when needed. In that case, I guess we can just initialise the delay elements directly in the class definition and it should do the job just fine. Is that what you meant?
Concerning the reset() method, I guess I should update it with
_delay_element_output_1 = {};
_delay_element_output_2 = {};
to make sure those are zeroed too, if my understanding of the method is correct.
There was a problem hiding this comment.
Hi @bresch , I have made changes according to your review, following the approach outlined my comment above. You can refer to a more detailed description within the commit message.
Not all of the CI tests are passing, with the Offboard position control test failing. However I believe it is a problem with the test itself as I haven't modified code relevant to that test.
Let me know if there are any further changes required. Thanks!
NotchFilter.hpp: - update method removed, replaced in functionality by setParameters method. - removed reset of filter values from setParameters, will be performed by reset method instead. - added reset of output delay elements to reset method. NotchFilterTest.cpp: - modified the update unit test to use the setParameter method instead. the whole test might not be needed anymore now - Now the reset method was called at the start of each unit tests (besides the first one) to match the new behavior of the class. NotchFilterArray.hpp: - Minor fix where the new output delay elements required for the DF1 implementation were not imported from the NotchFilter class.
| T reset(const T &sample); | ||
|
|
||
| void update(float sample_freq, float notch_freq, float bandwidth); |
There was a problem hiding this comment.
@francelico I just noticed the actual update() code is missing.
There was a problem hiding this comment.
I think the update isn't required as we can update the coefficients using setParameters
There was a problem hiding this comment.
I'll delete it quickly if there's no intention of using it. Thanks for the followup. #16061
Please use PX4 Discuss or Slack to align on pull requests if necessary. You can then open draft pull requests to get early feedback.
Describe problem solved by this pull request
A clear and concise description of the problem this proposed change will solve.
E.g. For this use case I ran into...
NotchFilter.hpp and NotchFilterArray.hpp were lacking methods to implement NotchFilters filtering out vibrations caused by propellers, by making use of esc rpm telemetry data.
Describe your solution
A clear and concise description of what you have implemented.
3 methods were added:
applyDF1(): Added Direct Form I implementation that support dynamic change of frequencies.
update(): Added update method to update frequency on an existing filter.
SetCoefficients() : Added setCoefficients method to easily and efficiently create clones of a filter.
Describe possible alternatives
A clear and concise description of alternative solutions or features you've considered.
Some extra logic could be added to the apply method to directly choose if direct form 1 or 2 should be used.
Test data / coverage
How was it tested? What cases were covered? Logs uploaded to https://review.px4.io/ and screenshots of the important plot parts.
Added new unit tests in NotchFilterTests.cpp to cover the new methods:
LowSide, HighSide, Onnotch and array tests testing the applyDF1 method.
Update method unit test
setCoefficients method unit test
All unit tests are passing.
Additional context
Add any other related context or media.
This is a smaller PR covering the new methods required to the existing NotchFilter file. I am working on a larger PR that will be implementing the rpm filtering.
Derivation of the Direct Form I from writing the system equations of: https://en.wikipedia.org/wiki/Digital_filter#Direct_form_I