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

Incompatible with Crystal #1

Closed
AAlon opened this issue Apr 17, 2019 · 6 comments
Closed

Incompatible with Crystal #1

AAlon opened this issue Apr 17, 2019 · 6 comments

Comments

@AAlon
Copy link

AAlon commented Apr 17, 2019

I'm using a source installation of ROS2 from tip of master on Ubuntu Bionic.
Built CycloneDDS, exported CycloneDDS_DIR=<install-dir>/share/CycloneDDS and appended LD_LIBRARY_PATH with <install-dir>/lib.
Cloned this repo into the src/ros2/ dir, and ran colcon build --symlink-install --cmake-force-configure which failed.

First error was rmw_cyclonedds_cpp/TypeSupport_impl.hpp trying to include a non existing file - "rosidl_generator_c/primitives_array_functions.h", which has been renamed to "rosidl_generator_c/primitives_sequence_functions.h" in crystal. After fixing that there were a ton of other issues though.

@eboasson
Copy link
Collaborator

Hi @AAlon,

I'm not surprised there are problems: it built at the time of the most recent commit, but that's a while ago. Contrary to appearances, that does not mean it has been abandoned, but rather that it got derailed for a bit by various other things. One of those things is that I sometimes have to do other things than Cyclone, the other is that it uses the ugliest of hacks for (de)serialising the data to get the proof-of-concept as quickly as possible.

As you can see I heavily borrowed from the then-current FastRTPS RMW layer for serialisation based on ROS2's type introspection, as it allowed me to avoid worrying about IDL preprocessing and how to integrate that with CMake. Since then, the ROS2 IDL pipeline changed dramatically and I believe the need for translating ROS2 msgidl to OMG IDL for further preprocessing no longer exists. So it may actually have become a lot easier.

Be that as it may, there were two limitations in Cyclone at the time that caused me some problems:

  1. The interface of Cyclone at the time did allow reading/writing raw CDR but didn't provide an interface to construct or manipulate those samples. The sources of this (partial) RMW implementation work around that by copy-pasting fragments of internal header files of Cyclone.

  2. At the time Cyclone did not yet implement the DDS built-in topics and could not implement the introspection functions of RMW.

Properly addressing the second necessitated changes to the internal representation of a sample. It is now generalised to an abstract thing that you can copy to/from application, to/from serialised form, and on which you can compare key values — however implemented. Regular data topics generated by the IDL preprocessor of Cyclone have a completely different internal representation than the built-in topics do. The casualty of this improvement is the hack I did in this RMW implementation and I suspect that's where the "ton of other issues" comes from.

The good news is that those changes also make it possible to remove all the copy-pasted fragments of those internal header files in this RMW implementation; the bad news is that (1) I still have to press the button to actually merge the handful of lines of CMake code to include the relevant Cyclone header files when installing it; and (2) that the updating of this RMW layer then still needs to be done.

That should bring it back up with the ROS2 version of that time. I know there have been some significant changes and extensions to the RMW layer since then, but I haven't looked at those in detail. I know the oft-discussed problem of reading/writing raw CDR is a triviality for Cyclone. I believe topic naming changed, I'm pretty sure there'll be some interoperability issue with, say, service invocations, but for the rest I have no idea yet.

It is hardly necessary to say that any help would be very much welcomed. In any case, I do intend to try getting it back up this month, even if it will still only be on that older version of ROS2.

@AAlon
Copy link
Author

AAlon commented Apr 17, 2019

There's a bit of a chicken and egg problem here - if we can't try it we don't have the data to say that this is something we'd want to invest in and contribute to :)
For context - we wanted to evaluate performance, and for that we'd prefer to use the latest version of ROS2. Also note that ROS2 Dashing is coming out in about two months, so it might be wise to skip crystal and invest in ROS2-D support. Anyway, we'll make sure to check back at a later time. Thanks!

@eboasson
Copy link
Collaborator

There's a bit of a chicken and egg problem here - if we can't try it we don't have the data to say that this is something we'd want to invest in and contribute to :)

Fair enough! Let me do my best to get it back to a working state. I can ping you when it works (that would be minimal support that I expect will allow you to do a performance evaluation), as full support may take a while.

For context - we wanted to evaluate performance, and for that we'd prefer to use the latest version of ROS2. Also note that ROS2 Dashing is coming out in about two months, so it might be wise to skip crystal and invest in ROS2-D support. Anyway, we'll make sure to check back at a later time. Thanks!

Fair enough as well …

I'm likely to meet some of the ROS2 guys soonish so hopefully they can give me some hints 😁

@eboasson
Copy link
Collaborator

eboasson commented Jun 3, 2019

@AAlon: just wanted to drop a note that it now works with Dashing and that all operations are supported. Of course there are some limitations and no doubt some dark corners with monsters also exist. I intend to drop a note on the ROS2 forums one of these days.

As to the version of Cyclone DDS: you have to use a recent commit — preferably simple the head of master, as the 0.1.0 is too old to support this. High on the agenda is getting a 0.2.0 release of Cyclone in the early summer as a baseline version for ROS2 support.

@AAlon
Copy link
Author

AAlon commented Jun 6, 2019

@AAlon: just wanted to drop a note that it now works with Dashing and that all operations are supported. Of course there are some limitations and no doubt some dark corners with monsters also exist. I intend to drop a note on the ROS2 forums one of these days.

As to the version of Cyclone DDS: you have to use a recent commit — preferably simple the head of master, as the 0.1.0 is too old to support this. High on the agenda is getting a 0.2.0 release of Cyclone in the early summer as a baseline version for ROS2 support.

Thanks for the update! we've started experimenting with it and the performance figures look promising.
Is there a summary of those existing "dark corners with monsters"? :) having those as GitHub issues would be helpful for us both for visibility and to possibly help you with development.

@eboasson
Copy link
Collaborator

eboasson commented Jun 7, 2019

we've started experimenting with it and the performance figures look promising.

☺️

Is there a summary of those existing "dark corners with monsters"? :) having those as GitHub issues would be helpful for us both for visibility and to possibly help you with development.

The known issues are listed in the readme, and certainly for a number of them it makes sense to turn them into issues and strip them from the readme. I'm not so sure that, e.g., "Cyclone DDS doesn't do security yet" would be worthy of an issue on this RMW implementation, but the missing checks in the deserializer definitely are. I'll do that shortly.

The "dark corners with monsters" are the unknown unknowns as it were ... Someone else has been kind enough to shine some light on two of them (#3) and those two did not enjoy the spotlight very much. I am pretty sure I've sent them looking for a new project to live in, but I don't know which other ones remain ... Getting it integrated in the ROS2 CI system will help.

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

2 participants