Mutation of the LowPassFilter into a generic filter

[tomcombriat]

Hi, first answer to #164 (second part).

This new generic filter, heavily based on the original LowPassFilter, can produce four different types of filters: low-pass, high-pass, band-pass and notch. The low-pass is exactly identical to the original version.

For cleanliness, I have created a new class: ResonantFilterNbits<unsigned_t type, FILTER_TYPE> where type is the type expected for both the cutoff frequency and the resonance (uint8_t or uint16_t), and FILTER_TYPE is the filter type (hence the name), to be chosen between: LOWPASS, HIGHPASS, BANDPASS and NOTCH.

For convenience the following "pre-made" types are defined:

• 8 bits versions:

  • LowPassFilter,
  • HighPassFilter
  • BandPassFilter
  • NotchFilter

• 16 bits versions:

  • LowPassFilter16
  • HighPassFilter16
  • BandPassFilter16
  • NotchFilter16

The LowPassFilter.h file has been emptied of all its content but is falling back (with a warning) to ResonantFilter.h. Thanks to the pre-made types, this does not break existing sketches.

I will soon make some examples for the other types, if you have some comments they are welcome!

Best,

[tomcombriat]

Added some examples! Waiting a bit of time before merging for comments.

Best,

[tfry-git]

Not much time to look at anything in detail. Maybe we can do a little better on the naming, though?

