Pass the robot_description as an arg in initialize() with fallback to using node's parameters

[SyllogismRXS]

Per the discussion in the admittance_controller of ros2_controllers (ros-controls/ros2_controllers#1094), pass the robot_description string as an input argument to initialize() and fallback to getting the robot_description from the node's parameters.

[saikishor]

Hello!

I think it is better to add a completely new initialize method that doesn't have the parameter interface but has the robot_description and then reuse it within the other initialize method

[christophfroehlich]

I think it is better to add a completely new initialize method that doesn't have the parameter interface but has the robot_description and then reuse it within the other initialize method

but there are more ROS parameters used with the kdl interface. how would you design the initialize method of the base class?

[codecov]

Codecov Report

Attention: Patch coverage is 55.55556% with 4 lines in your changes missing coverage. Please review.

Project coverage is 70.93%. Comparing base (c8e4791) to head (bbcea68).
Report is 10 commits behind head on master.

Files	Patch %	Lines
...ics_interface_kdl/src/kinematics_interface_kdl.cpp	55.55%	1 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #66      +/-   ##
==========================================
- Coverage   71.50%   70.93%   -0.57%     
==========================================
  Files           4        4              
  Lines         200      203       +3     
  Branches      134      136       +2     
==========================================
+ Hits          143      144       +1     
- Misses         25       26       +1     
- Partials       32       33       +1     

Flag	Coverage Δ
unittests	70.93% <55.55%> (-0.57%)	⬇️

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

Files	Coverage Δ
...lude/kinematics_interface/kinematics_interface.hpp	100.00% <ø> (ø)
...ics_interface_kdl/src/kinematics_interface_kdl.cpp	65.26% <55.55%> (-1.05%)	⬇️

[saikishor]

I think it is better to add a completely new initialize method that doesn't have the parameter interface but has the robot_description and then reuse it within the other initialize method

but there are more ROS parameters used with the kdl interface. how would you design the initialize method of the base class?

I agree it will be hard in that case?. Either we go with this implementation (or) have a method called set_robot_description to set it. Any of this should be fine

[SyllogismRXS]

The only other ROS parameter being used by the KDL plugin is alpha. We could remove rclcpp::node_interfaces::NodeParametersInterface completely and provide alpha as an argument to initialize as well. However, if you wanted to inject other variables through the parameters interface in the future (perhaps for a non-KDL plugin), this would make that harder (I personally think that hiding params through the Nodes interface is poor design). However, the user of the library could down cast the library, call their custom pre-initialize functions, and then call the initialize (which will probably be their overridden function).

Do we have any examples of other plugins besides KDL using kinematics_interface?

[christophfroehlich]

I don't think that there are others than _kdl publicly available @pac48?
The interfaces are loaded dynamically by admittance_controller using pluginlib, which means we need a general API for the base class. And I don't think we should start adding a dedicated parameter for _kdl. But we could use a parameter map maybe, if we want to remove the node_parameter_interface?

[SyllogismRXS]

Do you have a recommended parameter map implementation or library?

Using a parameter map will have the same potential issues as using the node_parameter_interface. It's not obvious to the user of the kinematics_interface library which parameters need to be present by the plugin that will be loaded. Thus, it seems that adding the robot_description back into the node parameter interface is just like using a separate parameter map interface. It would require the user of the kinematics_interface library to add the robot_description to the node_parameter_interface before calling initialize() like I did in the admittance controller.

Let me know what you think and I can make the change.

[christophfroehlich]

Do you have a recommended parameter map implementation or library?

I thought about a simple unordered_map like we use it here.

Using a parameter map will have the same potential issues as using the node_parameter_interface. It's not obvious to the user of the kinematics_interface library which parameters need to be present by the plugin that will be loaded. Thus, it seems that adding the robot_description back into the node parameter interface is just like using a separate parameter map interface. It would require the user of the kinematics_interface library to add the robot_description to the node_parameter_interface before calling initialize() like I did in the admittance controller.

I think that's up for the authors of the respective plugin to document that and give proper warnings if a parameter is not given or not within expected specs.
I'd give the robot_description explicitly, but I don't have preference between passing additional arguments via node options or any parameter-map. what you think @saikishor ?

[christophfroehlich]

Hm, maybe I was wrong: For example, to make the admittance controller work with different (possibly unknown) kinematics_interfaces, we can't use an unordered map without an additional parser for parameters which aren't known at compile time. So maybe it's cleaner to stay with ROS-parameters.

[saikishor]

I would avoid parameter maps. I think having things explicit makes it easier

[christophfroehlich]

I would avoid parameter maps. I think having things explicit makes it easier

but how to make things explicit with a dynamically loaded library, where we can't know all parameters when writing the caller (in admittance controller in our example)?

[saikishor]

but how to make things explicit with a dynamically loaded library, where we can't know all parameters when writing the caller (in admittance controller in our example)?

Sorry for the confusion, as for any kinematic interface it would need the robot description right?, this would be an explicit parameter for me, and all other individual parameters should be taken care of from the parameter interface we provide by those respective derived classes.

If it's possible, It is better to have another initialize method where the robot description is not the last argument as it is very clear about its importance here. This way we use the old API in the older ROS versions and from rolling on we use the new API and deprecate the other one eventually

[destogl]

Let's be crazy here and just replace "end_effector_name" with "param_base_name" to know where to search for the parameters. And make robot_description mandatory. To have backwards compatibility, I propose a new method with the following signature:

virtual bool initialize(
    const std::string & robot_description,
    std::shared_ptr<rclcpp::node_interfaces::NodeParametersInterface> parameters_interface,
    const std::string & param_base_name = "kinematics",
    ) = 0;

Then we read `end_effector` from the parameters by provided `parameters_interface`.

[bmagyar]

I think the word we are looking for is namespace, see below ;)

virtual bool initialize(
    const std::string & robot_description,
    std::shared_ptr<rclcpp::node_interfaces::NodeParametersInterface> parameters_interface,
    const std::string & param_namespace = "kinematics",
    ) = 0;

[saikishor]

Yes exactly! This is the right way to do it. I want to avoid making the robot_description an optional arg.

[bmagyar]

In general I like the way this is going.

[destogl]

Closed in favor of #83.