ros2_rust icon indicating copy to clipboard operation
ros2_rust copied to clipboard

Add parameter services

Open luca-della-vedova opened this issue 2 years ago • 1 comments

This PR builds on top of the basic parameter implementation and adds (to my knowledge) fully functional parameter services. Note that parameter callbacks are still not implemented so, for example, users have no way to register a callback to react to parameter change events when using services to set values, even though the value itself will be updated and can be fetched through a parameter.get() call. I didn't go into the rabbit hole of dealing with updates to the use_sim_time parameter to change the clock type at runtime between simulated and wall clock, but we should probably figure out how to do it soon, probably when we have such callbacks.

I tested this manually with all the use cases I could think of and it seems to work fine. I also added a fair bit of integration tests under the rclrs_tests and they all pass, the majority of this PR is actually test code.

Overview

There was already a skeleton for parameter services with empty callbacks, this PR fills it up by creating the actual service implementations. There are some loose ends that I believe could be cleaned up (I'd like to address at the very least all the TODO(luca) in the code, any hint is welcome!) but the bulk of the functionality should be there.

Test it!

Since use_sim_time has been introduced a very simple test can be done by running any example and doing:

ros2 param list
# Should return `use_sim_time`
ros2 param set /minimal_subscriber use_sim_time 42
# Should fail
ros2 param set /minimal_subscriber use_sim_time true
# Should succeed and set the parameter to true
ros2 param get /minimal_subscriber use_sim_time

Otherwise you can declare your own node with parameters, or just look at the tests under rclrs_tests/src/parameter_service_tests.rs

luca-della-vedova avatar Nov 07 '23 10:11 luca-della-vedova

Note that apart from the TODOs in the PR there is an additional gotcha. For the sake of simplicity (and because parameters were not really implemented), when adding time and clocks in #325, I left out runtime update of use_sim_time (what is currently accomplished by a parameter callback in other client libraries). The parameter is only checked, and acted on, once at node creation time. This PR would add an API for users to set parameters from the CLI ros2 param set which could cause confusing behaviors if the parameter is updated at runtime but the change is not reflected.

A true fix involves figuring out a way to trigger callbacks on parameter changes that could involve new public facing APIs and add considerable burden to the PR. My recommendation would be to set the use_sim_time parameter as ReadOnly for now and revisit in the future how we can handle runtime updates. My personal opinion is that changing the underlying clock base at runtime is a very niche (and in most cases very dangerous) approach so hopefully forbidding it explicitly in the API will offer one less footgun, until we figure out if / how we should support it.

luca-della-vedova avatar Mar 18 '24 04:03 luca-della-vedova

I need this feature! I am currently working on your parameter implementation and I have already noticed that ros2 does not show me the parameters, but I can adjust them with a launch file, as can be seen in an application example, for which you need a corresponding publisher, subscriber, and a corresponding launch file. Overall, I find the integration of ros2 parameters in ros2 rust quite successful and almost as good as with the generate_parameters library.

Guelakais avatar Apr 19 '24 10:04 Guelakais

I also addressed the loose end of having an external API that makes it easier to change use_sim_time but its changes being actually ignored by making the parameter explicitly read only and adding a TODO if we want to explore changing it at runtime in the future (which is quite a can of worms that I'm happy to push down a bit imho) https://github.com/ros2-rust/ros2_rust/pull/342/commits/00c7951778b4feddbe8329afd07dfbbaebcecbd8

luca-della-vedova avatar Apr 23 '24 03:04 luca-della-vedova

Thank you very much for all your hard work! I'm glad to see ROS 2 Rust finally get support for this!!!

jhdcs avatar May 14 '24 12:05 jhdcs