change generators to IDL-based pipeline, change char type, separate action types #334

dirk-thomas · 2018-11-21T22:04:06Z

This is the seventh PR integrating #298 step-by-step.

Builds on top of #331.

It adds a Python function usable by message generators to reduce duplicate logic.
It updates the rosidl_generator_c and rosidl_generator_cpp packages to use the new IDL-based extension points. One change effecting other packages is that the generators now generate the same set of files for each input .idl file (no different sets depending on the content of the interface file) - see next bullet.
It updates the includes of the generated code to match the reduced number of generated files.
It removes the rosidl_actions package as well as any other custom processing logic for actions.
Skip passing .action files to the legacy generators.

Similar to the third bullet this patch requires the following changes:

...

mjcarroll

First, this required installation of pydot on my machine.

Currently getting a build failure:

--- stderr: rosidl_generator_c                                
Unknown named type: boolean /home/mjcarroll/workspaces/ros2_ws/src/build/rosidl_generator_c/rosidl_adapter/rosidl_generator_c/msg/Bool.idl
Error processing idl file: /home/mjcarroll/workspaces/ros2_ws/src/build/rosidl_generator_c/rosidl_adapter/rosidl_generator_c/msg/Bool.idl
Traceback (most recent call last):
  File "/home/mjcarroll/workspaces/ros2_ws/src/ros2/rosidl/rosidl_generator_c/bin/rosidl_generator_c", line 40, in <module>
    sys.exit(main())
  File "/home/mjcarroll/workspaces/ros2_ws/src/ros2/rosidl/rosidl_generator_c/bin/rosidl_generator_c", line 35, in main
    args.generator_arguments_file,
  File "/home/mjcarroll/workspaces/ros2_ws/src/ros2/rosidl/rosidl_generator_c/rosidl_generator_c/__init__.py", line 35, in generate_c
    generate_files(generator_arguments_file, mapping)
  File "/home/mjcarroll/workspaces/ros2_ws/src/install/rosidl_cmake/lib/python3.6/site-packages/rosidl_cmake/__init__.py", line 122, in generate_files
    raise(e)
  File "/home/mjcarroll/workspaces/ros2_ws/src/install/rosidl_cmake/lib/python3.6/site-packages/rosidl_cmake/__init__.py", line 101, in generate_files
    idl_rel_path.stem) + '.png')
  File "/home/mjcarroll/workspaces/ros2_ws/src/install/rosidl_parser/lib/python3.6/site-packages/rosidl_parser/parser.py", line 60, in parse_idl_file
    content = parse_idl_string(string, png_file=png_file)
  File "/home/mjcarroll/workspaces/ros2_ws/src/install/rosidl_parser/lib/python3.6/site-packages/rosidl_parser/parser.py", line 80, in parse_idl_string
    return extract_content_from_ast(tree)
  File "/home/mjcarroll/workspaces/ros2_ws/src/install/rosidl_parser/lib/python3.6/site-packages/rosidl_parser/parser.py", line 129, in extract_content_from_ast
    resolve_typedefed_names(msg.structure, typedefs)
  File "/home/mjcarroll/workspaces/ros2_ws/src/install/rosidl_parser/lib/python3.6/site-packages/rosidl_parser/parser.py", line 232, in resolve_typedefed_names
    assert type_.name in typedefs, 'Unknown named type: ' + type_.name
AssertionError: Unknown named type: boolean

rosidl_generator_c/msg/PrimitiveValues.msg

dirk-thomas · 2018-11-21T22:29:44Z

this required installation of pydot on my machine.

I can disable the generation of the AST graph before the merge - or better make it conditional.

Currently getting a build failure:

That shouldn't be the case. Are you sure you have the python3-lark-parser dependency installed?

Anyway the build is currently expected to fail way later (around test_msgs) and needs branches from various repos for the changed header names.

sloretz · 2018-11-22T02:25:34Z

rosidl_generator_c/resource/msg__struct.h.em

-@[for field in spec.fields]@
-  @(msg_type_to_c(field.type, field.name));
+@[for member in message.structure.members]@
+  @(idl_declaration_to_c(member.type, member.name));


It looks like this is returning a declaration with uint8 for a field char[<=3] char_values in test_msgs/msg/BoundedArrayPrimitives.msg. It cases a warning while building

