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 support #296

Merged
merged 5 commits into from
Jun 2, 2021

Conversation

Barry-Xu-2018
Copy link
Contributor

Signed-off-by: Barry Xu Barry.Xu@sony.com

@Barry-Xu-2018
Copy link
Contributor Author

Related to #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.

as far as i see, rmw_connext / rmw_fastftps / rmw_cyclonedds all support wait_for_acknowledgments.

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Show resolved Hide resolved
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

I've left some minor comments

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

LGTM!

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
@ivanpauno
Copy link
Member

The next step is to complete the implementations for rmw_fastrtps, rmw_cyclonedds, rmw_connextdds.

@Barry-Xu-2018
Copy link
Contributor Author

The next step is to complete the implementations for rmw_fastrtps, rmw_cyclonedds, rmw_connextdds.

Yeah. I will implement them.

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.

i am good to go.

@wjwwood wjwwood removed their assignment Apr 4, 2021
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Show resolved Hide resolved
@Barry-Xu-2018
Copy link
Contributor Author

After checking some DDS implementations, I think DDS interface should be thread-safe.
So I change the description of Thread-Safe.

Copy link
Member

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

Sorry, we're past the feature freeze so we're not going to be able to merge this.

Holding it until the Galactic release.

@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-wait_for_all_acked branch from 649ff2c to 4ee4b4b Compare April 16, 2021 09:28
@Barry-Xu-2018
Copy link
Contributor Author

Do rebase.

Copy link
Member

@ivanpauno ivanpauno 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

@Barry-Xu-2018 @fujitatomoya can you run CI, including all the rmw* PRs related to this feature?
Please address the comments in ros2/rmw_connextdds#20 first.

My plan is to get these PRs in first, and then move forward with rcl, rcl* PRs.

@fujitatomoya
Copy link
Collaborator

@ivanpauno thanks for the effort, really appreciate it.

can you run CI, including all the rmw* PRs related to this feature?

sounds good, we will run CI with rmw_* changes at 1st.

@Barry-Xu-2018
Copy link
Contributor Author

@ivanpauno

can you run CI, including all the rmw* PRs related to this feature?
Please address the comments in ros2/rmw_connextdds#20 first

Okay. Thanks.

@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018

once you addressed ros2/rmw_connextdds#20, could you let me know? i will create the repo file and start CI for rmw_* changes.

@Barry-Xu-2018
Copy link
Contributor Author

once you addressed ros2/rmw_connextdds#20, could you let me know? i will create the repo file and start CI for rmw_* changes.

Okay,thanks.

Barry-Xu-2018 and others added 5 commits May 30, 2021 13:59
Signed-off-by: Barry Xu <Barry.Xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
Signed-off-by: Barry Xu <barry.xu@sony.com>
@Barry-Xu-2018 Barry-Xu-2018 force-pushed the topic-wait_for_all_acked branch from 4ee4b4b to a9a89e5 Compare May 30, 2021 06:10
@Barry-Xu-2018
Copy link
Contributor Author

Do rebase.

@Barry-Xu-2018
Copy link
Contributor Author

@fujitatomoya

All changes for rmw* are all approved.
Please help to create the repo file and start CI.

Except this PR, it includes below

@fujitatomoya
Copy link
Collaborator

CI: (for rmw related repositories)

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

@Barry-Xu-2018
Copy link
Contributor Author

macOS Build Status is failing

After checking the log, the cause seems to remote connection problem.
I have no idea to resolve this. Maybe we should re-execute it.

13:59:18 �]0;colcon build [238/308 done] [3 ongoing]�Starting >>> image_transport
13:59:18 �]0;colcon build [238/308 done] [4 ongoing]�[Processing: image_transport, rclcpp_lifecycle, rosbag2_cpp, tf2_ros]
13:59:48 FATAL: command execution failed
14:07:20 java.nio.channels.ClosedChannelException
14:07:20 	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer.onReadClosed(ChannelApplicationLayer.java:209)
14:07:20 	at org.jenkinsci.remoting.protocol.ApplicationLayer.onRecvClosed(ApplicationLayer.java:221)
14:07:20 	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.onRecvClosed(ProtocolStack.java:817)
14:07:20 	at org.jenkinsci.remoting.protocol.FilterLayer.onRecvClosed(FilterLayer.java:288)
14:07:20 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.onRecvClosed(SSLEngineFilterLayer.java:179)
14:07:20 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.switchToNoSecure(SSLEngineFilterLayer.java:281)
14:07:20 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processWrite(SSLEngineFilterLayer.java:501)
14:07:20 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.processQueuedWrites(SSLEngineFilterLayer.java:246)
14:07:20 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doSend(SSLEngineFilterLayer.java:198)
14:07:20 	at org.jenkinsci.remoting.protocol.impl.SSLEngineFilterLayer.doCloseSend(SSLEngineFilterLayer.java:211)
14:07:20 	at org.jenkinsci.remoting.protocol.ProtocolStack$Ptr.doCloseSend(ProtocolStack.java:785)
14:07:20 	at org.jenkinsci.remoting.protocol.ApplicationLayer.doCloseWrite(ApplicationLayer.java:172)
14:07:20 	at org.jenkinsci.remoting.protocol.impl.ChannelApplicationLayer$ByteBufferCommandTransport.closeWrite(ChannelApplicationLayer.java:311)
14:07:20 	at hudson.remoting.Channel.close(Channel.java:1502)
14:07:20 	at hudson.remoting.Channel.close(Channel.java:1455)
14:07:20 	at hudson.slaves.SlaveComputer.closeChannel(SlaveComputer.java:884)
14:07:20 	at hudson.slaves.SlaveComputer.access$100(SlaveComputer.java:110)
14:07:20 	at hudson.slaves.SlaveComputer$2.run(SlaveComputer.java:765)
14:07:20 	at jenkins.util.ContextResettingExecutorService$1.run(ContextResettingExecutorService.java:28)
14:07:20 	at jenkins.security.ImpersonatingExecutorService$1.run(ImpersonatingExecutorService.java:68)
14:07:20 	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:511)
14:07:20 	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
14:07:20 	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1149)
14:07:20 	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
14:07:20 	at java.lang.Thread.run(Thread.java:748)
14:07:20 Caused: hudson.remoting.ChannelClosedException: Channel "hudson.remoting.Channel@52f33c80:JNLP4-connect connection from 70-35-50-58.static.wiline.com/70.35.50.58:62051": Remote call on JNLP4-connect connection from 70-35-50-58.static.wiline.com/70.35.50.58:62051 failed. The channel is closing down or has closed down

@fujitatomoya
Copy link
Collaborator

CI (macOS):

  • macOS Build Status

@fujitatomoya
Copy link
Collaborator

@Barry-Xu-2018 i just restarted the CI for macOS.

@ivanpauno ivanpauno merged commit 2c58d9a into ros2:master Jun 2, 2021
@fujitatomoya
Copy link
Collaborator

@ivanpauno appreciate for the help, thanks 👍

@Barry-Xu-2018
Copy link
Contributor Author

@fujitatomoya @ivanpauno

Thank you.

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.

5 participants