Low Pass Filter with 16bits for cutoff frequency and resonance

[tomcombriat]

Hello,

This is in the same spirit than a previous PR on ADSR (PR #95): it allows to encode the frequency and the resonance on 16bits for the LowPassFilter. This allows much smoother transition, especially coupled with the Smooth class.

In order not to break any existing code using the LowPassFilter, I could not use a template to choose between the two versions: this would have needed all existing codes to add <> at the LowPassFilter declaration, as this was not a template before.

Instead I implemented (mostly copy and paste actually) a LowPassFilter16 for which all data types have been promoted to bigger containers. Unfortunately, as some of the data types ended being 64bits long (!) I am not sure this would be of any use on the 8 bitters, but it works great on the 32 bitters (tested on the STM32)!

As a sidenote, I also updated the keywords which did not contain the latest addition to LowPassFilter.

Best and let me know what you think!

Tom

[sensorium]

Hi Tom, if you think it might not suit 8 bit processors, it might be worth documenting up front the processors it's been tested on, in the class and also the example. Or have I missed something? (Quite likely)

[tfry-git]

In order not to break any existing code using the LowPassFilter, I could not use a template to choose between the two versions: this would have needed all existing codes to add <> at the LowPassFilter declaration, as this was not a template before.

I think, you should still use a template version "under the hood" in order to avoid code duplication:

template<typename T> class LowPassFilterAny {[implementation...]};

typedef LowPassFilterAny<uint8_t> LowPassFilter;  // for compatibility
typedef LowPassFilterAny<uint16_t> LowPassFilter16; 

In the somewhat longer run, C++-17 allows complete omission of the template brackets, if all template parameters have a default value. Thus, eventually, it should then be possible to adjust the above to:

template<typename T = unit8_t> class LowPassFilter {[implementation...]};

#define LowPassFilterAny LowPassFilter;  // for compatibility
typedef LowPassFilter<uint16_t> LowPassFilter16; 

[tomcombriat]

Hi! @sensorium I actually to test the compatibility of this with 8 bitters. I am sure it should work but it probably impede quite the performances, hence limiting the other tasks the MCU can do. I'll let you know.

@tfry-git Yes, a colleague suggested me to do a typedef in order to keep compatibility with LowPassFilter without template parameters (and before the C++17, even with default parameters, it would not compile here…). However, now I am a bit unsure what is the best approach… This 16bits version is not only changing uint8_t to uint16_t but also all the other types. Actually, all of them have to be 1byte bigger than the original version except for ifxmul which changed from: inline int ifxmul(int a, uint8_t b) { return ((a * b) >> FX_SHIFT); } to inline int32_t ifxmul(int64_t a, uint16_t b) { return ((a * b) >> FX_SHIFT16); } So the parameter a had to be broadcasted 2 bytes higher, is considering that int are 16bits (they are on Arduino).

Hence, it should be possible to move everything to a template, with only one argument and then specify all the different types in two specialized constructors, everything else would be common then, reducing code duplication. This would avoid having a "5ish parameters templates" with only a few valid configurations. An intermediate solution would be the current duplicate, with a few typedef to allow compilation by default on the 8 bits version without any <>. If you think it is better that everything is templated as much as possible, I'll try to do that! Best,

[tomcombriat]

Ok @tfry-git , every time I start implementing I actually figure out what you meant and how this is better than my initial commit, one day maybe I'll manage to get something good without help ;).

So, basically, I switched everything to a templated version. Unfortunately there are not less than 6 template parameters… However, I used some typedef to create most common aliases: LowPassFilter (without <>) compatible with the previous version, and LowPassFilter16 that uses 16bits for the parameters. So the template is only used "under the hood" to avoid code duplication, but should be rarely (or never) used "as it".

If you think that is worth getting it, I'll write an additional example and perform thoughtful testing on 8bitters.

Best, Tom

[tfry-git]

I usually don't know how much trouble my suggestions are causing ;-) I must admit I'm not so sure, whether this is an improvement.

Anyway, if this is the path to follow:

I'll yet have to think some, whether the number of template arguments cannot be brought down, somehow. Is my understanding right that there are two "real" parameters (su=data-type for frequencies, and ms=data-type for samples) while the job of the other parameters is to make sure that calculations are performed, correctly? At least these seem to be the only data types used public functions. So, supposing we can find a way to tell the compiler "if su is type x and ms is type y, then mu should be type z [...]", then no further parameters would be needed?

Anyway, as a first step, you should document, that it is not suggested to use LowPassFilterNBits directly.

As a second step, I wonder, whether this might be a good opportunity to replace "int" with int16_t and "long" with int32_t. Those are the types that were "meant" on AVR, anyway. Of course this is actually changing some details.

[tomcombriat]

I usually don't know how much trouble my suggestions are causing ;-) I must admit I'm not so sure, whether this is an improvement.

I think it is, even though the number of template parameters seems a bit overkill… Actually, there might be other "valid" combinations, and then the templates will really fulfill its duty. If I am correct, performance wise this should be the same than the previous version (there might be a slight memory overhead if you have more than one filter, as I moved two #define inside the class, but performance wise it should not be different as they should be evaluated at compile time). I agree that having only these two parameters would be ideal, but I have no idea how to do that (if possible). I am waiting for another weird (to me) thing from you to resolve that!

Anyway, as a first step, you should document, that it is not suggested to use LowPassFilterNBits directly.

I'll do that.

As a second step, I wonder, whether this might be a good opportunity to replace "int" with int16_t and "long" with int32_t. Those are the types that were "meant" on AVR, anyway. Of course this is actually changing some details.

As I was wondering the same thing, I think that would be a good idea. I am just seeing one thing that can go wrong: when using a large number of bits for audio on 32bitters with the "default" lowpass (which is what I used to do): for instance, for now, the int on STM32 would be 32bits long, accommodating largely audio samples up to 24 bits according to my tests. If we switch this code to int16_t there might be a problem. However, it might only be a problem for me! I also thought that maybe the "cleaner" way would be to use the fix decimal types already present in Mozzi, like Q0n16, however, there is not 64 bits types in there. That might turn open other stones…

[tfry-git]

Here's my solution (I hope):

/** This is a template to provide types "matching" a certain other integer type. Parameter is byte size of the first type. E.g. MatchingInts<sizeof(uint32_t)> */
template<uint8_t> struct MatchingInts {
};

/** Specialization for 8-bit types */
template<> struct MatchingInts<1> {
    typedef uint8_t unsigned_type;
    typedef int8_t signed_type;
    typedef uint16_t longer_unsigned_type;
    typedef int16_t longer_signed_type;
};

/** Specialization for 16-bit types */
template<> struct MatchingInts<2> {
    typedef uint16_t unsigned_type;
    typedef int16_t signed_type;
    typedef uint32_t longer_unsigned_type;
    typedef int32_t longer_signed_type;
};

/** TODO: Specialization for 32-bit types */

Then, after all that preamble (which could in theory be moved to a separate header, as it might be useful elsewhere, too), we might have

  su q;
  su f;
  mu fb;
  MatchingInts<sizeof(mu)>::signed_type buf0, buf1;

and

inline MatchingInts<sizeof(su)>::longer_unsigned_type ucfxmul(su a, su b)
    {
    return (((MatchingInts<sizeof(su)>::longer_unsigned_type)a * b) >> FX_SHIFT);
  }

etc.

I.e., the idea is, the MatchingInts-template will provide typedefs to be used to gain signed/unsigned integers of a given size, and of the next-largest size.

[tomcombriat]

Hi, This looks good! I can try to get that work on the real thing. There is one thing I am a bit concerned about is that I expect both the number of bytes for the audio_samples and for the parameters (q and f) to actually matter for some parameters, which is why I had to define ls1 and ls2 (long signed types). For instance for 8bits, ls1 is int and ls2 is long whereas for 16bits both of them are int64_t meaning that ls1 had to be promoted two times to a bigger size where ls2 needed only one promotion.

But it might turn out ok if good care is taken on which variable the sizeof is done on, or maybe with MatchingInts having two template parameters (but that might end in a lot of code for a few specialization in the end no?).

I will try to implement that one of these days and will let you know, Best, Thomas

[tomcombriat]

@tfry-git I think I got fooled by the fact that ints are not the same on STM and Arduino, which is why I had to tweak slightly the 16bits version. -_-. In the end, as 8bitters are restricted to 16bits audio samples (int) and 32bitters to 32 bits audio samples, maybe the use of int and unsigned int (which have different sizes on different platform) is actually a good idea? Otherwise I think the template for MatchingInts will need two parameters (depth of the audio, depth of the parameters) which will end up in a lot of specialization which, in the end, will not make our life easier…

Any thoughts?

[tfry-git]

1) On the question of ints: It is true that audio sample size is currently "int", and thus different across platforms. That is not really something I would like to keep in the long run, though. Many platforms have 32 bit ints, but no way to output more than 16bit resolution, and are restricted on RAM. Thus, long term, I think the direction to move to is to a) avoid int, and b) avoid assumptions on the size of audio samples, wherever possible. Actually, what that may come down to, however, is using AudioOutputStorage_t, where applicable.