/home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c: In function ‘test_msgs__msg__bounded_array_primitives__convert_from_py’: /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c:144:51: warning: passing argument 1 of ‘rosidl_generator_c__char__Sequence__init’ from incompatible pointer type [-Wincompatible-po inter-types] if (!rosidl_generator_c__char__Sequence__init(&(ros_message->char_values), size)) { ^ In file included from /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c:13:0: /home/developer/workspaces/ros2-dev0/install/rosidl_generator_c/include/rosidl_generator_c/primitives_sequence_functions.h:31:8: note: expected ‘rosidl_generator_c__char__Sequence * {aka struct rosidl_generator_c__char__Sequence *}’ but argument is of type ‘rosidl_generator_c__uint8__Sequence * {aka struct rosidl_generator_c__uint8__Sequence *}’ bool rosidl_generator_c__ ## STRUCT_NAME ## __Sequence__init( \ ^ ^~~~~~~~~~~

There are other warnings that all look related to char/uint8 confusion

/home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_dynamic_array_primitives_s.c: In function ‘test_msgs__msg__dynamic_array_primitives__convert_from_py’: /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_dynamic_array_primitives_s.c:144:51: warning: passing argument 1 of ‘rosidl_generator_c__char__Sequence__init’ from incompatible pointer type [-Wincompatible-po inter-types] if (!rosidl_generator_c__char__Sequence__init(&(ros_message->char_values), size)) { ^ In file included from /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_dynamic_array_primitives_s.c:13:0: /home/developer/workspaces/ros2-dev0/install/rosidl_generator_c/include/rosidl_generator_c/primitives_sequence_functions.h:31:8: note: expected ‘rosidl_generator_c__char__Sequence * {aka struct rosidl_generator_c__char__Sequence *}’ but argument is of type ‘rosidl_generator_c__uint8__Sequence * {aka struct rosidl_generator_c__uint8__Sequence *}’ bool rosidl_generator_c__ ## STRUCT_NAME ## __Sequence__init( \ ^ /home/developer/workspaces/ros2-dev0/install/rosidl_generator_c/include/rosidl_generator_c/primitives_sequence_functions.h:42:1: note: in expansion of macro ‘ROSIDL_GENERATOR_C__DECLARE_PRIMITIVE_SEQUENCE_FUNCTIONS’ ROSIDL_GENERATOR_C__DECLARE_PRIMITIVE_SEQUENCE_FUNCTIONS(char, signed char) ^ /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_dynamic_array_primitives_s.c:150:26: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] signed char * dest = ros_message->char_values.data; ^~~~~~~~~~~ /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_dynamic_array_primitives_s.c: In function ‘test_msgs__msg__dynamic_array_primitives__convert_to_py’: /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_dynamic_array_primitives_s.c:652:25: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] signed char * src = ros_message->char_values.data; ^~~~~~~~~~~ /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c: In function ‘test_msgs__msg__bounded_array_primitives__convert_from_py’: /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c:144:51: warning: passing argument 1 of ‘rosidl_generator_c__char__Sequence__init’ from incompatible pointer type [-Wincompatible-po inter-types] if (!rosidl_generator_c__char__Sequence__init(&(ros_message->char_values), size)) { ^ In file included from /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c:13:0: /home/developer/workspaces/ros2-dev0/install/rosidl_generator_c/include/rosidl_generator_c/primitives_sequence_functions.h:31:8: note: expected ‘rosidl_generator_c__char__Sequence * {aka struct rosidl_generator_c__char__Sequence *}’ but argument is of type ‘rosidl_generator_c__uint8__Sequence * {aka struct rosidl_generator_c__uint8__Sequence *}’ bool rosidl_generator_c__ ## STRUCT_NAME ## __Sequence__init( \ ^ /home/developer/workspaces/ros2-dev0/install/rosidl_generator_c/include/rosidl_generator_c/primitives_sequence_functions.h:42:1: note: in expansion of macro ‘ROSIDL_GENERATOR_C__DECLARE_PRIMITIVE_SEQUENCE_FUNCTIONS’ ROSIDL_GENERATOR_C__DECLARE_PRIMITIVE_SEQUENCE_FUNCTIONS(char, signed char) ^ /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c:150:26: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] signed char * dest = ros_message->char_values.data; ^~~~~~~~~~~ /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c: In function ‘test_msgs__msg__bounded_array_primitives__convert_to_py’: /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_bounded_array_primitives_s.c:652:25: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] signed char * src = ros_message->char_values.data; ^~~~~~~~~~~ /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_static_array_primitives_s.c: In function ‘test_msgs__msg__static_array_primitives__convert_from_py’: /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_static_array_primitives_s.c:117:26: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] signed char * dest = ros_message->char_values; ^~~~~~~~~~~ /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_static_array_primitives_s.c: In function ‘test_msgs__msg__static_array_primitives__convert_to_py’: /home/developer/workspaces/ros2-dev0/build/test_msgs/rosidl_generator_py/test_msgs/msg/_static_array_primitives_s.c:489:25: warning: pointer targets in initialization differ in signedness [-Wpointer-sign] signed char * src = ros_message->char_values; ^~~~~~~~~~~

It looks like this is returning a declaration with uint8 for a field char[<=3] char_values in test_msgs/msg/BoundedArrayPrimitives.msg.

That is the expected mapping. The ROS 1 type char is mapped to the IDL type uint8 - see https://github.com/ros2/design/blob/gh-pages/articles/143_legacy_interface_definition.md#mapping-to-idl-types This matches the definition of char from ROS 1: http://wiki.ros.org/msg#Field_Types

Regarding the warnings are you using ros2/rosidl_python#24 in your workspace?

dirk-thomas · 2018-11-26T16:15:33Z

Linux
Linux-aarch64
macOS
Windows

hidmic

A few comments on things I noticed. I do think that I'm lacking a bit of context, so I cannot fully assess any shortcuts taken.

rosidl_generator_c/include/rosidl_generator_c/u16string_functions.h

rosidl_generator_c/resource/idl.h.em

rosidl_generator_c/resource/idl__functions.c.em

rosidl_generator_c/src/u16string_functions.c

hidmic · 2018-11-27T00:11:34Z

rosidl_generator_c/test/test_interfaces.c

+#define EXPECT_EQ(arg1, arg2) if ((arg1) != (arg2)) { \
+    fputs(STRINGIFY(arg1) " != " STRINGIFY(arg2) "\n", stderr); \
+    return 1; \
+}


@dirk-thomas super minor: consider doing the same for EXPECT_NE.

rosidl_generator_cpp/resource/msg__struct.hpp.em

rosidl_generator_cpp/test/test_interfaces.cpp

rosidl_typesupport_introspection_cpp/resource/srv__type_support.cpp.em

dirk-thomas · 2018-11-28T01:47:13Z

Builds including the updated introspection typesupport packages:

Linux
Linux-aarch64 (using FastRTPS static and dynamic)
macOS
Windows

hidmic

Few minor comments, I'll have another pass tomorrow on those templates though.

...pesupport_introspection_c/cmake/rosidl_typesupport_introspection_c_generate_interfaces.cmake

...pport_introspection_cpp/cmake/rosidl_typesupport_introspection_cpp_generate_interfaces.cmake

dirk-thomas · 2018-12-03T18:47:44Z

Builds including the updated FastRTPS typesupport packages:

Linux
Linux-aarch64 (using FastRTPS static and dynamic)
macOS
Windows

dirk-thomas · 2018-12-05T01:24:09Z

Builds including the updated rosidl_generator_py package and the Connext TS changes:

Linux
Linux-aarch64
macOS
Windows

dirk-thomas · 2018-12-05T16:04:00Z

Latest builds with all RMW (without repeating failed test, so some might be flaky):

Linux
Linux-aarch64
macOS
Windows

rosidl_adapter/rosidl_adapter/parser.py

rosidl_cmake/cmake/rosidl_generate_interfaces.cmake

rosidl_generator_c/cmake/rosidl_generator_c_generate_interfaces.cmake

rosidl_generator_c/resource/idl__type_support.h.em

sloretz · 2019-02-25T18:35:22Z

rosidl_adapter/rosidl_adapter/msg/__init__.py

        elements = [repr(v) for v in value]
        while len(elements) < 2:
            elements.append('')
        return '"(%s)"' % ', '.join(e.replace('"', r'\"') for e in elements)

    if 'boolean' == idl_type:
        return 'TRUE' if value else 'FALSE'
-    if 'string' == idl_type:
+    if idl_type.startswith('string'):


It looks like this might trigger for fixed sized array's of nested messages if the package name starts with string because of

rosidl/rosidl_adapter/rosidl_adapter/msg/__init__.py

Lines 96 to 97 in ab1b43a

identifier = '{type_.pkg_name}::msg::{type_.type}' \

.format_map(locals())

and this return

rosidl/rosidl_adapter/rosidl_adapter/msg/__init__.py

Lines 102 to 103 in ab1b43a

if type_.is_fixed_size_array():

return '{identifier}[{type_.array_size}]'.format_map(locals())

when called here

rosidl/rosidl_adapter/rosidl_adapter/resource/struct.idl.em

Line 43 in ab1b43a

const @(get_idl_type(constant.type)) @(constant.name) = @(to_idl_literal(get_idl_type(constant.type), constant.value));

I assume startswith is used for bounded strings?

The function is only used for constant values and default values. So nested messages aren't supported in these cases atm. While certainly not future proof I think it is good enough for now.

hidmic

Just a few minor comments.

rosidl_generator_cpp/resource/msg__struct.hpp.em

hidmic · 2019-02-25T18:11:27Z

rosidl_generator_cpp/resource/msg__struct.hpp.em

-const @(MSG_TYPE_TO_CPP[c.type])
-@(spec.base_type.type)_<ContainerAllocator>::@(c.name) = "@(escape_string(c.value))";
+const @(MSG_TYPE_TO_CPP['string'])
+@(message.structure.type.name)_<ContainerAllocator>::@(c.name) = "@(escape_string(c.value))";


@dirk-thomas FYI, and probably out of scope, but since a string constant is actually a const char *, you could make this constexpr too just like it is done immediately below.

I will leave this as an enhancement in the future.

rosidl_generator_cpp/test/test_interfaces.cpp

rosidl_generator_c/rosidl_generator_c/__init__.py

rosidl_generator_c/src/u16string_functions.c

rosidl_generator_cpp/resource/msg__struct.hpp.em

rosidl_generator_c/src/u16string_functions.c

rosidl_generator_cpp/resource/idl__struct.hpp.em

dirk-thomas · 2019-02-26T18:07:48Z

Addressed all the feedback.

jacobperron

LGTM

jacobperron · 2019-04-27T01:23:14Z

rosidl_cmake/cmake/rosidl_generate_action_interfaces.cmake

-  list_append_unique(_ARG_RGAI_DEPENDENCY_PACKAGE_NAMES "unique_identifier_msgs")
-  ament_export_dependencies(action_msgs)
-  ament_export_dependencies(builtin_interfaces)
-  ament_export_dependencies(unique_identifier_msgs)


@dirk-thomas It looks like bringing along these dependencies for actions were lost during the pipeline change. See ros2/rcl_interfaces#75.

Bug introduced by #334. Signed-off-by: Jacob Perron <[email protected]>

dirk-thomas added the in progress Actively being worked on (Kanban column) label Nov 21, 2018

dirk-thomas self-assigned this Nov 21, 2018

dirk-thomas mentioned this pull request Nov 21, 2018

change generators to IDL-based pipeline ros2/rosidl_typesupport#39

Merged

dirk-thomas added the enhancement New feature or request label Nov 21, 2018

mjcarroll reviewed Nov 21, 2018

View reviewed changes

rosidl_generator_c/msg/PrimitiveValues.msg Show resolved Hide resolved

sloretz reviewed Nov 22, 2018

View reviewed changes

dirk-thomas force-pushed the idl-stage-7 branch from c43d7fa to be891f1 Compare November 24, 2018 17:23

This was referenced Nov 24, 2018

update tests to use separated action types ros2/rcl#340

Merged

update char type mapping, update to use separated action types ros2/system_tests#315

Merged

dirk-thomas force-pushed the idl-stage-7 branch 3 times, most recently from 08f947b to 8f8beed Compare November 25, 2018 05:33

dirk-thomas added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Nov 26, 2018

hidmic reviewed Nov 27, 2018

View reviewed changes

hidmic reviewed Nov 28, 2018

View reviewed changes

...pesupport_introspection_c/cmake/rosidl_typesupport_introspection_c_generate_interfaces.cmake Outdated Show resolved Hide resolved

...pport_introspection_cpp/cmake/rosidl_typesupport_introspection_cpp_generate_interfaces.cmake Outdated Show resolved Hide resolved

dirk-thomas force-pushed the idl-stage-7 branch from c849052 to ae09654 Compare November 30, 2018 18:17

jacobperron mentioned this pull request Dec 3, 2018

Update generator to convert from IDL ros2/rosidl_dds#46

Closed

jacobperron self-assigned this Dec 3, 2018

jacobperron mentioned this pull request Dec 5, 2018

change generator to IDL-based pipeline ros2/rosidl_dds#47

Merged

dirk-thomas mentioned this pull request Feb 22, 2019

Group message initializations by their appropriate checks #350

Merged

jacobperron reviewed Feb 25, 2019

View reviewed changes

sloretz reviewed Feb 25, 2019

View reviewed changes

hidmic reviewed Feb 25, 2019

View reviewed changes

jacobperron reviewed Feb 25, 2019

View reviewed changes

rosidl_generator_cpp/resource/idl__struct.hpp.em Show resolved Hide resolved

dirk-thomas added 12 commits February 26, 2019 09:24

remove obsolete constant

2c58d6c

targeting Dashing

2555d3e

removed comment

5217c46

moved template calls

f5ee01c

remove obsolete todo

19aea38

removed commented code

e155168

fix initialization of u16string

b0f4c42

fix u16string len

cca3f1a

avoid double initialization

e842f17

moved template calls

82cef24

comment in test

e668e85

fix has_bounded_size trait

48bd7d0

jacobperron approved these changes Feb 26, 2019

View reviewed changes

dirk-thomas added 2 commits February 28, 2019 08:52

readd explicit dependency on absolute paths of idl files

b51372f

reduce logic with all action related types being in a single file

1509ae8

dirk-thomas merged commit 416ebd8 into master Mar 12, 2019

dirk-thomas deleted the idl-stage-7 branch March 12, 2019 04:11

dirk-thomas removed the in review Waiting for review (Kanban column) label Mar 12, 2019

dirk-thomas mentioned this pull request Apr 15, 2019

Remove obsolete API #359

Merged

jacobperron reviewed Apr 27, 2019

View reviewed changes

jacobperron added a commit that referenced this pull request May 14, 2019

Fix condition for including resize functions

0a29a77

Bug introduced by #334. Signed-off-by: Jacob Perron <[email protected]>

jacobperron mentioned this pull request May 14, 2019

Fix condition for including resize functions #379

Merged

jacobperron added a commit that referenced this pull request May 14, 2019

Fix condition for including resize functions (#379)

6f8194a

Bug introduced by #334. Signed-off-by: Jacob Perron <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

change generators to IDL-based pipeline, change char type, separate action types #334

change generators to IDL-based pipeline, change char type, separate action types #334

dirk-thomas commented Nov 21, 2018

mjcarroll left a comment

dirk-thomas commented Nov 21, 2018

sloretz Nov 22, 2018

dirk-thomas Nov 22, 2018

dirk-thomas commented Nov 26, 2018 •

edited by hidmic

Loading

hidmic left a comment

hidmic Nov 27, 2018

dirk-thomas commented Nov 28, 2018 •

edited

Loading

hidmic left a comment

dirk-thomas commented Dec 3, 2018

dirk-thomas commented Dec 5, 2018 •

edited

Loading

dirk-thomas commented Dec 5, 2018 •

edited

Loading

sloretz Feb 25, 2019

dirk-thomas Feb 26, 2019

hidmic left a comment

hidmic Feb 25, 2019

dirk-thomas Feb 26, 2019

dirk-thomas commented Feb 26, 2019

jacobperron left a comment

jacobperron Apr 27, 2019

	identifier = '{type_.pkg_name}::msg::{type_.type}' \
	.format_map(locals())

	if type_.is_fixed_size_array():
	return '{identifier}[{type_.array_size}]'.format_map(locals())

change generators to IDL-based pipeline, change char type, separate action types #334

change generators to IDL-based pipeline, change char type, separate action types #334

Conversation

dirk-thomas commented Nov 21, 2018

mjcarroll left a comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 26, 2018 • edited by hidmic Loading

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 28, 2018 • edited Loading

hidmic left a comment

Choose a reason for hiding this comment

dirk-thomas commented Dec 3, 2018

dirk-thomas commented Dec 5, 2018 • edited Loading

dirk-thomas commented Dec 5, 2018 • edited Loading

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

dirk-thomas commented Feb 26, 2019

jacobperron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dirk-thomas commented Nov 26, 2018 •

edited by hidmic

Loading

dirk-thomas commented Nov 28, 2018 •

edited

Loading

dirk-thomas commented Dec 5, 2018 •

edited

Loading

dirk-thomas commented Dec 5, 2018 •

edited

Loading