Conversation
Signed-off-by: Hyungyu Kim <kimhg@robotis.com>
Summary of ChangesHello @GyuH13, 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 resolves critical issues in the TurtleBot3's ROS 2 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.
Code Review
This pull request fixes two issues with the Reset Trigger service: a non-functional odometry reset and a misleading service response. The changes introduce a new reset_odometry service and modify the reset logic to call it.
My review has identified a critical issue in turtlebot3_node/src/devices/reset.cpp where blocking calls (sleep_for) are used within a service handler on a single-threaded executor. This will freeze the node and should be replaced with an asynchronous approach, for which I've provided a detailed suggestion.
Additionally, I found that the new odometry reset logic in turtlebot3_node/src/odometry.cpp is incomplete, as it fails to reset all necessary state variables, which could lead to incorrect orientation calculations. I've suggested a fix for this as well.
There was a problem hiding this comment.
Pull request overview
This PR fixes two critical issues with the Reset Trigger service in ROS 2: the missing odometry reset functionality and misleading error messages during gyro calibration. The implementation adds a new reset_odometry service in the Odometry class and integrates it with the existing Reset service to provide complete reset functionality.
Key Changes:
- Implements odometry reset service to restore missing ROS 1 functionality
- Changes Reset service response to indicate trigger invocation success rather than waiting for calibration completion
- Establishes service client/server communication between Reset and Odometry components
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
turtlebot3_node/include/turtlebot3_node/odometry.hpp |
Adds reset service declaration, includes std_srvs header, and declares reset callback method |
turtlebot3_node/src/odometry.cpp |
Creates reset_odometry service and implements callback to reset robot pose and velocity |
turtlebot3_node/include/turtlebot3_node/devices/reset.hpp |
Adds service client member for calling reset_odometry |
turtlebot3_node/src/devices/reset.cpp |
Creates reset_odometry client, changes response handling, and makes async call to reset odometry after gyro calibration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
yun-goon
left a comment
There was a problem hiding this comment.
Please update the version and resolve the lint
Issue
In ROS 1, the Reset Trigger performed both gyro calibration and odometry reset correctly.
However, in ROS 2, there were two issues when using the Reset Trigger: #1126
1. The odometry reset was not executed
2. The service response returned an error message: "There is no status packet"
This PR addresses both issues.
Change
1. Implement odometry reset
The odometry reset logic was missing in the ROS 2 implementation.
This patch adds the missing implementation, allowing the odometry to be reset properly via the Reset Trigger service.
Now, calling the Reset Trigger will correctly reset the odometry.
2. Fix misleading service response message
The "There is no status packet" error occurred because:
Gyro calibration itself takes approximately 5 seconds
The code was waiting for an RX status packet for a shorter duration than the calibration time
As a result, the service incorrectly reported a failure even though the gyro calibration was actually in progress.
To resolve this:
The service response now indicates whether the trigger was successfully invoked, rather than waiting for the gyro calibration to fully complete
This prevents false error messages while maintaining correct behavior