Wip low pass filter #12
Conversation
README.md
Outdated
| Under common_libs/ we have our common libraries. These should be hardware agnostic, modular and testable. | ||
| We have an example here, that shows the general workflow for creating a common library module, including Catch2 testing. | ||
|
|
||
| #### Low Pass FIR Filter |
There was a problem hiding this comment.
Love the documentation, concise with an example. but might be better to make a readme in the low_pass_fir_filter directory instead of the repo readme.
There was a problem hiding this comment.
I created a separate readme and added a reference.
| #include <catch2/catch_all.hpp> | ||
| #include <low_pass_fir_filter.hpp> | ||
|
|
||
|
|
There was a problem hiding this comment.
Good usage of catch2.
Great test coverage.
| // Valid input check | ||
| if (order < 1 || order > MAX_LP_FIR_ORDER) { | ||
| throw std::invalid_argument("order must be at least 1 and less than " + | ||
| std::to_string(MAX_LP_FIR_ORDER)); |
There was a problem hiding this comment.
Great to validate construction.
Another way to do this is by templates, and with a static assert, which would force this error at compile time, but restricts the user to only instantiate the LowPassFIRFilter at compile time. For a common library its probably better to keep this check at runtime like you have it.
Ex.
template <int MAX_LP_FIR_ORDER>
LowPassFIRFilter::LowPassFIRFilter() ...
There was a problem hiding this comment.
Also, best to not use cpp strings or iostream. Try to do this with sprintf, although tbh im not sure how much of a difference that would make.
There was a problem hiding this comment.
i removed the conversion and just wrote "less than max" for simplcity. I believe sprintf requires creating a char array.
| class LowPassFIRFilter { | ||
| private: | ||
| bool buffer_is_empty; | ||
| unsigned int buffer[MAX_LP_FIR_ORDER]; |
There was a problem hiding this comment.
AFAIK this would allocate more memory than needed if user does not use max order.
Great use case for a template...
There was a problem hiding this comment.
I'm used to prioritizing speed over space (static allocation reduces overhead a bit - compared to dynamically allocating vector at runtime), is space usually more important for firmware?
There was a problem hiding this comment.
good question, but here a template will still be static allocation at compile time. I might be missing a use case so take this with a grain of salt, but regardless of a template or a hard coded buffer size its still up to the user to allocate the memory at runtime or compile time.
A vector also uses templates, but the template in a vector determines the type of data stored, not the size of the vector.
in my experience it depends on the project/team and micro-controller, but in robotics firmware i think speed is usually more important to prioritize. In other words, I've found its usually easier to allocate more memory without causing an issue compared to taking up more cpu time. Dynamic allocation on the heap is also just more prone to errors on a micro controller compared to a 'full computer' so always good to bias towards static allocation in firmware.
Sorry for the wordy explanation, happy to talk about this more over a call.
There was a problem hiding this comment.
I had trouble creating a template that could be used across both hpp and cpp files. I left this out for now.
Roozki
left a comment
Roozki
left a comment
There was a problem hiding this comment.
Approving as is, we can template it later
What and Why?
I made a simple low pass filter that can be configured to a desired
order. This filter can be used to de-noise incoming data from devices (e.g. a joystick).How
The algorithm outputs the average of the last
orderdata points (unsigned integers). It keeps track of old points using a buffer of sizeorder+1. Docs were added explaining the algorithm and nuances (readme and comments).Tests
Tests were added for the constructor and update function (along with CMakeLists configurations).
Improvements
More complex coefficients could be used. For instance, older points could influence the filter less than newer points.