added panda robot description and moveit_config #19

mayman99 · 2018-07-06T15:23:36Z

Can someone make sure of the licenses?
I copied it from franka_ros package.

One other thing, panda robot package is names franka_description while the Moveit! config is named panda_moveit_config, so I renamed franka_description to panda_description, is that Okay?. Would the opposite be better?, or do we leave it as a franka description and panada config?

v4hn · 2018-07-08T09:06:10Z

We use the robot in the tutorials a lot.
Is the idea here to move from depending on franka_description to moveit_resources?
I would agree. This might make sense to avoid the name clash on the package name panda_moveit_config.

mayman99 · 2018-07-09T12:38:12Z

I don't think moving from depending on franka_description to moveit_resources has not been decided yet, @davetcoleman @mlautman , right?
For now we are to duplicate the package and we are to use it in testing.

mlautman · 2018-07-09T18:15:59Z

@MohmadAyman I am strongly in favor of replacing franka_description with moveit_resources and replacing ros-planning/panda_moveit_config with a ros-planning/moveit_resources/panda_moveit_config

mlautman · 2018-07-09T18:17:22Z

panda_description/urdf/panda.urdf

+    <link name="panda_link0">
+        <visual>
+            <geometry>
+                <mesh filename="package://franka_description/meshes/visual/link0.dae" />


Replace franka_description with the correct package name so that this is pointing to the correct .dae files.

You should be able to remove your system install of franka_description and panda_moveit_config and still be able to launch these:

sudo apt remove ros-kinetic-franka-*
sudo apt remove ros-kinetic-panda-moveit-config

mayman99 · 2018-07-09T18:37:29Z

@mlautman What about renaming of franka_description to panada_description to match the naming of the config package?

mlautman · 2018-07-09T18:40:56Z

That is what I was thinking. just make sure you name it panda_description not panada_description or pnada_description like your branch name 😛

mayman99 · 2018-07-09T18:50:26Z

if it weren't for ctrl+f I wouldn't have found the differences between them 😅

davetcoleman · 2018-07-10T02:21:31Z

I'm not convinced we should change the tutorials to use the moveit_config and *_description packages from the normal ones to moveit_resources for several reasons:

moveit_resources is currently the name of the package, so all Franka dependencies might be confusing to tutorial readers who would see things like this in launch files:

textfile="$(find moveit_resources)/panda_moveit_config/config/panda.srdf"
We would cause a big break from Franka Emika's repos. Readers of our tutorials would have to make a lot of changes to become compatible with the hardware of Panda. The URDF for Panda would also be significantly different, with all meshes pointing to moveit_resources/panda_description/meshes...
We couldn't run the tutorials with debians for the two Franka packages anymore. This is already the case for panda_moveit_config, but will also become the case for franka_description.
We could have the Panda duplicated in both moveit_resources and in Panda's normal repos. This is exactly what was done for the PR2.

The reason for this PR is:

Enable unit tests to use the Panda, especially unit tests around the MoveIt! tutorials. By having a "frozen" copy it makes the tests less brittle
To prevent having to add the panda_moveit_config repo to the moveit.rosinstall. This would require anyone who builds moveit from source to also download Franka repos. This would be excessive brand-collocation IMHO.

The advantages to moving over exclusively to moveit_resources is:

Fewer duplicates
Hide the fact we are using Panda dependencies even further
Centralized location for all Panda files

davetcoleman · 2018-07-10T02:22:34Z

@MohmadAyman you added the panda_moveit_config as a submodule, but I'm pretty sure you did not intend to do that:

Submodule panda_moveit_config added at 320a27

mayman99 · 2018-07-19T09:00:27Z

@davetcoleman
That's my mistake, added the panda_moveit_congif not as a submodule.

mlautman · 2018-07-23T22:00:21Z

panda_description/urdf/panda.urdf

+    <link name="panda_link5">
+        <visual>
+            <geometry>
+                <mesh filename="package://panda_description/meshes/visual/link5.dae" />


Does this work or should it be package://moveit_resources/panda_description/meshes/...?

mlautman · 2018-07-23T22:02:56Z

panda_moveit_config/package.xml

