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

Add standalone version of LPF #222

Open
wants to merge 10 commits into
base: ros2-master
Choose a base branch
from

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Nov 4, 2024

Replaces #192 and closes #164

Changes on top:

  • I changed the base class to be independent of filters
  • Used the old class/file name and moved the new one to control_toolbox namespace.

The filter plugins should still work as before without any changes (see the test files).

@codecov-commenter
Copy link

codecov-commenter commented Nov 4, 2024

Codecov Report

Attention: Patch coverage is 70.78652% with 26 lines in your changes missing coverage. Please review.

Project coverage is 51.17%. Comparing base (e05102c) to head (b85a21d).
Report is 1 commits behind head on ros2-master.

Files with missing lines Patch % Lines
include/control_filters/low_pass_filter_ros.hpp 60.00% 12 Missing and 4 partials ⚠️
include/control_toolbox/low_pass_filter.hpp 79.59% 8 Missing and 2 partials ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           ros2-master     #222      +/-   ##
===============================================
+ Coverage        50.41%   51.17%   +0.76%     
===============================================
  Files                9       10       +1     
  Lines              486      510      +24     
  Branches            62       66       +4     
===============================================
+ Hits               245      261      +16     
- Misses             215      221       +6     
- Partials            26       28       +2     
Flag Coverage Δ
unittests 51.17% <70.78%> (+0.76%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
include/control_toolbox/low_pass_filter.hpp 79.59% <79.59%> (ø)
include/control_filters/low_pass_filter_ros.hpp 60.00% <60.00%> (ø)

@christophfroehlich
Copy link
Contributor Author

@roncapat what do you think?

Comment on lines +146 to 149
if (!this->configured_ || !lpf_ || !lpf_->is_configured())
{
if (logger_)
RCLCPP_ERROR_SKIPFIRST_THROTTLE((*logger_), *clock_, 2000, "Filter is not configured");
Copy link
Member

Choose a reason for hiding this comment

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

If the filter is not configured, i think it's better to throw an exception rather than continuing right?

Comment on lines -203 to -222
msg_filtered = b1_ * msg_old + a1_ * msg_filtered_old;
msg_filtered_old = msg_filtered;

// TODO(destogl): use wrenchMsgToEigen
msg_old[0] = data_in.wrench.force.x;
msg_old[1] = data_in.wrench.force.y;
msg_old[2] = data_in.wrench.force.z;
msg_old[3] = data_in.wrench.torque.x;
msg_old[4] = data_in.wrench.torque.y;
msg_old[5] = data_in.wrench.torque.z;

data_out.wrench.force.x = msg_filtered[0];
data_out.wrench.force.y = msg_filtered[1];
data_out.wrench.force.z = msg_filtered[2];
data_out.wrench.torque.x = msg_filtered[3];
data_out.wrench.torque.y = msg_filtered[4];
data_out.wrench.torque.z = msg_filtered[5];

// copy the header
data_out.header = data_in.header;
Copy link
Member

Choose a reason for hiding this comment

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

May be the update method specific to geometry_msgs Wrench should be removed?. It kinda has same content of templated one right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the math is the same, but here it uses the Eigen matrices to do the linear combination. this can't work with the templated method alone?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

LowPassFilter accepts parameters only from node
4 participants