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

kumar-sanjeeev · 2025-03-03T19:36:10Z

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

…ering_controller

…ing_controller

christophfroehlich

Looks fine, thanks!

…ntroller

…llers

kumar-sanjeeev · 2025-03-05T13:43:57Z

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 · 2025-03-05T13:48:27Z

@saikishor ?

saikishor

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

…sensor_broadcaster tests

…nd_controller

…l, without failures

…ollers

…roadcaster

…_controller

…per_controller

…roller

kumar-sanjeeev · 2025-03-15T19:49:50Z

@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 · 2025-03-15T20:02:08Z

apply the changes to all of them please

saikishor · 2025-03-15T20:27:03Z

Yes please

…ew_api

…rollers

…_controller

…ring_controller

christophfroehlich · 2025-04-16T19:46:26Z

@kumar-sanjeeev I haven't crosschecked your list, are there all controllers included now? (you might add a checklist of all controllers and just check the finished ones)

kumar-sanjeeev · 2025-04-17T07:29:08Z

@kumar-sanjeeev I haven't crosschecked your list, are there all controllers included now? (you might add a checklist of all controllers and just check the finished ones)

hi @christophfroehlich
I am almost done, just a few controllers are pending. I will update the list as checklists (nice suggestion :) )

christophfroehlich · 2025-05-02T08:39:19Z

hi @kumar-sanjeeev will you have time to finish this PR? otherwise, I'd like to review and merge the current status and fix he remaining ones in a future PR.

kumar-sanjeeev · 2025-05-03T07:44:20Z

@christophfroehlich Sorry, I was a bit busy. give me time until next weekend, I’ll try to finish this PR by then, and you can review it afterward. If it’s urgent, feel free to start reviewing and leave comments where improvements are needed. I’ll address those along with the pending controllers that require fixes.

bmagyar · 2025-05-03T09:00:57Z

Thank you, @kumar-sanjeeev , please keep an eye on the current conflict too ;)

…ontrollers/new_api

…rollers_library

…rollers

…ntrollers

…oint_trajectory_controllers

kumar-sanjeeev · 2025-05-11T19:13:49Z

@christophfroehlich I tried to address most of the deprecation warnings across all controllers. You can start reviewing whenever you have time. Feel free to leave comments wherever you see room for improvement or a better approach. I’ll make the necessary changes based on your feedback.

mergify · 2025-05-17T09:47:16Z

This pull request is in conflict. Could you fix it @kumar-sanjeeev?

…trollers/new_api

christophfroehlich

Thanks for finishing this.
The first review from my side. Some simple changes (please apply them from the github UI so that the next review is faster), and some conceptional questions:

what should we do if setting a command fails? return or try to set the other commands? This is not consistent
for filling the publisher with data: I'd suggest to resume and just leave the message field untouched (should be the old vale then if the msg struct is pre-allocated). what do you think?

christophfroehlich · 2025-05-17T10:11:11Z

effort_controllers/src/joint_group_effort_controller.cpp

+    if (!command_interface.set_value(0.0))
+    {
+      RCLCPP_WARN(get_node()->get_logger(), "Error while setting command interface value to 0.0");
+    }
+    return controller_interface::CallbackReturn::SUCCESS;


this should go into the if branch, right?

Suggested change

if (!command_interface.set_value(0.0))

{

RCLCPP_WARN(get_node()->get_logger(), "Error while setting command interface value to 0.0");

}

return controller_interface::CallbackReturn::SUCCESS;

if (!command_interface.set_value(0.0))

{

RCLCPP_ERROR(get_node()->get_logger(), "Unable to set command interface value to 0.0");

return controller_interface::CallbackReturn::SUCCESS;

}

christophfroehlich · 2025-05-17T10:12:12Z

forward_command_controller/src/forward_controllers_base.cpp

+
+    return controller_interface::return_type::OK;


Suggested change

return controller_interface::return_type::OK;

christophfroehlich · 2025-05-17T10:13:52Z

gpio_controllers/src/gpio_command_controller.cpp

-    command_interfaces_map_.at(full_command_interface_name)
-      .get()
-      .set_value(gpio_commands.interface_values[gpio_index].values[command_interface_index]);
+    if (!command_interfaces_map_.at(full_command_interface_name).get().set_value(command_value))


with the other controllers, we return true if setting the command interfaces fails. why not here,too?

Yes, you are right. It should be the case here too. We have to return true or false and act accordingly

christophfroehlich · 2025-05-17T10:17:41Z

joint_trajectory_controller/src/joint_trajectory_controller.cpp

          {
            // copy last command if cmd interface exists
-            it.positions.push_back(joint_command_interface_[0][index].get().get_value());
+            it.positions.push_back(position_command_value_op.has_value());


is there a possibility that the value is nan, which we still should catch here?

christophfroehlich · 2025-05-17T10:18:47Z

joint_trajectory_controller/src/joint_trajectory_controller.cpp

+              get_node()->get_logger(),
+              "Unable to retrieve position command value of joint at index %zu", index);
+          }
+          if (has_position_command_interface_)


Suggested change

if (has_position_command_interface_)

else if (has_position_command_interface_)

shouldn't this go in the else branch?

christophfroehlich · 2025-05-17T10:44:57Z

pid_controller/src/pid_controller.cpp

+        RCLCPP_DEBUG(
+          get_node()->get_logger(), "Unable to retrieve the state interface value for %s",
+          state_interfaces_[i].get_name().c_str());
+        return controller_interface::return_type::OK;


should we really return here? if not, it will just use the old measured_state_values_ right?

Maybe with PID it is not a big issue?
Usually, in my use case, the reference comes at lower frequency and the PID runs at high freq trying to follow the commands

christophfroehlich · 2025-05-17T10:45:56Z

pid_controller/src/pid_controller.cpp

+        RCLCPP_DEBUG(
+          get_node()->get_logger(), "Unable to retrieve the command interface value for %s",
+          command_interfaces_[i].get_name().c_str());
+        return controller_interface::return_type::OK;


should we really return here, or just publish the message without the output field?

christophfroehlich · 2025-05-17T10:47:16Z

steering_controllers_library/src/steering_controllers_library.cpp

