change RMW_GID_STORAGE_SIZE from 24 to 16

[ihasdapie]

RMW_GID_STORAGE_SIZE is #define'd to be 24 bytes long but the GUID is 16 bytes for all currently supported RMW implementations. The reason why it is 24 bytes long is likely due to the now-legacy rmw_opensplice. (Introduced in f45e4ce for the content filtering feature #35)
From my understanding the last 8 bytes are never checked outside of a few tests.

ConnextDDS

• Implemented as 16 octects (http://community.rti.com/rti-doc/500/ndds.5.0.0/doc/html/api_cpp/structDDS__GUID__t.html)
• Internal impl truncates 24 to 16

https://github.com/ros2/rmw_connextdds/blob/79b037561a9e8a719cab6fea2b128c600ccd7113/rmw_connextdds_common/src/common/rmw_impl.cpp#L2120-L2104

CycloneDDS

• Uses 16 octets and truncates 24 to 16

https://github.com/eclipse-cyclonedds/cyclonedds/blob/0e2cd3e303be2171dd0e4fc685cc5031f70b0f52/src/core/ddsc/include/dds/dds.h#L192-L184

https://github.com/ros2/rmw_cyclonedds/blob/35c63e2dc30b9f58e3aa4a41f428310aff0cef83/rmw_cyclonedds_cpp/src/rmw_node.cpp#L808-L800

FastRTPS

• GUID_t contains a prefix (12 bytes) and entity id (4 bytes) for a total of 16 bytes.

https://github.com/eProsima/Fast-DDS/blob/e9e933b7dc4d78d59f6cf714fadbde9c62a1119e/include/fastdds/rtps/common/Guid.h#L44-L42

Knocking RMW_GID_STORAGE_SIZE down to 16 bytes (which is standardized as 16 octets) would also make rmw_gid_t consistent with rmw_request_id_t which contains a 16 byte uuid.

rmw/rmw/include/rmw/types.h

Lines 356 to 363 in 2259c3f

	typedef struct RMW_PUBLIC_TYPE rmw_request_id_s
	{
	/// The guid of the writer associated with this request
	int8_t writer_guid[16];
	/// Sequence number of this service
	int64_t sequence_number;
	} rmw_request_id_t;

Signed-off-by: Brian Chen brian.chen@openrobotics.org

[ihasdapie]

• Linux
• Linux-aarch64
• Windows

Test failures in rmw_dds_common due to a few tests expecting 24 bytes.

[christophebedard]

Just my opinion, but that doesn't really seem like a great reason to change the GID size. RMW isn't just Cyclone DDS + Fast RTPS + Connext DDS, or DDS for that matter.

[ihasdapie]

Test regressions introduced by this PR resolved in ros2/rmw_dds_common#63

[ihasdapie]

Just my opinion, but that doesn't really seem like a great reason to change the GID size. RMW isn't just Cyclone DDS + Fast RTPS + Connext DDS, or DDS for that matter.

@christophebedard
I forgot to mention this when I first opened the PR. The reason why this came up was that I found that the 24 byte rmw_gid_t would introduce a discrepancy in gid sizes in rcl where we would have to truncate the 24 byte rmw_gid_t https://github.com/ros2/rcl/blob/a697e757796c8ddee4f619045a0fd770e39ef8c8/rcl/src/rcl/client.c#L293-L305 to make it fit within the unique_identifier_msgs/UUID specified in the REP (https://github.com/ros-infrastructure/rep/blame/57ae230755fdf9c8ae79b7ebb7579c00d6986f01/rep-2012.rst#L121)

It would also be inconsistent to have rmw_get_gid_for_client return a 16 byte array whereas rmw_get_gid_for_publisher returns a 24 byte rmw_gid_t.

Taking a step back it seems like a good idea to "rip the bandaid off" now to force consistency in gids. The change currently appears fairly harmless -- AFAIK nobody uses rmw_opensplice anymore or a rmw using a non-16-byte gid, and 24 byte uuids are definitely the exception to the norm.

[christophebedard]

I see, that makes sense. Thanks for the explanation @ihasdapie.

[clalancette]

Taking a step back it seems like a good idea to "rip the bandaid off" now to force consistency in gids. The change currently appears fairly harmless -- AFAIK nobody uses rmw_opensplice anymore or a rmw using a non-16-byte gid, and 24 byte uuids are definitely the exception to the norm.

While I agree with your reasoning in general, I do think it would be worthwhile to take a look at the non-core rmw implementations to see if any of them are making assumptions about the current size of RMW_GID_STORAGE_SIZE. If it is just an ABI change, that's fine, but let's make sure.

Once we know what assumptions are out there, we can make a decision on a way forward.

[ihasdapie]

rmw_iceoryx

https://github.com/ros2/rmw_iceoryx/blob/b8d547208928567dea477715356de1e6ca3943b6/rmw_iceoryx_cpp/src/internal/iceoryx_generate_gid.cpp#L28
https://github.com/eclipse-iceoryx/iceoryx/blob/2899cbce67cd2681ed2a0455bc303aa7310a5e01/iceoryx_hoofs/platform/win/include/iceoryx_hoofs/platform/types.hpp#L22
https://github.com/eclipse-iceoryx/iceoryx/blob/2899cbce67cd2681ed2a0455bc303aa7310a5e01/doc/design/chunk_header.md#L50

• Internal uid apperas to be a uint64. So this change shouldn't break rmw_iceoryx

rmw_dps

Internal uid type is uint8_t[16] https://intel.github.io/dps-for-iot/struct___d_p_s___u_u_i_d.html

rmw_ecal

https://github.com/eclipse-ecal/ecal/blob/218d0625a98282a71a63f459cb8b4ef601c0288b/ecal/core/include/ecal/ecal_publisher.h#L354

• Largely directly uses rmw_gid_t, internally appears to use a long long

rmw_email

https://github.com/christophebedard/rmw_email/blob/c0110b6fc6a61f389ec58f54496c39f6026a4a13/email/include/email/gid.hpp#L30

• Uses uint32_t, copies directly

gurumdds

https://github.com/ros2/rmw_gurumdds/blob/c982e8624c2409cb84533e7c831c658b984e3b35/rmw_gurumdds_cpp/include/rmw_gurumdds_cpp/types.hpp#L457-L453

• uses uint8_t[16]

rmw_microxrcedds

https://github.com/eProsima/Micro-XRCE-DDS-Client/blob/6f4edd25c0984fd30bf3bf84e2d1d33af03f24a1/include/uxr/client/core/session/object_id.h#L41-L46

• Uses a uint16_t and a uint8_t.

rmw_zenoh

https://github.com/eclipse-zenoh/zenoh/blob/2a0028e9bffb7da2d84e014190c8b1eaca2bb346/zenoh/src/net/types.rs#L166
https://github.com/eclipse-zenoh/zenoh/blob/2a0028e9bffb7da2d84e014190c8b1eaca2bb346/zenoh-protocol/src/core/mod.rs#L164-L186

• 16 u8s

After that digging I think it's reasonably safe to conclude that merging this shouldn't break popular existing rmws.

cc @clalancette

[ihasdapie]

• Linux
• Linux-aarch64
• Windows

[fujitatomoya]

lgtm

[jacobperron]

LGTM, but I'd like other members of @ros2/team to review as well.

[wjwwood]

I forgot to mention this when I first opened the PR. The reason why this came up was that I found that the 24 byte rmw_gid_t would introduce a discrepancy in gid sizes in rcl where we would have to truncate the 24 byte rmw_gid_t https://github.com/ros2/rcl/blob/a697e757796c8ddee4f619045a0fd770e39ef8c8/rcl/src/rcl/client.c#L293-L305 to make it fit within the unique_identifier_msgs/UUID specified in the REP (https://github.com/ros-infrastructure/rep/blame/57ae230755fdf9c8ae79b7ebb7579c00d6986f01/rep-2012.rst#L121)

I think it's important to point out that the rmw_gid_t does not necessarily contain a UUID. And so it isn't odd or inconsistent (in my opinion) that it doesn't fit into a message designed to contain a UUID. It is perhaps inconvenient or maybe even surprising, but they are actually different things and used for similar but ultimately different purposes.

I would even go so far as to say it is incorrect to try and put the contents of a rmw_gid_t into a UUID message, as a UUID has a very specific semantic meaning (https://en.wikipedia.org/wiki/Universally_unique_identifier). You can sometimes see it referred to as a GUID, but that is also different from a GID, the "unique" part being somewhat important. Specifically, the GIDs used in most rmws are not uniquely generated and referenced elsewhere, but instead they are predictably generated (based on things like identifying properties of the machine you're on, the network you're using, or the port your DDS participant is on). Whereas GUID/UUID are not specific to DDS or middlewares, designed to be practically unique when generated randomly, and are not tied in anyway to how they are being used (they are not reversible in any way).

The old OpenSplice GID did contain reversible information I think, e.g. like the domain or host information was part of it I think. Now that may not be a good idea or practical, but it is an example of how it differs semantically from UUIDs.

So if that is the main concrete motivation for changing the size of this, then I would push back on that.

That being said, changing the size back to 16-bytes is fine with me, if that works for all of the rmw implementations out there, but even if we do that "casting" it into a UUID isn't a good ideal still, at least in my mind.

[wjwwood]

To further my point, rmw_iceoryx uses:

Internal uid apperas to be a uint64. So this change shouldn't break rmw_iceoryx

Well a UUID is 128-bit and that's part of what makes it "universal" is the size of it (which allows it to be practically unique due to the space of numbers from which you're selecting one randomly). And if the "uid" iceoryx is creating is stored in only a 64-bit space then it cannot have the same guarantees that people assume from a UUID.

My point being, it would be misleading to take this value and advertise it as a UUID. Because anyone consuming it cannot safely make assumptions about it as if it were a UUID.

[jacobperron]

@wjwwood Your distinction between a GID and a GUID (or UUID) makes sense to me.

I guess it was confusing because all of the default rmw's use the term "guid", and explicitly convert those identifiers to "gid" when implementing the rmw API.

Also, the identifier for service request writers are represented as a 16 byte "guid" here:

rmw/rmw/include/rmw/types.h

Line 359 in b9036d4

int8_t writer_guid[16];

and when we decided to add a getter for this identifier we decided to store it in a rmw_gid_t (see #327). This is what sparked this pull request. Perhaps this is incorrect, and we should either define a new rmw_uuid_t type or change/document the existing references to writer_guid to clarify that is a "gid". Maybe change the API so it has the type rmw_gid_t.

All of this is motivated by the new service introspection feature, which aims to use the service client identifier to track request-response pairs over time. Ultimately, we just need to know the length of that identifier (16 or 24 bytes), we don't care if it is globally unique.

[jacobperron]

It seems like changing this line:

rmw/rmw/include/rmw/types.h

Line 359 in 2259c3f

int8_t writer_guid[16];

to

rmw_gid_t writer_gid;

is the more appropriate change to make (instead of this PR). Though it seems a lot more disruptive.

[ihasdapie]

It seems like changing this line:

rmw/rmw/include/rmw/types.h

Line 359 in 2259c3f

int8_t writer_guid[16];

to

rmw_gid_t writer_gid;

is the more appropriate change to make (instead of this PR). Though it seems a lot more disruptive.

I think the crux of the problem is that rmw seems to only really have a notion of a gid type; guid shows up spuriously in exactly one line of source code in ros2/rmw and existing implementations just dump their own gid into the guid field anyways without regard for the u in guid (see william's coments above).

Another argument for why it doesn't make sense to force dds to provide a UUID is that some rmw implementations for resource-constrained environments e.g. Micro-XRCE-DDS have smaller internal gid types.

I think the right choice to make would be to either implement Jacob's suggestion to nest a rmw_gid_t into rmw_request_id_t, or alternatively just rename it to

  uint8_t writer_gid[RMW_GID_STORAGE_SIZE];

Either way it'll involve the awfully tedious task of opening a bunch of small PRs to change that

[clalancette]

We decided that while we could shrink this to 16 bytes, there isn't really a need to do so. There may be RMWs in the future that need extra bytes. We do recognize that there are some possible inconsistencies in the current code, so we should have an issue for that and fix those. Because of all of that, closing this.