[rmw] Improve handling of dynamic discovery #338

gbiggs · 2022-12-14T07:14:36Z

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

It adds handling of the new discovery-related configuration parameters to rmw so they can be used by rcl to control the behaviour of RMW implementations.

ivanpauno

LGTM!

I have some minor comments/suggestions.

rmw/include/rmw/discovery_params.h

fujitatomoya · 2023-02-01T23:59:00Z

rmw/include/rmw/discovery_params.h

+  /// Uses ROS_LOCALHOST_ONLY environment variable.
+  RMW_AUTOMATIC_DISCOVERY_RANGE_DEFAULT = 0,


This means that if ROS_LOCALHOST_ONLY is not set, it is just up to rmw implementation? that means, changing rmw implementation via environmental variable changes application behavior pretty much? i think it would be nice to define the default in ROS 2?

The behavior in rcl is to use RMW_AUTOMATIC_DISCOVERY_RANGE_LOCALHOST if no value is given for the environment variable.

I think it's worth considering removing the RMW_AUTOMATIC_DISCOVERY_RANGE_DEFAULT value entirely since it currently will never get passed to rmw from rcl, and furthermore we're expecting the RMW implementations to implement ..._DEFAULT the same as ..._LOCALHOST anyway. Having this redundancy is probably just creating confusion and not accomplishing anything.

The QoS enums have a *_SYSTEM_DEFAULT value. I forget what this was for. Was it to allow setting QoS via a middleware specific configuration?

If so, RMW_AUTOMATIC_DISCOVERY_RANGE_SYSTEM_DEFAULT might be useful to say "don't mess with discovery settings, they're already configured with middleware specific APIs".

That's right @sloretz.

I think @mxgrey is also right, it's weird if we're never sending the value down to rmw, so we should either not have it as an rmw option or we should handle it in the rmw layer. It might make sense to have ROS/RCL version that has system default but not have one in RMW if that one doesn't make sense. I know we only have RMW enums atm, however.

However, it might just be that we shouldn't be automatically be converting the DEFAULT into LOCALHOST as we are now.

However, it might just be that we shouldn't be automatically be converting the DEFAULT into LOCALHOST as we are now.

If we are just converting DEFAULT to LOCALHOST ( I see that happening in the rmw_cyclonedds_cpp PR) then I agree it doesn't seem like an option at the RMW layer is necessary.

If someone wants to use an XML config, and doesn't want the rmw_* layer to mess with discovery settings, and the default discovery range is localhost, what option should they provide for the range? SUBNET since the default behavior seems to be to do nothing, at least for cyclonedds?

fujitatomoya · 2023-02-02T00:02:25Z

rmw/include/rmw/discovery_params.h

+  /// Allows multicast on the reachable network
+  RMW_AUTOMATIC_DISCOVERY_RANGE_SUBNET = 3,


What if system has multiple network interface? that is also rmw implementation dependent as it is now?

Yes. That is out of scope of this PR.

fujitatomoya · 2023-02-02T00:15:01Z

rmw/src/discovery_params.c

+    if (strncmp(
+        left->static_peers[ii],
+        right->static_peers[ii],
+        RMW_DISCOVERY_PARAMS_PEER_MAX_LENGTH) != 0)


What about the one is with hostname which can be DNSed to IP, and the other with the same IP address? that is not supported here?

The main use case for this is testing and configuration. So yes, this means that discovery_params which has a hostname that can be DNSed to some other host name is considered a different configuration.

ros-discourse · 2023-02-21T22:29:44Z

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

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

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

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

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

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

wjwwood

API and functions that are implemented here lgtm, but we should have some tests for those new functions that live here. I think @sloretz and I can help with that tomorrow.

rmw/include/rmw/discovery_options.h

wjwwood · 2023-03-27T05:25:00Z

I also think we need to address @fujitatomoya's comments, as I think they're still relevant even after the changes.

arjo129 · 2023-03-27T06:49:23Z

I don't have push access to this repo. Would you like me to open yet another PR?

clalancette · 2023-03-27T12:26:24Z

I don't have push access to this repo. Would you like me to open yet another PR?

@arjo129 I invited you to have push access. Once you accept, you should be able to continue here.

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

sloretz · 2023-03-27T23:57:31Z

@wjwwood FYI 2cf62e3 is a sizeable change. It:

adds rmw_discovery_options_init() to allocation options with a set number of static peers
adds an allocator member to the rmw_discovery_options_t object
uses a pointer to an allocator as an argument because I saw that used elsewhere
errors if copy is called with same object for src and dst
adds a whole bunch of tests

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

wjwwood

one small necessary (imo) change, otherwise lgtm

rmw/include/rmw/discovery_options.h

rmw/src/discovery_options.c

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

Signed-off-by: William Woodall <william@osrfoundation.org>

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

gbiggs requested review from methylDragon, ivanpauno and wjwwood as code owners December 14, 2022 07:14

gbiggs mentioned this pull request Dec 14, 2022

Improve handling of dynamic discovery ros2/ros2#1359

Closed

8 tasks

ivanpauno reviewed Dec 14, 2022

View reviewed changes

rmw/include/rmw/discovery_params.h Outdated Show resolved Hide resolved

