RFC: Fixing incorrect loading of parameter files when multiple GazeboControlPlugins are used.

[lkm1321]

Hi,

I found a bug in IgnitionROS2ControlPlugin::Configure that prevents using multiple controller managers with varying parameter files. IMO, this is a pretty important use case for people working on multi-robot simulation.

This bug is caused because the parameter file is passed as arguments to rclcpp::init:

// ign_ros2_control_plugin.cpp:301
  sdf::ElementPtr argument_sdf = sdfPtr->GetElement("parameters");
  while (argument_sdf) {
    std::string argument = argument_sdf->Get<std::string>();
    arguments.push_back(RCL_PARAM_FILE_FLAG);
    arguments.push_back(argument);
    parameter_file_names.push_back(argument);
    argument_sdf = argument_sdf->GetNextElement("parameters");
  }
// ign_ros2_control_plugin.cpp:357
  if (!rclcpp::ok()) {
    rclcpp::init(static_cast<int>(argv.size()), argv.data());
  }

When multiple instances of IgnitionROS2ControlPlugin are used, the very first instance calls rclcpp::init, and since rclcpp::ok() is true, rclcpp::init is not called from the second instance on. Therefore, the global default context only contains the parameters tag of the very first parameter file.

I have two possible fixes for this, and would like to get your comment. The first is to use different rclcpp::Context for different instances, and then pass the --params <params-file> as an argument to Context::init. The second is to use node options.

The first gives more separation between controller manager instances, but requires a corresponding fix in controller manager so that controller node instances inherit controller manager's context.

The second shares the same global context, and works without modification if the controller parameter yaml ensures to specify the params_file (even if the same file).

What do you guys think?

[destogl]

@lkm1321 can we go with the 2nd proposal? Less changes in upstream packages is better. Execpt this will help you to run multi-robot setup on the hardware, but I assume that you have different machine for each HW.

[lkm1321]

Sounds good. I'll clean up towards the second option. I'll be sending in the context inheritance PR to ros2_control anyway and see if they'd like to merge that, so the first option may be viable in the future.

Which branch should I point the PR to? I noticed that ign_ros2_control was renamed to gz_ros2_control after humble.

[azeey]

I just commented on #166 (comment) about using rclcpp::NodeOptions. This will potentially unblock using the new GzServer ROS Node (gazebosim/ros_gz#500) with gz_ros2_contorl 🤞🏾. @lkm1321 any progress on this?

[github-actions]

This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete.