Skip to content

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Aug 6, 2025

Resolves #442

I fixed the tests, we haven't tested all the parameters with initialize_from_ros_parameters

@codecov-commenter
Copy link

codecov-commenter commented Aug 6, 2025

Codecov Report

❌ Patch coverage is 92.30769% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 77.82%. Comparing base (2ad92f1) to head (0634ee9).

Files with missing lines Patch % Lines
control_toolbox/src/pid_ros.cpp 88.88% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@               Coverage Diff               @@
##           ros2-master     #443      +/-   ##
===============================================
+ Coverage        77.75%   77.82%   +0.07%     
===============================================
  Files               30       30              
  Lines             2009     2020      +11     
  Branches           125      129       +4     
===============================================
+ Hits              1562     1572      +10     
  Misses             379      379              
- Partials            68       69       +1     
Flag Coverage Δ
unittests 77.82% <92.30%> (+0.07%) ⬆️

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

Files with missing lines Coverage Δ
control_toolbox/test/pid_ros_parameters_tests.cpp 100.00% <100.00%> (ø)
control_toolbox/src/pid_ros.cpp 64.77% <88.88%> (+0.55%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

u_max = UMAX_INFINITY;
u_min = -UMAX_INFINITY;
bool antiwindup = false;
bool saturation = false;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
bool saturation = false;
bool saturation = true;

Shouldn't the default be true?

declare_param(param_prefix_ + "saturation", rclcpp::ParameterValue(true));

TO be coherent with the above one

Copy link
Contributor Author

@christophfroehlich christophfroehlich Aug 6, 2025

Choose a reason for hiding this comment

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

I'd change it in initialize_from_args to be true only if u_min or u_max argument is finite.
for ROS parameters, idk if it should be default false or default true? or use the same logic, depending on u_clamp_max/min parameter?

saikishor
saikishor previously approved these changes Aug 6, 2025
@christophfroehlich christophfroehlich added the backport-jazzy Triggers PR backport to ROS 2 jazzy. label Aug 13, 2025
@christophfroehlich christophfroehlich merged commit 4ea96f9 into ros2-master Aug 13, 2025
23 of 24 checks passed
@christophfroehlich christophfroehlich deleted the fix/pid/params branch August 13, 2025 18:15
mergify bot pushed a commit that referenced this pull request Aug 13, 2025
christophfroehlich added a commit that referenced this pull request Aug 13, 2025
(cherry picked from commit 4ea96f9)

Co-authored-by: Christoph Fröhlich <[email protected]>
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Latest Kilted sync generates exception from PID controller

3 participants