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

Adding Gazebo for rrbot #64

Closed

Conversation

olivier-stasse
Copy link
Collaborator

Objective

Following the nice work of @ahcorde on gazebo_ros2_control adding Gazebo for rrbot.

This pull requests modifies the rrbot_description to call the gazebo_ros2_control and the GazeboSystemplugins.
Given the interfaces of rrbot this is directly compatible.
The rrbot_system_description_position_only.urdf.xacro has been modified to take simu has parameter.
The default is false and switch quietly to the usual behavior.
If set to true it loads the gazebo system from gazebo_ros2_control.

The rrbot_gazebo.xacro and rrbot_system_position_only.ros2_control.xacro are modified accordingly.

What is left ?

  • Major: Testing by sending commands to the system. Should we try to use the same forward_command_trajectory or use the same example than gazebo_ros2_control ? The latter might lead to an unnecessary duplication of code between the two repositories.

  • Minor: Modifying the README to launch the test:
    ros2 launch ros2_control_demo_robot rrbot_system_position_only_gazebo.launch.py

@codecov-io
Copy link

codecov-io commented Feb 6, 2021

Codecov Report

Merging #64 (68bdb89) into master (5d45eee) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #64    +/-   ##
=======================================
  Coverage    0.00%   0.00%            
=======================================
  Files          15       1    -14     
  Lines        1065      71   -994     
=======================================
+ Misses       1065      71   -994     
Flag Coverage Δ
unittests 0.00% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
...l_demo_hardware/src/rrbot_system_position_only.cpp
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5d45eee...68bdb89. Read the comment docs.

Comment on lines +16 to +18
<param name="example_param_hw_start_duration_sec">2.0</param>
<param name="example_param_hw_stop_duration_sec">3.0</param>
<param name="example_param_hw_slowdown">2.0</param>
Copy link

Choose a reason for hiding this comment

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

This is not parsed in the plugin

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe I misunderstood your comment, but this part is not included in the file generated by xacro because it is in the unless statement.
Should it be included ?

@@ -0,0 +1,60 @@
# Copyright 2020 Open Source Robotics Foundation, Inc.
Copy link

Choose a reason for hiding this comment

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

Suggested change
# Copyright 2020 Open Source Robotics Foundation, Inc.
# Copyright 2021 Open Source Robotics Foundation, Inc.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done through commit 68bdb89

@@ -14,6 +14,7 @@
<depend>hardware_interface</depend>
<depend>pluginlib</depend>
<depend>rclcpp</depend>
<depend>gazebo_ros2_control</depend>
Copy link
Contributor

Choose a reason for hiding this comment

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

you're missing an <exec_depend>xacro</exec_depend> here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Fixed in e6497f1


spawn_entity = Node(package='gazebo_ros', executable='spawn_entity.py',
arguments=['-topic', 'robot_description',
'-entity', 'cartpole'],
Copy link
Contributor

Choose a reason for hiding this comment

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

what's that cartpole doing here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Fixed in e230567

@@ -5,7 +5,8 @@ Copied and modified from ROS1 example:
https://github.com/ros-simulation/gazebo_ros_demos/blob/kinetic-devel/rrbot_description/urdf/rrbot.xacro
-->
<robot xmlns:xacro="http://www.ros.org/wiki/xacro" name="2dof_robot">

<xacro:arg name="simu" default="false" />
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, but simu sounds kinda weird. I'd propose either just sim or something like use_sim.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks. Fixed in 5e726d4

joints:
- joint1
- joint2
interface_name: position
Copy link
Contributor

Choose a reason for hiding this comment

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

@destogl can you explain what that interface name does here? I'd assume that the joints are sufficiently described in the ros2_control tag with their control and state interfaces names.
What if I use position here and declare the joints as effort?

@olivier-stasse
Copy link
Collaborator Author

I forget to mention one behavior that I found strange (or at least that I do not have with the original example from @ahcorde): gazebo takes a long time to start.

Any feedback on this part is welcome.

@Karsten1987
Copy link
Contributor

Karsten1987 commented Feb 12, 2021

I've noticed the same behavior, that it takes a long time to start up.
My guess is however not that it's Gazebo's fault, but I suspect it to be xacro. I feel that instead of loading a xacro file, we should maybe try to load a URDF directly. Can you try this?

https://github.com/ros-simulation/gazebo_ros2_control/blob/master/gazebo_ros2_control_demos/launch/cart_example_position.launch.py#L40

Nevermind, I actually tried it and the xacro substitution is really fast. Must be somewhere else. For what it's worth, I have the same delay on Linux as well as OSX.

@Karsten1987 Karsten1987 mentioned this pull request Apr 28, 2021
@olivier-stasse
Copy link
Collaborator Author

Superseded by #85

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.

4 participants