+  <run_depend>xacro</run_depend>
+  <!-- This package is referenced in the warehouse launch files, but does not build out of the box at the moment. Commented the dependency until this works. -->
+  <!-- <run_depend>warehouse_ros_mongo</run_depend> -->
+  <build_depend>franka_description</build_depend>


Can we remove this dependency (or replace it with a reference to the panda_description package) now that panda_description is fully available in this package?

mlautman · 2018-07-23T22:04:40Z

panda_moveit_config/launch/planning_context.launch

+  <arg name="robot_description" default="robot_description"/>
+
+  <!-- Load universal robot description format (URDF) -->
+  <param if="$(arg load_robot_description)" name="$(arg robot_description)" command="$(find xacro)/xacro --inorder '$(find franka_description)/robots/panda_arm_hand.urdf.xacro'"/>


Please fix ALL references to franka_description.

You should uninstall franka_description and panda_moveit_config from your system wide install and re-test all of these launch scripts to ensure that they aren't referencing external packages.

mlautman · 2018-07-23T22:12:24Z

panda_moveit_config/launch/demo_chomp.launch

+  <arg name="use_gui" default="false" />
+
+  <!-- Load the URDF, SRDF and other .yaml configuration files on the param server -->
+  <include file="$(find panda_moveit_config)/launch/planning_context.launch">


Since moveit_resources can be installed from binary, it is unclear to me how the rospack will resolve potential naming collisions between the panda_moveit_config package available from ros-kinetic-panda-moveit-config (franka_ros/panda_moveit_config) and the one available from ros-kinetic-moveit-resources (moveit_resources/panda_moveit_config) please either find a way to test if a system install of this will trip of rospack or please rename this one to something else like panda_moveit and do a find-replace to ensure that all of these files reference the correct package.

I think renaming it to panda_config would be good idea, it will also help avoid the confusion that it is the same ros_planning panda_moveit_config, well right now its is the same but since it is not a sub-module they might diverge from each other in the future.
I have a question, wouldn't adding it as a sub module be better?

My intuition is that we should work towards replacing ros-planning/panda_moveit_config with moveit_resources/panda_config. I don't see a reason why we would want both of them hanging around.

I see.
Is there anything else need to be done before this PR gets merged?

i don't like changing the naming convention of *_moveit_config for our flagship robot example

if we don't have these as actual catkin packages there won't be a conflict with the debian version, or any other git cloned version

mayman99 · 2018-07-24T09:27:03Z

@mlautman
For now all the references point to moveit_resources/panda_*, I removed franka binaries and the launch files worked correctly.

davetcoleman · 2018-07-27T23:24:06Z

CMakeLists.txt

  DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
 install(
  FILES "${CATKIN_DEVEL_PREFIX}/include/${PROJECT_NAME}/.config_install.h"
  DESTINATION ${CATKIN_PACKAGE_INCLUDE_DESTINATION}
  RENAME "config.h")

 # fanuc moveit config
-install(DIRECTORY fanuc_moveit_config DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})
+install(DIRECTORY fanuc_moveit_config panda_config DESTINATION ${CATKIN_PACKAGE_SHARE_DESTINATION})


after discussing with @mlautman further, I think we should keep this folder named panda_moveit_config, sorry for the back and forth

No problem, will do 👍

davetcoleman · 2018-07-27T23:24:38Z

panda_config/README.md

@@ -0,0 +1,5 @@
+# Franka Emika Panda MoveIt! Config Package
+
+The Panda robot is the flagship MoveIt! integration robot used in the MoveIt! tutorials.


i think files in moveit_resources should only be used for tests, not the tutorials

davetcoleman · 2018-07-27T23:25:23Z

panda_config/launch/chomp_planning_pipeline.launch.xml

+
+   <param name="collision_detector" value="Hybrid" />
+
+   <rosparam command="load" file="$(find moveit_resources)/panda_config/config/chomp_planning.yaml" />


panda_moveit_config here and in the other launch files

Okay will do 👍

mlautman

+1 after answering my question about the dependencies

mlautman · 2018-08-03T22:39:09Z

panda_moveit_config/package.xml

