param set uri doesn't work for Player component

[KenYN]

• Relates #1419

Description

With the new component Player, I cannot change the file to play back

Expected Behavior

Given this YAML file:

launch:
    - node_container:
        pkg: rclcpp_components
        exec: component_container
        name: my_container
        namespace: ''
        composable_node:
            -   pkg: rosbag2_transport
                plugin: rosbag2_transport::Player
                name: bag_player
                namespace: ''
                param:
                    - name: uri
                      value: '/path/to/file1/'

Then the following commands:

$ ros2 param set /bag_player uri "/path/to/file2"
$ ros2 service call /bag_player/play rosbag2_interfaces/srv/Play "{start_offset: {sec: 0, nanosec: 0}, playback_duration: {sec: 65535, nanosec: 0}, playback_until_timestamp: {sec: 0, nanosec: 0}}"

(Note, Play doesn't seem to have a play_to_end value)

Should result in file2 being played back.

Actual Behavior

file1 is played back, even though:

$ ros2 param get /bag_player uri
String value is: /path/to/file2

To Reproduce

See above

System (please complete the following information)

• OS: Ubuntu 22.04
• ROS 2 Distro: rolling
• Install Method: source
• Version: patch-2

[MichaelOrlov]

@roncapat Any ideas why ros2 param set /bag_player uri "/path/to/file2" doesn't work as expected?

[roncapat]

Happy to check for sure. @KenYN thank you in the meantime for the testing you are doing on this, it is unvaluable :)

[roncapat]

Well, my first thought is that we are not handling any parameter update IMHO. Parameters are just parse one time at the beginning and stop.

Example:

storage_options.uri = node.declare_parameter<std::string>("uri", "");

after this line, storage_options.uri is not bound to any rclcpp::ParameterValue. It is a simple std::string. And nothing is done currently to support updated on the fly.

I will think a bit about how this could be done.

[roncapat]

I believe that a big shift of paradigm would be needed here to implement this. Currently, we have player_options and storage_options structures being a collection of parameter values, not rclcpp::ParameterValues. To do what you are asking, we should either inject in the already quite intricate architecture the concept of rclcpp::ParameterValue, down to classes like Reader or Writer. To avoid so, another option would be to inject instead explicit "setters" wherever needed to flow down any change of parameter (Assuming we can get notified by something that a parameter has changed).

This is just a preliminary analysis of course, feel free to add your point of view :)

An example: Reader has no way to change opened file. You need to close a Reader and open it again. And this can only happen for example when Player is stopped. Multiply this by a huge number of potentially run-time modifyable params and you get a big headache. I agree with you however that this would become more user-firendly instead of unloading/loading again a Player component.

[MichaelOrlov]

@KenYN @roncapat I think we will close this issue as unsupported by design.

@roncapat BTW as regards

(Assuming we can get notified by something that a parameter has changed).

Do you have ideas on how we can potentially get notified when the parameter got changed?

[roncapat]

There seems to be the possibility to add a callback. https://roboticsbackend.com/ros2-rclcpp-parameter-callback/

[clalancette]

There seems to be the possibility to add a callback. https://roboticsbackend.com/ros2-rclcpp-parameter-callback/

Yes there are callbacks, though I will point out that the advice in that particular link is incorrect.

In particular, you should never make state changes based on the "on_set_parameters_callback". The purpose of that callback is to validate that the parameters are valid, not to make state changes. This is important because there can be an arbitrary chain of parameter validation callbacks, so if you were to make state changes, and a callback further down the line rejected the set, your state would be out of sync with what the parameter database shows.

As of Iron, we have a callback specifically for this purpose called post_set_parameters_callback that should be used instead. See http://docs.ros.org/en/rolling/Concepts/Basic/About-Parameters.html#parameter-callbacks for some more information, and https://github.com/ros2/demos/blob/a29e65623e8ace15b8253ab33f19f3d8165c3cfe/demo_nodes_cpp/src/parameters/set_parameters_callback.cpp for an example.

[MichaelOrlov]

• Closing this issue as functionality is currently unsupported by design.