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

[rcl] Improve handling of dynamic discovery #1023

Merged
merged 41 commits into from Apr 8, 2023

Conversation

gbiggs
Copy link
Member

@gbiggs gbiggs commented Dec 14, 2022

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

It adds a new set of parameters for dynamic discovery, one for dynamic discovery range and one for static peers specification.

It removes the RMW_LOCALHOST_ONLY environment variable.

@ros-discourse
Copy link

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 and others added 6 commits March 21, 2023 12:58
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Only print out the warning if it is actually specified.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Mostly to get rid of the use of strtok_r, which is dangerous
and also should not be used on our static environment variables.
Instead, use rcutils_split(), which is much better.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
mxgrey and others added 5 commits March 21, 2023 13:00
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
This commit adds support for dynamic allocation for unlimited number of
static peers.

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

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Copy link
Member

@audrow audrow left a comment

Choose a reason for hiding this comment

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

The logic makes sense and the tests seem thorough, so I'd think this is good to go with green CI.

rcl/test/rcl/test_discovery_options.cpp Outdated Show resolved Hide resolved
Signed-off-by: Shane Loretz <sloretz@google.com>
rcl/include/rcl/discovery_options.h Outdated Show resolved Hide resolved
rcl/include/rcl/discovery_options.h Outdated Show resolved Hide resolved
rcl/include/rcl/discovery_options.h Outdated Show resolved Hide resolved
rcl/include/rcl/discovery_options.h Show resolved Hide resolved
rcl/src/rcl/discovery_options.c Outdated Show resolved Hide resolved
rcl/src/rcl/init.c Outdated Show resolved Hide resolved
rcl/src/rcl/init.c Outdated Show resolved Hide resolved
rcl/src/rcl/init.c Outdated Show resolved Hide resolved
rcl/src/rcl/localhost.c Outdated Show resolved Hide resolved
rcl/test/rcl/test_discovery_options.cpp Outdated Show resolved Hide resolved
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
arjo129 and others added 15 commits March 28, 2023 15:12
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
…e in test

Signed-off-by: Shane Loretz <sloretz@google.com>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@google.com>
…ingifying enums

Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood requested a review from sloretz March 29, 2023 23:14
@sloretz sloretz changed the title Improve handling of dynamic discovery - rcl changes [rcl] Improve handling of dynamic discovery Mar 29, 2023
@sloretz sloretz merged commit cf6fafa into rolling Apr 8, 2023
2 of 3 checks passed
@delete-merged-branch delete-merged-branch bot deleted the gbiggs/discovery-peers-specification branch April 8, 2023 04:35
achim-k pushed a commit to achim-k/rcl that referenced this pull request Apr 10, 2023
* Get discovery preferences from the environment

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

* Support specification of discovery range and static peers

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

* Add some debug helpers

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

* Cleanup the LOCALHOST_ONLY deprecation a bit.

Only print out the warning if it is actually specified.

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

* Rewrite parsing of static peers.

Mostly to get rid of the use of strtok_r, which is dangerous
and also should not be used on our static environment variables.
Instead, use rcutils_split(), which is much better.

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

* Fix some silly bugs.

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

* Use names instead of integers for discovery range env vars

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

* Add support for dynamic allocation

This commit adds support for dynamic allocation for unlimited number of
static peers.

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

* Add a warning if ROS_AUTOMATIC_DISCOVERY_OFF is set and STATIC_PEERS are
set.

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

* Update to latest rmw API

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

* Uncrustify

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

* Update for rmw_discovery_options_t changes

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

* Address feedback: use strncmp

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

* Address feedback: Log levels

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

* Address feedback: remove TODO

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

* Address feedback: rename function

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

* ddress feedback: Docstring

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

* Address feedback: comment

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

* Add `RCL_RET_ERROR` if fini fails.

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

* `snprintf`->`rcutils_snprintf`

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

* Rename tests

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

* rcl_get_discovery_automatic_range -> rcl_get_automatic_discovery_range in test

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

* Annotate tests

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

* More comments

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

* Style

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

* Style

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

* Uncrustify

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

* Constness and warning

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

* address TODO about IP address validation in the static peers

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

* NOT_SET and SYSTEM_DEFAULT values

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

* refactor discovery options to handle env vars better and simplify stringifying enums

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

* fixup docs

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

* (re)improve the discovery range debug message

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

* Set discovery options to NOT_SET to detect user changse

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

* lint

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

* strncpy_s on windows

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

* Change default range to SUBNET, and allow configuring it at build time

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

---------

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Shane Loretz <sloretz@google.com>
Signed-off-by: William Woodall <william@osrfoundation.org>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Michael X. Grey <grey@openrobotics.org>
Co-authored-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Co-authored-by: Shane Loretz <sloretz@google.com>
Co-authored-by: William Woodall <william@osrfoundation.org>
danthony06 pushed a commit to danthony06/rcl that referenced this pull request Jun 14, 2023
* Get discovery preferences from the environment

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

* Support specification of discovery range and static peers

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

* Add some debug helpers

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

* Cleanup the LOCALHOST_ONLY deprecation a bit.

Only print out the warning if it is actually specified.

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

* Rewrite parsing of static peers.

Mostly to get rid of the use of strtok_r, which is dangerous
and also should not be used on our static environment variables.
Instead, use rcutils_split(), which is much better.

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

* Fix some silly bugs.

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

* Use names instead of integers for discovery range env vars

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

* Add support for dynamic allocation

This commit adds support for dynamic allocation for unlimited number of
static peers.

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

* Add a warning if ROS_AUTOMATIC_DISCOVERY_OFF is set and STATIC_PEERS are
set.

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

* Update to latest rmw API

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

* Uncrustify

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

* Update for rmw_discovery_options_t changes

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

* Address feedback: use strncmp

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

* Address feedback: Log levels

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

* Address feedback: remove TODO

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

* Address feedback: rename function

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

* ddress feedback: Docstring

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

* Address feedback: comment

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

* Add `RCL_RET_ERROR` if fini fails.

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

* `snprintf`->`rcutils_snprintf`

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

* Rename tests

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

* rcl_get_discovery_automatic_range -> rcl_get_automatic_discovery_range in test

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

* Annotate tests

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

* More comments

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

* Style

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

* Style

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

* Uncrustify

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

* Constness and warning

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

* address TODO about IP address validation in the static peers

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

* NOT_SET and SYSTEM_DEFAULT values

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

* refactor discovery options to handle env vars better and simplify stringifying enums

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

* fixup docs

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

* (re)improve the discovery range debug message

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

* Set discovery options to NOT_SET to detect user changse

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

* lint

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

* strncpy_s on windows

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

* Change default range to SUBNET, and allow configuring it at build time

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

---------

Signed-off-by: Geoffrey Biggs <gbiggs@killbots.net>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Shane Loretz <sloretz@google.com>
Signed-off-by: William Woodall <william@osrfoundation.org>
Co-authored-by: Chris Lalancette <clalancette@openrobotics.org>
Co-authored-by: Michael X. Grey <grey@openrobotics.org>
Co-authored-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Co-authored-by: Shane Loretz <sloretz@google.com>
Co-authored-by: William Woodall <william@osrfoundation.org>
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.

None yet

8 participants