1. ResonantFilterNbits seems too cumbersome to me. It needs a data type as parameter, anyway, and it does not clash with existing classes, so why not plain ResonantFilter?
2. LowPassFilter is a given, already, but I wonder whether we really want to spam all those other classnames. Would it be good enough, having to write ResonantFilter<uint8_t, HIGHPASS>? If you swap the two parameters, and make uint8_t a default for su, that could be reduced to ResonantFilter<HIGHPASS> for the 8 bit case. Looks like a fairly readable use of template arguments to me. YMMV, but one advantage would be that if we add a different filter family, we'll have a reasonable naming scheme at hand, already.
3. As far as I understand, the only implementation difference between the filter types is in the returned value (and I haven't yet wrapped my head around why that works, but sweet that it does). Before setting anything in stone, I'd like to brainstorm the idea that this could all be handled inside a single class, as well. But rather than just next(), it would have nextLow(), nextHigh() etc. Further, we could keep next() as is, but allow to retrieve the other components separately. I.e. add inline AudioOutputStorage_t high() {return in - buf0;} etc. A single filter could now act as a frequency divider. Not sure, whether there is much practical value in this, though.

[tomcombriat]

Not much time to look at anything in detail. Maybe we can do a little better on the naming, though?

Agreed, also with the swap of the template parameters. For the spam of all the other classnames I do not have any hard opinion. My feeling was to be coherent with the existing LowPassFilter hence to make the life easy to the users and not to make only one "special case" but swapping the templates argument indeed make the template easier to write for users.

As far as I understand, the only implementation difference between the filter types is in the returned value (and I haven't yet wrapped my head around why that works, but sweet that it does).

Yup, it is fairly sweet and a pretty costless way to have three more filters!

The motivation of using only one next() and not separated ones was that next() should be called only once per audio cycle. Calling nextHigh(in) and nextLow(in) in the same cycle will throw off the algorithm. The solution would be to do kind of a void nextIn(in) and then call the separeted nextHigh/Low() with no argument (which breaks compatibility with previous sketches except if we use the default next() for the filter type specified in the template and make other ones available). This last solution sounded overcomplicated to me and preferred to bound to the same solution that is used for the StateVariable: the type of filer is decided at construction. Note that separated methods would need to keep the in sample in memory of the filter (but arguably a minor memory impact).

I will be away from my breadboard for a big week so won't change anything during that time but feel free to edit the PR the way you prefer.

[tomcombriat]

Hello!

First batch of modification following the comments by @tfry-git :

• the name of the class has been changed to ResonantFilter
• the order of the template arguments has been changed to template<int8_t FILTER_TYPE, typename su=uint8_t>
• the defined types for anything else than the LowPassFilter have been removed.

Concerning the possibility to have the different filters available at runtime with only one instance I have a suggestion:

• Adding a fifth filter type: MUTABLE or ALL
• this type will store the in sample and make the calculations in next(in), which will be void for this type
• this type will get extra output functions: nextLow(), nextHigh() etc (without argument). The reason for proceeding like this in my opinion are:
• avoid to have in stored in the filter for the cases the filter is not mutable (this class is super optimized for both memory and execution so I am a bit annoyed to make it more heavy, even thought I agree that it is minimal)
• avoid the fact that if the user uses nextLow(in) and then nextHigh for the same sample, the second one should be without argument for instance in order to compute two times the same sample. In the end, the MUTABLE one nearly become a new class with different member, without having code duplication.

What is your opinion?

[tfry-git]

Hi! Thanks for the modifications, and sorry for remaining silent meanwhile. It's been a pretty busy time for me.

I like your idea about the "mutable" filter. The word MUTABLE does not really tell me much, however, and ALL may be a bit too generic. Perhaps MULTIFILTER or something?

D'accord on your plans for void next(in) in that case, etc., too. How about using low() instead of nextLow(), however? To underline the idea that only next() really advances to the next sample.

[tomcombriat]

sorry for remaining silent meanwhile.

No problem, I have also been away from breadboard for some time! I never expect an answer in the minute and I'm grateful that you take the time to help me!

I think your comments make sound. I'll try to implement that soon (shouldn't be too hard but who knows?) and commit!

Best,

[tomcombriat]

Oooookay, as usual this did not go as smoothly as I expected (reference: see any other of my PR), but I think I ended up in something quite nice.

My primary goal was to avoid code duplication and not burden the ResonantFilter with a supplementary AudioOutput_t (probably over optimized, but I think this what Mozzi is about).

So in the end this ends up like this:

• ResonantFilter was slightly modified to allow the buffers to be processed without any output (I do not expect that to show on the binaries). Back-compatibility with LowPassFilter is ensured (even memory wise).
• a new class Multifilter inherits from ResonantFilter:
  • it keeps track of the last in sample
  • process the buffers with next(in) but do not return anything
  • different filtered outputs can be accessed via low(), high(), band() and notch().
  • its memory impact is slightly higher than ResonantFilter but I think it is really worth it.
  • available for 8bits and 16bits samples. An example is provided. Because templates are instantiated after the preproc, I did not find a way to add a fifth type to ResonantFilter but with a different function set.

Let me know if you have some comments!

[tfry-git]

I think your approach makes sense.

One last thing that confused me a bit was that you call the (protected) functions for returning the filtered sample next(). Aren't those really current(), logically?

[tfry-git]

Oh, and perhaps call the derived class MultiResonantFilter, again to avoid future name clashes?

[tomcombriat]

Hi!

One last thing that confused me a bit was that you call the (protected) functions for returning the filtered sample next(). Aren't those really current(), logically?

I have to say that I do not really understand what you mean :/… I did that in order not to have any code duplicates and, to access these members that I did not want to be public, I had to place myself in the ResonantFilter context. My understanding is that the compiler is not going to instantiate a new ResonantFilter for each of these functions.

Oh, and perhaps call the derived class MultiResonantFilter, again to avoid future name clashes?

Can be done ;)

[tomcombriat]

Oh, I think I understand what you meant. I have to say that I did not think much about it and copied the syntax from the StateVariable filter. It is true that for the MultiFilter it is more the current sample but at the same time it is called when asked for the next sample in ResonantFilter.

[tfry-git]

Ok, I have merged, but I'd still prefer if you could rename the protected "helper" next functions, when time permits. The difference to StateVariable is this: In StateVariable, the next helper functions are actually full implementations of the (public) next(). In ResonantFilter, they are only part of the implementation, and specifically they do not contain advanceBuffers().

[tomcombriat]

Ok, sounds legit I see your point. Will do very soon.

Thanks for your help!

[tomcombriat]

Done, see #167