-
Notifications
You must be signed in to change notification settings - Fork 136
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
REP 2007: Type Adaptation #303
REP 2007: Type Adaptation #303
Conversation
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
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.
looking good, just a few comments
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
As I wrote in ros2/examples#300, I do really like type masquerading. But as I also admitted, not because it's necessarily the standard terminology for this, but because of what it conveys: register some type pretending to be another type. But thinking about it more, it's not necessarily actually true. As @wjwwood writes:
this is no longer pretending, this is basically adding first-class support for non-msg types. Would that perhaps need to be represented somehow in the name for the technique? |
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.
Point of clarification - is this appropriate as a REP or does it belong in https://github.com/ros2/design? I ask because I haven't seen feature designs for ROS 2 in REP, to date - though there are some for ROS.
We're not quite there yet, but the long term goal is to retire design.ros2.org and move that content elsewhere. We haven't thought through all of the details (which is why we haven't announced it), but we're going to try to reduce the number of new things over there. |
Co-authored-by: Emerson Knapp <537409+emersonknapp@users.noreply.github.com>
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.
IIUC the proposal is that the adapter will have some functions to convert from a native type to a ros type and the opposite.
On the other hand, ROS 1 needed functions to serialize/deserialize the custom message to a stream (see here).
I think that the ROS 1 feature was way more efficient because of avoiding an extra copy, the proposal here will only improve zero-copy intraprocess communication.
There are some disadvantages of the ROS 1 approach though:
- It's harder to implement an adapter.
Ideally, it should be possible to implement an adapter in the two ways (the simpler one, and the one that allows better performance). - In the current rmw architecture, it's extremely hard to implement type masquerading the "ROS 1 way".
About the second point, the problem is that though in ROS 2 you can publish/take a serialized message, there's no "rmw" interface for the serializer/deserializer (so you have to use the one your rmw vendor uses).
e.g.: "typesupports" aren't "too different" from type masquerading, it's conceptually the same, allowing more than one representation for the message (e.g. C and CPP).
But unfortunately, because of not having an standard serializer/deserializer API, we generate C/CPP typesupport code for each rmw vendor (e.g. std_msgs connext C/CPP typesupport, fastrtps C/CPP typesupport, etc; except of a rmw vendor independent std_msgs C/CPP typesupport).
The effort of improving that situation is unfortunately big.
The plan is to support both convert and (de)serialize functions, and require at least one or the other, but also allow both. The reason for this is that convert is superior for intra-process communication and the (de)serialization functions are better for inter-process communication. We plan to start with the "convert" functions only because it gets the UX benefit of this feature, at the cost of extra conversions for inter-process communications, but no worse than what the user can do now. Adding support for the (de)serialize functions is in scope for this work, but will be done in a second step most likely. But I think this REP will eventually cover a plan for both cases. As you said, it will require either extending the rmw API to have a generic serialization API similar to Fast-CDR or something, or it will require allowing users to define custom typesupport structures, which would probably be rmw specific. Currently we are working on the machinery to handle all of this at the rclcpp API level (template metaprogramming). |
That's super cool!!!!
Yes, that's a good point. |
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.
lgtm
Co-authored-by: Jacob Perron <jacob@openrobotics.org>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow.nash@gmail.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com> Co-authored-by: William Woodall <william+github@osrfoundation.org>
Signed-off-by: Audrow Nash <audrow@hey.com>
Signed-off-by: Audrow Nash <audrow@hey.com>
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.
@audrow can you make a discourse post about this so we can get more eyes on it before merging it as accepted.
We've already started the reference implementation, but we have not finished all parts so it will remain "accepted" but not yet "final".
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/looking-for-feedback-on-ros-2-type-adaptation-rep/20465/1 |
I just read through the doc and I'm a bit concerned about the points raised in section What impacts would Currently, any language client library that at least supports the basic types listed in the Interface Definition and Language Mapping can easily interoperate with existing message types in the present ROS ecosystem, allowing users to abstract away what language or library is used on the other end of a topic, service or action. For example: if using OpenCV matrixes becomes a common pattern for exchanging messages between sensor drivers or machine learning pipelines written in C++, how would downstream users integrate community packages requiring such type adaptation when the user's language and client library have no such equivalent data types or library support? |
Chiming in as an author of a third-party language client library, and seconding @ruffsl concern. I have not sufficient expertise on ROS2 internals (my interaction is purely via If someone has the time to explain a bit the consequences for other languages I'd be grateful. From my reading, as there's the need to provide a conversion, in the end this should be transparent in the sense that non-rclcpp clients would see the ROS2-converted representation. But I'm not really sure. |
The language or library specific type isn't actually sent on the wire. (see the examplles in the REP, of
e.g. in that case, a ROS interface would be needed to adapt the opencv matrix to, so the other languages could use the bindings generated for that interface. |
Ah, re-reading doc in the morning I see I was mistaken on that point.
Ok, would the following be the equivalent example for the more involved
What would this then look like for rclpy, or if the matrix dimensionality >3, or if a ROI is defined? Would these be a lossy adaptation that the adapter designer have to consider? Is there way to ensure adapters in different languages are consistent with each other, or is that already captured by a separate REP standard? |
I think that those decisions depends on the adapter, though I would only write adapters for things that have a reasonable 1 to 1 conversion.
ROS messages have representations on multiple languages, but the types that can be adapted to a message ROS don't necessarely have. |
There's nothing in rclpy to handle this, and at this point, I'd argue should be a separate REP (which does not exist yet). This is unfortunate, but it's not something that we had time for in this work. I'll also point out there was no special logic in Separately, we could create a REP (or extend this REP), to include technical details on how different client libraries could share logic and/or avoid copies by working together via
And also:
Additionally, this enhancement (which is
All of these things would be up to the author of the adapter. I think you're getting hung up on
No. Inherently, adapters in different languages will look different because the types they are adapting (at least on the custom type side) are likely different. I.e., the mat object in pyopencv is subtly different from the c++ If you want to ensure the a) an adapter exists for a specific language, or b) that it is consistent with other languages, then I think that should be handled on a case by case basis. At the moment, this adapter will probably live in the
It would be reasonable to have a REP about how to handle the adaption in different languages, specifically for OpenCV's Mat class and the ROS interface |
I'll try to clarify this. We would have liked to have some functionality in
As I said above in a previous comment, taking this route does not prevent us from having similar functionality in I also would love to have as much logic in
This feature of At the moment, only the conversion methods ( Hopefully that answers your questions? |
Thanks @wjwwood and @ivanpauno for the clarifications. This type adaption looks like it could improve the ergonomics quite a bit in using rclcpp with common message types.
Interesting. Would there be any other past/present docs or examples that go over this that I could check out. I'd just like to learn more. |
You could read this comment and the preceding discussion: Also, this serialization/deserialization is how it worked in ROS 1: https://wiki.ros.org/roscpp/Overview/MessagesSerializationAndAdaptingTypes#Serializer_Specialization There was never a "convert to and from" API in ROS 1, so only the serialization thing was an option. The convert to/from is still useful though, since it is potentially more ergonomic for the user and can be more efficient when the publisher and subscriber are in the same process. |
Truly a great decision, I'd say. I only wish it was exploited even more :)
Thanks for taking the time to explain, @wjwwood. I understand better now the pros and cons of doing it at the cpp level. This was commented before in this thread (by @ivanpauno):
[My following questions are off-topic. Excuse me, and feel free to ignore, but this thread is directly touching matters I've chased for The crux of the issue is that I'd like to avoid doing unnecessary copying of data. If one copy is unavoidable and being done by the C typesupport, I'd like to replace that by my own typesupport [if there's a vendor-agnostic way]. If the C typesupports do not need to make copies as they use data in place, then my question is moot and I need another way to make the data more friendly for my language [using pointer wizardry]. Thanks to everybody participating, very informative thread. |
Thanks everyone for the discussion. |
This pull request has been mentioned on ROS Discourse. There might be relevant details there: https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-08-17/33061/14 |
Adds a REP for supporting type adaptation (a.k.a. Type Masquerading) in
rclcpp
.References and related links: