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 support for Cyclone DDS #75

Merged
merged 9 commits into from Jul 29, 2019

Conversation

@eboasson
Copy link
Contributor

commented Jul 25, 2019

This PR adds initial support for Cyclone DDS — "initial" because it has only been tested minimally on a VM. It follows the pattern of the FastRTPS and Connext Micro support code, so there is a PERFORMANCE_TEST_CYCLONEDDS_ENABLED variable in the CMake file and a macro in the source code that determines whether the support will be included, as well as a "CycloneDDS" option setting for the "-c" option.

It relies on find_package to locate Cyclone, so be sure to have it in the CMake module path or set CycloneDDS_DIR to <Cyclone DDS install prefix>/share/CycloneDDS.

(Note that there is a known issue building Cyclone with colcon as part of ROS2, caused by the CMake files not adhering to the structure that colcon expects. That'll be fixed, but for now it is probably more practical to build Cyclone outside of ROS2.)


This change is Reviewable

eboasson added 2 commits Jul 22, 2019
Signed-off-by: Erik Boasson <eb@ilities.com>
Signed-off-by: Erik Boasson <eb@ilities.com>
@esteve esteve self-assigned this Jul 25, 2019
@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

@eboasson thanks! Do you have binary packages of CycloneDDS or does it have to be built from source? I'd like to add it to our CI so that we can properly test this PR.

@eboasson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

No binary packages yet ... sorry ...

It sadly it also still needs Java for the IDL preprocessor, and so you also need to have a JDK installed to build it (it says so on the README, but I've had complaints from people that it proved harder than expected to build it). But at least it is an otherwise straightforward cmake-based build.

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

No problem, I'll submit a pull request to your branch to add the steps necessary for building Cyclone on our CI.

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 25, 2019

@eboasson is there a particular version of CycloneDDS that needs to be installed? I only see one release https://github.com/eclipse-cyclonedds/cyclonedds/releases I've built the master branch of CycloneDDS, is there a particular commit id that works best?

The issue with colcon you mention, is it because FreeRTOS-Sim is declared as a CMake project in ports/freertos-posix ?

@eboasson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 25, 2019

The head of master should be fine (I tried it with the state of yesterday, but I am not too worried about the PRs I merged — or I wouldn’t have merged them).

And you’re right, the problem is with the cmake project in ports/freertos-posix. The plan is to reorganize the repository a bit in the next few days. I inherited the current structure, but I really prefer to have the top-level directory have the right CMakeLists.txt ... and so it happens that that plays much nicer with colcon as well.

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

@eboasson I've submitted eboasson#1 to your fork to add Cyclone DDS to the CI. However, there are some style tests in your PR that are failing, see https://dev.azure.com/ApexAI/performance_test/_build/results?buildId=91

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

@eboasson do you plan to tag a release of Cyclone DDS in the short term? I'm building the current master with a specific commit id, but it'd be good to use a tag to refer to it.

@eboasson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 26, 2019

@esteve,

Thanks for helping out.

However, there are some style tests in your PR that are failing, see https://dev.azure.com/ApexAI/performance_test/_build/results?buildId=91

Could you please point me to the uncrustify configuration so that I can fix the style issues? (Perhaps it is https://github.com/ApexAI/ament_lint/blob/master/ament_uncrustify/ament_uncrustify/configuration/ament_code_style.cfg?)

do you plan to tag a release of Cyclone DDS in the short term? I'm building the current master with a specific commit id, but it'd be good to use a tag to refer to it.

I was working towards a release that'll probably take a bit too long — it is a bit involved for Eclipse projects, so I am perhaps procrastinating :) I suppose there's no problem in tagging a version, as long as I don't claim it to be a release. As I am mostly done with a shuffle to make the Cyclone sources play nice with colcon, putting that in first seems sensible. (That's days, not weeks.) Does that sound good?

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@eboasson

Could you please point me to the uncrustify configuration so that I can fix the style issues? (Perhaps it is https://github.com/ApexAI/ament_lint/blob/master/ament_uncrustify/ament_uncrustify/configuration/ament_code_style.cfg?)

Sure, we're currently using upstream's configuration (https://github.com/ament/ament_lint/blob/master/ament_uncrustify/ament_uncrustify/configuration/ament_code_style.cfg). If you've installed ROS 2 from the binary packages, the uncrustify configuration file can be found at
/opt/ros/dashing/lib/python3.6/site-packages/ament_uncrustify/configuration/ament_code_style.cfg

I was working towards a release that'll probably take a bit too long — it is a bit involved for Eclipse projects, so I am perhaps procrastinating :) I suppose there's no problem in tagging a version, as long as I don't claim it to be a release. As I am mostly done with a shuffle to make the Cyclone sources play nice with colcon, putting that in first seems sensible. (That's days, not weeks.) Does that sound good?

That's great. It's just that right now I'm using an arbitrary commit id, but a tag that has a descriptive name would be better. Just call it alpha-something or just a timestamp

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@eboasson BTW, my last commit in eboasson#1 requires Cyclone DDS to be found (74c47a5) because I wanted to make sure that it was being picked up by the CI, but it'd should be reverted before merging. Alternatively, we can add an external option to CMake such that Cyclone DDS needs to be found and fail if CMake can't.

eboasson added 2 commits Jul 29, 2019
Signed-off-by: Erik Boasson <eb@ilities.com>
@eboasson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@esteve I fixed the white space errors and I have reverted that commit.

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@eboasson I've implemented the option in eboasson#2 and updated the CI to require Cyclone DDS during the tests.

@@ -44,6 +44,7 @@ class ROS2QOSAdapter
inline rmw_qos_profile_t get() const
{
rmw_qos_profile_t ros_qos;
memset(&ros_qos, 0, sizeof(ros_qos));

This comment has been minimized.

Copy link
@esteve

esteve Jul 29, 2019

Contributor

Could you explain this a little bit?

This comment has been minimized.

Copy link
@eboasson

eboasson Jul 29, 2019

Author Contributor

I found that at least the "avoid_ros_namespace_conventions" field in the QoS was undefined, and that caused the CycloneDDS RMW layer to sometimes use one topic name, and sometimes another. It seemed very unlikely that undefined fields was intended behaviour, and memsetting ros_qos seemed the most sensible way of solving the problem.

This comment has been minimized.

Copy link
@esteve

esteve Jul 29, 2019

Contributor

Interesting, thanks for looking into it. This should probably be addressed upstream in ROS 2, but it's ok to keep it here until then.

@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@eboasson just one small comment explaining why you needed to memset the qos struct and that's all. Thanks!

Also, could you have a look at eboasson#2 and merge it if you like the changes in it before I merge this PR? Thanks.

Copy link
Contributor

left a comment

@eboasson one last thing. Could you update README.md to mention that Cyclone DDS is supported? Thanks.

Signed-off-by: Erik Boasson <eb@ilities.com>
@eboasson

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

@esteve Done.

@esteve
esteve approved these changes Jul 29, 2019
@esteve

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

@eboasson thanks! I'll merge once CI passes.

@esteve esteve merged commit fa25cd1 into ApexAI:master Jul 29, 2019
1 check passed
1 check passed
ApexAI.performance_test Build #20190729.3 succeeded
Details
@joespeed joespeed referenced this pull request Aug 15, 2019
27 of 29 tasks complete
daggarwa added a commit that referenced this pull request Sep 11, 2019
* Initialize all fields in ROS2 QoS

Signed-off-by: Erik Boasson <eb@ilities.com>

* Initial support for Eclipse Cyclone DDS

Signed-off-by: Erik Boasson <eb@ilities.com>

* Build and install Cyclone DDS

* Require CycloneDDS to test that it's been found

* Fix code formatting issues

Signed-off-by: Erik Boasson <eb@ilities.com>

* Revert "Require CycloneDDS to test that it's been found"

This reverts commit 74c47a5.

* Require finding CycloneDDS via option

* Require CycloneDDS in the CI

* Add Cyclone DDS to the list of communication means

Signed-off-by: Erik Boasson <eb@ilities.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.