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.

Sign up for GitHub

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

Jump to bottom

Store node pointer in publisher and subscription #1015

Open
wants to merge 1 commit into
base: rolling
Choose a base branch
from
Open

Store node pointer in publisher and subscription #1015

wants to merge 1 commit into from
+63 −4

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Oct 22, 2022

This is the successor of #987.

By storing the node pointer in the publisher and subscription, it is possible to check for the correct node pointer being passed to rcl_publisher_fini() and rcl_subscription_fini(). This PR also adds a new return code for this purpose.

@nnmm
Store the node pointer in publisher and subscription
cb9fd64 
The rcl_publisher_fini() and rcl_subscription_fini() functions now check that the
correct node was given to it. A new error code, RCL_RET_INCORRECT_NODE, is returned
on node mismatch

Signed-off-by: Nikolai Morin <[email protected]>
@fujitatomoya fujitatomoya mentioned this pull request Oct 24, 2022
Closed
@fujitatomoya
Copy link
Collaborator

I will go ahead to close this, see #987 (comment) and #987 (comment)

@fujitatomoya fujitatomoya reopened this Oct 24, 2022
fujitatomoya
fujitatomoya approved these changes Oct 25, 2022
View reviewed changes
Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

lgtm, all comments posted on #987

@fujitatomoya
Copy link
Collaborator

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@fujitatomoya
Copy link
Collaborator

@iuhilnehc-ynos can you take another review on this?

i believe that node argument to fini function is by contract to make it clear, we are not removing this. but extra check makes sense to me.

thanks,

@iuhilnehc-ynos
Copy link
Collaborator

iuhilnehc-ynos commented Oct 26, 2022 •
edited
Loading

@fujitatomoya

Storing the node pointer in publisher and subscription just to only check if the node pointer is correct or not in rcl_publisher_fini() and rcl_subscription_fini() is not reasonable to me.

I believe that not storing the node pointer in the entities(rcl_publisher_impl_s, rcl_subscription_impl_s, rcl_service_impl_s, rcl_client_impl_s) is something about the decoupling design.

@fujitatomoya
Copy link
Collaborator

I believe that not storing the node pointer in the entities(rcl_publisher_impl_s, rcl_subscription_impl_s, rcl_service_impl_s, rcl_client_impl_s) is something about the decoupling design.

I see that one of the design, but node and publisher / subscription are not exactly decoupled.
adding extra check to see if that is the correct node would make sense?

or are you suggesting something else?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya approved these changes

Assignees

@fujitatomoya fujitatomoya

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@nnmm @fujitatomoya @iuhilnehc-ynos