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

Make Panda robot Gazebo compatible #126

Closed
wants to merge 13 commits into from
Closed

Make Panda robot Gazebo compatible #126

wants to merge 13 commits into from

Conversation

tahsinkose
Copy link

@tahsinkose tahsinkose commented Jan 23, 2021

Continues from #52. I've rebased existing work onto melodic-devel. Changes can be listed as:

  • Introduces handy macros for joints&links in arm and handa .xacro files. Although one may find this structure slightly complicated, I believe it makes any future additions trivial-ish. Besides overall file lengths dropped slightly.
  • Brings back collision STLs. Adds xacro arg use_cylinder_collision_model for the .launch-time specification of collision model. It defaults to true. In MoveIt we need meshes. See usage at https://github.com/tahsinkose/panda_moveit_config/blob/c88412d75651e66dcfecae5b877e6d3c910d2c7c/launch/gazebo.launch#L16-L17.
  • Adds another xacro arg use_gazebo_sim. There is one major and one minor reason. Major one is that I did not want the xacro process to waste time on loading irrelevant sections. Minor one is the fact that mimic joints are not properly treated by Gazebo. Hence that field needs to be absent when launching robot for Gazebo.

<xacro:unless value="${not connected_to}">
<joint name="${arm_id}_joint_${connected_to}" type="fixed">
<joint name="virtual_joint" type="fixed">

Choose a reason for hiding this comment

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

I think it would make sense to add ${arm_id}_ to the virtual_joint, else you can get an error that virtual_joint is not unique when using multiple robots in the same environment or importing into moveit setup assistant (MSA). The dual arm example works on its own because of namespace, but if you import dual_panda_example.urdf.xacro into MSA you will get an error.

@tahsinkose
Copy link
Author

@simonbogh, are you authorized to merge PRs in this repo?

@fwalch
Copy link
Contributor

fwalch commented Mar 15, 2021

Hi, sorry for the late response, and thanks for the contribution! I tested Gazebo on Noetic, and everything worked fine. The robot is moving quite slowly however, which is causing ABORTED: Solution found but controller failed during execution warnings.

