Modified Twist imports to work with Jazzy. Also added Joy Teleop #1134
Conversation
Summary of ChangesHello @Dwilliestyle, 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 significantly enhances the TurtleBot3's control mechanisms by introducing joystick teleoperation capabilities. Concurrently, it updates an existing patrol server script to align with modern ROS 2 practices, specifically by migrating from Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces joystick teleoperation support by adding a new launch file and modifies the patrol server to use TwistStamped messages, likely for compatibility with newer ROS 2 distributions like Jazzy. The changes are a good step forward. My review focuses on improving the new launch file's configurability and correcting the usage of TwistStamped messages. Specifically, the joystick parameters are hardcoded, and I've suggested moving them to a configuration file. More critically, the TwistStamped messages are being published without populating the header timestamp, which is essential for proper operation. I've also identified a pre-existing logic bug in the patrol logic that is on a modified line and provided a fix.
| self.twist.twist.linear.x = 0.0 | ||
| self.twist.twist.angular.z = 0.0 |
There was a problem hiding this comment.
When using TwistStamped messages, it's crucial to populate the header field, especially the timestamp, before publishing. This allows other nodes to know when the command was issued. You should update the timestamp every time you are about to publish the message.
Additionally, it would be good practice to set the frame_id in the __init__ method, for example: self.twist.header.frame_id = 'base_link'.
| self.twist.twist.linear.x = 0.0 | |
| self.twist.twist.angular.z = 0.0 | |
| self.twist.header.stamp = self.get_clock().now().to_msg() | |
| self.twist.twist.linear.x = 0.0 | |
| self.twist.twist.angular.z = 0.0 |
| def go_front(self, position, length): | ||
| while True: | ||
| position += self.twist.linear.x | ||
| position += self.twist.twist.linear.x |
There was a problem hiding this comment.
There appears to be a logic error here. You are incrementing position using self.twist.twist.linear.x from the previous loop iteration (or from before this function was called). This will likely not work as intended, especially on the first iteration where the value might be zero. You probably meant to use self.linear_x, which is the speed you are setting in this loop.
A more robust implementation would use odometry to track the actual distance traveled instead of relying on time.sleep().
| position += self.twist.twist.linear.x | |
| position += self.linear_x |
| self.twist.twist.linear.x = self.linear_x | ||
| self.twist.twist.angular.z = 0.0 |
There was a problem hiding this comment.
The TwistStamped message's header needs to be updated with a new timestamp before each publication. This is crucial for systems that rely on the age of the velocity command.
| self.twist.twist.linear.x = self.linear_x | |
| self.twist.twist.angular.z = 0.0 | |
| self.twist.header.stamp = self.get_clock().now().to_msg() | |
| self.twist.twist.linear.x = self.linear_x | |
| self.twist.twist.angular.z = 0.0 |
| self.twist.twist.linear.x = 0.0 | ||
| self.twist.twist.angular.z = self.angular_z |
There was a problem hiding this comment.
As in other parts of the file, the timestamp in the TwistStamped message header should be updated with the current time before publishing.
| self.twist.twist.linear.x = 0.0 | |
| self.twist.twist.angular.z = self.angular_z | |
| self.twist.header.stamp = self.get_clock().now().to_msg() | |
| self.twist.twist.linear.x = 0.0 | |
| self.twist.twist.angular.z = self.angular_z |
| teleop_node = Node( | ||
| package='teleop_twist_joy', | ||
| executable='teleop_node', | ||
| name='teleop_twist_joy_node', | ||
| parameters=[{ | ||
| 'axis_linear.x': 1, | ||
| 'axis_angular.yaw': 3, | ||
| 'publish_stamped_twist': True, | ||
| 'scale_linear.x': 0.5, | ||
| 'scale_angular.yaw': 0.5, | ||
| }] | ||
| ) |
There was a problem hiding this comment.
For better reusability and to avoid hardcoding, it's recommended to define these parameters in a separate YAML configuration file. This allows users to easily adapt the joystick configuration without modifying the launch file. You could create a joy_teleop.yaml in the param directory and load it here. This would make your launch file more flexible and align with common ROS2 practices.
# Teleop node to convert joy to cmd_vel
teleop_node = Node(
package='teleop_twist_joy',
executable='teleop_node',
name='teleop_twist_joy_node',
parameters=[PathJoinSubstitution([
FindPackageShare('turtlebot3_bringup'),
'param',
'joy_teleop.yaml'
])]
)There was a problem hiding this comment.
I agree with this one. I would put the .yaml file in the config folder to keep it consistent however.
|
Hello @Dwilliestyle! Can you write a simple description?? Could you update package.xml accordingly for joystick_teleop.launch.py ?? |
I made the changes you requested and put the launch file in the teleop package. I put the description up top |
…eleop.launch.py. Modified setup.py for YAML file installation
Signed-off-by: Don <dwilliestyle@gmail.com>
Signed-off-by: Don <dwilliestyle@gmail.com>
2 participants
Fixed TwistStamped compatibility for ROS2 Jazzy
turtlebot3_patrol_server.pyto useTwistStampedinstead ofTwistmsg.twist.twist.linear.xandmsg.twist.twist.angular.zstructureturtlebot3_nodewhich expectsTwistStampedmessages on/cmd_velin JazzyAdded joystick teleoperation support
joystick_teleop.launch.pyinturtlebot3_teleoppackagesetup.pyto install launch files properlyThese changes ensure TurtleBot3 examples work correctly with ROS2 Jazzy.