Skip to content

[rmw_cyclonedds] Improve handling of dynamic discovery#429

Merged
sloretz merged 18 commits intorollingfrom
gbiggs/discovery-peers-specification
Apr 8, 2023
Merged

[rmw_cyclonedds] Improve handling of dynamic discovery#429
sloretz merged 18 commits intorollingfrom
gbiggs/discovery-peers-specification

Conversation

@gbiggs
Copy link
Copy Markdown
Member

@gbiggs gbiggs commented Dec 14, 2022 •
edited
Loading

This PR adds to rmw_cyclonedds functionality necessary to support the improved handling of dynamic discovery.

It adds handling of the new discovery parameters passed down from rcl.

Current status: The desired functionality is only partially complete. Some of the entries in the goal matrix are not working correctly.

gbiggs added 2 commits October 13, 2022 11:12
@gbiggs @clalancette
First go at getting Cyclone to work with the new discovery configurat…
17a5695 
…ion params

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs
Adopt suggestions from Erik
01f727e 
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
@gbiggs gbiggs mentioned this pull request Dec 14, 2022
Closed
8 tasks
mxgrey
mxgrey reviewed Jan 30, 2023
View reviewed changes
Comment thread rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated
@ros-discourse
Copy link
Copy Markdown

ros-discourse commented Feb 21, 2023

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-02-16/29927/1

@mxgrey mxgrey mentioned this pull request Mar 3, 2023
Merged
mxgrey added 2 commits March 7, 2023 23:52
@mxgrey @wjwwood
most tests passing
1bf62a6 
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey @wjwwood
Revised, recommended behavior for discovery
3af525c 
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@mxgrey mxgrey mentioned this pull request Mar 13, 2023
Closed
@wjwwood wjwwood mentioned this pull request Mar 21, 2023
Merged
@wjwwood wjwwood marked this pull request as ready for review March 21, 2023 20:29
@wjwwood wjwwood requested a review from ivanpauno as a code owner March 21, 2023 20:29
mxgrey and others added 2 commits March 21, 2023 13:33
wjwwood
wjwwood reviewed Mar 28, 2023
View reviewed changes
Comment thread rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated
Comment thread rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated
Comment thread rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated
sloretz added 2 commits March 29, 2023 00:29
@sloretz
C++ 17, debug log, and linters
8324e11 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
NOT_SET and SYSTEM_DEFAULT values
9392ae7 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz sloretz changed the title Improve handling of dynamic discovery - rmw_cyclonedds changes [rmw_cyclonedds] Improve handling of dynamic discovery Mar 29, 2023
sloretz added 8 commits March 29, 2023 23:36
@sloretz
Remove TODO that won't be fixed
5ed2f3b 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
Make linters happy
e398508 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
Call rmw_discovery_options_init()
20a4021 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
Set max participant index to largest value possible
f8cc7e1 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
Set ParticipantIndex to none when OFF
c010800 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
Merge branch 'rolling' into gbiggs/discovery-peers-specification
826910f
@sloretz
Error when range is an invalid value
fd2474c 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
Use localhost has peer address
9472664 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz
Copy link
Copy Markdown
Contributor

sloretz commented Apr 1, 2023 •
edited
Loading

EDIT seems to be solved by ros2/ros2#1402

@eboasson Any ideas why with automatic discovery range set to localhost, all these errors would show up on windows? The error WSAEADDRNOTAVAIL shows up while cyclone is trying to send a UDP packet.

[test_subscriber-9] 1680304639.633310 [1]        tev: slortez: WSAEADDRNOTAVAIL (or fall through from WSAEFAULT)slortez: WSAEAFNOSUPPORT (or fall through from WSAEADDRNOTAVAIL)ddsi_udp_conn_write to udp/127.0.0.1:7660 failed with retcode -3

The number seems to vary, but the later tests have 119 ports tried, which seems to match the maximum participant index. It's almost like on Windows there's an issue sending the discovery packets to other potential participants.

The job was built with the following commit with extra log messages in cyclone
https://ci.ros2.org/job/ci_windows/19050/testReport/junit/(root)/projectroot/test_publisher_subscriber__rclpy__rmw_cyclonedds_cpp/

sloretz/cyclonedds@c1b549b

@sloretz
Set MaxAutoParticipantIndex to 32
5cfb1b6 
Signed-off-by: Shane Loretz <sloretz@google.com>
@sloretz sloretz merged commit 6510ad2 into rolling Apr 8, 2023
@delete-merged-branch delete-merged-branch Bot deleted the gbiggs/discovery-peers-specification branch April 8, 2023 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sloretz sloretz sloretz left review comments

@eboasson eboasson eboasson left review comments

@wjwwood wjwwood wjwwood approved these changes

@ivanpauno ivanpauno Awaiting requested review from ivanpauno

+1 more reviewer

@mxgrey mxgrey mxgrey left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@gbiggs @ros-discourse @sloretz @wjwwood @mxgrey @eboasson