-
Notifications
You must be signed in to change notification settings - Fork 173
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
Integrate with Gazebo #68
Conversation
config/ros_controllers.yaml
Outdated
@@ -0,0 +1,93 @@ | |||
# MoveIt-specific simulation settings |
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.
so as to not get confused with
https://github.com/ros-planning/panda_moveit_config/blob/melodic-devel/config/panda_controllers.yaml
Can we rename this file to something like gazebo_ros_controllers.yaml
?
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.
But this file is the one auto-generated through the Setup Assistant tutorial. If I rename this, Gazebo integration tutorial should be updated and it might be confusing to have ros_controllers.yaml
and gazebo_ros_controllers.yaml
at the end of two tutorials (one after the other).
Moreover, I couldn't find any actual use of panda_controllers.yaml
. Are you sure it is being used?
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.
FWIW, panda_controllers.yaml
was used here.
config/sensors_3d.yaml
Outdated
@@ -0,0 +1,10 @@ | |||
# The name of this file shouldn't be changed, or else the Setup Assistant won't detect it |
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.
how is this file different from
https://github.com/ros-planning/panda_moveit_config/blob/melodic-devel/config/sensors_kinect_pointcloud.yaml
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.
This was auto-generated as well along with sensors_kinect_pointcloud.yaml
and sensors_kinect_depthmap.yaml
🙂
launch/move_group.launch
Outdated
|
||
<!-- Publish the planning scene of the physical robot so that rviz plugin can know actual robot --> | ||
<param name="planning_scene_monitor/publish_planning_scene" value="$(arg publish_monitored_planning_scene)" /> | ||
<param name="planning_scene_monitor/publish_geometry_updates" value="$(arg publish_monitored_planning_scene)" /> | ||
<param name="planning_scene_monitor/publish_state_updates" value="$(arg publish_monitored_planning_scene)" /> | ||
<param name="planning_scene_monitor/publish_transforms_updates" value="$(arg publish_monitored_planning_scene)" /> | ||
|
||
<remap from="/joint_states" to="/joint_states_desired" /> |
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.
you probably want to keep this
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.
If I keep this, MoveIt can't fetch proper joint values. In demo.launch
there is:
<node name="joint_state_desired_publisher" pkg="topic_tools" type="relay" args="joint_states joint_states_desired" />
followed by the inclusion of this launch file. When used in conjunction, these two work. But demo_gazebo.launch
lacks that hence just reverting this won't work.
launch/move_group.launch
Outdated
<arg name="moveit_controller_manager" value="panda" if="$(eval not arg('fake_execution') and not arg('load_gripper'))"/> | ||
<arg name="moveit_controller_manager" value="panda_gripper" if="$(eval not arg('fake_execution') and arg('load_gripper'))"/> |
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'm not sure why this difference has been ever introduced, there is no specified namespacing regarding to panda_gripper
in the base repo in any of the controller configurations.
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.
There is launch/panda_gripper_moveit_controller_manager.launch.xml
as well as src/panda_moveit_config/launch/panda_moveit_controller_manager.launch.xml
.
@davetcoleman PTAL. |
Needs frankaemika/franka_ros#126 to be merged. |
Thanks @tahsinkose for putting the effort into getting Panda available in Gazebo, hope to see this merged soon. I tested both your PRs together and everything works on my end, though the gripper fingers are a bit "soft" so maybe the gains need to be adjusted, see at 0:32 https://youtu.be/JrAq8LK5l1M. Any suggestions? |
Also, |
@simonbogh thank you very much for the video. I didn't actually test grippers for grasping. I'm not an expert on gripper dynamics, so cannot give any suggestions. Try to increase P gains of the fingers to have a stiffer grasp. |
I'm not cleaning up all the auto-generated files, so there may be artifacts that are not used at all. Please ignore them in this PR. |
Alright, as long as it is working 😊 (just a disclaimer, I am not an officiel reviewer, just here as a user). I also changed my local version to use effort controllers instead as I need that. Do you remember what version of moveit setup asssistant the package was generated with? I made a quick test importing it into the current 1.0.7 (Melodic), and besides some manual settings (e.g. PID gains), there seems to be some changes. According to the timestamp in .setup_assistant it was last generated in Feb. 2018, but I don't know if this is accurate. It could maybe make sense to regenerate the package with the latest setup assistant. |
Unfortunately not 😞 |
I didn't look into this in detail yet, just wanted to report some results on Noetic. I'm trying to run With d99e64f (current With this branch and frankaemika/franka_ros#126, I'm getting the following:
The warning |
config/ros_controllers.yaml
Outdated
- panda_joint5 | ||
- panda_joint6 | ||
- panda_joint7 | ||
gains: |
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.
Are these used for a position controller? I thought they only apply to effort/velocity controllers.
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.
@fwalch, I had the same understanding. I think the PID gains are a residual from the original commit of @erdalpekel in which an effort hardware interface was used. @tahsinkose, do you know if this is the case?
In a more recent commit, Erdal also migrated to using a position hardware interface. I, therefore, think the PID gains could be removed.
While doing this, I noticed that I could not remove the PID gains set for the gazebo_ros_control
plugin. In my current understanding, this is because of a known problem with gazebo (see this gazebo answers question for more information).
Alternatively, we could also change the controller interface to effort_controllers/JointTrajectoryController
but then we also need to change the hardware interface in the franka_ros/franka_description/robots/panda.control.macro
(see this [franka_ros](https://github.com/rickstaa/franka_ros/tree/noetic-devel-simulation-effort-hardware-interface and panda_moveit_config branch). I, however, think removing the PID gains and keeping the joint hardware interface, and the joint controller is cleaner.
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.
No they are not. This will be removed with https://github.com/tahsinkose/panda_moveit_config/pull/3
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.
@fwalch tahsinkose#3 was replaced by tahsinkose#4
@fwalch Probably there is a namespace mismatch. I'll try to look into it. |
config/ros_controllers.yaml
Outdated
- panda_joint5 | ||
- panda_joint6 | ||
- panda_joint7 | ||
gains: |
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.
@fwalch, I had the same understanding. I think the PID gains are a residual from the original commit of @erdalpekel in which an effort hardware interface was used. @tahsinkose, do you know if this is the case?
In a more recent commit, Erdal also migrated to using a position hardware interface. I, therefore, think the PID gains could be removed.
While doing this, I noticed that I could not remove the PID gains set for the gazebo_ros_control
plugin. In my current understanding, this is because of a known problem with gazebo (see this gazebo answers question for more information).
Alternatively, we could also change the controller interface to effort_controllers/JointTrajectoryController
but then we also need to change the hardware interface in the franka_ros/franka_description/robots/panda.control.macro
(see this [franka_ros](https://github.com/rickstaa/franka_ros/tree/noetic-devel-simulation-effort-hardware-interface and panda_moveit_config branch). I, however, think removing the PID gains and keeping the joint hardware interface, and the joint controller is cleaner.
config/ros_controllers.yaml
Outdated
- panda_joint5 | ||
- panda_joint6 | ||
- panda_joint7 | ||
gains: |
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.
@fwalch tahsinkose#3 was replaced by tahsinkose#4
This was because |
@tahsinkose Is this pull request ready to be merged? I added https://github.com/tahsinkose/panda_moveit_config/pull/7, which can also be included in this pull request. This pull request gives users the ability to specify the hardware interface that is used. It needs to be merged together with https://github.com/tahsinkose/franka_ros/pull/1. |
This comment has been minimized.
This comment has been minimized.
@tahsinkose Now that https://github.com/frankaemika/franka_ros/releases/tag/0.8.0 has been released #68 might need some changes. In v0.8.0 the robot For ROS noetic, I implemented two versions of the panda_moveit_config:
These pull requests are currently on an orphan branch. I did this so that it is easier to track the changes needed after the 'moveit_setup_assistant' was run. I am however happy to rebase them onto the To give my two cents. I think adding version 2 would make the most sense since it is more closely related to the real robot. The downside is that it is different from the implementation that is created when running the |
Unfortunately, I'm unable to take any action on this branch as I can't allocate time for this PR as of now @rickstaa. It needs to wait a couple of more weeks. Sorry for the inconvenience. |
@tahsinkose No problem, let's wait for some feedback from the @ros-planning team before we proceed with finalizing this feature. |
I created this issue to alert the Franka team about the issues we are having with the |
The following steps need to be performed when we want to use the new franka_gazebo package for the Gazebo simulation (see version 2 above):
|
The gazebo simulation is working on the noetic branch. We can backport it to melodic-devel when rhaschke#8 is merged. |
@rhaschke I think @tahsinkose can safely backport c46c81c into the melodic branch? To my knowledge, franka_ros v0.8.1 works with melodic (see https://github.com/frankaemika/franka_ros-release). |
If it was tested on Melodic, sure. |
Tested and not working. I think c46c81c alone doesn't suffice. |
@tahsinkose did you install franka_ros from source and pull all the upstream changes. You can use my branch to test it out. If so what are the problems you encounter? |
Yes sure. I'm already using your branch. My configuration is as follows:
The issue is |
This commit adds demo_gazebo.launch using the gazebo simulation of the franka_gazebo package Additionally, the virtual_joint is removed from the move group definition.
@rhaschke, @rickstaa we should stop targetting the latest release of I suggest merging frankaemika/franka_ros#181 ASAP and fixating the updated TL;DR: If someone needs more sim2real alignment in panda robot, then relatively more unstable Noetic is the way to go :P Docs todo:
|
@tahsinkose Sounds good to me. |
All this strongly depends on how |
@rhaschke I'm not saying that this repo should not evolve. The latest version -in this case Again, the idea behind this PR was to provide a proof of concept MoveIt-Gazebo integration, which was the subject of https://ros-planning.github.io/moveit_tutorials/doc/gazebo_simulation/gazebo_simulation.html. Yet it inadvertently evolved to full support of all the new changes from My point is focusing dev efforts of this repo on Noetic. So that means stopping Melodic with what we have now in this PR - an integration PoC- with a trace to a sufficiently working version of |
I completely agree with you regarding focussing our efforts on Noetic (or even better ROS2). |
I already suggested to wait for the immediately next minor of |
Ah. Now I understand your reasoning. The problem is: ROS apt repos don't maintain old versions. Hence you cannot lock to a specific version. |
Hmm, that's very sad. Then this will definitely break sometime in the future, it is just a matter of when 🙂 I got your reasoning too 👍🏼 |
Then this will definitely break sometime in the future, it is just a matter of when 🙂 I got your reasoning too 👍🏼
Again, that depends on upstream. noetic is under active development and melodic matured a lot and is considered mostly stable these days.
I fail to understand why they still push breaking changes there. They really shouldn't.
Either way it would definitely be a good idea to add a paragraph somewhere stating the exact commit/version for the franka repository the demo was tested with. Everything after that *might* be supported, but could fail.
|
@tahsinkose Thanks again for back-porting the noetic-devel simulation into your pull request. Can you maybe also apply the changes made in #91 (review) when it is merged. @rhaschke is also working on a panda simulation that doesn't require the |
I'm tempted to close this issue in favour of #89 and rhaschke#9. |
@rhaschke Do you mean back-porting those pull requests into the I think both cases are fine and greatly improve maintainability. It looks like there were no breaking changes regarding the |
I was thinking of updating/overring melodic-devel by merging noetic-devel. As long as franka_ros is the same on Melodic and Noetic, I don't expect any incompatibilities. |
@rhaschke Sound like a good idea. Makes the |
Superseded by new |
Needs a bit more work to have the minimal intrusion.Now it should be ready. The bulk of the change is due to now filled
moveit.rviz
. The version in the base repo is an almost empty -- default Rviz config file, whereas now all the juicy MoveIt stuff is loaded within.