Increase PID ROS wrapper test coverage by Theonewhomadethings · Pull Request #484 · ros-controls/control_toolbox

Theonewhomadethings · 2025-09-21T00:58:02Z

Overview

This PR increases test coverage for control_toolbox::PidROS, focusing on methods that previously had little or no coverage. (as described in #409)

void PidROS::reset()
void PidROS::reset(bool save_i_term)
double PidROS::compute_command(double error, double error_dot, const rclcpp::Duration & dt)
void PidROS::set_current_cmd(double cmd) { pid_.set_current_cmd(cmd); }
double PidROS::get_current_cmd() { return pid_.get_current_cmd(); }
void PidROS::print_values()

What changed

files changed are:

control_toolbox/control_toolbox/test/pid_ros_publisher_tests.cpp
control_toolbox/control_toolbox/test/pid_ros_parameters_tests.cpp

Added 6 tests to cover these methods in the below files

In control_toolbox/test/pid_ros_parameters_tests.cpp, we added five tests: two I-only tests that check reset() and reset(bool) either clear or keep the integral term; one PI test that calls print_values() and captures the log output; and two P-only tests that cover get_current_cmd() and set_current_cmd(), using tight limits so the results are predictable.

In control_toolbox/test/pid_ros_publisher_tests.cpp, we added one test that runs the publisher end-to-end on a LifecycleNode and confirms a PidState message is published.

Before every commit I made sure to build the package locally, run colcon test and do pre-commit commands. All tests passed and no formatting was flagged.

Notes for Reviewer

This is my first contribution to ros2_control, so I am very open to feedback and am happy to make any changes that are requested.

I can remove comments after a initial review has been made?

Adds a new test in pid_ros_parameters_tests.cpp that verifies PidROS::reset() honors the save_i_term ROS parameter. Using an integral-only controller makes the effect of retaining vs. clearing the I-term directly observable without P/D noise. The test confirms the integral is cleared when false and retained when true.

…term for PidROS::reset() and added new test for reset(bool save_i_term) Reworked the test for the void PidROS::reset() that had a mistake to use an I-only controller initialized via initialize_from_args with large finite clamps and to snapshot the integral state using a zero-error compute_command(0.0, dt) so assertions match the controller’s step semantics. The test now explicitly toggles the save_i_term ROS parameter and verifies that reset() clears or retains the I-term accordingly. Added a new test to cover the void PidROS::reset(bool save_i_term) method, asserting that reset(false) clears and reset(true) preserves the integral state using the same snapshot approach.

… that verifies the overload honors the supplied error_dot and publishes a consistent PidState. Using gains P=1, I=0, D=1 and wide output limits, the test expects cmd = e + e_dot, subscribes to /pid_state, and asserts the message’s error, timestep (converted to seconds), and output match the compute step. Test is placed in pid_ros_publisher_tests.cpp

… and verifies key fields are printed (gains, limits, anti-windup info, errors, command) without brittle formatting assumptions. The test initializes a deterministic PID state, avoids saturation effects, and ensures INFO output from the dedicated logger is correctly emitted.

Add a GTest that verifies PidROS::get_current_cmd() returns the current command across scenarios: default is 0 after init, it round-trips values set via set_current_cmd(), and it reflects commands produced by compute_command() (including saturation to ±5). Also sanity-checks a large finite value to ensure simple delegation to the underlying PID is robust.

It verifies round-trip behavior (set_current_cmd to get_current_cmd) for positive, negative, and large finite values, then confirms compute_command() remains stable (saturates as expected) and get_current_cmd() reflects the last computed output.

codecov-commenter · 2025-09-22T19:29:00Z

Codecov Report

❌ Patch coverage is 96.37681% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.77%. Comparing base (6a28f23) to head (83079f7).

Files with missing lines	Patch %	Lines
control_toolbox/test/pid_ros_parameters_tests.cpp	96.73%	2 Missing and 1 partial ⚠️
control_toolbox/test/pid_ros_publisher_tests.cpp	95.65%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@               Coverage Diff               @@
##           ros2-master     #484      +/-   ##
===============================================
+ Coverage        78.90%   80.77%   +1.86%     
===============================================
  Files               29       29              
  Lines             1896     2023     +127     
  Branches           119      124       +5     
===============================================
+ Hits              1496     1634     +138     
+ Misses             332      320      -12     
- Partials            68       69       +1

Flag	Coverage Δ
unittests	`80.77% <96.37%> (+1.86%)`	⬆️

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_publisher_tests.cpp	`94.44% <95.65%> (ø)`
control_toolbox/test/pid_ros_parameters_tests.cpp	`99.00% <96.73%> (-1.00%)`	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…ls for clarity.

…blePidROS construction and initialize_from_args() for clearer readability.

Theonewhomadethings · 2025-09-23T21:35:40Z

Ready for re-review @saikishor I believe I have addressed all your initial feedback

christophfroehlich

Overall LGTM, some minor comments

…r and executor.spin_some() in publisher test. This removes the deprecation warning and aligns with the guidance in ros-controls#488.

…ack fired, the received PidState is non-null, and its output matches the computed command.

saikishor

Looks fine to me

christophfroehlich

Lgtm, thanks!

(cherry picked from commit edeb343)

Theonewhomadethings added 6 commits September 20, 2025 18:59

Add test for PidROS::set_current_cmd.

2959710

It verifies round-trip behavior (set_current_cmd to get_current_cmd) for positive, negative, and large finite values, then confirms compute_command() remains stable (saturates as expected) and get_current_cmd() reflects the last computed output.

github-actions Bot requested review from ARK3r, LukasMacha97, VX792, destogl, homalozoa, mamueluth and saikishor September 21, 2025 00:58

Theonewhomadethings changed the title ~~Increase PID ROS wrapper test coverage (https://github.com/ros-controls/control_toolbox/issues/409)~~ Increase PID ROS wrapper test coverage Sep 21, 2025

Theonewhomadethings mentioned this pull request Sep 21, 2025

Add option to PID for Forward/Backward integration #469

Open

saikishor reviewed Sep 22, 2025

View reviewed changes

Theonewhomadethings added 3 commits September 23, 2025 21:12

Removed redundant inline /*save_i_term=*/ comments in pid.reset() cal…

fcaba19

…ls for clarity.

dropped inline /*arg=*/ comments and pass arguments directly in Testa…

0c6fd48

…blePidROS construction and initialize_from_args() for clearer readability.

dropped inline /*arg=*/ comments

9a05ae8

Theonewhomadethings requested a review from saikishor September 23, 2025 21:34

Merge branch 'ros2-master' into 409-PID-test-coverage

135a70f

christophfroehlich reviewed Sep 24, 2025

View reviewed changes

Comment thread control_toolbox/test/pid_ros_publisher_tests.cpp Outdated

Comment thread control_toolbox/test/pid_ros_publisher_tests.cpp

Theonewhomadethings added 2 commits September 24, 2025 22:51

Replace deprecated rclcpp::spin_some(node) with SingleThreadedExecuto…

b3b4d5d

…r and executor.spin_some() in publisher test. This removes the deprecation warning and aligns with the guidance in ros-controls#488.

add assertions across pid_ros_publisher_tests.cpp to verify the callb…

83079f7

…ack fired, the received PidState is non-null, and its output matches the computed command.

Theonewhomadethings requested a review from christophfroehlich September 24, 2025 23:41

saikishor approved these changes Sep 25, 2025

View reviewed changes

christophfroehlich approved these changes Sep 25, 2025

View reviewed changes

christophfroehlich added the backport-jazzy Triggers PR backport to ROS 2 jazzy. label Sep 26, 2025

christophfroehlich merged commit edeb343 into ros-controls:ros2-master Sep 26, 2025
25 checks passed

mergify Bot pushed a commit that referenced this pull request Sep 26, 2025

Increase PID ROS wrapper test coverage (#484)

981a080

(cherry picked from commit edeb343)

mergify Bot mentioned this pull request Sep 26, 2025

Increase PID ROS wrapper test coverage (backport #484) #492

Merged

christophfroehlich pushed a commit that referenced this pull request Sep 26, 2025

Increase PID ROS wrapper test coverage (#484) (#492)

297c231

Theonewhomadethings mentioned this pull request Oct 5, 2025

Remove deprecated rclcpp::spin_some() #488

Closed

8 tasks

christophfroehlich mentioned this pull request Nov 1, 2025

Improve test coverage of PID class #409

Closed

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Increase PID ROS wrapper test coverage #484

Increase PID ROS wrapper test coverage #484
christophfroehlich merged 12 commits intoros-controls:ros2-masterfrom
Theonewhomadethings:409-PID-test-coverage

Theonewhomadethings commented Sep 21, 2025

Uh oh!

codecov-commenter commented Sep 22, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Theonewhomadethings commented Sep 23, 2025

Uh oh!

christophfroehlich left a comment

Uh oh!

Uh oh!

Uh oh!

saikishor left a comment

Uh oh!

christophfroehlich left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

Theonewhomadethings commented Sep 21, 2025

Overview

What changed

Notes for Reviewer

Uh oh!

codecov-commenter commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Theonewhomadethings commented Sep 23, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

saikishor left a comment

Choose a reason for hiding this comment

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Sep 22, 2025 •

edited

Loading