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

Kuka sim support package added depends on #12. #15

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

Conversation

Robotgir
Copy link

contains launch file for gazebo classic and gazebo simulators, base models independent of ros2_control support

Copy link
Member

@destogl destogl left a comment

Choose a reason for hiding this comment

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

Except sim_support package we also need ros2_control_support package. So when we are using robot on the HW we don't need to install the simulators. This is dependency management thing.

Also, I have added some small comment to optimize files.

Comment on lines 3 to 4
<xacro:include filename="$(find kuka_resources)/urdf/common_materials.xacro"/>
<xacro:include filename="$(find kuka_resources)/urdf/common_properties.xacro"/>
Copy link
Member

Choose a reason for hiding this comment

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

Those two files are also not needed here. They should be in the simulation xacro files

Copy link
Author

@Robotgir Robotgir Mar 2, 2023

Choose a reason for hiding this comment

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

These were already here in some of the other base files, here only common_properties.xacro should have been added extra for getting gazebo to work, I can add it to simulation .xacro files this part as i added it for gazebo specific purpose. But the traces of the default_inertial tags will be there any way in the base files. If someone wants to use these base files alone without gazebo or ros2_control support, then they will get error as common_properties.xacro is not included in the base_macro that is using them instead in the simulation xacro.

I am not sure if every other simulator out there needs inertial tags. some of the base files had inertial tags defined, i just added default_inertial wherever it was needed. I created this PR #13 ,assuming default_inertial are needed in the base files.

i thin it should be included in the base_macro.xacro files instead of simulation .xacro files what do you think? @destogl

kuka_kr6_support/urdf/kr6r700sixx_macro.xacro Outdated Show resolved Hide resolved
@@ -12,7 +12,7 @@
<visual>
<origin xyz="0 0 0" rpy="${-radians(90)} 0 0"/>
<geometry>
<mesh filename="package://kuka_kr210_support/meshes/kr210l150/visual/base_link.dae"/>
<mesh filename="file://$(find kuka_kr210_support)/meshes/kr210l150/visual/base_link.dae"/>
Copy link
Member

Choose a reason for hiding this comment

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

Why is here no default_intertial used?

Copy link
Author

Choose a reason for hiding this comment

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

because it already had inertial tags defined

@@ -3,114 +3,122 @@
<xacro:macro name="kuka_lbr_iiwa_14_r820" params="prefix">
<!-- link list -->
<link name="${prefix}base_link">
<xacro:default_inertial/>
Copy link
Member

Choose a reason for hiding this comment

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

We should include file for default intertials here where we are also using it. Otherwise we have always add this in the file that is including this.

from launch_ros.actions import Node
from launch_ros.substitutions import FindPackageShare

def generate_launch_description():
Copy link
Member

Choose a reason for hiding this comment

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

apply the same comments as for gazebo classic file


<buildtool_depend>ament_cmake</buildtool_depend>

<test_depend>roslaunch</test_depend>
Copy link
Member

Choose a reason for hiding this comment

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

This should not be dependency anymore in ROS2

kuka_ros2_control_sim_support/package.xml Outdated Show resolved Hide resolved
@@ -1,14 +1,31 @@
<?xml version="1.0"?>
<robot xmlns:xacro="http://www.ros.org/wiki/xacro">

<xacro:macro name="kuka_common_ros2_control_macro" params="name prefix use_mock_hardware:=^|false">
<xacro:macro name="kuka_common_sim_ros2_control_macro" params="
Copy link
Member

Choose a reason for hiding this comment

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

I would put this into *_ros2_control_* package and sim package can use that one.

Copy link
Author

@Robotgir Robotgir Mar 2, 2023

Choose a reason for hiding this comment

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

i put both files separately as we dont want traces of the gazebo in the ros2_control only support package. they have their default values set in launch files, but still, for someone looking at ros2_control support package seeing gazebo related flags might be distracting, i thought @destogl

@Robotgir
Copy link
Author

Robotgir commented Mar 2, 2023

Except sim_support package we also need ros2_control_support package. So when we are using robot on the HW we don't need to install the simulators. This is dependency management thing.

Also, I have added some small comment to optimize files.

Except sim_support package we also need ros2_control_support package. So when we are using robot on the HW we don't need to install the simulators. This is dependency management thing.

Also, I have added some small comment to optimize files.

@destogl Below is the PR for ros2_control part alone
#12

@Robotgir Robotgir changed the title Kuka ros2 control sim support package added Kuka ros2 control and sim support package both added Mar 6, 2023
@@ -183,4 +191,4 @@
<origin xyz="0 0 0" rpy="0 ${radians(90)} 0"/>
</joint>
</xacro:macro>
</robot>
</robot>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
</robot>
</robot>

@@ -284,4 +284,4 @@
</joint>

</xacro:macro>
</robot>
</robot>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
</robot>
</robot>

kuka_ros2_control_sim_support/package.xml Outdated Show resolved Hide resolved
kuka_ros2_control_sim_support/package.xml Outdated Show resolved Hide resolved
kuka_ros2_control_support/CMakeLists.txt Show resolved Hide resolved
kuka_ros2_control_support/package.xml Outdated Show resolved Hide resolved
@destogl
Copy link
Member

destogl commented Mar 6, 2023

Please also add manual into README.md on how this repository can be used, dependencies installed and robot, mock hardware and simulation can be started.

@Robotgir Robotgir changed the title Kuka ros2 control and sim support package both added Kuka ros2 control and sim support package added Mar 11, 2023
@Robotgir Robotgir changed the title Kuka ros2 control and sim support package added Kuka ros2 control and sim support package added depends on #12 Mar 11, 2023
@Robotgir Robotgir changed the title Kuka ros2 control and sim support package added depends on #12 Kuka ros2 control and sim support package added depends on #12. Mar 11, 2023
@Robotgir Robotgir changed the title Kuka ros2 control and sim support package added depends on #12. Kuka sim support package added depends on #12. Mar 17, 2023
@destogl
Copy link
Member

destogl commented Mar 27, 2023

@muritane you had script to test this PR. Can we upload this for now?

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