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

Store node pointer in publisher and subscription #1015

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

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.

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
Copy link
Collaborator

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

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

@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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants