FIR - buffer window: too small / order mixed up

[MaKe81]

Hi, Thank for the really nice filter implementation.

But I see two issues on the convolution in firFilter.js:

• the buffer is initialized in line 36. The length is one too short.
  For this reason the convolution in line 41-43 doesn't loop over the entire filter length, but one element less
  This creates for filters with low order a gain which is significant smaller than 1.
  SUGGEST: z = initZero(f.length)
• Compliment for the logic how the index in line 42 is calculated. Very nice piece of code.
  But instead of "+ cnt" the calculation should call the previous index, so "- cnt"
  •this makes a different at the beginning, when the buffer is not completely filled yet.
  •to avoid negative index at the beginning there is the need to add d.buf.length in the calculation.
  SUGGEST: out += (f[cnt] * d.buf[(d.buf.length + d.pointer - cnt) % d.buf.length])

Can you please double-check?
Thanks

[markert]

Hi,

Thank you for looking so closely into the code. I did not really check the implementation of the FIR simulation other than simulating it on http://markert.github.io/fili.js/ I will write a unit test.

Point 1: It should be initZero(f.length - 1). If you look at an FIR implementation, you can see that you need 1 less storage (z^-1) than you have coefficients. This is because the last calculation is not stored but put out. Nevertheless, you are right about the loop being one too short. I will have a look at that. Maybe the easiest fix is making the buffer one element longer.

Point 2: It is just a ring buffer, so not really rocket science. It is usually a good idea, if you want to call it often since it prevents garbage collection and does not copy within the buffer.
It should really be -cnt (or I can decrease the pointer) and it makes a difference at the beginning and only at the beginning, since FIR filters are symmetrical.

I will fix it over the weekend.