+  <run_depend>xacro</run_depend>
+  <!-- This package is referenced in the warehouse launch files, but does not build out of the box at the moment. Commented the dependency until this works. -->
+  <!-- <run_depend>warehouse_ros_mongo</run_depend> -->
+  <build_depend>moveit_resources/panda_description</build_depend>


Is this syntax correct? Do you actually need the sub-directory?

im pretty sure that is incorrect - it would just be moveit_resources

That actually makes more sense, mb.

panda_moveit_config -> https://github.com/ros-planning/panda_moveit_config panda_description: https://github.com/frankaemika/franka_ros/tree/kinetic-devel/franka_description This reverts commit 5c71455.

mlautman suggested changes Jul 9, 2018

View reviewed changes

mayman99 force-pushed the add_pnada_robot branch from 7415890 to 1f80de7 Compare July 9, 2018 20:12

mayman99 force-pushed the add_pnada_robot branch 2 times, most recently from 090475d to 4cf67eb Compare July 19, 2018 08:58

mlautman suggested changes Jul 23, 2018

View reviewed changes

mayman99 force-pushed the add_pnada_robot branch from 4cf67eb to bac23b7 Compare July 24, 2018 09:17

davetcoleman requested changes Jul 27, 2018

View reviewed changes

mayman99 force-pushed the add_pnada_robot branch from bac23b7 to ede562d Compare July 28, 2018 08:54

mlautman approved these changes Aug 3, 2018

View reviewed changes

Added panda to moveit resources

30e5bb1

mayman99 force-pushed the add_pnada_robot branch from ede562d to 30e5bb1 Compare August 6, 2018 20:21

davetcoleman approved these changes Aug 6, 2018

View reviewed changes

davetcoleman merged commit 5c71455 into moveit:master Aug 6, 2018

rhaschke mentioned this pull request Sep 7, 2018

#19 introduced a conflicting set of launch files #20

Closed

davetcoleman mentioned this pull request Nov 13, 2018

Revert "Added panda to moveit resources (#19)" #22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added panda robot description and moveit_config #19

added panda robot description and moveit_config #19

mayman99 commented Jul 6, 2018

v4hn commented Jul 8, 2018

mayman99 commented Jul 9, 2018

mlautman commented Jul 9, 2018

mlautman Jul 9, 2018

mayman99 Jul 9, 2018

mayman99 commented Jul 9, 2018

mlautman commented Jul 9, 2018

mayman99 commented Jul 9, 2018

davetcoleman commented Jul 10, 2018

davetcoleman commented Jul 10, 2018

mayman99 commented Jul 19, 2018

mlautman Jul 23, 2018

mlautman Jul 23, 2018

mlautman Jul 23, 2018

mlautman Jul 23, 2018

mayman99 Jul 24, 2018

mlautman Jul 24, 2018

mayman99 Jul 24, 2018

davetcoleman Jul 27, 2018

mayman99 commented Jul 24, 2018

davetcoleman Jul 27, 2018

mayman99 Jul 28, 2018

davetcoleman Jul 27, 2018

mayman99 Jul 28, 2018

davetcoleman Jul 27, 2018

mayman99 Jul 28, 2018

mlautman left a comment

mlautman Aug 3, 2018

davetcoleman Aug 4, 2018

mayman99 Aug 6, 2018

		@@ -0,0 +1,5 @@
		# Franka Emika Panda MoveIt! Config Package

		The Panda robot is the flagship MoveIt! integration robot used in the MoveIt! tutorials.


		<param name="collision_detector" value="Hybrid" />

		<rosparam command="load" file="$(find moveit_resources)/panda_config/config/chomp_planning.yaml" />

added panda robot description and moveit_config #19

added panda robot description and moveit_config #19

Conversation

mayman99 commented Jul 6, 2018

v4hn commented Jul 8, 2018

mayman99 commented Jul 9, 2018

mlautman commented Jul 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayman99 commented Jul 9, 2018

mlautman commented Jul 9, 2018

mayman99 commented Jul 9, 2018

davetcoleman commented Jul 10, 2018

davetcoleman commented Jul 10, 2018

mayman99 commented Jul 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayman99 commented Jul 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlautman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment