ros2 / rcl_interfaces

A repository for messages and services used by the ROS client libraries
Apache License 2.0
38 stars 42 forks source link

Add GetTypeDescription.srv (rep2011) #153

Closed emersonknapp closed 1 year ago

emersonknapp commented 1 year ago

Part of https://github.com/ros2/ros2/issues/1159

Adds service interface for retrieving type descriptions from nodes

emersonknapp commented 1 year ago

@allenh1 @methylDragon @clalancette @wjwwood requesting review, this has no dependencies

emersonknapp commented 1 year ago

funny how it doesn't let you re-request all reviews, it removes everyone else if you re-request one person. anyway @allenh1 @clalancette @wjwwood, updated per discussion, happy to get feedback on the language in there, I tried to make it as clear as I could

methylDragon commented 1 year ago

I was looking at the REP again and realized that we didn't discuss in detail how we would be including information about the serialization library used to generate the buffer on the wire. I was wondering if it makes sense to have a "serialization library name" and/or version field in the service response for the publishers to state what form of buffer they are publishing, or if that sort of information should be in the implementation specific key-value pairs instead...

I understand from the discussion on the REP that we're not intending to support cross protocol communications, but information like serialization library name in my opinion is universalisable enough that putting it in an implementation specific key-value pair is probably not the right call, I think?

This might be out of scope, but I'm thinking of a use case where a user might send a protobuf byte buffer (as a bytestring) through FastRTPS, then comes out the other end and has to be interpreted. (i.e. the middleware implementation is treated as the same, but not the serialization.) Or perhaps more saliently, the bytestrings are stored in a rosbag and need to be interpreted.

wjwwood commented 1 year ago

This might be out of scope, but I'm thinking of a use case where a user might send a protobuf byte buffer (as a bytestring) through FastRTPS, then comes out the other end and has to be interpreted. (i.e. the middleware implementation is treated as the same, but not the serialization.) Or perhaps more saliently, the bytestrings are stored in a rosbag and need to be interpreted.

This must be handled on the transport layer in my opinion. We could include this information here, but I don't think it's actionable information. For example, if you are subscribing with fastrtps that uses CDR, and they are publishing fastrtps that is using protobuf, then they should never match and exchange data. The fact that this information might be reflected in the key-value pairs would only possibly be useful for debugging?

Put a different way, the key-value pairs should only be used to exchange information that could not be gathered on the receiving end, and I think the serialization type is either the same (and therefore you can get it on the subscription side) or is different and they should never exchange data.

However, something like library version might be interesting. In that case it might be that they match and communicate (say Humble to Rolling or something), but the libraries being used might have different versions, and this could potentially be useful information and information that the subscription side doesn't have.

methylDragon commented 1 year ago

This might be out of scope, but I'm thinking of a use case where a user might send a protobuf byte buffer (as a bytestring) through FastRTPS, then comes out the other end and has to be interpreted. (i.e. the middleware implementation is treated as the same, but not the serialization.) Or perhaps more saliently, the bytestrings are stored in a rosbag and need to be interpreted.

This must be handled on the transport layer in my opinion. We could include this information here, but I don't think it's actionable information. For example, if you a subscribing with fastrtps that uses CDR, and they are publishing fastrtps that is using protobuf, then they should never match and exchange data. The fact that this information might be reflected in the key-value pairs would only possibly be useful for debugging?

Put a different way, the key-value pairs should only be used to exchange information that could not be gathered on the receiving end, and I think the serialization type is either the same (and therefore you can get it on the subscription side) or is different and they should never exchange data.

However, something like library version might be interesting. In that case it might be that they match and communicate (say Humble to Rolling or something), but the libraries being used might have different versions, and this could potentially be useful information and information that the subscription side doesn't have.

I'm still a little unclear on this; I thought it was possible for FastRTPS to publish and subscribe to an opaque bytestream, and if that were the case, that it would be possible to shove in any bytestream from any serialization library and send it on the wire (as long as both the pub and sub are using FastRTPS as the middleware.)

So in that case, wouldn't there be a case where the middlewares match, the endpoints match, but the buffers themselves can't get interpreted (unless the correct serialization library is used to deserialize the messages?)


On the flipside, I can see how getting the GetTypeDescription service to work in the different-serialization-scenario described above to be pretty tough... I suppose then the only valid use-case would be if both sides are natively using the same middleware and serialization library, but for some reason the user decided to publish dynamic data with a different serialization library.

wjwwood commented 1 year ago

I'm still a little unclear on this; I thought it was possible for FastRTPS to publish and subscribe to an opaque bytestream, and if that were the case, that it would be possible to shove in any bytestream from any serialization library and send it on the wire (as long as both the pub and sub are using FastRTPS as the middleware.)

So in that case, wouldn't there be a case where the middlewares match, the endpoints match, but the buffers themselves can't get interpreted (unless the correct serialization library is used to deserialize the messages?)

In this case, I would argue that a) the serialization type of the content in the octet sequence should be communicated in the type name, e.g. SerializedProtobuf.msg, or some other external way, and that b) the middleware does not and should not know this information, and so I don't know how it would be added to these key-value pairs. I suppose anyone could put anything they wanted in the key-value pairs with some changes to rcl (or if they had a non-rcl based client library), but this is still not the job of the middleware.

wjwwood commented 1 year ago

but for some reason the user decided to publish dynamic data with a different serialization library.

That just cannot work, I think.

wjwwood commented 1 year ago

I'm still a little unclear on this; I thought it was possible for FastRTPS to publish and subscribe to an opaque bytestream, and if that were the case, that it would be possible to shove in any bytestream from any serialization library and send it on the wire (as long as both the pub and sub are using FastRTPS as the middleware.)

Sorry, I think I misunderstood your original point, it's not a message that contains bytes, but that you're using the "take serialized" methods from Fast-RTPS so it presumably doesn't have to deserialize it. If it tried that would fail because Fast-RTPS is expecting CDR and the data is something else.

Again, in this situation, that cannot happen. Deserializing or not, Fast-RTPS cannot match an endpoint that uses a different serialization scheme. I do not think that's current possible, and I don't think it should be allowed either.

emersonknapp commented 1 year ago

Gist: https://gist.githubusercontent.com/emersonknapp/4859c688aec691dc8d1f95fc9478e54f/raw/0d17f281cd87c92cf68378d663c4023280970d51/ros2.repos BUILD args: --packages-above-and-dependencies type_description_interfaces TEST args: --packages-above type_description_interfaces ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11644

emersonknapp commented 1 year ago

@wjwwood @james-rms @allenh1 updated, changed the language around a little bit

emersonknapp commented 1 year ago

Gist: https://gist.githubusercontent.com/emersonknapp/3de13aa51c4d4c48a63baaf3a0ec801c/raw/0d17f281cd87c92cf68378d663c4023280970d51/ros2.repos BUILD args: --packages-above-and-dependencies type_description_interfaces TEST args: --packages-above type_description_interfaces ROS Distro: rolling Job: ci_launcher ci_launcher ran: https://ci.ros2.org/job/ci_launcher/11659