Feature/fir #38
Feature/fir #38
Conversation
Includes a sample by sample implementation of an FIR filter. Provides initial state of filter memory and taps as vec[Complex<i16>] constructor arguments. Work remaining: - Batch implementation. - Documentation. - More unit tests.
This commit is in a non-working state, I'm getting a weird macro error I don't understand. I think once that is sorted it should work though. Looking for assistance on that error.
Added support for batch FIR filtering. Takes in Vec<Complex<i16>> and outputs Vec<Complex<i16>>. Eventually I'd like to support any PrimInt instead of only i16, but this is fine for now.
src/fir/fir_node.rs
Outdated
| Complex::new(6, 5), | ||
| Complex::new(4, 3), | ||
| Complex::new(2, 1)], | ||
| vec![Complex::zero(), |
There was a problem hiding this comment.
Instead of throwing five Complex::zero() calls into the vector, to me it seems easier to read to use something like vec![Complex::zero(); 5].
src/fir/fir_node.rs
Outdated
| /// Arguments: | ||
| /// taps - FIR filter tap Vec[Complex<i16>]. | ||
| /// state - Initial state for the internal filter state and memory. | ||
| pub fn batch_fir(taps: Vec<Complex<i16>>, |
There was a problem hiding this comment.
Maybe this is me misreading the test cases, but would there be a time where the state isn't all zeros? If not, I would say drop the state as an input to fir and batch_fir and just create a zero vector in the call versus having the user pass it in.
There was a problem hiding this comment.
Yeah that's something I was wondering about last night. I can't think of a reason I'd want the filter preloaded with something, but wasn't sure if I should deny them the ability. I guess I could compromise and just have versions that did and didn't require initial state. Would it be a good idea to like make this an Option type and have the None case automagically fill it with 0's?
- Proper use of vec![x; x] macro. - Added functions _with_state to allow user defined state vs. the common case where we assume an initial state of zeros.
Added batch processing mode to FIR filters, sorry this wasn't in the first PR. Hopefully this one shouldn't be too hard to review, it's the same test data, and the new node is only slightly different from the first one. I'll probably merge it with just one persons approval.