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

address how parameter callbacks work #722

Closed
wjwwood opened this issue May 15, 2019 · 0 comments · Fixed by #772
Closed

address how parameter callbacks work #722

wjwwood opened this issue May 15, 2019 · 0 comments · Fixed by #772
Labels
enhancement New feature or request

Comments

@wjwwood
Copy link
Member

wjwwood commented May 15, 2019

There was a long discussion here: #495 (comment)

Basically right now you can only have one parameter callback at a time that can control whether or not parameters may be set.

Note this is about set_on_parameters_set_callback, but some parts of this discuss will apply to the "on change" callback which happens after setting occurs and is just for observation (it cannot reject changes).

This callback is considered insufficient because it makes having one callback per parameter or per group of parameters very difficult to implement. At first it seems trivial as the user could just have a single callback which calls multiple callbacks, but if the system also wants to have callbacks for things like implicitly declared parameters (e.g. use_sim_time) or callbacks to check changes to parameters based on some state (maybe lifecycle node's) then it's easy for the user to "clobber" system callbacks or even for system callbacks to interfere with one another.

One way to solve this is to have the NodeParameters interface handle this itself, allowing you to set more than one callback at a time. That would remove the burden from the users to coordinate and not interfere with each other.

However, if we do that we need a way to unregister callbacks that were registered (so some sort of receipt is needed), and we need a policy for what order to call them in and what to do if callbacks which overlap (check the same parameter) conflict. Do we stop at the first that fails or continue and aggregate the failure reasons?


This is my personal opinion on what we should do:

I think it should be a list of callbacks stored in the node, which is called in the order that they are added (perhaps with an option to prepend to the callback list for special cases), and we stop calling callbacks when the first one returns SetParameterResults.successful = false.

I'm not sure if it's worth calling all the callbacks so that we can take all the ones that failed and join their reasons, e.g. it might result in a reason like "failed to set parameters because: the node is not in the unconfigured state, and the parameter is not in the range x to y, and the parameter is read only" versus just one of them (the first one).

@wjwwood wjwwood added the enhancement New feature or request label May 15, 2019
nnmm pushed a commit to ApexAI/rclcpp that referenced this issue Jul 9, 2022
* Expose qos setting for /rosout

Signed-off-by: Ada-King <[email protected]>

* Modify some details based on review

Signed-off-by: Ada-King <[email protected]>

* Add test

Signed-off-by: Ada-King <[email protected]>

* Modify based on reviews

Signed-off-by: Ada-King <[email protected]>
DensoADAS pushed a commit to DensoADAS/rclcpp that referenced this issue Aug 5, 2022
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

Successfully merging a pull request may close this issue.

1 participant