Skip to content
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

Use the urdf_ to set the robot_description in admittance controller #1094

Conversation

SyllogismRXS
Copy link
Contributor

I'm running ROS2 Iron, building ros2_control and ros2_controllers from their master branches, but the admittance controller was complaining about the lack of the robot_description parameter in kinematics_interface_kdl (https://github.com/ros-controls/kinematics_interface/blob/master/kinematics_interface_kdl/src/kinematics_interface_kdl.cpp#L34).

The controller_manager now requires that the robot_description to not be a parameter, but come from a topic. I'm doing that. However, it seems that the admittance controller is still expecting robot_description as a parameter. In this PR, I set the robot_description parameter for the admittance controller using the urdf_ member variable. This feels a little hacky. Am I doing something wrong? Is anyone using the latest admittance controller successfully?

Thanks!

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.

No I think you are totally right. But I suggest we should change the API of kinematics_interface to take the robot_description as argument in the initalize() method. This could be done with a default empty-string with fallback to the node parameter to avoid an API break. What do you think @pac48 @destogl?

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.

I think this should be an API change on the kinematics_interface_kdl side. I agree with @christophfroehlich. I would simply add a new method with the URDF argument and parse it directly

@pac48
Copy link
Contributor

pac48 commented Apr 7, 2024

@christophfroehlich I agree that passing the robot description in the initializer makes sense.

@christophfroehlich
Copy link
Contributor

@SyllogismRXS could you please add this change to the kinematics_interface first, and then update this PR here?

@SyllogismRXS
Copy link
Contributor Author

I modified the kinematics_interface package to accept the robot_description as an input argument with a fallback to using the node's parameters if the robot_description is empty: ros-controls/kinematics_interface#66

henrygerardmoore pushed a commit to henrygerardmoore/ros2_controllers that referenced this pull request Jul 19, 2024
Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

This looks good!

@@ -51,7 +53,7 @@ controller_interface::return_type AdmittanceRule::configure(
kinematics_ = std::unique_ptr<kinematics_interface::KinematicsInterface>(
kinematics_loader_->createUnmanagedInstance(parameters_.kinematics.plugin_name));
if (!kinematics_->initialize(
node->get_node_parameters_interface(), parameters_.kinematics.tip))
node->get_node_parameters_interface(), parameters_.kinematics.tip, robot_description))
Copy link
Member

@destogl destogl Jul 24, 2024

Choose a reason for hiding this comment

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

Suggested change
node->get_node_parameters_interface(), parameters_.kinematics.tip, robot_description))
node->get_node_parameters_interface(), "kinematics", robot_description))

Propose to set here param_base_name so that we search for the parameters under controller interface.kinematics. As we are doing in JointLimiters

@destogl
Copy link
Member

destogl commented Aug 14, 2024

Closed in favor of #1247

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.

5 participants