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

get_entities_info_by_topic is not setting gid #290

Open
ahcorde opened this issue Oct 3, 2024 · 5 comments
Open

get_entities_info_by_topic is not setting gid #290

ahcorde opened this issue Oct 3, 2024 · 5 comments
Assignees

Comments

@ahcorde
Copy link
Contributor

ahcorde commented Oct 3, 2024

get_entities_info_by_topic is not setting gid. It's setting name, namespace, endpoint type, qos and topic_type_hash.

We need to set the gid too wit rmw_topic_endpoint_info_set_gid

There is a TODO in the code, this will fix test_node_interfaces__node_graph in rclcpp

@Yadunund
Copy link
Member

Yadunund commented Oct 3, 2024

Summarizing the state of things before describing how to attack this problem

  • Every node/pub/sub/client/service entity publishes a liveliness token which is received by all contexts in the ROS graph. The subscription callback of these tokens populate the GraphCache.
  • Within the GraphCache, various Entities are reconstructed purely from liveliness tokens using this API
  • The liveliness token only includes these components. GID is not included.
  • We uniquely identify Entities in the GraphCache using a guid that is just the hash of the liveliness token
  • This guid is different from RMW GID that we generate using this function which is used by publishers, clients and services only like here.

Right now there is a disconnect between RMW GID and the liveliness::Entity::guid. #148 ticket was created to address this.
imo the best forward is

  1. Generate the 16byte GID from the keyexpr guid. ie update generate_random_gid to do this. I don't know whats a good way to do this but open to suggestions.
  2. We don't need to add a new component to the liveliness token (save bandwidth) since we can convert the liveliness token to GID using 1). We do this inside GraphCache when returning get_entities_info_by_topic

What do you think?

@ahcorde
Copy link
Contributor Author

ahcorde commented Oct 3, 2024

the size of liveliness::Entity::guid is std::size_t. it can vary depending on the platform and architecture. On most 32-bit systems, it is usually 32 bits, while on 64-bit systems, it is typically 64 bits.

On the other hand we have uint8_t gid[RMW_GID_STORAGE_SIZE];. RMW_GID_STORAGE_SIZE is 16 bytes (128 bits). We can fit he guid inside this structure and fill the rest with zeros.

What do you think?

@Yadunund
Copy link
Member

Yadunund commented Oct 3, 2024

That works for me. @clalancette any thoughts?

@ahcorde
Copy link
Contributor Author

ahcorde commented Oct 4, 2024

Related PR #291

@clalancette
Copy link
Collaborator

That works for me. @clalancette any thoughts?

Sorry, just coming back to this.

On the other hand we have uint8_t gid[RMW_GID_STORAGE_SIZE];. RMW_GID_STORAGE_SIZE is 16 bytes (128 bits). We can fit he guid inside this structure and fill the rest with zeros.

I'm not sure I love this. The whole point of a GID is that it is supposed to be globally unique. While this is not authoritative, it does seem like we want to use the full 128-bits to represent this: https://en.wikipedia.org/wiki/Universally_unique_identifier .

What I think we should do instead is a hybrid of these. Expand the GUID that we store in the liveliness token to 128-bits (I know this is less efficient, but I don't see how we get around that)(also, this will fix the problem of size_t being different sizes on different architectures). Continue to hash the liveliness token and store it there. And then define the GID to also be the 128-bit hash of the liveliness token, so it is all aligned (the GID only needs to be unique for services to work).

What do you think about that?

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

No branches or pull requests

3 participants