Fix service and action interface generation #76

jacobperron · 2020-01-30T01:19:03Z

Depends on #68.
After #68 is merged, I can rebase this branch.

Before, we were not generating code for the messages and services that make up service and action
interfaces.

Due to issues with duplicate definitions caused by instantiating the msg.cpp.em template
multiple times, I've opted to generate separate files for each service, action, and the interfaces
that they are made of. This is similar to what we are doing with the generated Java code.

I've added a test confirming that generated service code can be used. Adding a test for actions is
difficult at the moment due to a circular dependency with action_msgs.

Tests that involve services are broken due to issues related to interface generation. There's a separate PR for a fix: #76. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-02-08T01:01:59Z

In retrospect, I think it would be worth doing some further refactoring so that service and action sub-interfaces are member classes. Then, in code the symbols are referenced like:

examples_interfaces.srv.AddTwoInts.Request

versus how they are currently referenced:

examples_interfaces.srv.AddTwoInts_Request

It seems more intuitive and in line with the C++ and Python code generators. Since this would be a significant refactor, I've ticketed it separately to do after the current string of PRs have landed: #83

Tests that involve services are broken due to issues related to interface generation. There's a separate PR for a fix: #76. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update API for getting rcl error string Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add implementation for ROS Context A context represents an init/shutdown cycle and is used in the creation of top level entities like nodes and guard conditions. For convenience, a default context is created when rcljava is initialized. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add implementation for Clock Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Update for Dashing support * Update wait set API calls * Update entity creation API calls * Use Context objects to check 'ok()' status * Add Clock and Context members to NodeImpl * Fix static member reference: 'this.defaultContext' -> 'RCLJava.defaultContext' Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Avoid hiding errors when cleaning up init options Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Disable tests Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix typos in JNI library files Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix native node method signature Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Populate missing QoS settings with defaults Otherwise we run into a runtime error about Fast-RTPS not supporting liveliness. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix issues with Clock class * Load with JNIUtils * Rename native create method for consistency * Fix bug in native implementation Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable linter tests Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix lint errors Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable RCLJava test and fix bugs * Use Context.ok() and deprecate RCLJava.isInitialized(). * Move implementation loading to static initialization code. Otherwise, calls to getDefaultContext() fail if called before rclJavaInit(). It wasn't clear to me why the implementation should be loaded in a separate function call. We can probably refactor the code to avoid the error if we want to move the loading back into rclJavaInit(). * Refactor test into one init/shutdown test. Previously, not calling RCLJava.shutdown() was leaving a context around between tests. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix NodeTest Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable most tests Tests that involve services are broken due to issues related to interface generation. There's a separate PR for a fix: #76. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Before, we were not generating code for the messages and services that make up service and action interfaces. Due to issues with duplicate definitions caused by instantiating the msg.cpp.em template multiple times, I've opted to generate separate files for each service, action, and the interfaces that they are made of. This is similar to what we are doing with the generated Java code. I've added a test confirming that generated service code can be used. Adding a test for actions is difficult at the moment due to a circular dependency with action_msgs. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

This avoids name clashing with other generated files. Similar to what we do with generated Java files. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

…types For example, 'example_interfaces/srv/AddTwoInts_Request' should be mangled to 'example_1interfaces_srv_AddTwoInts_1Request'. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-02-11T17:30:02Z

With #68, I've rebased this PR.

clalancette

I've only got minor comments here; otherwise, looks good to me. It's a bit unfortunate that there is so much duplication between the msg.cpp.em, idl.cpp.em, and srv.cpp.em, but that's OK for now.

clalancette · 2020-02-26T20:04:47Z

rosidl_generator_java/resource/action.cpp.em

+output_file = os.path.join(
+    output_dir, *namespaces[1:], '{0}.ep.{1}.cpp'.format(goal_type_name, typesupport_impl))


Just a question; why the ".ep." portion of the filename?

ep originally stood for entry point, so that code could be split between common code and for each typesupport implementation

If there are suggestions for renaming (or removing it) I'm happy to do so.

rosidl_generator_java/resource/idl.cpp.em

rosidl_generator_java/resource/srv.cpp.em

rosidl_generator_java/resource/msg.cpp.em

esteve · 2020-02-27T15:49:55Z

rosidl_generator_java/resource/action.cpp.em

+output_file = os.path.join(
+    output_dir, *namespaces[1:], '{0}.ep.{1}.cpp'.format(goal_type_name, typesupport_impl))


ep originally stood for entry point, so that code could be split between common code and for each typesupport implementation

esteve · 2020-02-27T15:58:08Z

In retrospect, I think it would be worth doing some further refactoring so that service and action sub-interfaces are member classes. Then, in code the symbols are referenced like:
examples_interfaces.srv.AddTwoInts.Request
versus how they are currently referenced:
examples_interfaces.srv.AddTwoInts_Request
It seems more intuitive and in line with the C++ and Python code generators. Since this would be a significant refactor, I've ticketed it separately to do after the current string of PRs have landed: #83

@jacobperron I remember trying to get that working, but I can't remember why I couldn't. But in any case, yeah, I agree having Request and Response as member classes.

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron · 2020-03-04T19:02:03Z

@esteve I think I've address all of your comments.

jacobperron · 2020-03-11T17:04:59Z

@esteve Friendly ping.

esteve

@jacobperron thanks!

* Update API for getting rcl error string Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add implementation for ROS Context A context represents an init/shutdown cycle and is used in the creation of top level entities like nodes and guard conditions. For convenience, a default context is created when rcljava is initialized. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add implementation for Clock Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Update for Dashing support * Update wait set API calls * Update entity creation API calls * Use Context objects to check 'ok()' status * Add Clock and Context members to NodeImpl * Fix static member reference: 'this.defaultContext' -> 'RCLJava.defaultContext' Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Avoid hiding errors when cleaning up init options Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Disable tests Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix typos in JNI library files Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix native node method signature Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Populate missing QoS settings with defaults Otherwise we run into a runtime error about Fast-RTPS not supporting liveliness. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix issues with Clock class * Load with JNIUtils * Rename native create method for consistency * Fix bug in native implementation Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable linter tests Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix lint errors Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable RCLJava test and fix bugs * Use Context.ok() and deprecate RCLJava.isInitialized(). * Move implementation loading to static initialization code. Otherwise, calls to getDefaultContext() fail if called before rclJavaInit(). It wasn't clear to me why the implementation should be loaded in a separate function call. We can probably refactor the code to avoid the error if we want to move the loading back into rclJavaInit(). * Refactor test into one init/shutdown test. Previously, not calling RCLJava.shutdown() was leaving a context around between tests. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix NodeTest Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable most tests Tests that involve services are broken due to issues related to interface generation. There's a separate PR for a fix: #76. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix service and action interface generation Before, we were not generating code for the messages and services that make up service and action interfaces. Due to issues with duplicate definitions caused by instantiating the msg.cpp.em template multiple times, I've opted to generate separate files for each service, action, and the interfaces that they are made of. This is similar to what we are doing with the generated Java code. I've added a test confirming that generated service code can be used. Adding a test for actions is difficult at the moment due to a circular dependency with action_msgs. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add missing header include Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Rename top level generated cpp file This avoids name clashing with other generated files. Similar to what we do with generated Java files. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix JNI name mangling function so it works for service and action subtypes For example, 'example_interfaces/srv/AddTwoInts_Request' should be mangled to 'example_1interfaces_srv_AddTwoInts_1Request'. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove vestigal references to jni_package_name Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add comment about action and service headers Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Simplify include logic Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Update API for getting rcl error string Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add implementation for ROS Context A context represents an init/shutdown cycle and is used in the creation of top level entities like nodes and guard conditions. For convenience, a default context is created when rcljava is initialized. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add implementation for Clock Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Update for Dashing support * Update wait set API calls * Update entity creation API calls * Use Context objects to check 'ok()' status * Add Clock and Context members to NodeImpl * Fix static member reference: 'this.defaultContext' -> 'RCLJava.defaultContext' Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Avoid hiding errors when cleaning up init options Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Disable tests Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix typos in JNI library files Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix native node method signature Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Populate missing QoS settings with defaults Otherwise we run into a runtime error about Fast-RTPS not supporting liveliness. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix issues with Clock class * Load with JNIUtils * Rename native create method for consistency * Fix bug in native implementation Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable linter tests Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix lint errors Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable RCLJava test and fix bugs * Use Context.ok() and deprecate RCLJava.isInitialized(). * Move implementation loading to static initialization code. Otherwise, calls to getDefaultContext() fail if called before rclJavaInit(). It wasn't clear to me why the implementation should be loaded in a separate function call. We can probably refactor the code to avoid the error if we want to move the loading back into rclJavaInit(). * Refactor test into one init/shutdown test. Previously, not calling RCLJava.shutdown() was leaving a context around between tests. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix NodeTest Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Enable most tests Tests that involve services are broken due to issues related to interface generation. There's a separate PR for a fix: #76. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

* Fix service and action interface generation Before, we were not generating code for the messages and services that make up service and action interfaces. Due to issues with duplicate definitions caused by instantiating the msg.cpp.em template multiple times, I've opted to generate separate files for each service, action, and the interfaces that they are made of. This is similar to what we are doing with the generated Java code. I've added a test confirming that generated service code can be used. Adding a test for actions is difficult at the moment due to a circular dependency with action_msgs. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add missing header include Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Rename top level generated cpp file This avoids name clashing with other generated files. Similar to what we do with generated Java files. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Fix JNI name mangling function so it works for service and action subtypes For example, 'example_interfaces/srv/AddTwoInts_Request' should be mangled to 'example_1interfaces_srv_AddTwoInts_1Request'. Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Remove vestigal references to jni_package_name Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Add comment about action and service headers Signed-off-by: Jacob Perron <jacob@openrobotics.org> * Simplify include logic Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron mentioned this pull request Feb 5, 2020

Foxy Fitzroy Roadmap #82

Closed

25 tasks

jacobperron added a commit that referenced this pull request Feb 7, 2020

Enable most tests

e46dc1c

Tests that involve services are broken due to issues related to interface generation. There's a separate PR for a fix: #76. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron added a commit that referenced this pull request Feb 8, 2020

Enable most tests

430d2a6

Tests that involve services are broken due to issues related to interface generation. There's a separate PR for a fix: #76. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron added 4 commits February 11, 2020 09:28

Add missing header include

6196b04

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Rename top level generated cpp file

00260bf

This avoids name clashing with other generated files. Similar to what we do with generated Java files. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Fix JNI name mangling function so it works for service and action sub…

efaf468

…types For example, 'example_interfaces/srv/AddTwoInts_Request' should be mangled to 'example_1interfaces_srv_AddTwoInts_1Request'. Signed-off-by: Jacob Perron <jacob@openrobotics.org>

jacobperron force-pushed the fix-srv-action-interfaces branch from 0dd0877 to efaf468 Compare February 11, 2020 17:29

jacobperron mentioned this pull request Feb 12, 2020

Add Client methods for checking and waiting for service availability #85

Merged

clalancette approved these changes Feb 26, 2020

View reviewed changes

esteve requested changes Feb 27, 2020

View reviewed changes

jacobperron added 3 commits March 4, 2020 09:50

Remove vestigal references to jni_package_name

29d1eac

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Add comment about action and service headers

97d0ead

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Simplify include logic

dc5d64d

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

This was referenced Mar 9, 2020

Rename rcljava_common cpp headers to have .hpp suffix #93

Merged

Add GitHub workflow for Dashing #96

Merged

jacobperron self-assigned this Mar 10, 2020

jacobperron requested a review from esteve March 17, 2020 22:21

esteve approved these changes Mar 18, 2020

View reviewed changes

esteve merged commit bf1e4b6 into ros2-java:dashing Mar 18, 2020

jacobperron mentioned this pull request Oct 7, 2020

ROS Actions #138

Open

4 tasks

jacobperron mentioned this pull request Oct 20, 2020

Compile generated action-related services #140

Merged

ivanpauno pushed a commit that referenced this pull request Oct 28, 2021

Bump ros-tooling/setup-ros to v0.2 (#76)

f6fd54b

Signed-off-by: Jacob Perron <jacob@openrobotics.org>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix service and action interface generation #76

Fix service and action interface generation #76

jacobperron commented Jan 30, 2020

jacobperron commented Feb 8, 2020

jacobperron commented Feb 11, 2020

clalancette left a comment

clalancette Feb 26, 2020

esteve Feb 27, 2020

jacobperron Feb 27, 2020

esteve Feb 27, 2020

esteve commented Feb 27, 2020

jacobperron commented Mar 4, 2020

jacobperron commented Mar 11, 2020

esteve left a comment

		output_file = os.path.join(
		output_dir, *namespaces[1:], '{0}.ep.{1}.cpp'.format(goal_type_name, typesupport_impl))

Fix service and action interface generation #76

Fix service and action interface generation #76

Conversation

jacobperron commented Jan 30, 2020

jacobperron commented Feb 8, 2020

jacobperron commented Feb 11, 2020

clalancette left a comment

Choose a reason for hiding this comment

clalancette Feb 26, 2020

Choose a reason for hiding this comment

esteve Feb 27, 2020

Choose a reason for hiding this comment

jacobperron Feb 27, 2020

Choose a reason for hiding this comment

esteve Feb 27, 2020

Choose a reason for hiding this comment

esteve commented Feb 27, 2020

jacobperron commented Mar 4, 2020

jacobperron commented Mar 11, 2020

esteve left a comment

Choose a reason for hiding this comment