Skip to content

Use new handles API in ros2_controllers to fix deprecation warnings #1566

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

kumar-sanjeeev
Copy link
Contributor

@kumar-sanjeeev kumar-sanjeeev commented Mar 3, 2025

This PR applies the new API changes in the handles of ros2_control in following controllers in same fashion as #1565:

  • ackermann_steering_controller
  • bicycle_steering_controller
  • admittance_controller
  • effort_controllers
  • forward_command_controller
  • gpio_controllers
  • gripper_controllers
  • mecanum_drive_controller
  • parallel_gripper_controller
  • tricycle_controller
  • pid_controller
  • tricycle_steering_controller
  • mecum_driver_contoller
  • force_torque_sensor_broadcaster
  • steering_controllers_library
  • joint_state_broadcaster
  • position_controllers
  • joint_trajectory_controller (partial fixed)
  • diff_drive_controller

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.

Looks fine, thanks!

@kumar-sanjeeev
Copy link
Contributor Author

Looks fine, thanks!

@christophfroehlich hi, one question - I have made progress on other controllers as well locally on my system. Can I push these changes into this PR as well (will update description), or should I wait for more reviews on current submitted PR ?

@christophfroehlich
Copy link
Contributor

@saikishor ?

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

The changes already look good to me. I just forgot to comment.
@kumar-sanjeeev yes, please proceed with other packages as well

@kumar-sanjeeev
Copy link
Contributor Author

@saikishor and @christophfroehlich , one question, would you like me to complete all the controller packages first, and then you would review them of all together? Or would you prefer to first provide feedback on the commits that have already been pushed in this PR?

@christophfroehlich
Copy link
Contributor

apply the changes to all of them please

@saikishor
Copy link
Member

Yes please

@kumar-sanjeeev
Copy link
Contributor Author

@christophfroehlich hello, I have tried to resolve all pending comments except the one mentioned in parallel_gripper_controller/include/parallel_gripper_controller/parallel_gripper_action_controller_impl.hpp by @saikishor. I am not sure how to approach that one yet.

I have also rechecked all controllers for any get_value() and set_value() deprecation warnings - all of them should be covered (hopefully 🙂).

please let me know if I have missed or overlooked anything.

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 again! Some minor cleanups only, rest looks great!
let's see if @saikishor can specify what he meant with the parallel_gripper controller.

@kumar-sanjeeev
Copy link
Contributor Author

Thanks again! Some minor cleanups only, rest looks great! let's see if @saikishor can specify what he meant with the parallel_gripper controller.

Thanks for quick review :) , implemented your suggestions.

Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

@kumar-sanjeeev Great work, just some nitpicks and some critical things but rest looks great

christophfroehlich and others added 8 commits July 21, 2025 22:47
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Co-authored-by: Sai Kishor Kothakota <saisastra3@gmail.com>
Copy link
Member

@saikishor saikishor left a comment

Choose a reason for hiding this comment

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

LGTM

@christophfroehlich christophfroehlich merged commit 76c07f9 into ros-controls:master Jul 22, 2025
19 of 26 checks passed
@kumar-sanjeeev
Copy link
Contributor Author

@christophfroehlich, @saikishor, thank you both for reviewing this PR, and 🙌 @christophfroehlich for taking care code review suggestions from @saikishor.

@saikishor
Copy link
Member

@christophfroehlich, @saikishor, thank you both for reviewing this PR, and 🙌 @christophfroehlich for taking care code review suggestions from @saikishor.

Thanks to you for all the efforts @kumar-sanjeeev :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants