Review how to provide controller parameters

[v-lopez]

The current implementation summarized:

ControllerManager allows undeclared parameters here

If a controller with name arm_trajectory_controller must be loaded using the service interface, there must exist a /controller_manager/arm_trajectory_controller.type parameter with the type of the controller, so the controller manager can read it and instantiate the proper class.

On the other hand, there's also the parameters required by the controller itself.

Controllers are loaded and configured on the load_controller service, so in the namespace of the arm_trajectory_controller, there must be the parameters necessary to configure it by the time it is loaded.
But the controller node doesn't exist before loading the controller, so it's not possible to load them beforehand.

Maybe we could use SubNodes? In any way, the controllers should be within the namespace of the controller manager they are going to be loaded into.

Some related discussion here: https://github.com/ros-controls/ros2_control/pull/139/files#r491415213

[destogl]

I did some kind of "proposal" YAML during development of #147.

Here the out for the discussion:

control_manager:
  ros__parameters:
    controllers:
      - forward_command_controller_position

    forward_command_controller_position:
      type: forward_command_controller/ForwardCommandControllerNewComponents

forward_command_controller_position:
  ros__parameters:
    joints:
      - joint1
      - joint2
    interface_name: position

I am not very fond of this solution since there is a controller's name three times in the YAML.
I tend to be restrictive regarding parameters so we should not auto-declare them, but do it explicitly.

[v-lopez]

@destogl But in this proposal, at which point in time do you load the yaml?

Right now what happens is:

1. Start controller_manager
2. Load Controller "forward_command_controller_position" via service call for instance
3. Controller is configured as part of the load, reads parameters.
4. Start controller via service

The problem is that parameters can only be set after 2., because prior to that the controller node did not exist. But 3. occurs in the same operation and fails.

Possible solutions:
a) Split load into load and configure. And you have to set the parameters between load and configure. I don't like this at all.
b) Use the same approach as navigation2 Layers, in which then plugins are not nodes, and all of them read the params from the same Node. The downside of this is that controllers no longer have their unique LifecycleNode
c) Not sure if the SubNode approach I mentioned above would allow us to "store" the params in the main node, inside a namespace, that is later used by the SubNodes. Also create_sub_node exists for rclcpp::Node but not for rclcpp::LifecycleNode.
d) The params are declared in the controller_manager's node and read from there, but then the controller has its own node for the rest of stuff.
This has become quite critical as we cannot test a controller that takes any parameter right now, we're moving forward hard coding params in the code for now.

@bmagyar @Karsten1987 I'd love your take on this.

[Karsten1987]

From reading through this, there's really two problems here:

1.) How to load a controller by name

If a controller with name arm_trajectory_controller must be loaded using the service interface, there must exist a /controller_manager/arm_trajectory_controller.type parameter with the type of the controller, so the controller manager can read it and instantiate the proper class.

Is there a chance to enhance the service call to take not only a controller name but also it's appropriate type? I don't see a big drawback to this as I believe the type is just as well known as the controller name at the time of loading.

2.) How to propagate parameters to the controllers
At the time of the first edition of ros2_control we've opted for a dedicated parameter node. That was a simple solution to emulate the global parameter server of ROS1. And it actually worked pretty well in which each controller wouldn't get the parameters on their own node but from a central location.

From the options you've listed I think we could get it to work with a union of (a) and (d). Let's say we load a general parameter file into the controller manager's node, the controller manager could internally hold a map of parameters for each controller and forwards these onto the controller's namespace between loading and configure.
I haven't tried it yet, but I don't know if a node can easily fetch all parameters from a yaml file even if they are not addressed for their own node or if we have to manually parse the yaml file. But that's one approach I could see.
If that's too much to cope with though, I'd be okay to hold all parameters in the controller manager's namespace and let each controller fetch their appropriate parameters from the controller manager's node instead of their own explicitly.

Does this make sense?

[v-lopez]

Regarding 1) the service could be changed to take the type as an input, it will be less convenient for people who need to be stopping and starting controllers continuously, that for sure is us and some of our customers. If the parameter's are going to be in a central location as suggested in 2) I'd keep it like that.

2. I'd go for your second suggestion, to let controllers read parameters from the controller_manager Node, not because it's too much to cope, but because if there are some parameters that are going to be dynamically changed, we'd have to implement some refresh behavior on the controller_manager.

What are your thoughts? I could start to draft this later this week.

[bmagyar]

I've been thinking about this for the last couple of days and agree with declaring it all parameters in the controller_manager namespace. It's not ideal as we lose some nice online configurability but when the choice is between that and not having parameter handling or having to pass in redundant information, I am supporting to propagate parameters to the controllers from the controller manager namespace.

@v-lopez please go ahead.

[Karsten1987]

if there are some parameters that are going to be dynamically changed, we'd have to implement some refresh behavior on the controller_manager.

Don't you have to do exactly this when parameters are going to be changed? ROS2 gives you a nice way of subscribing to parameter changes (you get a callback). If you force each controller node to exclusively look for their parameters on another node (aka the controller manager node) you'll lose that one.

[v-lopez]

I was thinking about how you'd read those parameters from an intermediate structure provided by the controller manager, so unless that controller manager actively updates this structure you'd be using outdated params, but I didn't think about that parameter changes callback.

I'll get back to you once I've worked a bit more on it.

[bmagyar]

@Karsten1987 and this is what I meant by not focusing on this for now. Better lose niceties in the short term than not having a short term.

[Karsten1987]

