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

Documentation on parameter callbacks is unclear about how to use 'on_parameter_event' #2979

Closed
SanderVanDijk-StreetDrone opened this issue Aug 24, 2022 · 16 comments
Labels
help wanted Extra attention is needed

Comments

@SanderVanDijk-StreetDrone

The section on parameter callbacks in Concepts -> About parameters in ROS 2 has been a great addition in the beginning of this year.

It usefully mentions that add_on_set_parameters_callback should not have any side effects, which invalidates a bunch of examples that are out there. Instead it mentions that the on_parameter_event function should be used to register a callback that does the actual handling of the parameter change.

However, it is not clear where this function lives. From the writing, it seems like it is an alternative to add_on_set_parameters_callback which is a member of rclcpp::Node, however there is no function called on_parameter_event on rclcpp::Node.

Only after searching through the rclcpp repository on GitHub was I able to find that this is a method on Sync/AsyncParameterClient. If I understand the code, and some posts on answers.ros.org, correctly then a node would have to create such a client and register a callback with that, which then checks the events coming through for the parameter that it is interested in.

If this is correct, I can try to contribute an amendment to make that clearer. Or if there already is some documentation about such usage of a parameter client, it would be nice to reference that in this section.

@fujitatomoya
Copy link
Collaborator

I believe that your assumption is correct. if you are willing to contribute, we are happy to review! Thanks!

we have two different use cases for each callback.

parameters belong to Node object. user callbacks registered with add_on_set_parameters_callback will be called when anyone (self node or remote nodes) tries to set parameter which belongs to this Node. this user callback is used to inspect upcoming change to parameters in the Node.

on_parameter_event callback will be called any parameters update, it internally subscribe /parameter_events topic to check the all parameter activity, and if any event comes in that topic, on_parameter_event user callback will be called.

CC: @clalancette

@clalancette clalancette added the help wanted Extra attention is needed label Sep 8, 2022
@RFRIEDM-Trimble
Copy link
Contributor

RFRIEDM-Trimble commented Nov 6, 2022

Does the usage of it in the tutorials just need documentation? If so, I'm happy to contribute that.
^ Actually, that code doesn't work, evident looking at the README, seems like something else is needed for a place to put an example.

Should I add it to the existing section?

@fujitatomoya
Copy link
Collaborator

https://github.com/ros2/demos has both add_on_set_parameters_callback and on_parameter_event, so i think we just need to add documentation for both of them.

probably we can add more details on here? https://docs.ros.org/en/rolling/Concepts/About-ROS-2-Parameters.html#parameter-callbacks

@RFRIEDM-Trimble
Copy link
Contributor

RFRIEDM-Trimble commented Nov 6, 2022

Sounds good. I'm learning to use them.

I was leaning towards the best use case being an example. However that brings up the questions should it be with bind or lambda?

Using bind is more familiar since it's used for pup/sub and service style callbacks in all the tutorials.

@fujitatomoya
Copy link
Collaborator

thanks for pointing that out.

Using bind is more familiar since it's used for pup/sub and service style callbacks in all the tutorials.

agree on this. but i would say it is just up to you, lambda is something like nice-to-have atm.

@SanderVanDijk-StreetDrone
Copy link
Author

My idea was indeed mostly to expand on how on_parameter_event can actually be used in this section.

However, I have now learned about the new callback functions introduced here: ros2/rclcpp#1947, specifically add_post_set_parameter_callback. Probably the usage of on_parameter_event should be dropped from that section and replaced with a description of how to use add_post_set_parameter_callback.

@RFRIEDM-Trimble
Copy link
Contributor

RFRIEDM-Trimble commented Nov 29, 2022

Please assign to me, I can take this. While I think adding notes to the concepts section would be helpful, I would also like to add a how to guide or tutorial. Is there a recommendation on where I could put together an example with all the source code needed to demonstrate the different usages?

There is a tutorial on it at Tutorials/Beginner-CLI-Tools/Understanding-ROS2-Parameters/Understanding-ROS2-Parameters, however dynamic reconfiguration doesn't seem like a beginner topic.

@RFRIEDM-Trimble
Copy link
Contributor

I'd like to split the documentation of the new callbacks into a separate PR, because on_parameter event affects humble users, while the new callback functions are only in rolling so far. For the new callback methods, it would seem useful to link to the design doc.

@hacker1024
Copy link

hacker1024 commented Dec 22, 2022

How should on_parameter_event be used in Python? I can't find any references to on_parameter_event or AsyncParameterClient in Python (on Foxy).

@fujitatomoya
Copy link
Collaborator

@hacker1024

I can't find any references to on_parameter_event or AsyncParameterClient in Python (on Foxy).

we have AsyncParameterClient class in rclpy for humble or later. (foxy does not have it, and we cannot backport this either to keep the ABI compatibility.)

https://github.com/ros2/rclpy/blob/fdb04566e20e9ccbaceeeffc51916b65b1f86a19/rclpy/rclpy/parameter_client.py#L34

on_parameter_event method does not exist in rclpy for now, I believe that something we can add. basically we try to implement the features and methods in rclcpp and rclpy as consistent as possible, but actually there are some gaps between them.

@hacker1024
Copy link

on_parameter_event method does not exist in rclpy for now, I believe that something we can add.

I see. How then should one listen to parameter events in Python? I can't find any documentation about this anywhere, and most tutorials incorrectly use add_on_set_parameters_callback with side effects.

(If it isn't possible at all on Foxy, this too should be reflected in the docs.)

@clalancette
Copy link
Contributor

I see. How then should one listen to parameter events in Python?

You can always do:

        def callback(msg):
            print('Got parameter event callback')

        sub = self.node.create_subscription(
            rcl_interfaces.msg.ParameterEvent,
            '/parameter_events',
            callback)

(that's all that on_parameter_event does anyway)

@clalancette
Copy link
Contributor

Just as an FYI, I've just merged in ros2/rclpy#1061 , which adds the on_parameter_event method in rclpy. So for Rolling, at least, we can suggest to use that everywhere.

@deadln
Copy link

deadln commented Mar 20, 2024

You can always do:

        def callback(msg):
            print('Got parameter event callback')

        sub = self.node.create_subscription(
            rcl_interfaces.msg.ParameterEvent,
            '/parameter_events',
            callback)

It would be really useful if this example will be described somewhere in docs.

@fujitatomoya
Copy link
Collaborator

@deadln if you could consider the contribution, i would love to review!

@clalancette
Copy link
Contributor

See my comment in #3237 (comment) . Given that this has been rewritten already, I'm going to close this out for now. If you disagree, please feel free to reopen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

6 participants