2) The template pattern can do more than this. I have not checked all calculations, carefully, but what it all comes down to, is that if you are multiplying - say - a 2 byte integer with a 1 byte integer, you need at least 2+1 bytes for the result. But we can easily compute the type to use for that (at compile time, of course): MatchingInts<sizeof(typea) + sizeof(typeb)>.

Now, the obvious drawback is that we will now have to provide a solution e.g. for 3 bytes, too. So, instead of my previous solution, I suggest to go a little more generic:

template<uint8_t BYTES> struct IntegerType {
    // at an odd value, such as 3 bytes? Add one more byte (up to at most 8 bytes)..
    typedef typename IntegerType<(BYTES < 8) ? (BYTES+1) : 8>::unsigned_type unsigned_type;
    typedef typename IntegerType<(BYTES < 8) ? (BYTES+1) : 8>::signed_type signed_type;
};

// These are the specializations for the types that we actually assume to exist:
template<> struct IntegerType<1> {
    typedef uint8_t unsigned_type;
    typedef int8_t signed_type;
};

template<> struct IntegerType<2> {
    typedef uint16_t unsigned_type;
    typedef int16_t signed_type;
};

template<> struct IntegerType<4> {
    typedef uint32_t unsigned_type;
    typedef int32_t signed_type;
};

