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

Make constructor methods for pub/sub/etc. take rcl_node_t mutex #290

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Oct 28, 2022

The constructors are not public, so we're free to use internal types as arguments.

This signature makes it possible, or at least easier, to write parameter services that are owned by the node: With this change, the Node does not need to be constructed before the parameter services are constructed, which would somewhat contradict the node owning these services.

The constructors are not public, so we're free to use internal types as arguments.

This signature makes it possible, or at least easier, to write parameter services that are owned by
the node: With this change, the Node does not need to be constructed before the parameter services are constructed,
which would somewhat contradict the node owning these services.
@nnmm nnmm requested a review from a team October 28, 2022 18:22
Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me!

Copy link
Collaborator

@esteve esteve left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nnmm LGTM

@nnmm nnmm merged commit 2746996 into main Nov 10, 2022
@delete-merged-branch delete-merged-branch bot deleted the new_methods_take_rcl_node branch November 10, 2022 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants