Skip to content
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

Open
wants to merge 13 commits into
base: rolling
Choose a base branch
from

Conversation

aleunissen
Copy link

No description provided.

@Yadunund Yadunund self-requested a review November 5, 2023 07:15
Copy link
Collaborator

@Yadunund Yadunund left a 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!!

@@ -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>
Copy link
Collaborator

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.

Copy link
Author

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
Copy link
Collaborator

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?

Copy link
Author

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",
Copy link
Collaborator

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.

Copy link
Author

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",
Copy link
Collaborator

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

Copy link
Author

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",
Copy link
Collaborator

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

Copy link
Author

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=""/>
Copy link
Collaborator

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?

Copy link
Author

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>
Copy link
Collaborator

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.

Copy link
Author

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"/>
Copy link
Collaborator

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.

Copy link
Author

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
Copy link
Collaborator

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.

Copy link
Author

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>
Copy link
Collaborator

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed in 0b07883

@aleunissen
Copy link
Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants