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

avoid using dds common public mutex directly #474

Merged

Conversation

iuhilnehc-ynos
Copy link

It depends on ros2/rmw_dds_common#73

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
@iuhilnehc-ynos
Copy link
Author

@iuhilnehc-ynos iuhilnehc-ynos marked this pull request as ready for review October 19, 2023 01:11
@iuhilnehc-ynos iuhilnehc-ynos changed the title avoid usind dds common public mutex directly avoid using dds common public mutex directly Oct 19, 2023
Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

LGTM given the changes in ros2/rmw_dds_common#73, but I would have one more look at some details there (as I wrote in a review comment).

I'll mark this approved: if ros2/rmw_dds_common#73 changes, then this'll have to change anyway and get reviewed again; but if the decision is that the other PR is good as it is (I'm only suggesting another look, after all) then this seems ok as it is, too.

Chen Lihui added 2 commits October 19, 2023 17:33
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Copy link
Collaborator

@eboasson eboasson left a comment

Choose a reason for hiding this comment

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

Looks much nicer this way, thanks!

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left just a few comments, nothing major. I like this refactoring a lot.

rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated Show resolved Hide resolved
rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated Show resolved Hide resolved
@@ -5097,7 +5070,8 @@ static void rmw_fini_cs(CddsCS * cs)
dds_delete(cs->pub->enth);
}

static rmw_ret_t destroy_client(const rmw_node_t * node, rmw_client_t * client)
static rmw_ret_t destroy_client(
const rmw_node_t * node, rmw_client_t * client, bool destroy_graph = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please describe why we need the destroy_graph boolean here? It isn't totally clear to me.

Copy link
Author

Choose a reason for hiding this comment

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

After using make_scope_exit in the rmw_create_client, I removed the destroy_client and move its original implementation code into rmw_destroy_client since destroy_client is only called once, so there is no boolean flag.

Chen Lihui added 2 commits October 27, 2023 13:21
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Copy link
Contributor

@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

Signed-off-by: Chen Lihui <lihui.chen@sony.com>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI.

@clalancette clalancette merged commit d41705c into ros2:rolling Nov 3, 2023
1 of 2 checks passed
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.

None yet

4 participants