Feature/highpass #206
Feature/highpass #206
Conversation
There was a problem hiding this comment.
I think the algorithm is good ( i dont have correct ones memorized by heart, but this seems roughly correct so i trust you sourced it well.
only issueS
-
There isnt any room to add a new data point. The way this would likely to be used is that u create the filter, and can over time, add a new data point (ie, one more buffer). If done correctly, you shouldnt need to store the value of SAMPLES worth of data. Just the current data point, and the summation of the output up until this (prev_sum)
-
Right now, could only b used in one place/for one thing. Else, if you try and have two high pass filters, the data will combine and the static variables will belnd together.
the best solution is to create a high pass filter struct object, and instead of using statci variables, you store those as members of the struct, that way we can create multiple instances of the struct.
Id create a "high pass filter init" function that creates and returns a high pass filter object with a given alpha and scale, and the current "high_pass" API u have would just take in the new data point and the hgih pass object which should give access to the static data needed
general/include/pca9539.h
Outdated
| @@ -5,8 +5,7 @@ | |||
| #include <stdint.h> | |||
|
|
|||
| /* | |||
| PCA 9539 16 bit GPIO expander. Datasheet: | |||
| https://www.ti.com/lit/ds/symlink/pca9539.pdf?ts=1716785085909 | |||
| PCA 9539 16 bit GPIO expander. Datasheet: https://www.ti.com/lit/ds/symlink/pca9539.pdf?ts=1716785085909 | |||
There was a problem hiding this comment.
PCA changes shouldn't be in this PR
middleware/include/high_pass.h
Outdated
|
|
||
| #include <stdio.h> | ||
|
|
||
| int high_pass(int alpha, int scale); |
There was a problem hiding this comment.
There are no functions for the user to pass a value through the filter.
middleware/src/high_pass.c
Outdated
| { | ||
| static float buffer[SAMPLES] = { 0 }; //initialize changing buffer | ||
| static int index = 0; | ||
| static float prev_sum = 0.0; |
There was a problem hiding this comment.
This implementation does not allow for using multiple filters at once.
middleware/src/high_pass.c
Outdated
|
|
||
| //Scale Var allows adjustment of output strength | ||
| //Alpha Var controls weight of prev input & sum difference | ||
| float high_pass(int alpha, int scale) |
There was a problem hiding this comment.
Should these values be ints? Floats might also be useful here.
middleware/include/high_pass.h
Outdated
|
|
||
| #include <stdio.h> | ||
|
|
||
| int high_pass(int alpha, int scale); |
There was a problem hiding this comment.
Documentation for the function should be in the header file and use doxygen, javadoc style comments.
middleware/src/high_pass.c
Outdated
| #include "high_pass.h" | ||
|
|
||
| //Samples can be modified | ||
| #define SAMPLES 12 |
There was a problem hiding this comment.
Make this modifiable by the user. They should not have to edit code in embedded base. This also should be available to be edited on a per high-pass filter level.
middleware/include/high_pass.h
Outdated
|
|
||
| float prev_output; | ||
|
|
||
| } high_pass_st; |
There was a problem hiding this comment.
The convention when defining a type (atleast in our codebases) is to append _t to the type, so this should be high_pass_t
| void high_pass_init(float alpha, float scale, high_pass_st filter); | ||
| /** | ||
| @brief Initialization for high pass values | ||
| */ |
There was a problem hiding this comment.
Doxygen comments go above the function, and explain the parameters and what the function returns (if it returns anything)
middleware/src/high_pass.c
Outdated
| @@ -0,0 +1,43 @@ | |||
| /* | |||
There was a problem hiding this comment.
Delete notes from this if they won't be relevant to future programmers.
|
|
||
| void high_pass_init(float alpha, float scale, high_pass_st filter) | ||
| { | ||
| filter->alpha = alpha; |
There was a problem hiding this comment.
This is incorrect syntax and will not compile
| * @param alpha | ||
| * @param scale | ||
| * @param filter | ||
| */ |
There was a problem hiding this comment.
Need to fill in doc comments
|
Looks good. Merge after thorough testing. |
|
Outline the test cases tested in the header of this PR. |
-Implemented High Pass Algorithm with scale and alpha factors
-Code doesn't build because of issues with PCA driver code, don't think those are relevant
Any other notes go here
Test Cases
To Do
Any remaining things that need to get done
Checklist
It can be helpful to check the
ChecksandFiles changedtabs.Please reach out to your Project Lead if anything is unclear.
Please request reviewers and ping on slack only after you've gone through this whole checklist.
Closes #178 (issue #178 )