template<> struct IntegerType<8> {
    typedef uint64_t unsigned_type;
    typedef int64_t signed_type;
};

and then:

  su q;
  su f;
  mu fb;
  IntegerType<sizeof(mu)>::signed_type buf0, buf1;

[...]

inline IntegerType<sizeof(su)+sizeof(su)>::unsigned_type ucfxmul(su a, su b)
    {
    return (((IntegerType<sizeof(su)+sizeof(su)>::unsigned_type)a * b) >> FX_SHIFT);
  }

You'll note that I did away with longer_signed_type. If all you want is the next largest type, use IntegerType<sizeof(su)+1>.

[tomcombriat]

Hi again, sorry for the spamming…

Actually, I think that int is a good candidate for the audio sample size. I have a few hardware running Mozzi which actually outputting 24bits of audio or, at least using more than 16bits for intermediate calculations. Having some headroom is practical and except for the circular buffer the RAM impact is likely to be very small.

Anyway, even if this changed to AudioOutputStorage_t I think a cool hybrid solution could be done, actually reducing the number of template parameters for the filter to only one, something like (alongside your MatchingInts structure (I really need to get fluent with macro…)):

su q;
su f;
IntegerType<sizeof(AudioOutputStorage_t )>::unsigned_type fb;
AudioOutputStorage_t buf0, buf1;

[...]

inline IntegerType<sizeof(su)+sizeof(su)>::unsigned_type ucfxmul(su a, su b)
{
return (((IntegerType<sizeof(su)+sizeof(su)>::unsigned_type)a * b) >> FX_SHIFT);
}

inline IntegerType<sizeof(AudioOutputStorage_t )+sizeof(su)-1>::signed_type ifxmul(IntegerType<sizeof(AudioOutputStorage_t )+sizeof(su)-1>::signed_type a, su b) { return ((a * b) >> FX_SHIFT); } 

Note the -1 in IntegerType<sizeof(AudioOutputStorage_t )+sizeof(su)-1>::signed_type which is needed for this function when you do the calculations (it is basically an optimisation for 8bitters): Arduino - LPF8: 2+1-1 = 2 -> 16bits (int) (like the original code) 32bitters - LPF8: 4+1-1 = 3 -> 4 (upscaling to the closest higher) -> 32bits (like the original code) Arduino - LPF16: 2+2-1 = 3 -> 4 (upscale) -> 32 bits 32Bitters - LPF16: 4+2-1 = 5 -> 8 (upscale) -> 64 bits (needed in my tests when working with more than 16bits in order to avoid the overflow before the shift).

That way would make a solution with only one template parameter (su) and even allowing for greater audio samples sizes if needed, all done at the compilation. Everything will scale with both the audio size used and the necessary precision on the parameters.

What do you think?

W

[tfry-git]

Thinking about it, I'm not so much opposed to audio samples being int by default (I believe that's hard to change, too), but what I dislike is that this is not configurable. Actually, you PR is - also - one out of many steps into the direction of making this configurable, as it allows to generalize LowPassFilter for different data types.

Your point about different requirements for intermediate calculations and output buffering is quite valid. Anyway, I think it would be a step ahead to -gradually- move all cases where audio samples are concerned to AudioOutputStorage_t, rather than a nameless int. As for having sample size as a template parameter, I agree that this does not look like something that needs to be configurable at this level, so, if only one parameter is needed: all the better.

Regarding the "-1" in byte size, I trust that you have checked, carefully, and that the original logic is actually safe enough. I haven't checked, but looking at the (existing) code, I fear it may actually be working on the assumption that there are never more than about 9 bits in the sample?

Note that it would be equally possible to define IntegerType<> in terms of bits, not bytes, required. That would allow for more micro-optimization.

[tomcombriat]

