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

Decouple idl generation pipeline from rmw implementations #50

Closed
ivanpauno opened this issue Aug 28, 2019 · 9 comments
Closed

Decouple idl generation pipeline from rmw implementations #50

ivanpauno opened this issue Aug 28, 2019 · 9 comments
Assignees
Labels
enhancement New feature or request

Comments

@ivanpauno
Copy link
Member

Feature request

Feature description

Currently, rosidl_generate_interfaces from rosidl_cmake can't be called before the rmw implementations are built.
Decoupling idl generation from the rmw implementations will allow to use the same message abstractions at rmw level for implementing ros2/design#250.

Implementation considerations

Currently, rosidl_typesupport_c and rosidl_typesupport_cpp have a build dependency on rmw_implementation, which depends on the available implementations (see 1 2).
I first believed that they depend on rmw_implementation, because the typesupports are being registered there (e.g.: rmw_fastrtps_cpp), but I later realize that that resource isn't needed in rmw_typesupport_* packages nor in rosidl_cmake.
If that the case, we could simply delete that dependency.

For using the idl pipeline generation (calling rosidl_generate_interfaces macro), you may want to use the default generators by adding a build tool dependency on rosidl_default_generators, which depends on the rmw_typesupport_* and also in rosidl_generator_py. The latest, also depends on the rmw_implementation package, because it is using the previously commented resource registered by the rmw implementations.
I think that we could avoid using that resource, and directly query this other.
As I don't need the python generated file, I will only add a dependency on the packages I need instead of using rosidl_default_generators.

@ivanpauno ivanpauno added the enhancement New feature or request label Aug 28, 2019
@ivanpauno
Copy link
Member Author

@dirk-thomas Can you check if my description is correct?

@dirk-thomas
Copy link
Member

I first believed that they depend on rmw_implementation, because the typesupports are being registered there (e.g.: rmw_fastrtps_cpp), but I later realize that that resource isn't needed in rmw_typesupport_* packages nor in rosidl_cmake.

The RMW implementations register their typesupports with this: https://github.com/ros2/rmw/blob/9f2e181306c66ac0f09674086a108d4657611766/rmw/cmake/register_rmw_implementation.cmake#L65

And the rosidl_typesupport_c/cpp packages query that information (

get_used_typesupports(typesupports "rosidl_typesupport_cpp")
) to determine how many typesupports for a specific language exist since it generates different code / logic depending on if there is a single typesupport or multiple available.

Therefore I doubt that dependency can simply be eliminated.

@ivanpauno
Copy link
Member Author

ivanpauno commented Aug 28, 2019

I first believed that they depend on rmw_implementation, because the typesupports are being registered there (e.g.: rmw_fastrtps_cpp), but I later realize that that resource isn't needed in rmw_typesupport_* packages nor in rosidl_cmake.

The RMW implementations register their typesupports with this: https://github.com/ros2/rmw/blob/9f2e181306c66ac0f09674086a108d4657611766/rmw/cmake/register_rmw_implementation.cmake#L65

And the rosidl_typesupport_c/cpp packages query that information (

get_used_typesupports(typesupports "rosidl_typesupport_cpp")

) to determine how many typesupports for a specific language exist since it generates different code / logic depending on if there is a single typesupport or multiple available.
Therefore I doubt that dependency can simply be eliminated.

Yes, I ran into that problem after trying to build the interfaces before the rmw implementations.
What is the rationale behing checking that the available typesupports are used by an rmw implementation?
Why not delete all this checking and generate interfaces for all the available typesupports?

@dirk-thomas
Copy link
Member

What is the rationale behing checking that the available typesupports are used by an rmw implementation?

If you only have a single typesupport you avoid the overhead of the indirection by directly linking against it.

@ivanpauno
Copy link
Member Author

ivanpauno commented Sep 4, 2019

Closing, as I've already found a way of solving the problem. See:

@dirk-thomas
Copy link
Member

I will reopen this since the previous work around has reached its limits with recent changes. The goal described by this ticket is what is imo the correct approach to solve ros2/rmw_dds_common#8.

@dirk-thomas dirk-thomas reopened this Apr 9, 2020
@dirk-thomas dirk-thomas self-assigned this Apr 9, 2020
@ivanpauno
Copy link
Member Author

ivanpauno commented Apr 10, 2020

I've cancelled both linux jobs, as there are a lot of failures in the macos and Windows ones.
It seems to be related with the change in rosidl_python.

@dirk-thomas
Copy link
Member

The cpplint / uncrustify warnings were resolved by ros2/rmw_fastrtps@ff60121.

The flake8 warnings are unrelated and targeted by ros2/rclpy#539.

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

No branches or pull requests

2 participants