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

Supports subdirectories in msg folder #821

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

ryought
Copy link

@ryought ryought commented Dec 8, 2021

Public API Changes

None

Description

Fixed the generate-ros-messages command not working properly if the msg file is in a subdirectory inside the msg directory.

For example, consider the following simple ROS2 package.

tutorial_interfaces/
|-- CMakeLists.txt
|-- msg
|   |-- Foo.msg
|   `-- bar
|       `-- Bar.msg
`-- package.xml

This is the example package used in the ros2 tutorial, but has two message definitions, one is in the subdirectory bar.

After the npx generate-ros-messages is executed for this package,

|-- tutorial_interfaces__bar__Bar.js
`-- tutorial_interfaces__msg__Foo.js

which comes from these msg files after build

/workspace/ros/install/tutorial_interfaces/share/tutorial_interfaces/msg/Foo.msg
/workspace/ros/install/tutorial_interfaces/share/tutorial_interfaces/bar/Bar.msg

are generated under generated/ folder. The first one should be tutorial_interfaces__msg__Bar.js, but is named incorrectly, so the message Bar cannot be used in publisher or listener.

So I changed rosidl_gen/packages.js so that it works even if some messages are in subdirectories.
Caveats: This modification does not work for services in subdirectories and only supports messages in subdirectories.

Related issues
RobotWebTools/ros2-web-bridge#175
ros2/rosidl#213

@wayneparrott
Copy link
Collaborator

@ryought Thx for the PR. We've been heads down getting the update to node16 in place. Will review asap (next few days).

@ryought ryought marked this pull request as ready for review February 2, 2022 04:21
@ryought
Copy link
Author

ryought commented Feb 2, 2022

@wayneparrott Thanks for your reply. I forgot to change to the ready-to-review PR. Could you check/review this PR?

@wayneparrott
Copy link
Collaborator

wayneparrott commented Feb 10, 2022

@ryought apologies for the slow follow up. When I read the initial PR comment it referenced a tutorial that I assumed demonstrated a subfolder hierarchy for msg IDL files. After looking at the tutorial it promotes the classic convention of msg idl files residing in a subdirectory of depth 1, i.e., msg/<myinterface>.msg and not msg/subfolder/<myinterface>.msg.

You ref'ed the open ros2/rosidl#213 where the team is reluctant to add the support you are seeking. Without core ros2 level support I don't see how rclnodejs would be able to support this proposal (maybe I'm missing something here).

I ran a quick experiment where I created a ros2 package named xxx with a couple of msg files.

xxx/
  msg/
    Num.msg
    subfolder/
      SubNum.msg

After updating my CMakefile to include msg/Num.msg and msg/subfolder/SubNum.msg and building with colcon I ran the following ros2 cmds to see how ros2 deals with msg files in a subdirectory:

> ros2 interfaces packages xxx
xxx/subfolder/SubNum
xxx/msg/SubNum
xxx/msg/Num

Interesting to see xxx/msg/SubNum listed in the output.

Next I output the interfaces individually:

> ros2 interface show xxx/msg/Num
int64 num

> ros2 interface show xxx/subfolder/SubNum
Could not find the interface '/home/wayne/dev/ros/xxx/install/xxx/share/xxx/subfolder/SubNum.idl'

> ros2 interface show xxx/msg/SubNum
Error processing '// generated from rosidl_adapter/resource/msg.idl.em' of 'xxx/SubNum': '//'
... stacktrace ...

Flakey results that lead me to believe there are other issues waiting to be discovered that will result from unconventional msg file organization. In general I'm NOT excited about rclnodejs taking up a feature not generally supported in ros2 and that the ros2cli doesn't support. With that I'll defer to @minggangw for his insights and direction.

@minggangw
Copy link
Member

Thanks for @wayneparrott trying to verify it. The rclnodejs depends on rosidl to generate the typesupport libraries (.so). Even if the .idl files can be found and generate the .js message files when running rclnodejs, the rclnodejs still need to load the .so dynamically during runtime. So the support of rosidl is a must have here.

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

Successfully merging this pull request may close these issues.

3 participants