Add typesupport for actions in c and c++ #36

apojomovsky · 2018-11-07T17:33:16Z

This PR addresses the second part of ros2/rosidl#303 and ros2/rosidl#302 , which was coupled to the first one.

Extends the logic of the rosidl_typesupport_c and rosidl_typesupport_cpp to support the generation of typesupport headers for action files.
Adds the required templates to generate both c and cpp action typesupport files.

sloretz · 2018-11-09T21:32:50Z

rosidl_typesupport_c/include/rosidl_typesupport_c/action_type_support_dispatch.h

@@ -0,0 +1,36 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.


Actually I don't think this file is necessary like it is for messages and services. The action type support doesn't change with rmw implementation, so there's no need to have a dispatch.

~~That's true, but don't we need it to get proper typesupport for the underlying services and messages?~~ Nvm, wrong assumption.

That's true, but don't we need it to get proper typesupport for the underlying services and messages?

Yes, but that can use the dispatch on the generated messages and services. The generated action type support just needs to call get_service_type_support<>() or get_message_type_support<>(), and in C ROSIDL_GET_SRV_TYPE_SUPPORT() or ROSIDL_GET_MSG_TYPE_SUPPORT()

Right, I realized shortly after I commented.

sloretz · 2018-11-09T21:38:38Z

rosidl_typesupport_c/resource/action__type_support.cpp.em

@@ -0,0 +1,124 @@
+// generated from rosidl_typesupport_c/resource/action__type_support.cpp.em


Same as above comment I don't think this file is needed because no dispatch is needed for actions.

sloretz · 2018-11-09T21:42:47Z

rosidl_typesupport_c/src/action_type_support_dispatch.cpp

@@ -0,0 +1,34 @@
+// Copyright 2016 Open Source Robotics Foundation, Inc.


Same as above comments, I don't think this file is needed.

sloretz · 2018-11-09T21:50:27Z

rosidl_typesupport_cpp/resource/action__type_support.cpp.em

+{
+
+template<>
+ROSIDL_GENERATOR_C_PUBLIC_@(spec.pkg_name)


It looks like this will need a different visibility macro ROSIDL_GENERATOR_C_PUBLIC_<package name>_ACTION. This is needed to get the dllexport and dllimport correct when generating these files.

See changes in this diff.

https://github.com/ros2/rcl_interfaces/pull/47/files/a70935a2109267bb4ea7cf9ee17fdd50f3995d5b..6d6da5801ed3ee1438246bc5b31898d12d522ff0

Good point. Added.

hidmic

LGTM but for a few minor nits and pending CI.

hidmic · 2018-11-13T13:32:51Z

rosidl_typesupport_cpp/resource/action__type_support.cpp.em

+@#    Parsed specification of the .action file
+@#  - subfolder (string)
+@#    The subfolder / subnamespace of the message, usually 'action'
+@#  - type_supports (list of strings)


@apojomovsky unused type_supports list?

true that, just removed references to it where unused. thanks!

hidmic · 2018-11-13T13:33:17Z

rosidl_typesupport_cpp/rosidl_typesupport_cpp/__init__.py

+                    args['output_dir'], subfolder, generated_filename %
+                    convert_camel_case_to_lower_case_underscore(spec.action_name))
+
+                data = {'spec': spec, 'subfolder': subfolder, 'type_supports': type_supports}


@apojomovsky same as above, are we using this type_supports list?

it's not being used, just removed it. thanks!

hidmic · 2018-11-13T13:33:53Z

rosidl_typesupport_c/resource/action__type_support.c.em

+@#    Parsed specification of the .action file
+@#  - subfolder (string)
+@#    The subfolder / subnamespace of the message, usually 'action'
+@#  - type_supports (list of strings)


@apojomovsky unused type_supports list?

yup! removed, thanks!

apojomovsky · 2018-11-13T14:03:56Z

CI with ros2/rosidl_typesupport#36, ros2/rosidl#312 and ros2/rcl_interfaces#48

Linux
Linux-aarch64
macOS
Windows

hidmic · 2018-11-13T14:50:50Z

@sloretz @apojomovsky I can't seem to be able to compile ros2/rcl#319 locally with #36, ros2/rosidl#312 and ros2/rcl_interfaces#48.

--- stderr: rcl_action
CMakeFiles/test_action_client.dir/test/rcl_action/test_action_client.cpp.o: In function `TestActionClientBaseFixture_test_action_client_init_fini_Test::TestBody()':
test_action_client.cpp:(.text+0x50): undefined reference to `rosidl_typesupport_c__get_action_type_support_handle__test_msgs__action__Fibonacci'
CMakeFiles/test_action_client.dir/test/rcl_action/test_action_client.cpp.o: In function `TestActionClientFixture::SetUp()':
test_action_client.cpp:(.text._ZN23TestActionClientFixture5SetUpEv[_ZN23TestActionClientFixture5SetUpEv]+0x45): undefined reference to `rosidl_typesupport_c__get_action_type_support_handle__test_msgs__action__Fibonacci'
collect2: error: ld returned 1 exit status
make[2]: *** [test_action_client] Error 1
make[1]: *** [CMakeFiles/test_action_client.dir/all] Error 2
make: *** [all] Error 2
---
Failed   <<< rcl_action	[ Exited with code 2 ]

However, the action interfaces library is generated and installed, and it does contain the missing symbol above, so it looks like there's a missing dependency relationship in the CMake infrastructure. I tried a fix but it isn't obvious to me where the chain broke.

hidmic · 2018-11-13T15:16:10Z

rosidl_typesupport_cpp/cmake/rosidl_typesupport_cpp_generate_action_interfaces.cmake

