Add parameter services

[luca-della-vedova]

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]

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.

[mxgrey]

I think this is generally a very good implementation of parameter services.

I've left some comments, many of which aren't really actionable in this PR but more to stir a conversation and see if anyone else has thoughts about how to improve quality in the future.

Some comments do require action, though. The way the services respond to requests that include unknown parameter names does not match up with the ROS 2 documentation, which I link to in my inline comments. It should be very easy to correct that in the code, but it's a significant correctness detail that we shouldn't overlook.

[mxgrey]

As usual I'm uncomfortable with unwraps, but I understand it's an awkward situation because it's very unclear what the behavior should be here if we get an Err here since that means the mutex is poisoned and the parameter value may be garbage.

One argument in favor of a simple .unwrap() is that we don't give the user write access to this mutex, so there is virtually no possibility of this mutex being poisoned. This mutex will only be write-unlocked in the set function which has no discernible way to panic.

If that's the logic we want to rely on here, we should document it at each of these unwraps, both for our own benefit (a reminder of the safety invariants that we need to uphold) and as reassurance to anyone auditing the code that we're not doing careless unwraps.

[jhdcs]

I think documenting the unwraps is a reasonable idea, though this may mean that we need to dig through our already-written code to make sure any other unwraps are in compliance with these requirements.

[luca-della-vedova]

Added a comment for this new unwrap 5da50a2. I agree that there are a lot of unwraps throughout the libraries though, and that many of them are mutex related

[mxgrey]

A disadvantage of taking Arc<str> here is that it forces a heap allocation to always happen when it might not be necessary (I believe the BTreeMap could search for the entry using &str instead of Arc<str>), but then if we use &str we don't get the benefit of the very convenient Entry API.

This is probably a micro-optimization that we don't need to waste time worrying about right now, but it might create friction for users that want to very strictly avoid unnecessary heap allocation. We can worry about them in the future though.

[jhdcs]

Agreed. If and when we implement benchmark tests for rclrs we can decide if such an optimization is needed. For now, I think that getting parameter services up-and-running in a usable state takes priority.

[jhdcs]

Just a couple minor tweaks, but looks good otherwise!

[jhdcs]

Are we all ok with adding a test dependency? Personally, I feel it's all right, just want to make sure the other maintainers are cool with it.

[luca-della-vedova]

I personally don't see any other way to do it, if we want to test clients and services we need some sort of async runtime. I am personally not very familiar with tokio so it wouldn't have been my first choice but I saw that it was the framework of choice for the services example so I stuck to it for consistency.

[mxgrey]

I don't see any reason to be shy about test dependencies if they're able to add any amount of value to our ability to have worthwhile tests. Especially a dependency as well vetted as tokio.

I could understand hesitation if it were a dependency on the actual package because that could limit what platforms are able to use rclrs, but test dependencies don't have any negative downstream impacts.

[jhdcs]

I think documenting the unwraps is a reasonable idea, though this may mean that we need to dig through our already-written code to make sure any other unwraps are in compliance with these requirements.

[jhdcs]

Agreed. If and when we implement benchmark tests for rclrs we can decide if such an optimization is needed. For now, I think that getting parameter services up-and-running in a usable state takes priority.

[Guelakais]

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.

[luca-della-vedova]

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) 00c7951

[maspe36]

A few very small things, thanks for your patience on the review!

[maspe36]

I'm not really understanding this, shouldn't the executor eventually need handles to these services?

[luca-della-vedova]

I'm a bit fuzzy on the details since it's been a few months since I wrote this but I believe that the following happens:

• You call create_service, it returns a strong reference but holds a weak reference internally.
• The executor's spin_once call creates a new WaitSet that waits and collects ready entities.
• The new_for_node wait set function, collects the subscriptions by using the live_* functions, that iterate through all Weak objects and try to upgrade them to a strong reference, returning all valid pointers.

The result I believe, is that the caller of create_service must hold on to a strong reference otherwise that service will be dropped. However, the node itself will store its own Weak reference and the strong reference will actually never be used at all, but we must still be sure to keep the service alive for the necessary time.

[maspe36]

Ahh, I see! Thanks for the clarification.

Is this TODO still accurate then? Seems like because we will drop these strong references when the node is dropped that our lifetimes are aligned.

ros2_rust/rclrs/src/node.rs

Line 273 in 875bda3

// TODO: make service's lifetime depend on node's lifetime

[luca-della-vedova]

I haven't introduced that TODO but for my understanding it would still be accurate.
Since the strong reference is returned by the create_* call but the node only holds a weak one, what could actually happen is something like this (pathological case):

let mut sub;
{
    let node = create_node([...]);
    sub = node.create_subscription([...]);
    // Node is dropped!
}
// sub still has an owning reference, what now?

[maspe36]

Ahh, I see! Thanks for the clarification.

Is this TODO still accurate then? Seems like because we will drop these strong references when the node is dropped that our lifetimes are aligned.

ros2_rust/rclrs/src/node.rs

Line 273 in 875bda3

// TODO: make service's lifetime depend on node's lifetime

[jhdcs]

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