+      {
+        RCLCPP_WARN(
+          logger, "Failed to set traction command %.3f for interface '%s' (index %zu).", value,
+          command_interfaces_[i].get_name().c_str(), i);


like above, should we abort and return if one command fails?

I would say return

christophfroehlich · 2025-05-17T10:47:25Z

steering_controllers_library/src/steering_controllers_library.cpp

+      {
+        RCLCPP_WARN(
+          logger, "Failed to set steering command %.3f for interface '%s' (index %zu).", value,
+          command_interfaces_[i].get_name().c_str(), i);


like above, should we abort and return if one command fails?

christophfroehlich · 2025-05-17T10:48:27Z

steering_controllers_library/src/steering_controllers_library.cpp

@@ -632,24 +653,58 @@ controller_interface::return_type SteeringControllersLibrary::update_and_write_c
    {
      if (params_.position_feedback)


like above, should we publish the message without the failing fields instead of aborting?

mergify · 2025-05-17T13:14:56Z

This pull request is in conflict. Could you fix it @kumar-sanjeeev?

saikishor · 2025-05-17T14:31:11Z

forward_command_controller/src/forward_controllers_base.cpp

+    if (!command_interfaces_[index].set_value(data[index]))
+    {
+      RCLCPP_WARN(
+        logger, "Error while setting command interface value at index %zu: value = %f", index,


Suggested change

logger, "Error while setting command interface value at index %zu: value = %f", index,

logger, "Unable to set the command interface value at index %zu: value = %f", index,

saikishor · 2025-05-17T14:33:01Z

gpio_controllers/src/gpio_command_controller.cpp

+        get_node()->get_logger(), "Error while setting command for interface '%s' with value '%f'.",
+        full_command_interface_name.c_str(), command_value);


Suggested change

get_node()->get_logger(), "Error while setting command for interface '%s' with value '%f'.",

full_command_interface_name.c_str(), command_value);

get_node()->get_logger(), "Unable to set the command for interface '%s' with value '%f'.",

full_command_interface_name.c_str(), command_value);

saikishor · 2025-05-17T14:33:27Z

gpio_controllers/src/gpio_command_controller.cpp

-    command_interfaces_map_.at(full_command_interface_name)
-      .get()
-      .set_value(gpio_commands.interface_values[gpio_index].values[command_interface_index]);
+    if (!command_interfaces_map_.at(full_command_interface_name).get().set_value(command_value))


Yes, you are right. It should be the case here too. We have to return true or false and act accordingly

saikishor · 2025-05-17T14:34:37Z

gpio_controllers/src/gpio_command_controller.cpp

+
+    if (!state_msg_interface_value_op.has_value())
+    {
+      RCLCPP_DEBUG(


Suggested change

RCLCPP_DEBUG(

RCLCPP_WARN(

saikishor · 2025-05-17T14:35:04Z

joint_trajectory_controller/src/joint_trajectory_controller.cpp

+      const auto joint_state_interface_value_op = joint_interface[index].get().get_optional();
+      if (!joint_state_interface_value_op.has_value())
+      {
+        RCLCPP_DEBUG(


Suggested change

RCLCPP_DEBUG(

RCLCPP_WARN(

saikishor · 2025-05-17T14:50:04Z

tricycle_controller/src/tricycle_controller.cpp

+  {
+    RCLCPP_WARN(
+      get_node()->get_logger(),
+      "Error while setting velocity command for traction joint to value: '%f'.", Ws_write);


Suggested change

"Error while setting velocity command for traction joint to value: '%f'.", Ws_write);

"Unable to set the velocity command for traction joint to value: '%f'.", Ws_write);

saikishor · 2025-05-17T14:50:16Z

tricycle_controller/src/tricycle_controller.cpp

+  {
+    RCLCPP_WARN(
+      get_node()->get_logger(),
+      "Error while setting position command for steering joint to value: '%f'.", alpha_write);


Suggested change

"Error while setting position command for steering joint to value: '%f'.", alpha_write);

"Unable to set the position command for steering joint to value: '%f'.", alpha_write);

saikishor · 2025-05-17T14:50:38Z

tricycle_controller/src/tricycle_controller.cpp

+      "Error while setting velocity command for traction joint to value 0.0");
+  }
+  if (!steering_joint_[0].position_command.get().set_value(0.0))
+  {
+    RCLCPP_WARN(
+      get_node()->get_logger(),
+      "Error while setting position command for steering joint to value 0.0");


Suggested change

"Error while setting velocity command for traction joint to value 0.0");

}

if (!steering_joint_[0].position_command.get().set_value(0.0))

{

RCLCPP_WARN(

get_node()->get_logger(),

"Error while setting position command for steering joint to value 0.0");

"Unable to set the velocity command for traction joint to value 0.0");

}

if (!steering_joint_[0].position_command.get().set_value(0.0))

{

RCLCPP_WARN(

get_node()->get_logger(),

"Unable to set the position command for steering joint to value 0.0");

saikishor · 2025-05-17T14:50:56Z

tricycle_steering_controller/src/tricycle_steering_controller.cpp

+      !traction_right_wheel_value_op.has_value() || !traction_left_wheel_value_op.has_value() ||
+      !steering_position_op.value())
+    {
+      RCLCPP_DEBUG(


Suggested change

RCLCPP_DEBUG(

RCLCPP_WARN(

saikishor · 2025-05-17T14:51:19Z

velocity_controllers/src/joint_group_velocity_controller.cpp

-    command_interface.set_value(0.0);
+    if (!command_interface.set_value(0.0))
+    {
+      RCLCPP_WARN(get_node()->get_logger(), "Error while setting command interface to value 0.0");


Suggested change

RCLCPP_WARN(get_node()->get_logger(), "Error while setting command interface to value 0.0");

RCLCPP_WARN(get_node()->get_logger(), "Unable to set the command interface to value 0.0");

kumar-sanjeeev · 2025-05-20T08:30:47Z

Thanks for finishing this. The first review from my side. Some simple changes (please apply them from the github UI so that the next review is faster), and some conceptional questions:

what should we do if setting a command fails? return or try to set the other commands? This is not consistent

for filling the publisher with data: I'd suggest to resume and just leave the message field untouched (should be the old vale then if the msg struct is pre-allocated). what do you think?

@christophfroehlich and @saikishor Thanks for comments. I’ll look at them over the weekend and get back to you.

kumar-sanjeeev added 3 commits March 2, 2025 23:14

Fix deprecation warnings with use of new handles API in ackermann_ste…

0bf0670

…ering_controller

Fix deprecation warnings with use of new handles API in bicycle_steer…

1466fdc

…ing_controller

Fix typo

a447f31

github-actions bot requested review from arne48, christophfroehlich, DasRoteSkelett, homalozoa, mhubii, saikishor and VX792 March 3, 2025 19:36

christophfroehlich reviewed Mar 4, 2025

View reviewed changes

kumar-sanjeeev added 2 commits March 5, 2025 12:46

Fix deprecation warnings with use of new handles API in admittance_co…

d356e08

…ntroller

Fix deprecation warnings with use of new handles API in effort_contro…

e4f6aef

…llers

saikishor reviewed Mar 5, 2025

View reviewed changes

kumar-sanjeeev added 10 commits March 5, 2025 15:02

Fix deprecation warnings with use of new handles API in force_torque_…

44dcfcb

…sensor_broadcaster tests

Fix deprecation warnings with use of new handles API in forward_comma…

39200aa

…nd_controller

Fix deprecation warnings with use of new handles API in gpio_controllers

0c044b8

Add more descriptive message for warning

3b4685e

Change return from ERROR to SUCCESS to make it work in sychronous cal…

12d9639

…l, without failures

Fix deprecation warnings with use of new handles API in gripper_contr…

9592271

…ollers

Fix deprecation warnings with use of new handles API in joint_state_b…

704096b

…roadcaster

Fix deprecation warnings with use of new handles API in mecanum_drive…

214dcd1

…_controller

Fix deprecation warnings with use of new handles API in parallel_grip…

eaec22b

…per_controller

Fix deprecation warnings with use of new handles API in tricycle_cont…

a64136a

…roller

Merge branch 'ros-controls:master' into fix_warnings_in_controllers/n…

bf43abd

…ew_api

kumar-sanjeeev and others added 5 commits March 16, 2025 08:18

Fix deprecation warnings with use of new handles API in velocity_comt…

663a131

…rollers

Fix deprecation warnings with use of new handles API in pid_controller

2ef6d10

Fix deprecation warnings with use of new handles API in mecanum_drive…

6628b14

…_controller

Fix deprecation warnings with use of new handles API in tricycle_stee…

31d574c

…ring_controller

Merge branch 'master' into fix_warnings_in_controllers/new_api

85a06ce

kumar-sanjeeev and others added 6 commits May 11, 2025 09:03

Merge remote-tracking branch 'upstream/master' into fix_warnings_in_c…

116cf22

…ontrollers/new_api

Fix deprecation warnings with use of new handles API in steering_cont…

96736c6

…rollers_library

Discard implemented changes to resolve conflict

9af0948

Fix deprecation warnings with use of new handles API in position_cont…

5d5567e

…rollers

Fix deprecation warnings with use of new handles API in diff_drive_co…

764ff05

…ntrollers

Fix [partially] deprecation warnings with use of new handles API in j…

374538f

…oint_trajectory_controllers

Merge remote-tracking branch 'origin/master' into fix_warnings_in_con…

4a45a4b

…trollers/new_api

christophfroehlich requested changes May 17, 2025

View reviewed changes

saikishor reviewed May 17, 2025

View reviewed changes

	if (has_position_command_interface_)
	else if (has_position_command_interface_)

		@@ -632,24 +653,58 @@ controller_interface::return_type SteeringControllersLibrary::update_and_write_c
		{
		if (params_.position_feedback)

	logger, "Error while setting command interface value at index %zu: value = %f", index,
	logger, "Unable to set the command interface value at index %zu: value = %f", index,

		get_node()->get_logger(), "Error while setting command for interface '%s' with value '%f'.",
		full_command_interface_name.c_str(), command_value);

	"Error while setting velocity command for traction joint to value: '%f'.", Ws_write);
	"Unable to set the velocity command for traction joint to value: '%f'.", Ws_write);

	"Error while setting position command for steering joint to value: '%f'.", alpha_write);
	"Unable to set the position command for steering joint to value: '%f'.", alpha_write);

	RCLCPP_WARN(get_node()->get_logger(), "Error while setting command interface to value 0.0");
	RCLCPP_WARN(get_node()->get_logger(), "Unable to set the command interface to value 0.0");

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

kumar-sanjeeev commented Mar 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

kumar-sanjeeev commented Mar 5, 2025

Uh oh!

christophfroehlich commented Mar 5, 2025

Uh oh!

saikishor left a comment

Choose a reason for hiding this comment

Uh oh!

kumar-sanjeeev commented Mar 15, 2025

Uh oh!

christophfroehlich commented Mar 15, 2025

Uh oh!

saikishor commented Mar 15, 2025

Uh oh!

christophfroehlich commented Apr 16, 2025

Uh oh!

kumar-sanjeeev commented Apr 17, 2025

Uh oh!

christophfroehlich commented May 2, 2025

Uh oh!

kumar-sanjeeev commented May 3, 2025

Uh oh!

bmagyar commented May 3, 2025

Uh oh!

kumar-sanjeeev commented May 11, 2025

Uh oh!

mergify bot commented May 17, 2025

Uh oh!

christophfroehlich left a comment

Choose a reason for hiding this comment

Uh oh!

christophfroehlich May 17, 2025 • edited by saikishor Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mergify bot commented May 17, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kumar-sanjeeev commented Mar 3, 2025 •

edited

Loading

christophfroehlich May 17, 2025 •

edited by saikishor

Loading