Avoid redeclaration of parameters (automatically declared parameters)

[livanov93]

PR addresses #149. Parameters are needed for checks within jtc.
As mentioned here in the future adjusting detailed per joint parameters could be done dynamically.

EDIT:
PR enables existence of parameters uploaded via overrides (e.g. yaml files). It is blocked by #ros2_control/PR504

[destogl]

Are you sure this didn't work before? Controllers should have auto-declaration of parameters enabled...

Would be nice to get some test for this, showing what exactly wasn't working.

[livanov93]

Pretty sure, I will try to make tests to show the problem.

[destogl]

This line enables auto-declaration of parameters. That's the reason I am asking...

[Ace314159]

I can confirm that this is needed. If you change the log level to debug, it will log the parameters. Regardless of the parameters specified, it is always 0.

[bmagyar]

Ok. We may need to revisit this in Galactic but if it's blocking on Foxy, I'm tempted to merge it now

[bmagyar]

Speaking too soon perhaps :D

@livanov93 @Ace314159 , what ROS distro are you guys running?
The tests came back with this:

 - joint_trajectory_controller.PositionVelocityAccelerationTrajectoryControllers/TrajectoryControllerTestParameterized correct_initialization_using_parameters/1
  <<< failure message
    unknown file
    C++ exception with description "parameter 'constraints.joint1.trajectory' has already been declared" thrown in the test body.
  >>>
- joint_trajectory_controller.PositionVelocityAccelerationTrajectoryControllers/TrajectoryControllerTestParameterized correct_initialization_using_parameters/2
  <<< failure message
    unknown file
    C++ exception with description "parameter 'constraints.joint1.trajectory' has already been declared" thrown in the test body.
  >>>

Is something wrong with how we test this perhaps? Or some bug in rclcpp that makes it work with the test setup but not otherwise?

[livanov93]

@bmagyar
I am on foxy, however f996475 will fix the test passing where configure is called multiple times (like here and here) within the same test. The controller should be configurable multiple times definitely, so this commit tackles that problem.

Still I think that there is a misunderstanding of allow_undeclared_parameters in controller_interface.
In documentation we can find that this is enough for putting the parameters into node's namespace without declaration.
If you take a closer look few lines on a flag automatically_declare_parameters_from_overrides and its documentation it says if it is false the parameters from overrides (yaml files) won't appear even though allow_undeclared_parameters is true.

There is a small discussion on the documentation improvements here and here.

So in my understanding controller_interface should have allow_undeclared_parameters and automatically_declare_parameters_from_overrides to true and then in each instance of controller (e.g. joint_trajectory_controller, JointGroupVelocityController...) we should check if node has some parameter or not, and then act accordingly.

[bmagyar]

Using automatically_declare_parameters_from_overrides sounds like a better fix than this.
I vaguely remember something about removing that for some reason... let's check

[bmagyar]

ros-controls/ros2_control#276

It seems this was removed due to some error report from you earlier ;) could you test locally that fixing in controller_interface.cpp actually works?

[livanov93]

Yes, it is correct. Still I realized we had wrong approach in ros_controls/ros2_control#276. We just had to add check if some parameter already exist before its declaration in all of the controllers (everything that inherits from controller_interface). Something like this.
I will test it, but I am also sure that we need change in controller_interface (put automatically_declare_parameters_from_overrides to true ) and in all controllers add check if any parameter exists before its declaration.

[destogl]

Looks good and sensible chnage! Thanks!