+  VERBATIM
+)
+
+set(_target_suffix "__generate_action_interfaces__rosidl_typesupport_cpp")


@apojomovsky @sloretz any reason for this to be reversed with respect to https://github.com/ros2/rosidl_typesupport/pull/36/files#diff-d9cdf0cebb0a07a690ecd5bcec505105R89 ?

Made consistent in 31efaca

sloretz · 2018-11-13T17:15:21Z

@hidmic The linking issue should be solved by ros2/rosidl@2467acf which changes rosidl_generate_action_interfaces() from a function to a macro (thanks @dirk-thomas). The linking issue happened because _AMENT_EXPORT_LIBRARIES was being set in a different scope than ament_package() was being called in.

apojomovsky · 2018-11-13T18:45:56Z

Thanks a lot for the fixes @sloretz ! Just verified that the rcl_action tests from @hidmic 's branch are now able to build/run successfully with the latest changes 👍

hidmic

LGTM!

apojomovsky requested review from jacobperron and hidmic November 7, 2018 17:33

tfoote added the in progress Actively being worked on (Kanban column) label Nov 7, 2018

sloretz reviewed Nov 9, 2018

View reviewed changes

apojomovsky and others added 3 commits November 9, 2018 21:22

Added action__type_support.cpp.em template

1e31d74

Added support for actions in rosild_typesupport_c

6daf470

[urgent] Addresses peer review comments.

c4b9758

hidmic force-pushed the apojomovsky/cpp_typesupport_for_actions branch from 6d4db14 to c4b9758 Compare November 10, 2018 01:33

sservulo and others added 18 commits November 12, 2018 17:46

v1 Action cmake typesupport

3bb5682

v2 cmake action support

a4d3312

v3 cmake action support

8c8cf02

v4 Split workflow between existing typesupport and action typesupport

62c71eb

v5 Small edits to action_interfaces generator

3bc0b02

v6 Propagate update action tags from rosidl

f38570e

Full action pipeline for C and C++ working

8f0cb4e

Deduplicate aciton target names

b29c85e

Terminate namespace correctly

0e2b742

Depend on targets created by rosidl_generator_cpp

4330cc0

Depend on typesupport library for messages and services

d5ca00e

Actions typesupports depend on msg and srv typesupports

16e9279

use correct namespace for c++ typesupport

830f693

depend on generated C headers

5b1d93d

2016 -> 2018

7254749

Undo whitespace change

5c47212

CMake comment mentions actions

712ec23

Templatize file

3f816b6

sloretz mentioned this pull request Nov 13, 2018

Generate action type support / Remove hardcoded action type support ros2/rcl_interfaces#48

Merged

rosidl_generator_* generates visibility file

a98a3ff

hidmic reviewed Nov 13, 2018

View reviewed changes

Addressed peer review comments

c84356f

apojomovsky mentioned this pull request Nov 13, 2018

Add rosidl_generator_cpp templates for actions ros2/rosidl#312

Merged

hidmic reviewed Nov 13, 2018

View reviewed changes

Target name consistency

31efaca

hidmic approved these changes Nov 13, 2018

View reviewed changes

sloretz merged commit 0461362 into master Nov 13, 2018

sloretz deleted the apojomovsky/cpp_typesupport_for_actions branch November 13, 2018 18:51

sloretz removed the in progress Actively being worked on (Kanban column) label Nov 13, 2018

This was referenced Nov 13, 2018

Generate C typesupport for Actions ros2/rosidl#302

Closed

Generate C++ typesupport for actions ros2/rosidl#303

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add typesupport for actions in c and c++ #36

Add typesupport for actions in c and c++ #36

apojomovsky commented Nov 7, 2018 •

edited by sloretz

Loading

sloretz Nov 9, 2018

sloretz Nov 9, 2018

hidmic Nov 9, 2018 •

edited

Loading

sloretz Nov 9, 2018

hidmic Nov 10, 2018

sloretz Nov 9, 2018

hidmic Nov 10, 2018

sloretz Nov 9, 2018

hidmic Nov 10, 2018

sloretz Nov 9, 2018

hidmic Nov 10, 2018

hidmic left a comment

hidmic Nov 13, 2018

apojomovsky Nov 13, 2018

hidmic Nov 13, 2018

apojomovsky Nov 13, 2018

hidmic Nov 13, 2018

apojomovsky Nov 13, 2018

apojomovsky commented Nov 13, 2018

hidmic commented Nov 13, 2018 •

edited

Loading

hidmic Nov 13, 2018

sloretz Nov 13, 2018

sloretz Nov 13, 2018

sloretz commented Nov 13, 2018

apojomovsky commented Nov 13, 2018

hidmic left a comment

		@@ -0,0 +1,36 @@
		// Copyright 2016 Open Source Robotics Foundation, Inc.

		@@ -0,0 +1,124 @@
		// generated from rosidl_typesupport_c/resource/action__type_support.cpp.em

		@@ -0,0 +1,34 @@
		// Copyright 2016 Open Source Robotics Foundation, Inc.

Add typesupport for actions in c and c++ #36

Add typesupport for actions in c and c++ #36

Conversation

apojomovsky commented Nov 7, 2018 • edited by sloretz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic Nov 9, 2018 • 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

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

hidmic 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

apojomovsky commented Nov 13, 2018

hidmic commented Nov 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sloretz commented Nov 13, 2018

apojomovsky commented Nov 13, 2018

hidmic left a comment

Choose a reason for hiding this comment

apojomovsky commented Nov 7, 2018 •

edited by sloretz

Loading

hidmic Nov 9, 2018 •

edited

Loading

hidmic commented Nov 13, 2018 •

edited

Loading