On the long run, when everything else is adapted, I think the type definition of AudioOutputStorage_tshould be placed in the configuration file, in order for the user not to have to touch any other file for a particular case.

For the -1, I checked that it would give the same type than originally (a few knots in my brain to find the correct one). Actually this is only for

buf1 += ifxmul(buf0 - buf1, f); // could overflow if input changes fast

with f being 8bits, hence buf0-buf1 is indeed restricted in size but I have not checked yet what is the actual expected range of this difference. Note the original comment, @sensorium was probably very aware of that but chose that type, probably as a good tradoff between performances and good sounding.

Anyway, I will implement that, probably this weekend, and test it carefully on both 8 bitters and 32 bitters, for both filters (8/16 bits). If that works as expected, I'll add an example for the 16bits filter.

Thanks for your help,

[tomcombriat]

Alright, one template parameter! I have been extra careful, normally the default version should compile exactly with the same type than the original. Tested and works well on 32bitters for both version (8/16). Will test soon on an Arduino. If that works well I'll add some comments, and an example for usage of the 16bits version of the filter.

[tomcombriat]

Ok, I think I am converging. First thing first: I think, after compilation, it should be exactly the same than before for 8bitters. For 32 bitters, there is a small size reduction for fb for the 8bits filter (before was int hence 32, now is 16) but that seems to have no effect according to initial tests.

Now come the big question: I have seen that, even the original filter can only work on 8bits samples on Arduino (I never realized that before, I am never using Arduino…). As this one is exactly the same for 8bits, this is also the case but this is a bit limiting when one is using an external DAC for instance (PR #87 and the following serie). However, a cool effect of the 16bits filter is that it does work fine on samples up to 15 bits even on 8bitters (you need 1 bit of headroom). The 16bits version for 8bitters is, I think, exactly what would be needed to be coded for allowing greater number of bits as audio samples in, except the fact that parameters do not have the same range. Hence, I think we have two choices:

• or we put a second template parameter to take into account the expected number of bytes in there
• or we keep it as it is now and use LowPassFilter16 if more than 8 bits in are needed on 8 bitters.

I have to say that I am more for the second solution, this already made a few knots in my brain… Also, the current limitation is mainly for optimization purposes, and a 16 bits in able version would look exactly as LowPassFilter16. Note that, as AudioOuputStorage_t is taken into account for the sizes calculation, this is scalable for higher input sizes also. The default version is an really optimized version for 8bits, the LowPassFilter16 should be slower but also safer for all sizes, as long as the AudioOutputStorage_t is set to something that is actually used.

Will propably try to make an example tomorrow or in the following days if you have no further comments. Best

[tfry-git]

I'll mention that we have the AUDIO_BITS define, which - in theory - would allow to throw a warning / error when using the 8bit optimized version while using larger samples. I'm not really sure on how to turn this into a decent solution, though.

[tfry-git]

... or to apply the "-1" optimization only when dealing with 8 AUDIO_BITS?

[tomcombriat]

... or to apply the "-1" optimization only when dealing with 8 AUDIO_BITS?

I will argue against that: if this is done this way, the use of an optimized LowPassFilter will be prevented even if the user, aware of the limitation, took good care to limit the input of the filter. For instance one can imagine filtering an oscillator (8bits) before multiplying it with an enveloppe, and not after, leading to a more optimized result.

I'll try to see if I can make a good warning somewhere!

[tomcombriat]

Ok, in the end I could not find a god way to put a warning showing up only when su=uint8_t. The size of su apparently cannot be tested using preprocessor tests (maybe you will enlighten me on that ;) ). One way to go would be to specialize it in order to put the warning only when compiling the 8bits version of the filter but in that case we lose completely the advantages of making this class a template. So, in the end, I put it very clear in the source file, but also in the examples. As this limitation is not new (it was already the case before) I think it should be enough for users to find the problem if they try to use LowPassFilter in an incorrect manner.

I think this starts to look good, what do you think?

[tfry-git]

The size of su apparently cannot be tested using preprocessor tests (maybe you will enlighten me on that ;) )

That should be static_assert() (but I keep forgetting, which C++ features we can use in Arduino, you'll have to check whether that is available on AVR).

I think this starts to look good, what do you think?

Yes. Could you give a quick glance at the above? After that (whatever the outcome), I think we can merge this.

[tomcombriat]

@tfry-git Thanks I will test that! The only problem I see is that static_assert() will probably block the compilation (#error equivalent in C) whereas a #warning would be better as there are situations where the low pass filter can be used on samples smaller than the audio.

We'll see if I can figure something around.

[tfry-git]

True, I overlooked that. I don't see an easy way around that (short of requiring the user to use something like #define I_KNOW_WHAT_I_AM_DOING true before #include <LowPassFilter.h>. Not very convincing, I'll admit).

[tomcombriat]

Great! Thanks!