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

Add rmw_publisher_wait_for_all_acked support #20

Merged
merged 4 commits into from
Jun 2, 2021
Merged

Add rmw_publisher_wait_for_all_acked support #20

merged 4 commits into from
Jun 2, 2021

Conversation

Barry-Xu-2018
Copy link
Collaborator

Related to ros2/rmw#295

@Barry-Xu-2018 Barry-Xu-2018 mentioned this pull request Mar 31, 2021
Closed
asorbini
asorbini requested changes Mar 31, 2021
View reviewed changes
Copy link
Collaborator

@asorbini asorbini left a comment

Choose a reason for hiding this comment

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

Hi @Barry-Xu-2018, thank you for your submission!

I added a few comments and suggestions to make sure the code builds with RTI Connext DDS Micro, and a couple of other minor fixes.

Do you know if rmw#295 will be merged for Galactic?

rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
rmw_connextdds_common/src/common/rmw_impl.cpp Show resolved Hide resolved
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
@fujitatomoya
Copy link
Collaborator

Do you know if rmw#295 will be merged for Galactic?

hopefully 🤞

@wjwwood @clalancette @ivanpauno what do you think?

i guess we could have this in rmw for Galactic, then we can work on rcl, rclcpp, rosbag2 and tests accordingly for Turtle-H. but we do not have strong opinion about that.

fujitatomoya
fujitatomoya reviewed Apr 1, 2021
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.

looks good to me with @asorbini 's comments

@wjwwood
Copy link
Member

wjwwood commented Apr 1, 2021

hopefully 🤞

It's on my list to review/run CI for tonight.

Make sure it is rebased if you can, I've been having that stall the others.

asorbini
asorbini approved these changes Apr 1, 2021
View reviewed changes
Copy link
Collaborator

@asorbini asorbini left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you for the updates @Barry-Xu-2018.

We should run some CI, but I assume that will happen to validate the rest of the changes in the other PRs.

This was referenced Apr 3, 2021
Merged
Merged
@asorbini
Copy link
Collaborator

@fujitatomoya @Barry-Xu-2018 any updates on whether this feature will make it into Galactic?

Paging @clalancette too.

@Barry-Xu-2018
Copy link
Collaborator Author

@asorbini

Galactic API/Feature Freeze.
So this feature will be added to Turtle-H.

@asorbini asorbini added the humble PR scheduled for the H-turtle label Apr 14, 2021
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-wait_for_all_acked-feature branch from f39632a to a254e98 Compare April 16, 2021 09:33
@Barry-Xu-2018
Copy link
Collaborator Author

Do rebase.

ivanpauno
ivanpauno reviewed May 28, 2021
View reviewed changes
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
rmw_connextddsmicro/src/rmw_api_impl_rtime.cpp Outdated Show resolved Hide resolved
@ivanpauno ivanpauno mentioned this pull request May 28, 2021
Merged
@Barry-Xu-2018
Add rmw_publisher_wait_for_all_acked support
9aa0b17 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Update based on review comments
59275f6 
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Update rmw_connextdds_duration_from_ros_time() and fix deletion by mi…
6bed763 
…stake

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-wait_for_all_acked-feature branch from 9fdbd74 to 6bed763 Compare May 30, 2021 07:55
@Barry-Xu-2018
Copy link
Collaborator Author

Do rebase.

ivanpauno
ivanpauno reviewed May 31, 2021
View reviewed changes
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
rmw_connextdds_common/src/common/rmw_impl.cpp Outdated Show resolved Hide resolved
@Barry-Xu-2018
Move the check of infinite duration to rmw_connextdds_duration_from_r…
e32fa9d 
…os_time()

Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018
Copy link
Collaborator Author

@ivanpauno Address your comments.

@ivanpauno ivanpauno merged commit 69e91a6 into ros2:master Jun 2, 2021
@ivanpauno ivanpauno mentioned this pull request Jun 2, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fujitatomoya fujitatomoya fujitatomoya left review comments

@ivanpauno ivanpauno ivanpauno approved these changes

@asorbini asorbini asorbini approved these changes

Assignees
No one assigned
Labels
humble PR scheduled for the H-turtle
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Barry-Xu-2018 @fujitatomoya @wjwwood @asorbini @ivanpauno