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

Update documentation about expectations for GIDs #335

Merged
merged 1 commit into from
Mar 7, 2023

Conversation

jacobperron
Copy link
Member

Replaces #329
Follow-up to #328


This PR contains two changes:

  • Use gid struct for request id
    Everywhere else in rmw, we use the type rmw_gid_t for global identifiers.
    For example, rmw_get_gid_for_publisher and rmw_get_gid_for_client.
    Furthermore, other global identifiers have a maximum size of 24 bytes, not 16 bytes.
    Changing the type and name of the client id in the rmw_request_id_t struct makes it consistent with other parts of the rmw API.

  • Update documented expectations for GIDs

    • Clarify that GIDs are globally unique within a ROS domain and consistent between processes/hosts.
    • Specify that any unused bytes in the GID should be set to zero (e.g. in cases where not all 24 bytes are used).
      This allows application code to directly compare GIDs without having to ask the middleware.

The latter change is especially useful for the proposed service introspection feature. E.g. if tooling wants to compare 24 byte GIDs, we want to ensure that any unused bytes are not set to garbage values and that they are consistent between processes and hosts.

If these changes are acceptable, I'll proceed in making the necessary changes to rmw implementations to:

  • Update any references to "writer_guid" to use the new "client_gid.data" member.
  • Ensure that they set unused bytes in the GID to zeros

@jacobperron jacobperron requested a review from wjwwood October 13, 2022 21:29
@jacobperron
Copy link
Member Author

For context, for DDS implementation the DDS-RTPS spec describes a GUID as being 16 octets and globally unique within a domain.

From section 8.2.1.2:

Type used to hold globally-unique RTPS-entity identifiers. These are identifiers used
to uniquely refer to each RTPS Entity in the system.
Must be possible to represent using 16 octets.
The following values are reserved by the protocol: GUID_UNKNOWN

Specifically, for a DDS-Entity, from 8.2.4 The RTPS Entity:

Globally and uniquely identifies the
RTPS Entity within the DDS
domain

I think these align well with documentation changes proposed in this PR. But, please let me know if you think these expectations may be problematic.

@christophebedard
Copy link
Member

But, please let me know if you think these expectations may be problematic.

This means that ros2/rmw_cyclonedds#377 will have to be fixed (which I am totally in favour of, of course).

* Clarify that GIDs are globally unique within a ROS domain.
* Specify that any unused bytes in the GID should be set to zero (e.g. in cases where not all 24 bytes are used).
  This allows application code to directly compare GIDs without having to ask the middleware.

Signed-off-by: Jacob Perron <[email protected]>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've rebased this, and just kept the changes to the documentation. With that in place, this looks good to me so I'm going to go ahead and run CI on it.

@clalancette clalancette changed the title Change GID type in rmw_request_id_t and expectations for GIDs Update documentation about expectations for GIDs Mar 7, 2023
@clalancette
Copy link
Contributor

clalancette commented Mar 7, 2023

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

@clalancette
Copy link
Contributor

One of the failing tests on Windows is a known flake. The other one is new to me, but I honestly don't see how this PR could possibly have caused it, given that these are documentation updates only. So I'm going to go ahead and merge this anyway.

@clalancette clalancette merged commit 33f1cf0 into rolling Mar 7, 2023
@clalancette clalancette deleted the jacob/rmw_gid branch March 7, 2023 20:30
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.

4 participants