On Melodic, we ran into an issue with panda_moveit_config seemingly not being compatible with the MoveIt packages from the Ubuntu binary repos (moveit/panda_moveit_config#76). Are you compiling Moveit from source on Melodic?

@tahsinkose
Copy link
Author

The robot is moving quite slowly however, which is causing ABORTED: Solution found but controller failed during execution

It probably needs fine-tuning. The idea is to provide a baseline that could be also adapted by the other robots. After merging this PR, "Panda in Gazebo" becomes a dedicated use-case that people can solve in particular.

Are you compiling Moveit from source on Melodic?

Yes. moveit/panda_moveit_config#68 is the PR that applies the changes suggested in https://ros-planning.github.io/moveit_tutorials/doc/gazebo_simulation/gazebo_simulation.html. You may want to use that PR or manually apply the changes on a freshly generated panda_moveit_config.

@rickstaa
Copy link
Contributor

rickstaa commented Apr 30, 2021

@tahsinkose Thanks a lot for creating this pull request and the accompanying documentation. When it merges this pull request allows me to switch back from my local forks to the upstream franka_ros and panda_moveit_config branches in my projects. However, when testing out your pull request, I noticed that some dependencies are missing in the package.xml of the panda_moveit_config branch. I created a pull request onto your branch. I further also removed several redundant PID gains from the ros_controllers.yaml file. The pull request for this change can be found here.

@tahsinkose
Copy link
Author

@fwalch probably you didn't receive notification on my reply at #126 (comment) since I didn't explicitly mention you. I believe PID tuning in Noetic needs to be separately addressed after enabling native MoveIt support. What do you think?

@tahsinkose
Copy link
Author

The robot is moving quite slowly however,

@fwalch, this was because I adjusted velocity and acceleration scalars to be 0.1 😆 I made them 1.0 again in moveit/panda_moveit_config@c3eed9c#diff-0b89fdfad571d8e4315bc8654941cd7bed8155ed97b9a71756716619333566efR250.

gollth added a commit that referenced this pull request Aug 3, 2021
…azebo

* commit 'ef756fb937df84ea8a0d1df305d01124bf93bd38': (35 commits)
  FIX: Remove TODO
  CHANGE: Use default controller configs too avoid code duplication
  ADD: Default case if joint type is unknown
  ADD: Example Launch file to franka_gazebo
  ADD: TODO regarding NE/EE/K Frames
  FIX: Conditional Build for older Gazebo Versions (kinetic)
  CHANGE: Lookup equilibrium_pose topic relatively
  ADD: Lint & Format franka_gazebo
  FIX: Insert default case for `segment`
  CHANGE: Lookup contact & collision thresholds similar to franka_hw
  FIX: Use `eigen_conversions` to convert KDL::Frame -> Eigen::Affine3d
  FIX: Remove TODOs
  ADD: Gravity Tests for franka_gazebo::ModelKDL
  CHANGE: Move math util functions as static functions in header
  CHANGE: Split initialization of FrankaHWSim into sub-methods
  ADD: Dependencies to Docker
  FIX: Review Comments
  FIX: Remove outdated non-template methods
  FIX: Naming for private member of ModelKDL::chain
  CHANGE: Move util methods inside class namespace
  ...
@gollth
Copy link
Contributor

gollth commented Aug 5, 2021

Hello and thanks for your contribution on this topic. We had a close look at your changes and found them very valuable in simulating the robot with Gazebo.

However due to some internal processes we are not able to merge this directly. Rather we integrated your changes together with some other features in a complete Gazebo integration of franka_ros. Next to the necessary URDF changes this includes also:

  • Gravity Compensation in Gazebo, similar to how the real Panda does it
  • Offering the same FrankaState- & FrankaModelInterface as franka_control does
  • Offering the same action interfaces like franka_gripper does
  • Offering similar service (non-realtime) interfaces similar to how franka_control does it

Hopefully this will allow you to simulate existing ROS controllers now in Gazebo as well.

Check out our documentation for more information.

Since this feature is now implemented I will close this PR now. Thanks for your effort!

@gollth gollth closed this Aug 5, 2021
@rickstaa
Copy link
Contributor

rickstaa commented Aug 6, 2021

@gollth Thanks a lot for this feature improvement! It takes many things on my to-do list that I planned to implement for my research (i.e. gravity composition, better collision meshes, etc.). Can I maybe ask you some questions about your implementation?

Demo.launch warnings

[ WARN] [1628672207.286446541]: Link panda_leftfinger has visual geometry but no collision geometry. Collision geometry will be left empty. Fix your URDF file by explicitly specifying collision geometry.
[ WARN] [1628672207.286477598]: Link panda_rightfinger has visual geometry but no collision geometry. Collision geometry will be left empty. Fix your URDF file by explicitly specifying collision geometry.

@rickstaa
Copy link
Contributor

rickstaa commented Aug 10, 2021

@gollth Can I ask you one small other question? I was wondering why you are publishing the /joint_state_desired topic when controlling the real robot but not for the simulated robot? I think adding '/joint_states_desired' topic both in simulation and the real robot would improve translating between the two. I created pull request #145 to show what I mean.

@gollth
Copy link
Contributor

gollth commented Aug 17, 2021

Thanks you @rickstaa for your interest in this topic.

  • Your suggestion with better finger collision meshes definitely makes sense. Reason was that we started testing with convex collision shapes in the beginning and never changed them since then. We will have a more in depth look to your PR in near future. I think that would also solve your third point, not?
  • Regarding the mesh vs. geometry differences: Geometries plus a safety distance are used for the real hardware since this relates to its internal collision model. However in simulation convex cylinders (plus a safety distance) can easily overlap with other links causing weird effects in Gazebo, which is why we chose to go with simple meshes here. Remember: You can always assemble and specify your own URDF according to what collision model you want right now.
  • Would be solved by first point, not?
  • Answered by first point

Regarding the /joint_state_desired topic, we will have a deeper look as well, thanks in advance already

@rickstaa
Copy link
Contributor

@gollth Thanks a lot for the fast response and the clear explanation.

@rickstaa
Copy link
Contributor

@gollth I added one last pull request that is related to point 2 of my initial post:

Why are geometries + an extra safety boundary used instead of meshes? You appeared to have used meshes in the past (see aa2f01e), but these were replaced by geometries by @MohamadrezaSabaghian in 616055e. My guess is that by doing this, you can provide users a way to dynamically specify the safety boundary? Maybe you can provide a xacro argument to let users decide which approach they want to use.

This pull request #154 gives users the ability to switch between a mesh based robot-description and a geometry based robot_description. This allows users to use mesh based collision properties for an external planner like MoveIt. I use this pull request in my own projects, but I am unsure if it is desired to give users the ability to do this. I will leave that decision up to you.

@rickstaa
Copy link
Contributor

rickstaa commented Aug 19, 2021

@gollth can I ask you one last question about the urdf.

I looked through both the libfranka and franka_ros code to see where you used the collision geometries defined in the urdf. I however saw no use of the urdf::Link::collision method. I only saw usage of other methods of the urdf library.

After this search, I came to the conclusion that the (when working with the real robot) collision properties in the urdf are there to be used by third party planners like MoveIt. You replaced the simple meshes that were used in earlier version by geometries to make sure that movements that are computed by these third party planners are guaranteed to be executable on the real robot. Preferably, you would also like to use these geometries in the gazebo simulation, but due to instabilities in the Gazebo simulation this was not possible. As a result, for the gazebo simulation, you use simple meshes.

If I understood the above correctly, that means that the motions that are allowed are different when working with the simulated one compared to the real one. This means that if I train a RL algorithm in simulation, it might learn motions that are restricted in the real robot. The only way to solve this discrepancy is to create collision geometries, similar to those used in the FCI, that do not cause earlier mentioned instabilities in the gazebo simulation.

@rickstaa
Copy link
Contributor

rickstaa commented Aug 19, 2021

In that case I think merging #137, #139, #145 and #147 would possibly improve user experience.

For #153, it currently only changes the safety_distance of the urdf not the one used in your FCI. Is the safety_distance in the FCI set to 0.0 or also to 0.03?

Additionally, you could merge #154 to give users the ability to switch to simple meshes instead of collisions. This however is up to you, as users can also just make these changes themselves if they really need them.

I am not sure about #150, #151 and #152 which I created to solve warnings thrown by MoveIt when no collision geometry is found in any of the joints that are controlled. I do not know which solution is the best, as I don't know the exact collision geometries that are used in the FCI. Maybe you can the team can take a look if solving these warning is desired.

@rickstaa
Copy link
Contributor

rickstaa commented Aug 23, 2021

@gollth I created #155, #156 and #157 since I ran into some problems when switching between controlling the real and simulated robot using MoveIt.

@rickstaa
Copy link
Contributor

rickstaa commented Aug 30, 2021

@gollth I created feature request #161, #162, #163, #164 to make the issues I explained it easier to track. We can continue our discussion on any of these feature requests.

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.

7 participants