-
Notifications
You must be signed in to change notification settings - Fork 0
New pkg for IMU example, include rmw_zenoh for Dockerfile #10
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
base: main
Are you sure you want to change the base?
Conversation
… mount /dev/shm for improved resource sharing
…nal configuration
…gration with dynamixel_hardware_interface
Summary of ChangesHello @Woojin-Crive, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request primarily enhances the existing Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a new package dynamixel_hardware_interface_example_2 for an IMU sensor example and updates the Docker configuration to include rmw_zenoh. The changes are generally good, but there are several issues in the new package that need to be addressed. These include an invalid YAML configuration file, a missing TF frame definition in the URDF, an incorrect default value for a launch argument, and a version mismatch in the setup.py file. I've left specific comments with suggestions to fix these critical and high-severity issues.
| <xacro:dynamixel_system_ros2_control name="dynamixel_system" port_name="$(arg port_name)" baud_rate="$(arg baud_rate)"/> | ||
|
|
||
| <link name="$(arg prefix)base_link"/> | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To resolve the missing imu_link TF frame required by ros2_controllers.yaml, you need to define the link and a joint connecting it to the robot's structure, for example, base_link.
<link name="$(arg prefix)imu_link"/>
<joint name="imu_joint" type="fixed">
<parent link="$(arg prefix)base_link"/>
<child link="$(arg prefix)imu_link"/>
<origin xyz="0 0 0.1" rpy="0 0 0"/>
</joint>
| /**: | ||
| controller_manager: | ||
| ros__parameters: | ||
| update_rate: 500 # Hz | ||
|
|
||
| joint_state_broadcaster: | ||
| type: joint_state_broadcaster/JointStateBroadcaster | ||
|
|
||
| imu_sensor_broadcaster: | ||
| type: imu_sensor_broadcaster/IMUSensorBroadcaster | ||
|
|
||
| /**: | ||
| imu_sensor_broadcaster: | ||
| ros__parameters: | ||
| sensor_name: "imu" | ||
| frame_id: imu_link No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This YAML file is invalid because the /**: key is duplicated. YAML does not allow duplicate keys at the same level, which will cause parsing errors. Please structure the parameters under their respective node and controller names.
controller_manager:
ros__parameters:
update_rate: 500 # Hz
joint_state_broadcaster:
type: joint_state_broadcaster/JointStateBroadcaster
imu_sensor_broadcaster:
type: imu_sensor_broadcaster/IMUSensorBroadcaster
imu_sensor_broadcaster:
ros__parameters:
sensor_name: "imu"
frame_id: imu_link| imu_sensor_broadcaster: | ||
| ros__parameters: | ||
| sensor_name: "imu" | ||
| frame_id: imu_link No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| declared_arguments.append( | ||
| DeclareLaunchArgument( | ||
| 'prefix', | ||
| default_value='""', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| setup( | ||
| name=package_name, | ||
| version='0.0.3', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| <state_interface name="orientation.x"/> | ||
| <state_interface name="orientation.y"/> | ||
| <state_interface name="orientation.z"/> | ||
| <param name="Return Delay Time">0</param> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter name Return Delay Time contains spaces, which is unconventional for ROS parameters. The standard convention is to use snake_case (e.g., return_delay_time). If the dynamixel_hardware_interface plugin supports it, please consider renaming this parameter for consistency and improved maintainability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new example package (dynamixel_hardware_interface_example_2) demonstrating IMU sensor integration with the Dynamixel hardware interface, bumps version numbers across existing example packages to 0.0.4, and adds rmw_zenoh support to the Docker configuration.
- New package
dynamixel_hardware_interface_example_2demonstrating IMU sensor integration with ros2_control - Version bump to 0.0.4 for all example packages
- Docker configuration enhanced with rmw_zenoh support and additional host configurations
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| dynamixel_hardware_interface_example_2/setup.py | Python package setup for new IMU example package |
| dynamixel_hardware_interface_example_2/setup.cfg | Configuration for package installation paths |
| dynamixel_hardware_interface_example_2/package.xml | ROS 2 package manifest with dependencies including imu_sensor_broadcaster |
| dynamixel_hardware_interface_example_2/launch/hardware.launch.py | Launch file configuring ros2_control node with IMU sensor broadcaster |
| dynamixel_hardware_interface_example_2/config/ros2_controllers.yaml | Controller configuration for joint state and IMU sensor broadcasters |
| dynamixel_hardware_interface_example_2/config/dynamixel_system.urdf.xacro | Robot description wrapper with configurable port and baud rate |
| dynamixel_hardware_interface_example_2/config/dynamixel_system.ros2_control.xacro | ros2_control configuration defining IMU sensor interfaces and GPIO mapping |
| dynamixel_hardware_interface_example_2/README.md | Documentation for the IMU example usage |
| dynamixel_hardware_interface_example_2/CHANGELOG.rst | Changelog entry for the new package |
| dynamixel_hardware_interface_example_1/setup.py | Version bump to 0.0.4 |
| dynamixel_hardware_interface_example_1/CHANGELOG.rst | Changelog entry for version 0.0.4 |
| dynamixel_hardware_interface_example/setup.py | Version bump to 0.0.4 |
| dynamixel_hardware_interface_example/CHANGELOG.rst | Changelog entry for version 0.0.4 |
| dynamixel_hardware_interface_demos/package.xml | Added dependency on new example_2 package |
| dynamixel_hardware_interface_demos/CHANGELOG.rst | Changelog documenting new IMU example package |
| docker/docker-compose.yml | Added ipc/pid host mode, shared memory volume, and commented rmw_zenoh environment variable |
| docker/Dockerfile | Added rmw_zenoh repository to build dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| setup( | ||
| name=package_name, | ||
| version='0.0.3', | ||
| version='0.0.4', |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version in setup.py has been updated to 0.0.4, but the corresponding package.xml file still has version 0.0.3. These versions should be synchronized to avoid confusion during package distribution and dependency resolution.
| version='0.0.4', | |
| version='0.0.3', |
| <buildtool_depend>ament_cmake</buildtool_depend> | ||
| <exec_depend>dynamixel_hardware_interface_example</exec_depend> | ||
| <exec_depend>dynamixel_hardware_interface_example_1</exec_depend> | ||
| <exec_depend>dynamixel_hardware_interface_example_2</exec_depend> |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The package.xml version is 0.0.3, but the CHANGELOG indicates this is version 0.0.4. The package version should be updated to 0.0.4 to match the changelog entry and the new functionality being added.
| pid: host | ||
| environment: | ||
| - DISPLAY=${DISPLAY} | ||
| - QT_X11_NO_MITSHM=1 |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commented-out RMW_IMPLEMENTATION environment variable lacks documentation explaining when or why a user should uncomment it. Consider adding a comment above this line explaining the purpose and usage of rmw_zenoh.
| - QT_X11_NO_MITSHM=1 | |
| - QT_X11_NO_MITSHM=1 | |
| # Uncomment the line below to use the Zenoh-based ROS 2 RMW implementation (rmw_zenoh_cpp) | |
| # instead of the default middleware, if rmw_zenoh_cpp is installed in the image. |
| setup( | ||
| name=package_name, | ||
| version='0.0.3', | ||
| version='0.0.4', |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version in setup.py has been updated to 0.0.4, but the corresponding package.xml file still has version 0.0.3. These versions should be synchronized to avoid confusion during package distribution and dependency resolution.
| version='0.0.4', | |
| version='0.0.3', |
|
|
||
| setup( | ||
| name=package_name, | ||
| version='0.0.3', |
Copilot
AI
Dec 17, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The version in setup.py (0.0.3) does not match the version in package.xml (0.0.4). These versions should be synchronized to avoid confusion during package distribution and dependency resolution.
| version='0.0.3', | |
| version='0.0.4', |
…e repository and adjust rosdep commands
…ADME, Python scripts, and launch files for consistency
…ocol compatible IMU sensor
New pkg for IMU example, include rmw_zenoh for Dockerfile