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

[rmw_fastrtps] Improve handling of dynamic discovery #653

Merged

sloretz merged 46 commits into rolling from gbiggs/discovery-peers-specification

Apr 8, 2023

Member

gbiggs commented Dec 14, 2022 •

edited

Loading

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

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

This implementation allows a host specified in static peers to be a host name or an IPv4 or an IPv6 addresses. It will automatically manage translating between these to ensure that the static peer is correctly identified, no matter what it reports itself as.

Current status: The desired functionality is mostly complete. Most of the entries in the goal matrix are working correctly. There is currently a bug where specifying a static peer appears to disable dynamic discovery. This needs to be resolved.

gbiggs mentioned this pull request

Improve handling of dynamic discovery ros2/ros2#1359

Closed

8 tasks

arjo129 mentioned this pull request

[WIP] Improve handling of dynamic discovery - rmw_fastrtps changes #667

Merged

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

gbiggs added 4 commits

March 21, 2023 13:01


          Support specification of discovery range and static peers

765933f

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>


          Apply suggestions from eProsima

acc9541

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>


          Use participant ignoring

b2a6a24

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>


          Improve handling of aliases for hosts

24bdb5a

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>

wjwwood force-pushed the gbiggs/discovery-peers-specification branch from 3f2b322 to 24bdb5a Compare

March 21, 2023 20:06

wjwwood marked this pull request as ready for review

March 21, 2023 20:06

wjwwood requested a review from sloretz as a code owner

March 21, 2023 20:06

arjo129 and others added 8 commits

March 21, 2023 13:27


          Adds support for using IP addresses to specify peers

7c38fef

This commit adds support for using IP addresses to specify peers. It
also refactors out some networking function so that they can be used by
other files.

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>


          Remove excessive logging

05daea6

Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>


          Add name lookup and clean up implementation.

05d3836

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>


          Two more scenarios fixed.

5bb9754

Two more to go.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>


          Revert rmw changes

4ab38d3

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>


          Add support for dynamic allocations

fdf6e83

See:
* ros2/rmw#349
* ros2/rcl#1038

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>


          Support new requirements

30c7ee1

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>


          Update to latest rmw API

8f1ac6b

Signed-off-by: Michael X. Grey <grey@openrobotics.org>

sloretz mentioned this pull request

ROS 2: Improve the discovery methods used by nodes to prevent conflicts and accidental discoveries RobotLocomotion/drake-ros#183

Closed


          Merge branch 'rolling' into gbiggs/discovery-peers-specification

d320029

Contributor

sloretz commented Mar 25, 2023

@EduPonz Any ETA on when the changes from this branch eProsima/Fast-DDS@master...poc/ros2-iron-discovery-options will get merged into Fast-DDS? They're required for these discovery changes to work with Fast-DDS.

I'm hoping we can get this in before the upcoming ROS Iron RMW freeze deadline.

sloretz added 3 commits

March 27, 2023 19:13


          Update to use API on Fast-DDS master

033af73

Signed-off-by: Shane Loretz <sloretz@google.com>


          Merge branch 'rolling' into gbiggs/discovery-peers-specification

6ff095d


          Update with rmw_discovery_options_t changes

0fbce8e

Signed-off-by: Shane Loretz <sloretz@google.com>

wjwwood reviewed

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/src/utils/net_utils.cpp Outdated Show resolved Hide resolved

sloretz added 5 commits

March 28, 2023 22:33


          Minimize diff with rolling in custom_participant_info.hpp

65de1fb

Signed-off-by: Shane Loretz <sloretz@google.com>


          Minimize diff with rolling in participant.cpp

f055b23

Signed-off-by: Shane Loretz <sloretz@google.com>


          Collapse lines

a2577c5

Signed-off-by: Shane Loretz <sloretz@google.com>


          conditional on one line

dfd7dd2

Signed-off-by: Shane Loretz <sloretz@google.com>


          return instead of setting variable

fe77f29

Signed-off-by: Shane Loretz <sloretz@google.com>

sloretz added 6 commits

March 30, 2023 02:34


          Set discovery range in test

e1d6787

Signed-off-by: Shane Loretz <sloretz@google.com>


          Lint

930a833

Signed-off-by: Shane Loretz <sloretz@google.com>


          Call rmw_discovery_options_init()

2bf0af2

Signed-off-by: Shane Loretz <sloretz@google.com>


          Workaround deadlock with rclcpp global logging mutex

426253c

Signed-off-by: Shane Loretz <sloretz@google.com>


          Add shared memory transport for LOCALHOST traffic

7e80152

Signed-off-by: Shane Loretz <sloretz@google.com>


          Configure max initial peers range on udp transport

969c2b4

Signed-off-by: Shane Loretz <sloretz@google.com>

MiguelCompany reviewed

View reviewed changes

