add idl pipeline using a separate extension point #331
Conversation
dirk-thomas
added
enhancement
dirk-thomas
force-pushed
the
idl-stage-6
branch
from
73d2b40 to
0119915
Compare
|
The latest commit derives the actually communicated goal service, result service and feedback message from the three pieces provided by the |
|
Need to rebase on |
dirk-thomas
force-pushed
the
idl-stage-6
branch
2 times, most recently
from
b9f64e4 to
1d368cc
Compare
| --arguments-file "${arguments_file}" | ||
| --output-dir "${CMAKE_CURRENT_BINARY_DIR}/rosidl_adapter/${PROJECT_NAME}" | ||
| --output-file "${idl_output}") | ||
| execute_process( |
There was a problem hiding this comment.
It looks like execute_process() happens at configure time. It doesn't look like it can have file or target dependencies, or even run at build time. If I configure a package, change an .msg file and build, then I would expect this to not re-generate the .idl file. Does anything tell CMake to re-run rosidl_adapter at build time?
There was a problem hiding this comment.
This call ensures that CMake is re-run when any of the passed in interface files changes.
| # Split .srv into two .msg files | ||
| foreach(_idl_file ${_idl_files}) | ||
| get_filename_component(_extension "${_idl_file}" EXT) | ||
| set(_non_idl_files "") |
There was a problem hiding this comment.
Since this makes the foreach() below do nothing, and since services are now split by rosidl_adapter, can the foreach() below be deleted?
There was a problem hiding this comment.
I am not sure I see which line you are referring to? _non_idl_files is being populated in line 153 and further below used..
There was a problem hiding this comment.
I confused _non_idl_files and _non_idl_tuples. Nevermind
| assert isinstance(goal_request, Message) | ||
| assert goal_request.structure.type.namespaces == type_.namespaces | ||
| assert goal_request.structure.type.name == type_.name + \ | ||
| ACTION_GOAL_SERVICE_SUFFIX + SERVICE_REQUEST_MESSAGE_SUFFIX |
There was a problem hiding this comment.
I think the struct names for actions in .idl would be fine without SERVICE_<REQUEST/RESPONSE>_MESSAGE_SUFFIX at the end of them, but this works
There was a problem hiding this comment.
I think the suffix is needed to avoid potential collisions. The idea is that each type of interface lives in its own "namespace" without colliding with items of other types. E.g. a package can define a Fibonacci.msg, Fibonacci.srv and Fibonacci.action without the risk of collisions.
There was a problem hiding this comment.
I thought ACTION_GOAL_SERVICE_SUFFIX by containing an underscore avoided the namespace collision already.
There was a problem hiding this comment.
Can you describe the exact change you propose.
Do you want the user defined goal / result types to not have a request / response suffix?
dirk-thomas
force-pushed
the
idl-stage-6
branch
2 times, most recently
from
873b97a to
4ab49ff
Compare
|
dirk-thomas
force-pushed
the
idl-stage-6
branch
from
4ab49ff to
cf3d10f
Compare
|
Squashed the last two commit and merging... |
This is the sixth PR integrating #298 step-by-step.
Builds on top of #326. Needs ros2/system_tests#310.
rosidl_generate_idl_interfacesbeside the existing one namedrosidl_generate_interfaces. Existing generators will continue to work and can be migrate one-by-one to the new interface. Once that is completed the code related to the legacy pipeline can be removed..actionfiles to.idlfiles..idlfiles into an object representation.Linux build: