Skip to content

Conversation

ViktorCVS
Copy link
Contributor

@ViktorCVS ViktorCVS commented Jul 31, 2025

Overview

Remove legacy and deprecated PID parameters from the pid_controller package.

What was added/changed in this PR

  • Removed legacy and deprecated PID parameters from the pid_controller package

About tests

The packages compile correctly and have passed the pre‑commit and colcon tests.

About change in tests

One test file was modified. Updates required by the removal of the legacy anti-windup strategy.

Related PR's

Final notes

I'm very open to any recommendations to improve this code.

@bmagyar
Copy link
Member

bmagyar commented Aug 3, 2025

Did the tests pass for you locally? There seems to be numerical inconsistencies now

@christophfroehlich
Copy link
Contributor

these come from the change of backward to forward integration of the integral action, we are discussing this in the upstream pr

@ViktorCVS ViktorCVS marked this pull request as draft August 19, 2025 12:37
@ViktorCVS ViktorCVS closed this Sep 3, 2025
@ViktorCVS ViktorCVS reopened this Sep 3, 2025
@christophfroehlich christophfroehlich added the check-prerelease-downstream Runs the pre-release workflow with 1st level downstream dependencies label Sep 3, 2025
@ViktorCVS ViktorCVS marked this pull request as ready for review September 3, 2025 13:18
Copy link

codecov bot commented Sep 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.25%. Comparing base (4f9c462) to head (5956291).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1845      +/-   ##
==========================================
- Coverage   85.26%   85.25%   -0.01%     
==========================================
  Files         143      143              
  Lines       13794    13794              
  Branches     1193     1194       +1     
==========================================
- Hits        11761    11760       -1     
  Misses       1636     1636              
- Partials      397      398       +1     
Flag Coverage Δ
unittests 85.25% <100.00%> (-0.01%) ⬇️

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

Files with missing lines Coverage Δ
...ory_controller/src/joint_trajectory_controller.cpp 83.95% <100.00%> (ø)
...ntroller/test/test_trajectory_controller_utils.hpp 84.47% <100.00%> (-0.05%) ⬇️
pid_controller/test/test_pid_controller.cpp 100.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.

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.

Thanks, LGTM. I try to get the downstream checks running before merging this (and releasing to kilted).

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.

Can you please also update joint_trajectory_controller parameters?

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.

Thanks!

@christophfroehlich
Copy link
Contributor

Do you mind opening a PR to fix ur_controllers now, too?

2025-09-03T17:39:24.3426206Z /tmp/ws2/src/ur_controllers/src/scaled_joint_trajectory_controller.cpp: In member function ‘void ur_controllers::ScaledJointTrajectoryController::update_pids()’:
2025-09-03T17:39:24.3427674Z /tmp/ws2/src/ur_controllers/src/scaled_joint_trajectory_controller.cpp:361:36: error: ‘const struct joint_trajectory_controller::Params::Gains::MapJoints’ has no member named ‘i_clamp’; did you mean ‘i_clamp_max’?
2025-09-03T17:39:24.3428531Z   361 |     antiwindup_strat.i_max = gains.i_clamp;
2025-09-03T17:39:24.3428794Z       |                                    ^~~~~~~
2025-09-03T17:39:24.3429033Z       |                                    i_clamp_max
2025-09-03T17:39:24.3430025Z /tmp/ws2/src/ur_controllers/src/scaled_joint_trajectory_controller.cpp:362:37: error: ‘const struct joint_trajectory_controller::Params::Gains::MapJoints’ has no member named ‘i_clamp’; did you mean ‘i_clamp_max’?
2025-09-03T17:39:24.3430891Z   362 |     antiwindup_strat.i_min = -gains.i_clamp;
2025-09-03T17:39:24.3431151Z       |                                     ^~~~~~~
2025-09-03T17:39:24.3431391Z       |                                     i_clamp_max
2025-09-03T17:39:24.3432257Z gmake[2]: *** [CMakeFiles/ur_controllers.dir/build.make:104: CMakeFiles/ur_controllers.dir/src/scaled_joint_trajectory_controller.cpp.o] Error 1
2025-09-03T17:39:24.3432950Z gmake[1]: *** [CMakeFiles/Makefile2:409: CMakeFiles/ur_controllers.dir/all] Error 2
2025-09-03T17:39:24.3433323Z gmake: *** [Makefile:146: all] Error 2
2025-09-03T17:39:24.3433534Z ---
2025-09-03T17:39:24.3433691Z --- stderr: ur_controllers
2025-09-03T17:39:24.3434480Z /tmp/ws2/src/ur_controllers/src/scaled_joint_trajectory_controller.cpp: In member function ‘void ur_controllers::ScaledJointTrajectoryController::update_pids()’:
2025-09-03T17:39:24.3435901Z /tmp/ws2/src/ur_controllers/src/scaled_joint_trajectory_controller.cpp:361:36: error: ‘const struct joint_trajectory_controller::Params::Gains::MapJoints’ has no member named ‘i_clamp’; did you mean ‘i_clamp_max’?
2025-09-03T17:39:24.3436841Z   361 |     antiwindup_strat.i_max = gains.i_clamp;
2025-09-03T17:39:24.3437086Z       |                                    ^~~~~~~
2025-09-03T17:39:24.3437332Z       |                                    i_clamp_max
2025-09-03T17:39:24.3438396Z /tmp/ws2/src/ur_controllers/src/scaled_joint_trajectory_controller.cpp:362:37: error: ‘const struct joint_trajectory_controller::Params::Gains::MapJoints’ has no member named ‘i_clamp’; did you mean ‘i_clamp_max’?
2025-09-03T17:39:24.3439230Z   362 |     antiwindup_strat.i_min = -gains.i_clamp;
2025-09-03T17:39:24.3439485Z       |                                     ^~~~~~~
2025-09-03T17:39:24.3439713Z       |                                     i_clamp_max
2025-09-03T17:39:24.3440290Z gmake[2]: *** [CMakeFiles/ur_controllers.dir/build.make:104: CMakeFiles/ur_controllers.dir/src/scaled_joint_trajectory_controller.cpp.o] Error 1
2025-09-03T17:39:24.3441034Z gmake[1]: *** [CMakeFiles/Makefile2:409: CMakeFiles/ur_controllers.dir/all] Error 2

@christophfroehlich christophfroehlich merged commit b58af5a into ros-controls:master Sep 4, 2025
22 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
check-prerelease-downstream Runs the pre-release workflow with 1st level downstream dependencies
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants