Add support for message definitions in omg idl syntax#617
Add support for message definitions in omg idl syntax#617woutervdstoel wants to merge 4 commits into
Conversation
6983f20 to
36bd9c2
Compare
mvukov
left a comment
There was a problem hiding this comment.
Last time I looked at this, there were constraints:
- 1 msg in .idl file -> ROS msg
- 2 msg in .idl file -> ROS srv
- 3 msg in .idl file -> ROS action
and that logic is/was pretty much hard-coded in the parser (IIRC).
There should be decent disclaimer about this. Instead of silently failing, what can be done to ensure proper error handline? -- not for this PR but I am curious.
Also, what's the motivation to use .idl directly?
| outputs.append(src_file) | ||
| idl_manifest_contents.append(paths.join("msg", src.basename)) | ||
|
|
||
| elif src.extension == "idl": |
There was a problem hiding this comment.
Please document why is this necessary.
There was a problem hiding this comment.
Updated the PR with simplified logic and some explanation about generated_idl_files.
| return struct( | ||
| package_name = target.label.name, | ||
| srcs = target[Ros2InterfaceInfo].info.srcs, | ||
| generated_idl_files = target[IdlAdapterAspectInfo].generated_idl_files if IdlAdapterAspectInfo in target else None, |
There was a problem hiding this comment.
Why are changes needed in this file?
There was a problem hiding this comment.
I explained in a comment in ament.bzl why we also need the generated_idl_files . I'm open to implementation suggestions that don't involve changing this struct because I agree this is a bit of a detour.
b609bea to
893c59d
Compare
I don't think we can handle this at analysis time since it requires looking at the content of the files. We could of course generate tests to assert it. I think that since it's a feature for advanced users anyway, maybe it doesn't need the greatest error handling?
In our setup: we want to annotate certain message fields with some custom annotations that another tool looks as (which reuses the existing rosidl parser). The IDL format is much more flexible in this regard than the msg format. I also happen to know that Apex.AI recommends their customers directly write IDL because of the extended feature set. |
| implementation = _ros2_interface_collector_aspect_impl, | ||
| attr_aspects = _ROS2_COLLECTOR_ATTR_ASPECTS, | ||
| provides = [Ros2InterfaceCollectorAspectInfo], | ||
| required_aspect_providers = [IdlAdapterAspectInfo], |
There was a problem hiding this comment.
This is necessary for the logic above to correctly be able to access the IdlAdapterAspectInfo
For starters, I think it would be enough to clearly document this e.g. in ros2_interface_library rule docs. In a follow-up a proper build time testing can be introduced. |
|
@woutervdstoel Any plans to wrap up this work? |
|
@mvukov I just pushed the documentation change you requested - please let me know if there's anything else that still needs improvement |
82929f6 to
2997cde
Compare
2c144ed to
4737dbe
Compare
mvukov
left a comment
There was a problem hiding this comment.
On second thought, I have more questions.
Please let me know if you have time to work on this in the coming days -- let's see whether makes sense to iron out this PR or I can proceed with #558 .
BTW, I pushed a small fix to your branch recently today.
|
|
||
| new_file = ctx.actions.declare_file("{}/{}/{}.idl".format(package_name, idl_submodule_name, stem)) | ||
| idl_files.append(new_file) | ||
| ctx.actions.run_shell( |
| def _get_stem(path): | ||
| return path.basename[:-len(path.extension) - 1] | ||
|
|
||
| def _get_idl_submodule_name(path): |
There was a problem hiding this comment.
On second thought... submodule is misleading and not readable. I'd rename submodule -> type because this essentially determines a type (action, msg, srv).
| For .msg/.srv and other files, return the extension | ||
| """ | ||
| if path.extension == "idl": | ||
| return path.dirname.split("/")[-1] |
There was a problem hiding this comment.
If anything other than action/msg/srv this should error.
| return idl_files, idl_tuples | ||
| return idl_files, generated_idl_files, idl_tuples | ||
|
|
||
| IdlAdapterAspectInfo = provider("TBD", fields = [ |
There was a problem hiding this comment.
TBH, I don't see a need to touch this. But I'd like to learn more.
| output_dir = adapter_map.dirname | ||
|
|
||
| idl_files = [] | ||
| generated_idl_files = [] |
There was a problem hiding this comment.
On second thought: why don't you dump everything in idl_files?
IIUC, generated_idl_files is needed such that we put idl files to the ament index. We can dump all idl_files to the index IMO.
| # Generated IDL files in the output tree such that the rosbag mcap writer can include them in the metadata. | ||
| # mcap supports both IDL and msg definitions, but not combinations. Therefore, if any handwritten IDL files are | ||
| # present, the rosbag writer also needs any generated IDL files it depends on. | ||
| if idl.generated_idl_files != None: |
There was a problem hiding this comment.
IIUC, here you dump generated IDL files but not user-provided IDL files, right? How then e.g. rosbag knows how to handle user-provided IDL files?
mvukov
left a comment
There was a problem hiding this comment.
Until we sort out some details I pointed out earlier, I'll just make this PR as a not yet ready to be merged.
This PR adds support for directly including .idl files as message definitions rather than having them be generated by rosidl. It also supports mixing and matching of idl and msg definitions.
In our repo I also verified this works with rosbag recording and replay.