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

[WIP] restructure rosidl to work with IDL files #298

Closed
wants to merge 39 commits into from

Conversation

dirk-thomas
Copy link
Member

@dirk-thomas dirk-thomas commented Sep 19, 2018

These changes modify the rosidl processing pipeline to use .idl files (using a subset of OMG IDL 4.2). See https://github.com/ros2/ros2/blob/idl/README.rst#goals for a collection of bullets and references.

A high level overview of this work (the patch has been split up into separated PRs - see referenced PRs at the bottom of this ticket):

  • The CMake API to generate ROS interface files is being extended to allow passing message / service interface files explicitly (these must be .idl files). For backward compatibility .msg / .srv files can be passed as before (their extension already determines the interface type).
  • The package rosidl_adapter is aded which handles all incoming interface files (arbitrary file types).
    • The package contains the original logic to parse .msg / .srv files. Currently support to convert .msg / ,srv files to .idl is hard coded.
    • The msg / srv parser should support extracting comments and using them in the generated .idl files, see heuristic to extract comments from .msg files #316.
    • Simple CLI msg2idl / srv2idl is available.
    • The converters should be pluggable using Python entry points.
    • More of the tests need to be moved from rosidl_parser to rosidl_adapter.
  • The logic to parse .idl files using a grammar file for lark is now in rosidl_parser (since that will be the main interface language).
    • The object representation currently can't contain all the information from .idl files and needs to be extended replaced.
    • Any additional information beside simple constants and fields need to be extracted from the AST and converted into the object representation.
  • The CMake files from all message generators which registered themselves under the rosidl_generate_interfaces extension point must now handle IDL files - they never receive .msg / .srv files anymore.
  • Introduce a new extension point message generators need to switch to in order to use the IDL-based pipeline.
    • rosidl_generator_c has been updated.
      • Use updated type mapping as defined in the design draft
      • U16String type and functions
    • rosidl_generator_cpp has been updated.
      • The generator needs to make use of any additional information.
    • All other message generators

Currently this branch can be used to build some interface packages. For that all not yet changes message generators need to be skipped, e.g.:

=== ./ament_cmake (git) ===
diff --git a/ament_cmake_core/cmake/core/ament_execute_extensions.cmake b/ament_cmake_core/cmake/core/ament_execute_extensions.cmake
index 9f29963..bee969e 100644
--- a/ament_cmake_core/cmake/core/ament_execute_extensions.cmake
+++ b/ament_cmake_core/cmake/core/ament_execute_extensions.cmake
@@ -23,6 +23,7 @@
 macro(ament_execute_extensions extension_point)
   if(AMENT_EXTENSIONS_${extension_point})
     foreach(_extension ${AMENT_EXTENSIONS_${extension_point}})
+     if(NOT "${extension_point}" STREQUAL "rosidl_generate_interfaces" OR "${_extension}" STREQUAL "rosidl_generator_c:rosidl_generator_c_generate_interfaces.cmake")
       string(REPLACE ":" ";" _extension_list "${_extension}")
       list(LENGTH _extension_list _length)
       if(NOT _length EQUAL 2)
@@ -36,6 +37,7 @@ macro(ament_execute_extensions extension_point)
       assert_file_exists("${_extension_file}"
         "ament_execute_extensions(${extension_point}) registered extension '${_extension_file}' does not exist")
       include("${_extension_file}")
+     endif()
     endforeach()
   endif()
 endmacro()

@dirk-thomas dirk-thomas added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Sep 19, 2018
@dirk-thomas dirk-thomas self-assigned this Sep 19, 2018
@dirk-thomas dirk-thomas mentioned this pull request Sep 19, 2018
34 tasks
@mikaelarguedas
Copy link
Member

I gave this a shot, it seems that it fails to build on Xenial / Python 3.5:

Traceback (most recent call last):
  File "/usr/lib/python3.5/runpy.py", line 184, in _run_module_as_main
    "__main__", mod_spec)
  File "/usr/lib/python3.5/runpy.py", line 85, in _run_code
    exec(code, run_globals)
  File "/home/mikael/work/ros2/idl_ws/build_debug/rosidl_adapter/rosidl_adapter/__main__.py", line 19, in <module>
    sys.exit(main())
  File "/home/mikael/work/ros2/idl_ws/build_debug/rosidl_adapter/rosidl_adapter/main.py", line 64, in main
    output_dir)
  File "/home/mikael/work/ros2/idl_ws/build_debug/rosidl_adapter/rosidl_adapter/__init__.py", line 42, in convert_to_idl
    output_dir / package_name / 'msg'))
  File "/home/mikael/work/ros2/idl_ws/build_debug/rosidl_adapter/rosidl_adapter/msg/__init__.py", line 35, in convert_msg_to_idl
    expand_template('msg.idl.em', data, output_file)
  File "/home/mikael/work/ros2/idl_ws/build_debug/rosidl_adapter/rosidl_adapter/resource/__init__.py", line 25, in expand_template
    if os.path.exists(output_file):
  File "/usr/lib/python3.5/genericpath.py", line 19, in exists
    os.stat(path)
