-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add GetTypeDescription.srv (rep2011) #153
Conversation
Signed-off-by: Emerson Knapp <[email protected]>
@allenh1 @methylDragon @clalancette @wjwwood requesting review, this has no dependencies |
Signed-off-by: Emerson Knapp <[email protected]>
There was a problem hiding this 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.
# 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 |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
Signed-off-by: Emerson Knapp <[email protected]>
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 |
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. |
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. |
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 |
Signed-off-by: Emerson Knapp <[email protected]>
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. |
That just cannot work, I think. |
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. |
Gist: https://gist.githubusercontent.com/emersonknapp/4859c688aec691dc8d1f95fc9478e54f/raw/0d17f281cd87c92cf68378d663c4023280970d51/ros2.repos |
Signed-off-by: Emerson Knapp <[email protected]>
@wjwwood @james-rms @allenh1 updated, changed the language around a little bit |
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
Signed-off-by: Emerson Knapp <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@emersonknapp thanks! LGTM
There was a problem hiding this 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).
# 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. |
There was a problem hiding this comment.
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:
# 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. |
There was a problem hiding this comment.
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.
Signed-off-by: Emerson Knapp <[email protected]>
Gist: https://gist.githubusercontent.com/emersonknapp/3de13aa51c4d4c48a63baaf3a0ec801c/raw/0d17f281cd87c92cf68378d663c4023280970d51/ros2.repos |
Part of ros2/ros2#1159
Adds service interface for retrieving type descriptions from nodes