#664 breaks `bridge_params` argument in launch files

[Amronos]

With #664 the bridge_params argument added to various launch files in #628 will no longer work in them.
The launch files need to be updated to work with the changes made in #664 or the bridge_params argument has to be removed from them.

[azeey]

Ah, yes, #664 does break passing bridge_params through from a launch file to RosGzBridge. I'm wondering if we do want to support this use case. Generally, in ROS, it is typically not possible to include a launch file that spawns a node and set the ROS parameters of the node. Consider thef following launch file

<!-- example.launch.xml -->
<launch>
  <node name='my_node' pkg='some_pkg' exec='some_exec'>
    <param name="p1" value="true"/>
  </node>
</launch>

If you want to include that launch file from somewhere, like so:

<!-- consumer.launch.xml -->
<launch>
  <include file="example.launch.xml">
  </include>
</launch>

There is no direct way for us to set the parameters of my_node from consumer.launch.xml. You can do it by using <set_parameters_from_file>, but that would involve creating a separate file.

#628 works around this issue by passing a string to bridge_params, but I question how robust that workaround is. For example, does it support substitutions in the values of the parameters?

I can see if I can restore part of #628 and still parse <param> tags and try to answer the question above.

[azeey]

I checked out #628 and tried setting other parameters through bridge_params and it doesn't seem to work

<launch>
  <let name="expand_topics" value="true"/>
  <include file="$(find-pkg-share ros_gz_bridge)/launch/ros_gz_bridge.launch">
    <arg name="bridge_name" value="br1" />
    <arg name="config_file" value="$(find-pkg-share ros_gz_sim_demos)/config/air_pressure.yaml" />
    <arg name="bridge_params" value='"qos_overrides./air_pressure.publisher.reliability": "best_effort", "expand_gz_topic_names": $(var expand_topics)' />
  </include>
</launch>

Prints the error:

[bridge_node-1] terminate called after throwing an instance of 'rclcpp::exceptions::InvalidParameterTypeException'
[bridge_node-1]   what():  parameter 'expand_gz_topic_names' has invalid type: Wrong parameter type, parameter {expand_gz_topic_names} is of type {bool}, setting it to {string} is not allowed.

Note that even setting "expand_gz_topic_names": True or "expand_gz_topic_names": "True" doesn't seem to work.

[azeey]

Okay, I think the bridge_params work around introduced in #628 is problematic as I have mentioned above. My proposal is that remove all bridge_params launch arguments from all our launch files. Users will have two ways to set these parameters:

1. Use <ros_gz_bridge> or RosGzBridge directly in their launch files and use the <param> or parameters arguments (with Change bridge_name to name and bridge_params to parameters #673).

2. Use <set_parameter> or SetParameter launch actions.
   Examples:
   XML:

   <launch>
     <set_parameter name="qos_overrides./air_pressure.publisher.reliability" value="best_effort" />
     <set_parameter name="expand_gz_topic_names" value="true" />
     <include file="$(find-pkg-share ros_gz_bridge)/launch/ros_gz_bridge.launch">
       <arg name="bridge_name" value="br1" />
       <arg name="config_file" value="$(find-pkg-share ros_gz_sim_demos)/config/air_pressure.yaml" />
     </include>
   </launch>  

   Python:

   from launch import LaunchDescription
   from launch.actions import DeclareLaunchArgument, IncludeLaunchDescription
   from launch.launch_description_sources import PythonLaunchDescriptionSource
   from launch.substitutions import LaunchConfiguration
   from launch_ros.substitutions import FindPackageShare
   from launch_ros.actions import SetParameter
   
   def generate_launch_description():
   
       return LaunchDescription(
           [
               SetParameter(name="qos_overrides./air_pressure.publisher.reliability", value='best_effort'),
               SetParameter(name="expand_gz_topic_names", value='true'),
               IncludeLaunchDescription(
                   PythonLaunchDescriptionSource([ FindPackageShare("ros_gz_bridge"), "/launch/ros_gz_bridge.launch.py"]),
                   launch_arguments={
                       'bridge_name': 'br1',
                       'config_file': [FindPackageShare("ros_gz_sim_demos"), "/config/air_pressure.yaml"],
                       }.items(),
               )
           ]
       )

   Note that, these would set the parameters for all nodes in a given context, so it might be necessary to use Group to group the SetParameter and IncludeLaunchDescriptions` so that it doesn't affect other nodes.

@Amronos, @caguero, @ahcorde thoughts?

[Amronos]

IMO, #664 makes the RosGzBridge action act more like a node than an action. Initially, in #628, the reason for shifting the logic to the action was the bridge_params argument that PR introduced. Now, it feels like we are making the action a copy of bridge_node. With #664 I would personally use the bridge_node only instead of the action in Python launch files (this was the case before #628). #664 does defeat one of the purposes of #628 which was to allow passing parameters to the bridge from the launch file's arguments, the other purpose being the ability to pass parameters to the bridge from XML launch files which they weren't able to do before as they used the action.
Also, the problem with not being able to pass boolean parameters through bridge_params is fixed with #658.

[Amronos]

@azeey, I think we can continue merging #672. In the future, a way could be figured out to implement passing parameters to the bridge from the launch file's arguments while also having the parsing of #664.
There are many great bug fixes and features which should be released into Jazzy and this issue shouldn't block them.

[Amronos]

Since many releases have already been made into Jazzy without #672, I will close this issue.