Skip to content

Add configure() overload to set the initial LPF state - #657

Open
davmoli wants to merge 4 commits into
ros-controls:masterfrom
davmoli:add-lpf-initial-value
Open

Add configure() overload to set the initial LPF state#657
davmoli wants to merge 4 commits into
ros-controls:masterfrom
davmoli:add-lpf-initial-value

Conversation

@davmoli

@davmoli davmoli commented Aug 13, 2026

Copy link
Copy Markdown

Description

LowPassFilter always initializes its state from the first update() input, so there's
no way to start it from a known value.

This PR adds a configure(const T & initial_state) overload that seeds the state: the first
update() returns initial_state and the filter converges towards the input from there.

Fixes #643

Is this user-facing behavior change?

Yes, additive only. Existing configure() calls behave exactly as before.

Did you use Generative AI?

Implementation written by hand. Claude Code (Opus 5) reviewed it and helped write the unit tests.

Additional Information

The new tests cover all three supported types (double, std::vector<double>,
WrenchStamped). These are the first direct unit tests of control_toolbox::LowPassFilter;
the existing ones only reach it through the plugin.

TODOs

To send us a pull request, please:

  • Fork the repository.
  • Modify the source; please focus on the specific change you are contributing. If you also reformat all the code, it will be hard for us to focus on your change.
  • Ensure local tests pass. (colcon test and pre-commit run (requires you to install pre-commit by pip3 install pre-commit)
  • Commit to your fork using clear commit messages.
  • Send a pull request, answering any default questions in the pull request interface.
  • Pay attention to any automated CI failures reported in the pull request, and stay involved in the conversation.

@christophfroehlich
christophfroehlich requested a lite review from Copilot August 17, 2026 18:20
@christophfroehlich christophfroehlich added the backport-kilted Triggers PR backport to ROS 2 kilted. label Aug 17, 2026
@codecov-commenter

codecov-commenter commented Aug 17, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.96970% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 83.78%. Comparing base (f2d1a85) to head (decb789).

Files with missing lines Patch % Lines
...oolbox/include/control_toolbox/low_pass_filter.hpp 90.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #657      +/-   ##
==========================================
+ Coverage   83.60%   83.78%   +0.17%     
==========================================
  Files          31       32       +1     
  Lines        2202     2232      +30     
  Branches      126      128       +2     
==========================================
+ Hits         1841     1870      +29     
  Misses        277      277              
- Partials       84       85       +1     
Flag Coverage Δ
unittests 83.78% <96.96%> (+0.17%) ⬆️

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

Files with missing lines Coverage Δ
control_toolbox/test/low_pass_filter_tests.cpp 100.00% <100.00%> (ø)
...oolbox/include/control_toolbox/low_pass_filter.hpp 82.50% <90.00%> (+0.68%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR adds an overload to control_toolbox::LowPassFilter::configure() that allows seeding the filter’s internal state from a known initial value, enabling deterministic startup behavior instead of implicitly seeding from the first update() input.

Changes:

  • Add configure(const T& initial_state) overload and internal initialize_state() helper to seed the filter state.
  • Update update() first-call initialization code to use the shared initialize_state() helper.
  • Add new unit tests covering the new behavior for double, std::vector<double>, and geometry_msgs::msg::WrenchStamped, and wire them into CMake.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
control_toolbox/test/low_pass_filter_tests.cpp Adds direct unit tests validating seeded vs unseeded first-update behavior across supported types.
control_toolbox/include/control_toolbox/low_pass_filter.hpp Introduces configure(initial_state) and a shared initialize_state() helper; updates first-update init to reuse it.
control_toolbox/CMakeLists.txt Registers and links the new low_pass_filter_tests target in the test build.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread control_toolbox/include/control_toolbox/low_pass_filter.hpp

@christophfroehlich christophfroehlich left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, but please update the docstring as suggested by copilot. btw as this is header-only, I think we also can backport this to other stable distros.

@davmoli

davmoli commented Aug 19, 2026

Copy link
Copy Markdown
Author

Docstring updated, thanks.
On the backports, jazzy and humble do not contain the FilterTraits refactor, so it'd need a separate implementation against the old structure. Happy to open a separate PR for those if you'd like them backported.

@christophfroehlich christophfroehlich left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport-kilted Triggers PR backport to ROS 2 kilted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

In LPF, shouldn't there be a way to initialize the filtered state?

4 participants