Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for multiple parameter change callbacks #296

Closed
rohbotics opened this issue Dec 22, 2016 · 2 comments
Closed

Support for multiple parameter change callbacks #296

rohbotics opened this issue Dec 22, 2016 · 2 comments
Labels
enhancement New feature or request

Comments

@rohbotics
Copy link
Contributor

This will be long, and might be better suited for the discourse, let me know.

Currently rclcpp only supports registering one param_change_callback, and if you try to register a second one, it overwrites the first callback.

The problem with having multiple callbacks is supporting atomically setting parameters. Currently the callbacks that are registered do two things, verify that the parameters are within any constraints, as well as update the operation of the node to work with the new parameters (if verification passed). With only one callback per node, this all works fine, and the parameters are updated atomically across the node.
(Ex of current callback: https://github.com/rohbotics/turtlebot2_demo/blob/0f33a3c3caadd13fe89be060690d6e4b797cd2ba/turtlebot2_drivers/src/kobuki_node.cpp#L42)

With multiple callbacks, this approach doesn't work any more, the parameters can only be atomic per registered callbacks. Here are some proposals I have to fix this.

A: Namespacing

Figure out how we are going to namespace parameters within the node, and then only support atomic operations on parameters within a single namespace, and allow for one callback per namespace.

Pros: Easy to interface with at an API level.
Cons: Pretty restrictive, could be hard to implement cleanly/correctly

B: Seperation of Concerns

After discussing with @tfoote, this is the approach that we came up with. The user will provide two functions, one that only verifies the parameter updates, which provides another function that will actually execute updating the operation of the node with the new parameters. This could be made easier with dynamic-configure ish code generation.

Pros: Keeps atomicity support everywhere
Cons: Could be messy from a user perspective (without code gen)

C: Make Atomicity Optional

Not every node requires support for atomically setting a group of variables, so make support for it optional. For nodes that want support they can use approach B, and those who don't can use basic callbacks that verify and set parameters.

Pros: Easy to add parameters without worrying about atomicity
Cons: Might make nobody bother with atomic parameter updates


Footnote: there should also be a way to de-register callbacks if we support multiple

@rohbotics rohbotics added the enhancement New feature or request label Dec 22, 2016
@wjwwood
Copy link
Member

wjwwood commented Dec 23, 2016

Thanks for starting the discussion @rohbotics, at some point this might belong on the parameter's design doc (once we have somewhat of a consensus and a list of the possible options).


I think option B is the most flexible but also the most complicated to use. However, I am actually leaning towards A. I think A becomes necessary anyways to allow group verification of some parameters but not others.

You could also implement A+B, where the process is still split into pre/verification and post parameter change (B) but also support registering callbacks to parts of the namespace (A).

I might be wrong, but I don't think that option C avoids this issue. Even if you're non-atomically setting a single parameter, you can still have multiple callbacks for verifying that parameter, any of which can fail and prevent the actual change of the parameter.


We could also make it so that you can only have one "verifying" callback (a callback that can fail and therefore block the parameter change) but many "notified" callbacks (callbacks that cannot prevent the change). Then you always run the verifying callback first, and if it succeeds would then call the other callbacks after that. You'd have to combine this with option A to allow for granularity and you'd have to prevent overlapping of the regions of the different verification callbacks. Basically, each parameter can only be associated with exactly one verification callback. That's a big constraint, but I think it's the best trade-off you can get without requiring a separate set of verification callbacks and notification callbacks.

The idea behind this is that the verification callback would both verify and react to the parameter change in order to keep it simple. I think that the common case will be to have only one verification callback that verifies and reacts all at once. And then you could use one of the notification callbacks for tools and logging, since they only need to observe the change and don't care about the verification part.

I think the viability of this alternative comes down to whether or not you think that a single parameter needs more than one verification callback at a time. Personally, I think that's not a common use case, and in the rare cases where it is, it could be accomplished by the user writing a verification function that calls other verification functions and then chooses not to react, leaving that to the "notified" callbacks instead.

@clalancette
Copy link
Contributor

This was done in #772, so closing this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants