Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Filter Development #68

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

[RFC] Filter Development #68

wants to merge 1 commit into from

Conversation

lucienpeach
Copy link
Collaborator

@lucienpeach lucienpeach commented Sep 21, 2022

Introduces an abstract Filter class from which discrete-time filter implementations can be derived. Also provides several Filter implementation classes corresponding to various commonly used filter types.

This is a new feature. Addresses #62.

Change List

  • Provides Filter object, an abstract class for implementing discrete-time filtering.
  • Provides LowPassFilter object (a child of Filter), which implements a discrete time low-pass filter.

Upcoming Changes

  • Inclusion of DifferenceEquation object (name subject to change) which will determine a LCCDE from Transfer Function polynomials.
  • Will provide additional common filter implementation, facilitated by DifferenceEquation, including:
    • HighPassFilter object implementing a high-pass filter
    • BandPassFilter object implementing a band-pass filter
    • BandStopFilter object implementing a band-stop filter
    • NotchFilter object implementing a notch filter
  • Improvements to generalizability.

Potential (Long-Term) Future Changes

  • Dynamic time step implementation.

Testing

This PR draft has yet to be tested. Testing will be conducted by implementing the various filter implementations in the SimpleControlIOExample example behavior.

Questions

Any feedback on how to improve or enhance the generalizability of this feature would be much appreciated.

Initial commit for filter-dev branch. Introduces an abstract base class for support of filter implementation, as well as a basic, first version of a low-pass filter child class.
@lucienpeach lucienpeach linked an issue Sep 21, 2022 that may be closed by this pull request
@lucienpeach lucienpeach added enhancement New feature or request priority-med Medium priority issues labels Sep 21, 2022
Copy link
Collaborator

@ethanmusser ethanmusser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like you're on the right track! One major structural change, but otherwise just some documentation/formatting updates. Looking forward to seeing the full implementation!

TODOs

Resources

I recommend you reference the following pages as you write code.

  • The Google C++ Style Guide for Drake is the style guide for all C++ code in this repository. All naming, formatting, and general structure should follow this.
  • The Doxygen Manual outlines how to comment your code so that the documentation generates correctly.
  • The ISO C++ Core Guidelines provide all kinds of guidelines for writing C++ code. I link to a few of these in my comments, but here is the full page if you want to check it out.

include/kodlab_mjbots_sdk/filter.h Show resolved Hide resolved
include/kodlab_mjbots_sdk/filter.h Show resolved Hide resolved
namespace kodlab
{

class Filter {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[General] Filter should be a class template. At a minimum, it should be assumed that Filter takes in some kind of scalar type (e.g., int, float, etc.). Abstracting further, it could take in containers containing scalars (e.g., Eigen::Vector<float, ...>, std::array<double, ...>, std::vector<int>, etc.). Your current implementation only accepts Eigen::Vector3f data, which is not always the desired type.

For the scalar case, this might be

Suggested change
class Filter {
template<typename Scalar>
class Filter {

Then, the data types inside would be Scalar instead of Eigen::Vector3f.

include/kodlab_mjbots_sdk/filter.h Show resolved Hide resolved
include/kodlab_mjbots_sdk/filter.h Show resolved Hide resolved
namespace kodlab
{

class LowPassFilter : public Filter {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[General] Same comment as in Filter. This should probably be a class template.
Note that, for this class, both the definition and implementation would have to live in the header file if you make this a class template. See here for more information.

src/low_pass_filter.cpp Show resolved Hide resolved
Comment on lines +18 to +25
LowPassFilter::LowPassFilter(float dt, float k_frequency_cutoff, Eigen::Vector3f raw_data) {

dt_ = dt;
raw_data_ = raw_data;
filtered_data_ = raw_data;
k_frequency_cutoff_ = k_frequency_cutoff;

};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[General] Per this ISO C++ Standard item, you should use an initializer list here instead of assignment.

include/kodlab_mjbots_sdk/low_pass_filter.h Show resolved Hide resolved
src/low_pass_filter.cpp Show resolved Hide resolved
@lucienpeach lucienpeach changed the title Filter Development [RFC] Filter Development Sep 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request priority-med Medium priority issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Digital filter classes
2 participants