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

Add rmw_publisher_wait_for_all_acked #188

Merged
merged 3 commits into from
Jun 17, 2021

Conversation

Barry-Xu-2018
Copy link
Contributor

Related to ros2/rmw#295

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

@ivanpauno
Copy link
Member

Could you add a test case in https://github.com/ros2/rmw_implementation/tree/master/test_rmw_implementation?

I don't need a test that actually confirms that all messages were acked (which is a bit hard to test), only test basic things:
e.g. passing a nullptr publisher returns an error, passing a valid BEST_EFFORT publisher always return immediatly RMW_RET_OK, etc.

@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-wait_for_all_acked-feature branch from b1ff332 to e1c1b94 Compare May 30, 2021 06:12
@Barry-Xu-2018
Copy link
Contributor Author

Barry-Xu-2018 commented May 30, 2021

Could you add a test case in https://github.com/ros2/rmw_implementation/tree/master/test_rmw_implementation?

I don't need a test that actually confirms that all messages were acked (which is a bit hard to test), only test basic things:
e.g. passing a nullptr publisher returns an error, passing a valid BEST_EFFORT publisher always return immediatly RMW_RET_OK, etc.

Okay. I add a simple test case (commit: cc154d9).
FYI, I add more test cases (such as timeout) in rcl PR ros2/rcl#913).

@ivanpauno
Copy link
Member

@ros-pull-request-builder retest this please (this should pass now)

@ivanpauno
Copy link
Member

@Barry-Xu-2018 can you check the test failures?

@Barry-Xu-2018
Copy link
Contributor Author

@ivanpauno

The failure is related to test_rmw_implementation.TestPublisherUse__rmw_cyclonedds_cpp.wait_for_all_acked_with_best_effort .
It is because ros2/rmw_cyclonedds#294 isn't merged now.

Could you check and merge it ?

@ivanpauno
Copy link
Member

Thanks @Barry-Xu-2018, I was sure I merged that one 😄

@Barry-Xu-2018
Copy link
Contributor Author

@ivanpauno

After ros2/rmw_cyclonedds#294 is merged, we should re-execute CI.
Could you help to execute CI again ?

@fujitatomoya
Copy link
Collaborator

@ros-pull-request-builder retest this please

CC: @Barry-Xu-2018 @ivanpauno

@ivanpauno
Copy link
Member

The PR checker should pass after ros/rosdistro#29924 is merged.
No need to run CI again, this is only wrapping new API and adding tests for it.

Thanks for the patience, but merging changes in rmw_* without breaking the rolling builds is a long process ...

@ivanpauno
Copy link
Member

@ros-pull-request-builder retest this please

1 similar comment
@ivanpauno
Copy link
Member

@ros-pull-request-builder retest this please

@ivanpauno
Copy link
Member

finally ... 😃
going in!!

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.

4 participants