Skip to content

Conversation

@hjh059
Copy link

@hjh059 hjh059 commented Dec 14, 2025

Fixed issue #437. I submitted two pull requests: one in the ros2_controllers repository and another in the control_toolbox repository.
The current PR resolves the following issues:

  1. Modified the logic of RateLimiter::set_params to prevent the original parameters from being modified before an exception is thrown. This change allows exceptions thrown by set_params to be caught by the caller.

Copy link
Member

@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.

Thanks for your contribution. The change in logic is good, but do we really need all the tmp variables? The arguments are not const qualified, I think you can just override them?

@hjh059
Copy link
Author

hjh059 commented Dec 15, 2025

Thanks for your contribution. The change in logic is good, but do we really need all the tmp variables? The arguments are not const qualified, I think you can just override them?感谢您的贡献。逻辑上的变更很好,但我们真的需要所有这些临时变量吗?参数不是 const 修饰的,我认为您可以直接覆盖它们?

@christophfroehlich You're absolutely right — thank you for the careful review.
I've updated the implementation to address your suggestion and pushed the changes.

@codecov-commenter
Copy link

Codecov Report

❌ Patch coverage is 62.85714% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.90%. Comparing base (91ef18a) to head (be681a0).

Files with missing lines Patch % Lines
...l_toolbox/include/control_toolbox/rate_limiter.hpp 62.85% 0 Missing and 13 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #554      +/-   ##
==========================================
+ Coverage   83.85%   83.90%   +0.04%     
==========================================
  Files          30       30              
  Lines        2094     2100       +6     
  Branches      112      112              
==========================================
+ Hits         1756     1762       +6     
  Misses        268      268              
  Partials       70       70              
Flag Coverage Δ
unittests 83.90% <62.85%> (+0.04%) ⬆️

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

Files with missing lines Coverage Δ
...l_toolbox/include/control_toolbox/rate_limiter.hpp 75.36% <62.85%> (+2.34%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Member

@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.

LGTM, thanks!
Btw: The failing jobs are not related to this PR.

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

Labels

backport-jazzy Triggers PR backport to ROS 2 jazzy. backport-kilted Triggers PR backport to ROS 2 kilted.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RateLimiter: Don't update parameters before input checks

4 participants