I wouldn’t consider this a nice thing to have. It’s fundamentally different from what all the ros2 parameter tutorials would tell you how to deal with parameters. We at least have to make this clear that the regular ros2 concepts don’t apply here. And that concerns me a bit.

…

On Nov 11, 2020, at 6:20 AM, Bence Magyar ***@***.***> wrote:  @Karsten1987 and this is what I meant by not focusing on this for now. Better lose niceties in the short term than not having a short term. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

[v-lopez]

Please take a look at #234.

I've added the callback thing, so the parameters are updated on the controller's node, I believe operation will not be different in general.

One thing that will differ, is that if someone updates directly the controller's parameters, the controller_manager copy of the parameters will not be updated.
If needed we could add a callback in that direction as well, not sure how often this situation will occur.

[Karsten1987]

@v-lopez how did you test the current master branch?

I've tried to load a test controller via the service call and it immediately crashes:

 ➭ ros2 run controller_manager ros2_control_node --ros-args -p update_rate:=10 -p "robot_description:=`cat install/test_robot_hardware/share/test_robot_hardware/test_robot_hardware.urdf`" -p "joint_state_controller/JointStateController.type:=joint_state_controller::JointStateController"
[ERROR] [1605250223.552178696] [rcl]: Failed to parse global arguments
libc++abi.dylib: terminating with uncaught exception of type rclcpp::exceptions::RCLInvalidROSArgsError: failed to initialize rcl: Couldn't parse parameter override rule: '-p joint_state_controller/JointStateController.type:=joint_state_controller::JointStateController'. Error: Expected lexeme type 22, got 19 at index 22, at /Users/karsten/workspace/ros2/ros2_foxy/src/ros2/rcl/rcl/src/rcl/lexer_lookahead.c:240, at /Users/karsten/workspace/ros2/ros2_foxy/src/ros2/rcl/rcl/src/rcl/arguments.c:327

given a name with a / makes the parameter name invalid from what I get from the console output. How am I supposed to make this work?

[Karsten1987]

Then further, even if I hardcode the type within the controller manager, the instantiation of the node crashes with the following:

[INFO] [1605251330.738224404] [controller_manager]: Loading controller 'joint_state_controller/JointStateController'

joint_state_controller/JointStateController
test_controller
libc++abi.dylib: terminating with uncaught exception of type rclcpp::exceptions::InvalidNodeNameError: Invalid node name: node name must not contain characters other than alphanumerics or '_':
  'joint_state_controller/JointStateController'

I feel like the loading mechanism is currently broken :(

[v-lopez]

What's the branch you are using for getting the urdf?
Anyway, the name of the controller cannot have /.
Shouldn't param be: -p "joint_state_controller.type:=joint_state_controller::JointStateController"

[Karsten1987]

I am working on this PR: #236

but yes, you're right. The name of the controller here got me confused. It's essentially the node name of the controller, not the name of the plugin being exported via pluginlib.xml.

Running it with your suggestion make it work. The controller I am using is here: ros-controls/ros2_controllers#109

[destogl]

@destogl But in this proposal, at which point in time do you load the yaml?

@v-lopez: I am loading them in the launch file before of anything else.
I will try got get a demo work in the next days, so you can check "hands-on" the idea.

[destogl]

The problem is that parameters can only be set after 2., because prior to that the controller node did not exist. But 3. occurs in the same operation and fails.

@v-lopez: I just look again at this issue and corresponding PR #234
My first answer were probably confusing to you, as well as this issue to me :)
In all my tests I didn't experience it at all. Not at #147 nor in the new version demo explained here.

I am using this yaml, loaded when starting the ros2_control_node via launch file:

controller_manager:
  ros__parameters:
    update_rate: 2  # Hz

    forward_command_controller_position:
      type: forward_command_controller/ForwardCommandController

    joint_state_controller:
      type: joint_state_controller/JointStateController

forward_command_controller_position:
  ros__parameters:
    joints:
      - joint1
      - joint2
    interface_name: position

Are you trying to get the same YAML format as in ROS1? E.g.:

controller_manager:
  ros__parameters:
    update_rate: 2  # Hz     

forward_command_controller_position:
  ros__parameters:
    type: forward_command_controller/ForwardCommandController
    joints:
      - joint1
      - joint2
    interface_name: position

 joint_state_controller:
  ros__parameters:
    type: joint_state_controller/JointStateController

[v-lopez]

My guess is that if the parameters are loaded as part of the launch file, they are stored in a file in /tmp, and each time you instantiate a Node inside that application, it loads the parameters from the file if there are any.

But if you run using a launch file, a controller_manager with some default controllers with their parameters, and later you want to load a different controller, you cannot set the parameters of the new controller until its Node exists, and then the situation exposed here occurs.

And just to anticipate any other question, that is one of our main use cases and of our customers, they developer their controllers and load then dynamically in ROS1 without having to edit our launch files.

[destogl]

But if you run using a launch file, a controller_manager with some default controllers with their parameters, and later you want to load a different controller, you cannot set the parameters of the new controller until its Node exists, and then the situation exposed here occurs.

Thanks, I think I am getting it now...

So, you are using the same name for the controller?
This would explain that you need type parameter in the ControllerInterface class

[v-lopez]

Yeah, the most reduced example is that you start a controller_manager without controllers configured.

Later, you set the parameter controller_manager.joint_trajectory_controller.type to "joint_state_controller/JointTrajectoryController". And you need to be able to load the rest of the parameters in the joint_trajectory_controller namespace before loading the controller itself, or the configure will fail.