Skip to content

Implemented a generic filters plugin #1634

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

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

ankurbodhe
Copy link

This PR migrates and enhances the chained_filter_controller originally implemented during the ROSCon 2024 control workshop. The controller enables chaining and filtering of state interfaces through a sequence of filters.

Changes introduced:

  1. Ported chained_filter_controller from workshop reference implementation.
  2. Developed unit tests covering:
    • Initialization and lifecycle behavior.
    • Filter configuration and chaining.
    • Correct propagation of state values through the filter stack.

@ankurbodhe
Copy link
Author

@christophfroehlich I just made another commit to fix the issues in the workflow.. requesting you to approve to workflow

@saikishor
Copy link
Member

@ankurbodhe can you fix the pre-commit jobs?

Is this ready for review?

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

Thank you for contribution!
In general, have a look at other packages in this repository to follow the (code) style, copyright claims etc.

Please address my comments and also fix the pre-commit failures as Sai already has mentioned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test if a filter is loaded and used to process data.

@christophfroehlich
Copy link
Contributor

@ankurbodhe can I help you to finish this PR? We also have to apply the changes of #1697 to the new CMakeLists.

@ankurbodhe
Copy link
Author

@christophfroehlich Sorry had a hectic work schedule... my calendar just freed up and would be working on completing the PR.. will be working on this everyday now until completion.

@christophfroehlich
Copy link
Contributor

no worries. don't hesitate to ask if there are any doubts how to proceed

@ankurbodhe ankurbodhe force-pushed the abodhe-impl-filters-plugin branch from c61a23d to c3a79de Compare May 30, 2025 23:40
@christophfroehlich
Copy link
Contributor

@ankurbodhe please don't force-push to already reviewed PRs, it just makes subsequent reviews slow. There is no need for a clean history, as we squash PRs at the time of merging into the base branches.
Ping me again if it is ready for the next review round.

Copy link
Contributor

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.

@github-actions github-actions bot added the stale label Jul 16, 2025
@github-actions github-actions bot removed the stale label Jul 22, 2025
Copy link

codecov bot commented Jul 22, 2025

Codecov Report

Attention: Patch coverage is 92.07317% with 13 lines in your changes missing coverage. Please review.

Project coverage is 85.74%. Comparing base (9a5cf64) to head (16c3303).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
chained_filter_controller/src/chained_filter.cpp 78.68% 9 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1634      +/-   ##
==========================================
+ Coverage   85.66%   85.74%   +0.08%     
==========================================
  Files         123      127       +4     
  Lines       12407    12571     +164     
  Branches     1056     1065       +9     
==========================================
+ Hits        10628    10779     +151     
- Misses       1430     1439       +9     
- Partials      349      353       +4     
Flag Coverage Δ
unittests 85.74% <92.07%> (+0.08%) ⬆️

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

Files with missing lines Coverage Δ
...ned_filter_controller/test/test_chained_filter.cpp 100.00% <100.00%> (ø)
...oller/test/test_load_chained_filter_controller.cpp 100.00% <100.00%> (ø)
...r_controller/test/test_multiple_chained_filter.cpp 100.00% <100.00%> (ø)
chained_filter_controller/src/chained_filter.cpp 78.68% <78.68%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@christophfroehlich christophfroehlich left a comment

Choose a reason for hiding this comment

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

I needed something like this in a project of mine and finished this PR.

As an example, adding this to example_16 gives the wheel acceleration by a derivative filter

left_wheel_filter:
  ros__parameters:
    type: chained_filter_controller/ChainedFilter
    filter_chain:
      filter1:
        name: filter1
        type: "filters/TransferFunctionFilterDouble"
        params:
          # Transfer function coefficients for a third-order state variable filter, giving the first derivative
          a: [1.0, -1.104, 0.406, -0.04979]
          b: [1.839, -1.163, -0.6767]
    input_interfaces: ["left_wheel_joint/velocity"]
    output_interfaces: ["left_wheel_joint/acceleration"]
image

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

Very impressive work 👏🏾

Leaving some comments of my partial review

Comment on lines 157 to 162
rclcpp::NodeOptions ChainedFilter::define_custom_node_options() const
{
return rclcpp::NodeOptions()
.allow_undeclared_parameters(true)
.automatically_declare_parameters_from_overrides(false);
}
Copy link
Member

Choose a reason for hiding this comment

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

Won't it work with the default settings?, Just curious

Copy link
Contributor

Choose a reason for hiding this comment

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

haven't tested it before, but it seems to work without

Comment on lines +47 to +50
{
auto node_options = controller_->define_custom_node_options();
node_options.parameter_overrides(parameters);

Copy link
Member

Choose a reason for hiding this comment

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

Don't we need to parse multiple_chained_filter.yaml this file here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Co-authored-by: Sai Kishor Kothakota <[email protected]>
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.

3 participants