ros2_control icon indicating copy to clipboard operation
ros2_control copied to clipboard

use_sim_time handling in ros2_control and controllers

Open v-lopez opened this issue 4 years ago • 10 comments

Since there's no global param server anymore, all nodes need to manage their use_sim_time parameter.

Failure to do so will lead to nodes publishing messages with different times from the rest of the system, different real time factors and other stuff.

The simple solution is to set it on each controller parameter file, as well as on the controller_manager node.

I was hoping we could agree on a solution that doesn't imply that we have to duplicate all controller parameters files for simulation/non-simulation.

In navigation2 the main launch files takes use_sim_time as a launch argument and passes it to other launch files. This other launch files overwrite the use_sim_time of all the nodes' parameter files.

In our case, the controllers might not be started all from the same launch file. I was going to suggest that the controller_manager could copy its use_sim_time value to the controllers when configuring them, if the controllers don't have a value set.

v-lopez avatar Feb 10 '21 17:02 v-lopez

My 2c, I think launch argument is a good plan. Setting a launch argument in a root launch file also affects included launch files so a variable set in the root can set the use_sim_time for all launch files in the launch hierarchy. I use this for Gazebo launch settings.

guru-florida avatar Feb 15 '21 16:02 guru-florida

The problem with launch argument is that in the way the controller_manager is launched with gazebo, I don't think it inherits parameters from a file in the same way as from a launch file.

If I am not mistaken, when you launch controller_manager via the launch Node tag, it launches the executable with a --params-file ... argument. And if in this file, there's a wildcard namespace (**/), nodes launched within it also read those params.

But gazebo creates the controller manager in the code, and loads the parameters manually, not via such a file. @ahcorde is this correct?

v-lopez avatar Feb 15 '21 16:02 v-lopez

Yes, I believe you are correct.

guru-florida avatar Feb 15 '21 17:02 guru-florida

As discussed on March 10th WG meeting, if the controller manager has use_sim_time set, it will overwrite the controller's parameter when loading them. An INFO message should be printed when this happens.

v-lopez avatar Mar 10 '21 18:03 v-lopez

But gazebo creates the controller manager in the code, and loads the parameters manually, not via such a file. @ahcorde is this correct?

just confirming that I understand correctly and that there is currently no "clean" way to set use_sim_time for all controllers through the controller_manager when gazebo is starting the controller_manager, and that the workaround is to set use_sim_time in the yaml for each controller individually?

shonigmann avatar Jul 13 '21 19:07 shonigmann

Currently this is not implemented, and you have to set the controller param manually yes.

But this should not be very difficult to implement if someone had the time

v-lopez avatar Jul 14 '21 07:07 v-lopez

Thanks for the quick reply. I may have some time to take a look and propose a fix.

Though I wonder if its not more direct to fix this in gazebo_ros2_control by setting use_sim_time=True when gazebo_ros2_control creates the controller_manager? Is there ever a case where you would not want sim_time when using Gazebo?

Then a small addition to spawner.py could allow the controllers to match the use_sim_time setting from the controller_manager since they wait for it to be available anyways.

But maybe I'm missing a use case where you need mixed clock usage and that would be too restrictive?

shonigmann avatar Jul 14 '21 17:07 shonigmann

I don't think mixed clock usage is needed.

gazebo_ros2_control should set use_sim_time=True for the controller_manager, but the controller_manager should set it for the controllers it loads.

The spawner is only one of the ways of loading controllers, so probably it should not be the responsible of setting this.

v-lopez avatar Jul 14 '21 20:07 v-lopez

For reference, I've submitted a PR that I think addresses this issue

shonigmann avatar Jul 17 '21 18:07 shonigmann

I believe this issue can now be closed

shonigmann avatar Jul 22 '21 15:07 shonigmann