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

change ParameterEventHandler to take events as const ref instead of shared pointer #494

Merged
merged 3 commits into from
Apr 3, 2021

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Apr 1, 2021

This is related to, and requires, ros2/rclcpp#1598

@mjeronimo can you think of any other places we need to update something like this? Documentation?

…hared pointer

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood added the enhancement New feature or request label Apr 1, 2021
@wjwwood wjwwood requested a review from mjeronimo April 1, 2021 08:41
@wjwwood wjwwood self-assigned this Apr 1, 2021
Signed-off-by: Audrow Nash <audrow@hey.com>
lifecycle/CMakeLists.txt Outdated Show resolved Hide resolved
Signed-off-by: Audrow Nash <audrow@hey.com>
@mjeronimo
Copy link
Contributor

Yes, probably need to update the ROS2 tutorial for the ParameterEventHandler. I'll take a look.

@mjeronimo
Copy link
Contributor

mjeronimo commented Apr 2, 2021

As it turns out the ParameterEventHandler tutorial doesn't need to be updated as it doesn't use the add_parameter_event_handler() method (which is the one use to monitor all parameter events); instead, it uses add_parameter_handler(), which is unaffected by the change from the client code point of view.

So, the changes in the #494 and ros2/rclcpp#1598 are sufficient.

@wjwwood wjwwood merged commit eba767b into master Apr 3, 2021
@delete-merged-branch delete-merged-branch bot deleted the refactor_any_subscription branch April 3, 2021 00:30
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 this pull request may close these issues.

None yet

3 participants