rmw/include/rmw/discovery_params.h Outdated Show resolved Hide resolved

fujitatomoya reviewed Feb 2, 2023

View reviewed changes

This was referenced Feb 28, 2023

Warns if new DDS OOBE settings are used with Connext ros2/rmw_connextdds#107

Closed

Improve handling of dynamic discovery - rmw changes (Extends #338) #349

Merged

gbiggs and others added 4 commits March 21, 2023 11:46

Add types to store user preferences for how discovery works

3617a59

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

Support specification of discovery range and static peers

5cf8a88

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

Add an equality tester

cb43a8c

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

Small cleanups.

a493020

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

wjwwood force-pushed the gbiggs/discovery-peers-specification branch from ee413aa to a493020 Compare March 21, 2023 18:47

arjo129 and others added 13 commits March 21, 2023 11:52

only change const correctness.

e632cca

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

Add support for dynamic allocation

02a0550

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

Fix equality comparison for discovery params

37ffa56

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

Fix equality comparison for discovery params

231091a

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

Iterate on struct names

5ef8a0d

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

Update function documentation

761d1e7

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

More params -> parameters

678e8a6

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

Pass allocators by value instead of by pointer

1d6c8be

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

Rename discovery_parameters to discovery_options

bb86cd6

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

Style tweaks

536f613

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

Revert to strncpy for now

0c7b376

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

Uncrustify

5f35562

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

Fix docstrings.

e7e1e8d

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

This comment was marked as resolved.

Sign in to view

sloretz mentioned this pull request Mar 24, 2023

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

fcc75b4

wjwwood reviewed Mar 27, 2023

View reviewed changes

rmw/include/rmw/discovery_options.h Outdated Show resolved Hide resolved

rmw/include/rmw/discovery_options.h Outdated Show resolved Hide resolved

rmw/include/rmw/discovery_options.h Outdated Show resolved Hide resolved

rmw/include/rmw/discovery_options.h Outdated Show resolved Hide resolved

sloretz added 4 commits March 27, 2023 21:18

Avoid 'multicast' in comments

9e72cec

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

parameters -> options

91beb13

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

One sentence per line

5fc6ce9

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

Add rmw_discovery_options_init and tests

2cf62e3

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

const allocator pointer

796b6ee

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

wjwwood requested changes Mar 28, 2023

View reviewed changes

rmw/include/rmw/discovery_options.h Outdated Show resolved Hide resolved

rmw/src/discovery_options.c Show resolved Hide resolved

sloretz added 2 commits March 28, 2023 00:46

non-const allocator

8258369

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

Document error if src == dst in copy

6086e68

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

wjwwood approved these changes Mar 28, 2023

View reviewed changes

sloretz added 2 commits March 29, 2023 21:33

SYSTEM_DEFAULT

cbd538f

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

NOT_SET and SYSTEM_DEFAULT values

f058084

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

sloretz changed the title ~~Improve handling of dynamic discovery - rmw changes~~ [rmw] Improve handling of dynamic discovery Mar 29, 2023

sloretz and others added 4 commits March 30, 2023 15:55

LOCALHOST default in discovery_options_init

c6b7949

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

add message about NOT_SET not being valid for rmw to act on

2dbd6ef

Signed-off-by: William Woodall <william@osrfoundation.org>

strncpy_s on windows

82a22d3

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

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

b769c85

wjwwood approved these changes Apr 7, 2023

View reviewed changes

sloretz merged commit 418781a into rolling Apr 8, 2023

delete-merged-branch bot deleted the gbiggs/discovery-peers-specification branch April 8, 2023 04:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rmw] Improve handling of dynamic discovery #338

[rmw] Improve handling of dynamic discovery #338

gbiggs commented Dec 14, 2022 •

edited

Loading

ivanpauno left a comment

fujitatomoya Feb 1, 2023

mxgrey Mar 27, 2023

sloretz Mar 27, 2023

wjwwood Mar 28, 2023

sloretz Mar 28, 2023

fujitatomoya Feb 2, 2023

arjo129 Mar 27, 2023

fujitatomoya Feb 2, 2023

arjo129 Mar 27, 2023

ros-discourse commented Feb 21, 2023

This comment was marked as resolved.

wjwwood left a comment

wjwwood commented Mar 27, 2023

arjo129 commented Mar 27, 2023

clalancette commented Mar 27, 2023

sloretz commented Mar 27, 2023

wjwwood left a comment

		/// Uses ROS_LOCALHOST_ONLY environment variable.
		RMW_AUTOMATIC_DISCOVERY_RANGE_DEFAULT = 0,

		/// Allows multicast on the reachable network
		RMW_AUTOMATIC_DISCOVERY_RANGE_SUBNET = 3,

[rmw] Improve handling of dynamic discovery #338

[rmw] Improve handling of dynamic discovery #338

Conversation

gbiggs commented Dec 14, 2022 • edited Loading

ivanpauno left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ros-discourse commented Feb 21, 2023

This comment was marked as resolved.

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood commented Mar 27, 2023

arjo129 commented Mar 27, 2023

clalancette commented Mar 27, 2023

sloretz commented Mar 27, 2023

wjwwood left a comment

Choose a reason for hiding this comment

gbiggs commented Dec 14, 2022 •

edited

Loading