-
Notifications
You must be signed in to change notification settings - Fork 37
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
Add irb2600 185 #53
base: rolling
Are you sure you want to change the base?
Add irb2600 185 #53
Conversation
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.
Thanks a lot for this contribution! Left some minor comments as feedback.
One big request is to simplify the visual meshes for the two new robots. Some of the files are several megabytes which would bloat this repository. Really appreciate the effort to simplify the collision meshes!!
abb_bringup/package.xml
Outdated
@@ -16,6 +16,8 @@ | |||
<test_depend>rclcpp</test_depend> | |||
<test_depend>abb_irb1200_5_90_moveit_config</test_depend> | |||
<test_depend>abb_irb1200_support</test_depend> | |||
<test_depend>abb_irb2600_12_185_moveit_config</test_depend> |
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.
I don't see any changes being made to abb_bringup/test_command_topics.py so we don't have to add these pkgs as dependencies.
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.
fixed in 0b07883
SRDF: | ||
relative_path: config/abb_irb2600_12_185.srdf | ||
CONFIG: | ||
author_name: Andrew Short |
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.
Just to check, are you the author listed here?
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.
Fixed in 02fb59e
os.path.join( | ||
robot_description_path, | ||
"urdf", | ||
"irb2600_12_185.xacro", |
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.
Since you have urdfs for irb2600_12_185
and the irb2600_12_165
variants, it would be good to add a new launch configuration that stores the variant name (default can be irb2600_12_185
) so that the robot to be visualised can be set via launch arg.
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.
fixed in ea62793
robot_description = {"robot_description": robot_description_config.toxml()} | ||
|
||
robot_state_publisher_node = Node( | ||
package="robot_state_publisher", |
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.
Add <exec_depend>robot_state_publisher</exec_depend>
to package.xml
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.
fixed in 0b07883
) | ||
|
||
joint_state_sliders = Node( | ||
package="joint_state_publisher_gui", |
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.
Add <exec_depend>joint_state_publisher_gui</exec_depend>
to package.xml
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.
fixed in 0b07883
<?xml version="1.0" encoding="UTF-8"?> | ||
<robot name="abb_irb2600_12_165" xmlns:xacro="http://ros.org/wiki/xacro"> | ||
<xacro:include filename="$(find abb_irb2600_support)/urdf/irb2600_12_165_macro.xacro"/> | ||
<xacro:abb_irb2600_12_165 prefix=""/> |
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.
Missing ros2_control
tags like you have in irb2600_12_185.xacro
?
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.
Fixed in a66a60a
</hardware> | ||
<joint name="${prefix}joint_1"> | ||
<command_interface name="position"> | ||
<param name="min">{-2*pi}</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.
None of the joint command interface limits here match the limits specified in the urdf. Kindly update these values.
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.
fixed in commit e633967
<parent link="${prefix}link_1"/> | ||
<child link="${prefix}link_2"/> | ||
<!-- See note 2 in package.xml about effort limits and dynamics values --> | ||
<limit lower="-2.705" upper="1.658" effort="0" velocity="3.054"/> |
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.
Same comments regarding joint limits as the irb2600_12_185
file above.
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.
fixed in commit e633967
joint_limits: | ||
joint_1: | ||
has_velocity_limits: true | ||
max_velocity: 5.027 |
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 max_velocity
values in this file do not correspond to the limits set in the urdf. The values here can be lower than max limits specified in the urdf but not higher.
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.
Changed in db0e85f
<buildtool_depend>ament_cmake</buildtool_depend> | ||
|
||
<exec_depend>abb_irb2600_support</exec_depend> | ||
<exec_depend>joint_state_publisher</exec_depend> |
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 dependencies other than abb_irb2600_support
should theoretically be moved into the package.xml
of abb_irb2600_support
as they are required to launch view_robot.launch.py
.
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.
fixed in 0b07883
@Yadunund Thank you for your comments, I think I solved all of them. Can you maybe check again? I also reduce the size of the meshes by half. |
No description provided.