RobotWebTools / rclnodejs

Node.js version of ROS 2.0 client
https://docs.ros.org/en/humble/Concepts/Basic/About-Client-Libraries.html?highlight=rclnodejs#community-maintained
Apache License 2.0
312 stars 70 forks source link

Supports subdirectories in msg folder #821

Open ryought opened 2 years ago

ryought commented 2 years ago

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 https://github.com/RobotWebTools/ros2-web-bridge/issues/175 https://github.com/ros2/rosidl/issues/213

wayneparrott commented 2 years ago

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

ryought commented 2 years ago

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

wayneparrott commented 2 years ago

@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 https://github.com/ros2/rosidl/issues/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 commented 2 years ago

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.