Enable generation of ROS2 nodes directly from C++ code #984

cmnrd · 2022-02-28T14:28:10Z

This PR provides basic support for compilation of ROS2 nodes directly from LF programs in the C++ target. There are still many features missing that will be part of subsequent PRs. Also, it is unclear to me how the new ROS2 specific generator should interact with the LSP implementation. However, I would also address this in a future PR.

With this PR basic compilation works in lfc and Epoch. To validate it, I added a ros2 CI flow which compiles all the C++ tests for the ros2 platform.

~~Note that this PR is based on #978 and hence currently includes all changes from this PR in the diff.~~

edwardalee

This looks pretty cool to me. It seems to offer a nice simple way to make an LF program into a ROS node.

edwardalee · 2022-03-05T02:03:03Z

example/Cpp/src/ROS/MinimalPublisher.lf

+
+main reactor {
+    private preamble {=
+        // FIXME: forward declaration to make the node visible


Is this really necessary? Can't you generate this declaration?

Perhaps, lf_ros_node can be a reserved state variable?

This is just a temporary workaround until I found a better solution. I am trying to keep the code for generating reactor classes agnostic of the target platform, as it keeps the code generator clean. However, there needs to be some way of extending the generated code. I will fix this mechanism as soon as I have found a satisfactory solution.

Soroosh129

Looks great!

I agree with @edwardalee that this provide a simple way of turning an LF program into a ROS node.

I left a few comments based on my knowledge of ROS 2.

Soroosh129 · 2022-03-05T03:22:01Z

org.lflang/src/org/lflang/generator/cpp/CppRos2PackageGenerator.kt

+            |<?xml version="1.0"?>
+            |<?xml-model href="http://download.ros.org/schema/package_format3.xsd" schematypens="http://www.w3.org/2001/XMLSchema"?>
+            |<package format="3">
+            |  <name>${fileConfig.name}</name>


Should this be converted to all lowercase to conform to ROS2's package naming convention?

Package names should follow common C variable naming conventions: lower case, start with a letter, use underscore separators, e.g. laser_viewer

I guess we don't have a naming convention for LF programs yet, but the tests, for example, are all capitalized.

Right, I don't think automagically converting the name is a good idea. My current idea is to add a package-name property to the ros2 section. There we could also add other details such as the license and maintainer information required in the package.xml.

Soroosh129 · 2022-03-05T03:23:33Z

org.lflang/src/org/lflang/generator/cpp/CppRos2PackageGenerator.kt

+            |  <depend>rclcpp</depend>
+            |  <depend>rclcpp_components</depend>
+            |  <depend>std_msgs</depend>
+            |  <depend>$reactorCppName</depend>


Do you have a method in mind for programmers to be able to add to these dependencies?

Not yet, but it is planned and I would also add this to the ros2 property.

Soroosh129 · 2022-03-05T03:26:56Z

org.lflang/src/org/lflang/generator/cpp/CppRos2PackageGenerator.kt

+        """.trimMargin()
+    }
+
+    fun generatePackageCmake(sources: List<Path>): String {


Would the ordinary CMake generator for the Cpp target plus an extension with added lines such as

find_package(ament_cmake_auto REQUIRED) ament_auto_find_build_dependencies()

work here? That design strikes me as more future-proof.

Here is how we do this in the C target.

I don't think so. Basically, only the header section is the same. While this could be factored out in a method to facilitate code reuse, I don't think we would gain too much from it here. Overall, I don't like to make predictions as to what the future might bring ;) I prefer a simple design and only add complexity when I am sure it is needed.

Soroosh129 · 2022-03-05T03:28:51Z

org.lflang/src/org/lflang/generator/cpp/CppStandaloneGenerator.kt

+import java.nio.file.Path
+import java.nio.file.Paths
+
+class CppStandaloneGenerator(generator: CppGenerator) :


Missing documentation for the class.

Soroosh129 · 2022-03-05T03:41:39Z

.github/workflows/cpp-ros2-tests.yml

+          ref: ${{ inputs.runtime-ref }}
+        if: ${{ inputs.runtime-ref }}
+      - name: Setup ROS2
+        uses: ros-tooling/[email protected]


This would be very useful to test the C target's ROS 2 support as well.
Is it okay to call this just ros2-tests, or should we extract the setup here and share it among two separate workflows?

We could do something similar as in the benchmark tests, by adding a target parameter and making the jobs conditional. Feel free to add the C tests (but probably in another PR).

Soroosh129 · 2022-03-05T05:30:10Z

org.lflang/src/org/lflang/generator/cpp/CppRos2PackageGenerator.kt

+    fun generateBinScript(): String {
+        val relPath = fileConfig.binPath.relativize(fileConfig.outPath).toUnixString()
+
+        return """


You might want to consider generating a ROS2 launch file instead of a bash script.

Thanks for the pointer! I was vaguely aware of launch files, but did not think of them when creating the script.

Soroosh129 · 2022-03-05T05:34:46Z

example/Cpp/src/ROS/MinimalPublisher.lf

+
+main reactor {
+    private preamble {=
+        // FIXME: forward declaration to make the node visible


Perhaps, lf_ros_node can be a reserved state variable?

Soroosh129 · 2022-03-05T05:40:22Z

example/Cpp/src/ROS/MinimalSubscriber.lf

+    reaction(startup) -> message {=
+        subscription = lf_node->create_subscription<std_msgs::msg::String>(
+            "topic", 10, [&message](const std_msgs::msg::String::SharedPtr msg) { message.schedule(msg->data); } );
+        // FIXME: Why can't we use a reference type in the lambda argument?


You mean something like this?

I guess so, but I am not sure. I tried to use const std_msgs::msg::String::SharedPtr& msg in the signature, but that fails with a lot of weird C++ errors. I just realized after checking the documentation again that galactic uses const std_msgs::msg::String& msg in the signature and foxy uses const std_msgs::msg::String::SharedPtr msg. So I guess const std_msgs::msg::String::SharedPtr& msg is not supported, but const std_msgs::msg::String& msg is.

Soroosh129 · 2022-03-05T06:21:40Z

example/Cpp/src/ROS/MinimalPublisher.lf

+        auto message = std_msgs::msg::String();
+        message.data = "Hello, world! " + std::to_string(count++);
+        reactor::log::Info() << "Publishing: " << message.data;
+        publisher->publish(message);


I don't think the default rclcpp executor is thread-safe, so calling publish for the same topic from multiple LF worker threads will be problematic without acquiring a mutex. This is of course fine for this example, but I think we should be mindful of this limitation, especially if reactors other than the main reactor will be publishing messages.

Unfortunately, thread-safety is not discussed in the documentation. According to this post, however, these operations should be thread-safe.

Soroosh129 · 2022-03-05T06:24:27Z

org.lflang/src/lib/cpp/lf_timeout.hh

+class __lf_Timeout : public reactor::Reactor {
+ private:
+  reactor::Timer timer;
+  reactor::Reaction r_timer{"r_timer", 1, this, [this]() { environment()->sync_shutdown();} };


How is timeout coordinated with rclcpp's shutdown?

The node invokes rclcpp's shutdown after the reactor execution terminated (i.e. after all shutdown reactions were processed and the main scheduler thread terminates).

cmnrd · 2022-03-07T13:32:57Z

Thanks for your suggestions! I consider this PR just as the start and there will be more PRs in the future to extend functionality and fix things where needed.

cmnrd added 30 commits January 28, 2022 13:17

add a ros2 target property

1c8782c

rename classes needed for standalone code generation

1c914da

modularize the C++ build process

fa5fbbb

cleaner interface between CppGenerator and the platform specific classes

a7d886d

add stub for ROS2 compilation

5a3b1c8

generate ros2 package description

1358dff

generate ros2 specific cmake file

ce7911a

fix typo; add email

f97cd1c

use a more sophisticated build structure

e84256a

Merge branch 'master' into cpp-ros2-platform

321f461

avoid using the fsa for writing files

7679227

Merge branch 'master' into cpp-ros2-platform

8431932

destination of copyFileFromClassPath should be given by a Path object

2253b3d

remove unused method

e1d8329

delete another unused method

67cdfb6

remove more unused methods and fix a few warnings

37e20ec

Merge branch 'file-config-cleanup' into cpp-ros2-platform

f7a3669

fix compile errors

aafc117

fix more bugs after merge

c1fdc89

generate src files in 'src' when compiling for ROS2

5eb36d7

generate a complete ros2 node

1a580ee

various fixes, Minimal test compiles now

4e73d4c

Merge branch 'master' into cpp-ros2-platform

accbff4

Merge branch 'fileconfig-cleanup' into cpp-ros2-platform

cc60770

fix ros2 build

5c2139d

use a single package generator class

c2c3982

properly shutdown ros2 nodes after LF execution terminates

f54f7be

don't change the Minimal test

42b0fac

generate a script for executing a compiled node

416fb12

invoke colcon build

456bd27

cmnrd and others added 8 commits March 1, 2022 12:22

fix branch name

148fea2

fix xi config

048f85c

fix test name

02bbc9c

bugfix

d1370df

source the ROS2 setup script

6208686

try sourcing and running in the same run

9a41527

abort if no ROS2 environment was found

798bef5

relatevize setup script path

763c7a8

cmnrd added this to the 0.1.0 milestone Mar 1, 2022

implement timeout for ros2 code generation

772d167

cmnrd marked this pull request as ready for review March 1, 2022 18:08

cmnrd added 4 commits March 2, 2022 07:38

ensure that there is a valid config object before adding a property

3cd3b1c

Merge branch 'master' into cpp-ros2-platform

f583e4d

fix include path for headers located in src

5727fb0

support cmake include

54b3000

cmnrd requested review from lhstrh and Soroosh129 March 2, 2022 10:10

cmnrd added 3 commits March 2, 2022 11:23

add a global variable giving C++ reaction code access to the node

ad261e7

add a minimal ros example

4e42a1c

don't test examples with ROS2 and fix warnings in CCppTest

d959b80

edwardalee approved these changes Mar 5, 2022

View reviewed changes

Soroosh129 approved these changes Mar 5, 2022

View reviewed changes

cmnrd added 3 commits March 7, 2022 13:01

Merge branch 'master' into cpp-ros2-platform

2cd8784

fix merge conflict

1d27e09

fix typos and add brief class documentation

a064d35

cmnrd merged commit 1534be4 into master Mar 7, 2022

cmnrd deleted the cpp-ros2-platform branch March 7, 2022 13:33

lhstrh mentioned this pull request Mar 17, 2022

Create an automated release workflow #338

Closed

lhstrh added the enhancement Enhancement of existing feature label Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable generation of ROS2 nodes directly from C++ code #984

Enable generation of ROS2 nodes directly from C++ code #984

cmnrd commented Feb 28, 2022 •

edited

Loading

edwardalee left a comment

edwardalee Mar 5, 2022

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

Soroosh129 left a comment

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

Soroosh129 Mar 5, 2022

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

Soroosh129 Mar 5, 2022

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

Soroosh129 Mar 5, 2022 •

edited

Loading

cmnrd Mar 7, 2022

Soroosh129 Mar 5, 2022

cmnrd Mar 7, 2022

cmnrd commented Mar 7, 2022

Enable generation of ROS2 nodes directly from C++ code #984

Enable generation of ROS2 nodes directly from C++ code #984

Conversation

cmnrd commented Feb 28, 2022 • edited Loading

edwardalee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Soroosh129 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Soroosh129 Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnrd commented Mar 7, 2022

cmnrd commented Feb 28, 2022 •

edited

Loading

Soroosh129 Mar 5, 2022 •

edited

Loading