-
Notifications
You must be signed in to change notification settings - Fork 115
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
added panda robot description and moveit_config #19
Conversation
We use the robot in the tutorials a lot. |
I don't think moving from depending on |
@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 |
panda_description/urdf/panda.urdf
Outdated
<link name="panda_link0"> | ||
<visual> | ||
<geometry> | ||
<mesh filename="package://franka_description/meshes/visual/link0.dae" /> |
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.
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
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.
mb 😅
@mlautman What about renaming of |
That is what I was thinking. just make sure you name it |
if it weren't for |
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:
The reason for this PR is:
The advantages to moving over exclusively to moveit_resources is:
|
@mohmadAyman you added the panda_moveit_config as a submodule, but I'm pretty sure you did not intend to do that:
|
090475d
to
4cf67eb
Compare
@davetcoleman |
panda_description/urdf/panda.urdf
Outdated
<link name="panda_link5"> | ||
<visual> | ||
<geometry> | ||
<mesh filename="package://panda_description/meshes/visual/link5.dae" /> |
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.
Does this work or should it be package://moveit_resources/panda_description/meshes/...
?
panda_moveit_config/package.xml
Outdated
<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> |
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.
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?
<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'"/> |
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.
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.
<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"> |
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.
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.
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 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?
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.
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.
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 see.
Is there anything else need to be done before this PR gets merged?
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 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 clone
d version
4cf67eb
to
bac23b7
Compare
@mlautman |
CMakeLists.txt
Outdated
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}) |
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.
after discussing with @mlautman further, I think we should keep this folder named panda_moveit_config
, sorry for the back and forth
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 problem, will do 👍
panda_config/README.md
Outdated
@@ -0,0 +1,5 @@ | |||
# Franka Emika Panda MoveIt! Config Package | |||
|
|||
The Panda robot is the flagship MoveIt! integration robot used in the MoveIt! tutorials. |
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 think files in moveit_resources
should only be used for tests, not the tutorials
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.
Ah Okay.
|
||
<param name="collision_detector" value="Hybrid" /> | ||
|
||
<rosparam command="load" file="$(find moveit_resources)/panda_config/config/chomp_planning.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.
panda_moveit_config here and in the other launch files
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.
Okay will do 👍
bac23b7
to
ede562d
Compare
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.
+1 after answering my question about the dependencies
panda_moveit_config/package.xml
Outdated
<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> |
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.
Is this syntax correct? Do you actually need the sub-directory?
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.
im pretty sure that is incorrect - it would just be moveit_resources
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.
That actually makes more sense, mb.
ede562d
to
30e5bb1
Compare
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.
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 namedpanda_moveit_config
, so I renamedfranka_description
topanda_description
, is that Okay?. Would the opposite be better?, or do we leave it as afranka
description andpanada
config?