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

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

Merged

Conversation

arjo129
Copy link
Contributor

@arjo129 arjo129 commented Feb 6, 2023

Takes off from where #653 leaves off. Currently added support for IPv4 addresses as well as hostname based addressing.

Current status according to goal matrix (Ideal goal matrix can be found here, tools to compute the matrix can be found here):

Other Host
                           static defined       static not defined
                           1      2      3      1      2      3      default      
static defined     1       ✓      ✓      ✓      ✗      ✗      ✓      ✗      
                   2       ✓      ✓      ✓      ✗      ✗      ✓      ✓      
                   3       ✓      ✓      ✓      ✗      ✗      ✓      ✓      
static not defined 1       ✗      ✗      ✗      ✗      ✗      ✗      ✗      
                   2       ✗      ✗      ✗      ✗      ✗      ✗      ✗      
                   3       ✓      ✗      ✓      ✗      ✗      ✗      ✓      
                   default ✓      ✓      ✗      ✗      ✗      ✓      ✓      

@EduPonz
Copy link
Contributor

EduPonz commented Feb 6, 2023

Hi @arjo129,

A while back we had some conversations with @gbiggs and @clalancette about this. As a guideline, I prepared a PoC using a Fast DDS example that implements all this functionality. You can check it out in this branch. Important notes there are:

  1. All the expected functionality works on that PoC.
  2. It depends on a PoC implementation of DomainParticipant::ignore_participant that is not in master (although we plan to have it for Fast DDS v2.10 which in the intended Fast DDS version for ROS 2 Iron).
  3. Fast DDS already resolves DNS names, so you don't need to worry about those.
  4. At the time, I had to make some assumptions on how thing would be configured from a configuration formatting point of view, but I think you can adapt those easily.

Hope that helps!

@arjo129
Copy link
Contributor Author

arjo129 commented Feb 6, 2023

Hi @EduPonz, thanks for pointing me in this direction. I might be using the wrong branch for the tests. I'll take a look at the work done and see what needs to be done.

@arjo129
Copy link
Contributor Author

arjo129 commented Feb 15, 2023

Current status (some of the check marks are quite flaky, for instance a participant could be discovered but then message delivery does not take place):

☑️ means correct behaviour, ❌ means incorrect.

samehost
                               static defined                   static not defined
                               LOCALHOST  OFF        SUBNET     LOCALHOST  OFF        SUBNET     default
static defined      LOCALHOST  ✓          ✓          ✓          ✓          ✓          ✓          ✗
                    OFF        ✓          ✓          ✓          ✗          ✓          ✗          ✗
                    SUBNET     ✓          ✓          ✓          ✓          ✗          ✓          ✗
static not defined  LOCALHOST  ✓          ✓          ✓          ✓          ✓          ✓          ✗
                    OFF        ✓          ✓          ✓          ✓          ✓          ✓          ✗
                    SUBNET     ✓          ✓          ✓          ✓          ✓          ✓          ✗
                    default    ✓          ✓          ✓          ✓          ✓          ✓          ✗

Other Host
                               static defined                   static not defined
                               LOCALHOST  OFF        SUBNET     LOCALHOST  OFF        SUBNET     default
static defined      LOCALHOST  ✓          ✓          ✓          ✓          ✓          ✓          ✓
                    OFF        ✓          ✓          ✓          ✓          ✓          ✗          ✓
                    SUBNET     ✓          ✗          ✗          ✓          ✓          ✓          ✓
static not defined  LOCALHOST  ✓          ✓          ✓          ✓          ✓          ✓          ✓
                    OFF        ✓          ✓          ✓          ✓          ✓          ✓          ✓
                    SUBNET     ✓          ✓          ✓          ✓          ✓          ✓          ✗
                    default    ✓          ✓          ✓          ✓          ✓          ✗          ✗

@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

@wjwwood wjwwood changed the base branch from rolling to gbiggs/discovery-peers-specification March 8, 2023 07:53
@wjwwood wjwwood changed the base branch from gbiggs/discovery-peers-specification to rolling March 8, 2023 07:56
@wjwwood
Copy link
Member

wjwwood commented Mar 8, 2023

After discussing it we decided to merge all of these into the original pr's and go from there. However, it's not clear to me if this replaces @gbiggs pr or if it replaces it. Currently this targets rolling and doesn't have commits from the other pr, so I'm assuming the latter, but then it needs rebasing.

arjo129 and others added 8 commits March 21, 2023 13:26
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>
Signed-off-by: Arjo Chakravarty <arjo@openrobotics.org>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Two more to go.

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
See:
* ros2/rmw#349
* ros2/rcl#1038

Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Arjo Chakravarty <arjoc@intrinsic.ai>
Signed-off-by: Michael X. Grey <grey@openrobotics.org>
@wjwwood wjwwood changed the base branch from rolling to gbiggs/discovery-peers-specification March 21, 2023 20:26
@wjwwood wjwwood marked this pull request as ready for review March 21, 2023 20:27
@wjwwood
Copy link
Member

wjwwood commented Mar 21, 2023

Merging with #653

@wjwwood wjwwood merged commit 8f1ac6b into ros2:gbiggs/discovery-peers-specification Mar 21, 2023
2 checks passed
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

5 participants