ParameterAlreadyDeclaredException for parameter position_proportional_gain

[TipluJacob]

Since PR 248 I get an rclcpp::exceptions::ParameterAlreadyDeclaredException for the parameter position_proportional_gain:

This due to my URDF setup, where I have macros, in which I have defined ros2_control tags that contain the line <plugin>ign_ros2_control/IgnitionSystem</plugin>. When the macro is called the first time, everything is fine, when it is called the second time, I get the ParameterAlreadyDeclaredException.
My macro looks like this:

<xacro:macro name="rubber_wheel_transmission" params="prefix ">
    <ros2_control name="${prefix}_wheel_joint_ros2_control" type="system">
        <hardware>
            <plugin>ign_ros2_control/IgnitionSystem</plugin>
	</hardware> 
	<joint name="${prefix}_wheel_joint">
	    <command_interface name="velocity">
	        <param name="min">-1</param>
		<param name="max">1</param>
	    </command_interface>
	    <state_interface name="position"/>
            <state_interface name="velocity"/>
	</joint>
    </ros2_control>
</xacro:macro>

My questions now are:

1. Do I use the <plugin>ign_ros2_control/IgnitionSystem</plugin> tag wrongly, so should it only be once in my complete URDF description?
2. How do I set the parameter position_proportional_gain? In my understanding it should be possible to set the parameter independently for every joint, so something like

<hardware>
    <plugin>ign_ros2_control/IgnitionSystem</plugin>
    <param name="position_proportional_gain">0.5</param>
</hardware>

[roncapat]

IMHO we may check for parameter already defined and avoid redefinition... Allow me some time to check :)

[roncapat]

Ok, my thoughts on this:

• prior to Fix #247 - explicitly declare position_proportional_gain parameter #248, I do not believe there was a way to set that parameter aside from usual rclcpp API - I mean, I am not aware of methods for changing that parameter via URDF.
  • please, correct me if I'm wrong, and we simply revert Fix #247 - explicitly declare position_proportional_gain parameter #248.
• easiest fix could be to handle the exception and skip redeclaration
  • as @TipluJacob says, it may be desiderable to set a gain for each ign_ros2_control/IgnitionSystem instance, and this solution would not solve this case
• the reason the parameter gets redeclared is that if you see this diff even the previous implementation relied on rclcpp parameter API on the same Node instance (the one declared in these lines) so the current architecture IMHO would not help in having a different gain parameter per each system.

Any thoughts or counter-deductions? I may have missed something.

[MoffKalast]

I'm also seeing the following segfault on newly upgraded machines running the turtlebot4 sim after the recent humble package sync, seemingly caused by the same change:

ruby $(which ign) gazebo-1] Stack trace (most recent call last):
[ruby $(which ign) gazebo-1] #19   Object "[0xffffffffffffffff]", at 0xffffffffffffffff, in 
[ruby $(which ign) gazebo-1] #18   Object "ign gazebo gui", at 0x55fca60091c4, in _start
[ruby $(which ign) gazebo-1] #17   Object "/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8bb1429e3f, in __libc_start_main
[ruby $(which ign) gazebo-1] #16   Object "/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8bb1429d8f, in 
[ruby $(which ign) gazebo-1] #15   Object "ign gazebo gui", at 0x55fca600917e, in 
[ruby $(which ign) gazebo-1] #14   Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb18a8e19, in ruby_run_node
[ruby $(which ign) gazebo-1] #13   Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb18a5317, in 
[ruby $(which ign) gazebo-1] #12   Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb1a3a30c, in rb_vm_exec
[ruby $(which ign) gazebo-1] #11   Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb1a34c96, in 
[ruby $(which ign) gazebo-1] #10   Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb1a31fc5, in 
[ruby $(which ign) gazebo-1] #9    Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb1a2fc34, in 
[ruby $(which ign) gazebo-1] #8    Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb197ba27, in 
[ruby $(which ign) gazebo-1] #7    Object "/lib/x86_64-linux-gnu/libruby-3.0.so.3.0", at 0x7f8bb18a8db5, in ruby_stop
[ruby $(which ign) gazebo-1] #6    Object "/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8bb144560f, in exit
[ruby $(which ign) gazebo-1] #5    Object "/lib/x86_64-linux-gnu/libc.so.6", at 0x7f8bb1445494, in 
[ruby $(which ign) gazebo-1] #4    Object "/lib/x86_64-linux-gnu/libQt5Qml.so.5", at 0x7f8ba9e9c64c, in 
[ruby $(which ign) gazebo-1] #3    Object "/lib/x86_64-linux-gnu/libQt5Qml.so.5", at 0x7f8ba9eaa611, in 
[ruby $(which ign) gazebo-1] #2    Object "/lib/x86_64-linux-gnu/libQt5Qml.so.5", at 0x7f8ba9ec544c, in QQmlPropertyCache::~QQmlPropertyCache()
[ruby $(which ign) gazebo-1] #1    Object "/lib/x86_64-linux-gnu/libQt5Qml.so.5", at 0x7f8ba9ec5108, in QQmlPropertyCache::~QQmlPropertyCache()
[ruby $(which ign) gazebo-1] #0    Object "/lib/x86_64-linux-gnu/libQt5Qml.so.5", at 0x7f8ba9e60ff0, in 
[ruby $(which ign) gazebo-1] Segmentation fault (Address not mapped to object [0x7f8b73562f80])
[ERROR] [ir_intensity_vector_publisher-34]: process has died [pid 392740, exit code -11, cmd '/home/user/colcon_ws/install/irobot_create_nodes/lib/irobot_create_nodes/ir_intensity_vector_publisher --ros-args -r __node:=ir_intensity_vector_publisher -r __ns:=/ --params-file /home/vid/colcon_ws/install/irobot_create_common_bringup/share/irobot_create_common_bringup/config/ir_intensity_vector_params.yaml --params-file /tmp/launch_params_1yw4mkm8'].

It took me a while to figure out the package that was causing it, but I can confirm that it's commit
501be7f that's the cause, if I compile at 2c3c46f it's all sorted.

I'm not entirely sure what the sync protocol is, but I don't suppose there's a way to push a hotfix deb to the OSRF apt repo after this is reverted, so this doesn't stay broken until the next sync? I sort of have over fifty students over here whose simulation will be effed if they ever run apt upgrade and will need to manually clone and compile this to get it working again.

[TipluJacob]

I honestly don't have a perfect overview about the functionality, but from a user experience point of view I would prefer if the parameter position_proportional_gain could be set via the URDF and would be read in like done here for example. But there may be reasons against it, I don't know.
For now I agree that an easy fix would be to handle the exception and skip redeclaration.

[roncapat]

@TipluJacob I will try to propose a patch soon

[joanvallve]

Is there any estimate of when debian packages will be updated with this fix? Thanks