TypeError: argument should be string, bytes or integer, not PosixPath

@dirk-thomas
Copy link
Member Author

it seems that it fails to build on Xenial / Python 3.5:

I didn't try with Python 3.5 but it should be fixed by 9b0e9d0.

@dirk-thomas
Copy link
Member Author

With the latest commit the pipeline works and the rosidl_generator_c has been updated to provide at least the same features as before. The changes in the test_interfaces.c file will provide a good idea how existing code will need to change:

  • ROS 1 msg/srv files using char will map to uint8 (as it is specified in ROS 1) rather then signed char as it was the case in the ROS 2 implementation
  • the rosidl_generator_c__<TYPE>__Array__* functions are using IDL type names rather then ROS 1 type names: bool -> boolean, byte -> octet, float32 -> float, float64 -> double

While this can't be merged until all message generators have been updated to process .idl files instead of .msg / .srv files I am looking for some early feedback especially on how the message generator itself and its templates are organized / changed:

This new structure avoids any repetition in the templates (e.g. between messages and services) and allows easy integration of action and other interface definitions in the future. Also generators unable to handle new types like actions would automatically gracefully fall back to do nothing.

Copy link
Contributor

@sloretz sloretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review. The changes in test_interfaces.c looks good to me. I didn't look as closely as I would like at rosidl_generator_c/__init__.py, but the templates themselves look fine.


from rosidl_adapter.main import main

sys.exit(main())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need to be an entry point to work on windows?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't since it is invoked with python -m rosidl_adapter.

rosidl_adapter/rosidl_adapter/resource/srv.idl.em Outdated Show resolved Hide resolved
rosidl_cmake/cmake/rosidl_generate_interfaces.cmake Outdated Show resolved Hide resolved
locator, png_file=os.path.join(
args['output_dir'], str(idl_rel_path.parent),
idl_rel_path.stem) + '.png')
for template_file, generated_filename in mapping.items():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can multiple messages or service definitions live in the same IDL file? If so, can a generator generate code for messages and services in an IDL file while ignoring an IDL module or struct describing an action?

I'm wondering if it would be possible to add a step to the pipeline where an <action>.idl can be converted to an <action_plus_generated_messages_and_services>.idl. Some generators (including rmw specific ones) parse the idl, find messages and services, and generate code for messages and services. Other generators (not rmw specific ones) get the same file, parse it, find an action, and generate code for only the action.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, I see now that parse_idl_string() allows one message or one service per file.

Instead, would it be possible to add a step after rosidl_adapter but before calling the generators that parses an <action>.idl and creates a <send goal service>.idl, <get result service>.idl and <feedback message>.idl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can multiple messages or service definitions live in the same IDL file? If so, can a generator generate code for messages and services in an IDL file while ignoring an IDL module or struct describing an action?

Nevermind, I see now that parse_idl_string() allows one message or one service per file.

Yeah, to keep the complexity down it only accepts single definitions in the parse functions. The actual logic in the C generator though is capable of handling any number of definitions. So the parse functions can certainly be extended in the future to enable that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if it would be possible to add a step to the pipeline where an <action>.idl can be converted to an <action_plus_generated_messages_and_services>.idl.

Instead, would it be possible to add a step after rosidl_adapter but before calling the generators that parses an <action>.idl and creates a <send goal service>.idl, <get result service>.idl and <feedback message>.idl?

I don't think these files need to be generated explicitly. When an .action file is parsed (actually the equivalent .idl file) the object representation can provide these extended derived definitions. So each generator handling an action can feed these sub-parts to an existing message / service template. Similar to how a service objects contains two message objects and just invoked the message template twice - once for the request, once for the response - and optionally generates something service specific in addition. The action object would just contain not only plain messages / services which have 1-to-1 the same content as in the .action file but contain the "enriched" versions of the derived types.

@{
from rosidl_parser.definition import Service
}@
@[for service in content.get_elements_of_type(Service)]@
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC parse_idl_string() will only result in an IdlContent created with one service definition. Is this a hook for supporting multiple services in an IDL file in the future?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as #298 (comment) In the future parse_idl_string should be extended to support multiple definitions in a single file. It is just more complex to do so I haven't done it in this round.

@dirk-thomas
Copy link
Member Author

Closing in favor of #346 and #334.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants