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

Review of the rosidl_generator_c package #446

Closed
9 of 11 tasks
dirk-thomas opened this issue Mar 9, 2020 · 4 comments
Closed
9 of 11 tasks

Review of the rosidl_generator_c package #446

dirk-thomas opened this issue Mar 9, 2020 · 4 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@dirk-thomas
Copy link
Member

dirk-thomas commented Mar 9, 2020

Comments / feedback

Please provide feedback in separate comments - one per topic - to allow hiding them when they have been resolved - to keep the discussion readable and pending items visible.
Beside any additional feedback it is also valuable to comment if you agree/disagree with the check boxes indicating proposed to-dos.

Scope of the package

At the moment the package has two purposes:

  1. provide the message generator for C which generates C code / library at the build time of each message package
  2. provide the C headers / library which messages will depend on at runtime

This poses an unnecessary runtime dependency for message packages.
Therefore this package should be split into two parts.

Package dependencies

  • build dependency on ament_cmake_ros: it uses the shared/static library option provided by that package

  • export the build dependencies on ament_cmake: it registers itself as an extension

  • export the build dependencies on rosidl_cmake: it uses the CMake API provided by that package when generating code for message packages

  • build and run dependency on rosidl_typesupport_interface: the code compiled in this package as well as the headers exposed by message packages use headers from that package

All of these dependencies look reasonable.

ℹ️ The test dependencies are not being reviewed since they are not part of the public interface of the package.

The group membership currently lists both groups rosidl_generator_packages as well as rosidl_runtime_packages.

Directories and Files

bin

The script contains the CLI for the C generator.
All logic is in the Python module.

cmake

  • register_c.cmake contains a macro to register the rosidl_generate_idl_interfaces extension.
  • rosidl_generator_c_extras.cmake sets some CMake variables an invokes the above macro to register the extension.
  • rosidl_generator_c_generate_interfaces.cmake contains the CMake logic of the C generator extension which generates the C code for interfaces, builds a library and installs the artifacts.

include

All C headers end in .h.

All headers are in the subdirectory rosidl_generator_c.
While #443 moves the headers into the new package rosidl_runtime_c it doesn't rename the directory.
As a consequence functions, macros and types are still prefixed with the package name rosidl_generator_c.

Each header is named after the struct it declares or contains functions related to a certain type.
If applicable the header might also declare functions related to that struct.
Since all headers are in a subdirectory rosidl_generator_c their filename doesn't contain that part even if the struct is prefixed with the package name.

resource

The generated code the templates expand to is split into multiple files:

  • <interface-name>.h is the header commonly included by code using the interface.
    This file includes the following three headers:

    • <interface-name>__struct.h contains the structs of the messages.
      For a message that is a single struct whereas for a service it is the structs for the request and response.

      • The header also contains the struct for a sequence of messages.
        That implies that you can't use just the struct without also the sequence API which might be undesired.
        Maybe the sequence API could be split into a separate header (not being done for now).
    • <interface-name>__functions.h declares the functions operating on the interface struct.

      • create / destroy to allocate and deallocate a message.
      • init / fini to initialize and finalize the fields of a message.
      • The same four function for sequences of messages.
    • <interface-name>__type_support.h declares additional functions to access type information about the struct type.

Downstream packages commonly only use the generated code.
Therefore the API of the templates isn't considered public.

The templates starting with idl are the once being evaluated first.
Some of those templates then include other templates starting with action, msg, and srv depending on the interface type.

Additionally for each interface package a header with specific macros to control the visibility of all symbols is being generated.

rosidl_generator_c

The Python module contains the logic to parse IDL files and generate C code for them.
User land code commonly doesn't interact with this API but only uses the CMake extension to trigger the code generator in an interface package.

src

The source files contain the implementation of the functions declared in the headers.

test

The unit tests aren't part of the public interface of the package and are therefore not being considered here.

Naming of API

  • rosidl_generator_c__String: similar to the C++ type std::string except that it uses camel-case following the style guide for structs.
    Additionally a sequence type is generated using a macro.

  • rosidl_generator_c__U16String: similar to the C++ type std::u16string except that it uses camel-case following the style guide for structs.
    Additionally a sequence type is generated using a macro.

  • rosidl_generator_c__String__bounds: contains only a single numeric value.
    At the moment this structure is neither used in this package nor in any other package.

  • rosidl_message_bounds_t: ...

  • rosidl_generator_c__<TYPE>__Sequence: similar to the C++ type std::vector.
    A sequence type for all basic types is provided as well as a few legacy type for backward compatibility.

  • rosidl_runtime_c_message_initialization: defines different initialization options if/how the memory of an interface struct should be initialized with.

  • The type support structures combine an opaque pointer with an identifier to ensure that provided type support information for an interface are from a specific type before casting them.
    Beside the structures functions are being defined to retrieve the type support information for a specific interface type.

API signatures

Messages

Each interface struct contains the field names as member variables - without any prefix / suffix.

The create / destroy functions mimic the same signature as malloc and free.
The first signals failures by returning a null pointer.

The init function signals success through a boolean return value.

fini doesn't have to signal any result - similar to the free function.

Sequences

A sequence stores a data pointer to the items as well as a size and a capacity.
Separating the last two allows to decouple the allocation from the current number of stored items.

The init and fini function have the same signature as the ones interfaces described above.

String, U16String

The string structures behave like a sequence of characters and as such have the same interface as sequences described before.
For strings a resize function exists though.

The functions assign and assignn are provided to populate the characters with input data

For U16String the function assignn_from_char allows to populate the data from a pointer of differently typed characters.

For 16 bit characters the function len is provided to count the number of non-null characters.

@dirk-thomas dirk-thomas added enhancement New feature or request help wanted Extra attention is needed labels Mar 9, 2020
@mxgrey

This comment has been minimized.

@ros-discourse

This comment has been minimized.

@dirk-thomas
Copy link
Member Author

dirk-thomas commented Mar 24, 2020

Feedback from a review meeting on Mar 20th with the following attendees: Michael Carroll, Karsten Knese, Jacob Perron, Louise Poubel, Dirk Thomas, William Woodall.

Directories and Files - include

  • Use c_detail subdirectory under msg / srv / action for the non-entry point headers. The subdirectory name isn't being reflected in the symbol names.

API signature - Messages

@dirk-thomas
Copy link
Member Author

dirk-thomas commented May 4, 2020

With all items addressed, ticketed or pending in PRs (for the last doc update in #472) I will go ahead and close this.

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

No branches or pull requests

3 participants