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

Add GetTypeDescription.srv (rep2011) #153

Conversation

emersonknapp
Copy link
Collaborator

Part of ros2/ros2#1159

Adds service interface for retrieving type descriptions from nodes

Signed-off-by: Emerson Knapp <[email protected]>
@emersonknapp emersonknapp changed the title Add GetTypeDescription.srv [WIP] Add GetTypeDescription.srv Mar 2, 2023
@emersonknapp emersonknapp marked this pull request as ready for review March 7, 2023 19:17
@emersonknapp emersonknapp changed the title [WIP] Add GetTypeDescription.srv Add GetTypeDescription.srv Mar 7, 2023
@emersonknapp emersonknapp changed the title Add GetTypeDescription.srv Add GetTypeDescription.srv (rep2011) Mar 7, 2023
@emersonknapp
Copy link
Collaborator Author

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

@emersonknapp emersonknapp requested review from wjwwood and removed request for gbiggs March 11, 2023 00:20
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.

Changes from in-person discussion.

type_description_interfaces/srv/GetTypeDescription.srv Outdated Show resolved Hide resolved
type_description_interfaces/srv/GetTypeDescription.srv Outdated Show resolved Hide resolved
type_description_interfaces/srv/GetTypeDescription.srv Outdated Show resolved Hide resolved
# True if the type description information is available and populated in the response
bool successful
# Empty if 'successful' was true, otherwise contains details on why it failed
string failure_reason
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename this to reason instead of failure_reason.

Copy link

@achim-k achim-k Mar 23, 2023

Choose a reason for hiding this comment

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

I think something went wrong here, this became failure_reason again (1db9478)

Copy link
Member

Choose a reason for hiding this comment

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

I think that might have been intentional since it's no longer multi purpose? I'm not sure I haven't re-reviewed this yet.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct, changed back on purpose, see #153 (comment)

@emersonknapp emersonknapp requested review from clalancette and allenh1 and removed request for wjwwood, clalancette and allenh1 March 15, 2023 00:30
@emersonknapp
Copy link
Collaborator Author

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

@wjwwood wjwwood requested review from wjwwood and allenh1 March 15, 2023 00:44
@methylDragon
Copy link

methylDragon commented Mar 15, 2023

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
Copy link
Member

wjwwood commented Mar 15, 2023

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
Copy link

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.

@emersonknapp emersonknapp requested review from clalancette and removed request for allenh1 March 16, 2023 01:20
@wjwwood
Copy link
Member

wjwwood commented Mar 16, 2023

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
Copy link
Member

wjwwood commented Mar 16, 2023

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

That just cannot work, I think.

@wjwwood
Copy link
Member

wjwwood commented Mar 16, 2023

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
Copy link
Collaborator Author

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

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

@emersonknapp
Copy link
Collaborator Author

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

@emersonknapp emersonknapp requested review from wjwwood and removed request for clalancette March 22, 2023 23:40
Copy link

@allenh1 allenh1 left a comment

Choose a reason for hiding this comment

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

@emersonknapp thanks! LGTM

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 left one suggestion on how to clarify the description of type_sources. Otherwise, I really like how this turned out; this is much nicer. I'm going to approve and assume you do something to update that comment (event if you don't take exactly what I suggested).

Comment on lines 20 to 22
# List of sources, including all comments and whitespace.
# Sources can be matched with IndividualTypeDescriptions by their `type_name`.
# The `encoding` field of each entry informs how to interpret its contents.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if someone were to look at this cold, it would be hard for them to understand a) what a "source" is, and b) that this is a list of the type that was requested, and all of the recursive types it depends on. For that reason, I think we should update this comment to something like:

Suggested change
# List of sources, including all comments and whitespace.
# Sources can be matched with IndividualTypeDescriptions by their `type_name`.
# The `encoding` field of each entry informs how to interpret its contents.
# A list containing the interface definition source text of the originally requested type,
# along with all of the types it depends on (recursively).
# Each source text is a copy of the original contents of the
# `.msg`, `.srv`, `.action`, or `.idl` file, including comments and whitespace.
# Sources can be matched with IndividualTypeDescriptions in the `type_description` by
# their `type_name`.
# The `encoding` field of each entry informs how to interpret its contents.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated to approximately this language, plus more detail in TypeSource.msg. Added a new "well-known encoding" - "implicit" for subtypes of complex types (services/actions), where the source will be available at the .srv/.action level.

@emersonknapp
Copy link
Collaborator Author

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

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

@clalancette clalancette merged commit aafd90e into ros2:rolling Mar 23, 2023
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.

6 participants