search

ros2 / rmw

The ROS Middleware (rmw) Interface.
Apache License 2.0
95 stars 67 forks source link

change RMW_GID_STORAGE_SIZE from 24 to 16 #328

Closed ihasdapie closed 2 years ago

ihasdapie commented 2 years ago

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 https://github.com/ros2/rmw/commit/f45e4cef6d9e3e28a282b46abe40ce71bf4a0907 for the content filtering feature https://github.com/ros2/rmw/pull/35) From my understanding the last 8 bytes are never checked outside of a few tests.

ConnextDDS

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

CycloneDDS

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

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.

https://github.com/ros2/rmw/blob/2259c3f6f3de8c479a9d5c74cdcd03d0010413cd/rmw/include/rmw/types.h#L356-L363

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

ihasdapie commented 2 years ago

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

christophebedard commented 2 years ago

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 commented 2 years ago

Test regressions introduced by this PR resolved in https://github.com/ros2/rmw_dds_common/pull/63

ihasdapie commented 2 years ago

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 commented 2 years ago

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

clalancette commented 2 years ago

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 commented 2 years ago

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

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

rmw_email

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

gurumdds

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

rmw_microxrcedds

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

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

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

cc @clalancette

ihasdapie commented 2 years ago
wjwwood commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

@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:

https://github.com/ros2/rmw/blob/b9036d42dab44b2a44e3da6ed1ad347cef1cd449/rmw/include/rmw/types.h#L359

and when we decided to add a getter for this identifier we decided to store it in a rmw_gid_t (see https://github.com/ros2/rmw/pull/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 commented 2 years ago

It seems like changing this line:

https://github.com/ros2/rmw/blob/2259c3f6f3de8c479a9d5c74cdcd03d0010413cd/rmw/include/rmw/types.h#L359

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 commented 2 years ago

It seems like changing this line:

https://github.com/ros2/rmw/blob/2259c3f6f3de8c479a9d5c74cdcd03d0010413cd/rmw/include/rmw/types.h#L359

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 commented 2 years ago

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.