Moving Average Filter Implementation #43
Conversation
There was a problem hiding this comment.
Overall looks good - just some small changes.
I'd suggest moving all the filtering metatypes into "common" folder, and then keep the IMU stuff inside AM for now.
Would love if you could test this in person to see it working! Then get some code profiling going, I see lots and lots of math & memory operations, and wonder if we could improve our performance by using DMA or TCM better :D . That can be a second PR though, don't try to push it all in one.
Overall super great though! very excited to see this
| #include "MovingAverage.hpp" | ||
|
|
||
| IMUMovingAverageFilter::IMUMovingAverageFilter() { | ||
| imuObj = &BMX160::getInstance(); |
There was a problem hiding this comment.
i'll let this pass for now, but want you to know that we'll need to change it to IMU::getInstance() eventually, the IMU type eventually needs to get defined in the model config, but it's not relevant atm and is an easy change for now.
| checksum += gyryVals.getAverage(windowSize, &FilteredIMUData->gyry); | ||
| checksum += gyrzVals.getAverage(windowSize, &FilteredIMUData->gyrz); | ||
|
|
||
| return checksum == 0 ? FAILURE : SUCCESS; |
There was a problem hiding this comment.
still not sure if checksums are the best way to do this - seems like a lot of computations and I'm still not sure what we're looking for....
| template<typename T> | ||
| void Window<T>::update(T newValue) { | ||
| window[pos] = newValue; | ||
| pos++; |
There was a problem hiding this comment.
you can post-increment within your array access I believe:
https://stackoverflow.com/questions/34250975/increment-operator-inside-array
|
|
||
| // Reset pos once in a while to ensure it doesn't get too big. | ||
| if (pos == MAX_POS_LIMIT) { | ||
| pos = 0; |
There was a problem hiding this comment.
would this reset your entire moving average occasionally?
| } | ||
|
|
||
| int sum = 0; | ||
| for (int i = pos; i > pos - n; i--) { |
There was a problem hiding this comment.
oh I see .....
what is "n" supposed to keep track of here?
Also - negative indices aren't really good style. You'd have to wrap it all the way around.
At that point - I would consider making a linkedlist data struct in common, and using a circular version of that? So you can always just go next next next next next and never worry about resetting your data?
| sum += window[realIdx]; | ||
| } | ||
|
|
||
| *result = static_cast<T>(sum) / n; |
There was a problem hiding this comment.
I think the idea for the static cast is good - the data that IMU's deal with is almost always floating point decimal (i.e. 0.9g, 1.1g, 1.7137g, etc), so it might be worth forcing a float return.
Something about safety-critical applications and never "guessing" your return type. Search for "cast" in this doc: https://embeddedgurus.com/barr-code/category/coding-standards/page/2/ as a starting point
Description
What was completed, changed, or updated?
Adding a moving average filter, which should be used to preprocess the data going into our AHRS solution.
Why was this done (if applicable)?
Testing
What manual tests were used to validate the code?
Todo
What unit tests were used to validate the code?
Working on gTesting right now
Documentation
Milestone number and name:
Simple IMU Data filters, M3
Link to Asana task:
https://app.asana.com/0/1203458353737758/1206513612014108/f
Link to Confluence documentation:
Todo
Reminders
Add reviewers to the PR
Mention the PR in the appropriate discord channel