rmw_fastrtps_shared_cpp/src/participant.cpp Show resolved Hide resolved

sloretz added 2 commits

March 31, 2023 16:14


          Disable built-in transports and fix lint

d1e94da

Signed-off-by: Shane Loretz <sloretz@google.com>


          Error when range is an invalid value

1f56fb7

Signed-off-by: Shane Loretz <sloretz@google.com>

MiguelCompany suggested changes

View reviewed changes

rmw_fastrtps_cpp/test/test_get_native_entities.cpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/src/participant.cpp Outdated Show resolved Hide resolved

rmw_fastrtps_shared_cpp/src/participant.cpp Show resolved Hide resolved

rmw_fastrtps_shared_cpp/src/participant.cpp

Comment on lines +233 to +237

+                if (RMW_AUTOMATIC_DISCOVERY_RANGE_LOCALHOST == discovery_options->automatic_discovery_range) {
+                  // Add localhost as a static peer
+                  eprosima::fastrtps::rtps::Locator_t peer;
+                  eprosima::fastrtps::rtps::IPLocator::setIPv4(peer, "127.0.0.1");
+                  domainParticipantQos.wire_protocol().builtin.initialPeersList.push_back(peer);

Contributor

MiguelCompany Apr 1, 2023

For the case of RMW_AUTOMATIC_DISCOVERY_RANGE_SUBNET, if initialPeersList is not empty, we should add the default multicast address to it.

Contributor

sloretz Apr 4, 2023

To the initial peer list, or as a multicast locator? I assumed the latter in cf9bf83

MiguelCompany reviewed

View reviewed changes

rmw_fastrtps_shared_cpp/src/participant.cpp Show resolved Hide resolved

sloretz added 4 commits

April 3, 2023 23:54


          undo unnecessary test change

60ce225

Signed-off-by: Shane Loretz <sloretz@google.com>


          Document Setting range to SYSTEM_DEFAULT

a5dae76

Signed-off-by: Shane Loretz <sloretz@google.com>


          Limit participants to 1 when discover is OFF

581ab86

Signed-off-by: Shane Loretz <sloretz@google.com>


          With SUBNET and initial peers, add default multicast address as a mul…

cf9bf83

…ticast locator

Signed-off-by: Shane Loretz <sloretz@google.com>

MiguelCompany suggested changes

View reviewed changes

rmw_fastrtps_shared_cpp/src/participant.cpp Outdated

+                  eprosima::fastrtps::rtps::Locator_t locator;
+                  eprosima::fastrtps::rtps::IPLocator::setIPv4(locator, 239, 255, 0, 1);
+                  domainParticipantQos.wire_protocol()
+                  .builtin.metatrafficMulticastLocatorList.push_back(locator);

Contributor

MiguelCompany Apr 4, 2023

I meant the initialPeersList. Announcements are always sent to the initial peer list. When it is empty, the middleware sets its value to the metatrafficMulticastLocatorList.

So if we want to publish announcements to 192.168.1.10 and the multicast address, both should be in the initial peer list.

Suggested change

      
                .builtin.metatrafficMulticastLocatorList.push_back(locator);
          
                .builtin.initialPeersList.push_back(locator);

Contributor

sloretz Apr 4, 2023

Added to Initial peers list instead in 9cec2d6

MiguelCompany reviewed

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Show resolved Hide resolved

sloretz added 5 commits

April 4, 2023 16:14


          Add multicast address to initial peer list when there are other stati…

9cec2d6

…c peers and SUBNET range

Signed-off-by: Shane Loretz <sloretz@google.com>


          Bump required Fast-DDS version to 2.10

47facb9

Signed-off-by: Shane Loretz <sloretz@google.com>


          Set maxInitialPeersRange to 32

b68b332

Signed-off-by: Shane Loretz <sloretz@google.com>


          Grammar

5faeb3e

Signed-off-by: Shane Loretz <sloretz@google.com>


          Shorten sentence

df6139e

Signed-off-by: Shane Loretz <sloretz@google.com>

MiguelCompany approved these changes

View reviewed changes

Contributor

MiguelCompany left a comment

Nice job!

sloretz mentioned this pull request

Pick smallest available participant ID for new paricipants eProsima/Fast-DDS#3437

Merged

10 tasks

Contributor

sloretz commented Apr 5, 2023

Need one small fix/enhancement to Fast-DDS eProsima/Fast-DDS#3437

wjwwood approved these changes

View reviewed changes


          Merge branch 'rolling' into gbiggs/discovery-peers-specification

sloretz merged commit c83749a into rolling

delete-merged-branch bot deleted the gbiggs/discovery-peers-specification branch

April 8, 2023 04:35

mergify bot mentioned this pull request

Pick smallest available participant ID for new paricipants (backport #3437) eProsima/Fast-DDS#4555

Merged

10 tasks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment