Adding RPBT config by ct2034 · Pull Request #43 · moveit/moveit_resources

ct2034 · 2020-09-16T09:22:47Z

For moveit/moveit#1893, we need the part to be part of moveit resources.

davetcoleman

It seems this PR would be much simpler, and future code maintenance would be much simpler, if we didn't bring in a custom IKFast plugin. What if instead we just used KDL?

davetcoleman · 2020-09-30T01:47:14Z

prbt_ikfast_manipulator_plugin/.clang-format

+---
+SortIncludes: false
+DisableFormat: true
+...


Shouldn't we just have a .clang-format in the root of this repo, using the same one as https://github.com/ros-planning/moveit/blob/master/.clang-format

Since the ikfast code is autogenerated we did not clang-format it. This allows easier diff with upstream ikfast.h and autogenerated solver(s).

davetcoleman · 2020-09-30T01:49:00Z

prbt_ikfast_manipulator_plugin/include/ikfast.h

@@ -0,0 +1,378 @@
+// -*- coding: utf-8 -*-


do we really need an ikfast plugin for the tests, or can we just use the built-in KDL? I suppose you'll get fairly different results if you switched though?

prbt_ikfast_manipulator_plugin/patches/add-clang-tidy.patch

prbt_moveit_config/launch/joystick_control.launch

jschleicher · 2020-09-30T07:41:02Z

It seems this PR would be much simpler, and future code maintenance would be much simpler, if we didn't bring in a custom IKFast plugin. What if instead we just used KDL?

Of course you could do that. IKfast has the great feature to give reproducible results. I'm not sure, if tests could get flaky with the numerical KDL solver? Would anyone want to try?

@v4hn Regarding the unneeded files inside the moveit_config package: You did add the setup_assistant-autogenerated packages for panda and fanuc as well - what do you think about stripping unneeded files for the tests? I don't mind but would suggest to do it consistently across all moveit_resources robot configs.

v4hn

Regarding the unneeded files inside the moveit_config package: You did add the setup_assistant-autogenerated packages for panda and fanuc as well - what do you think about stripping unneeded files for the tests?

I explicitly kept all files generated by the setup assistant to keep the packages close to our setup assistant templates.
It's true that many of them might not be needed (at the moment?), but I prefer consistent resources over many individual files people edit by hand.

v4hn · 2020-09-30T09:12:33Z

panda_moveit_config/config/fake_controllers.yaml

@@ -1,5 +1,6 @@
 controller_list:
  - name: fake_panda_arm_controller
+    type: $(arg execution_type)


This should be specified for the "fake_hand_controller" below as well.

v4hn · 2020-09-30T09:19:49Z

panda_moveit_config/launch/pilz_industrial_motion_planner_planning_pipeline.launch.xml

@@ -0,0 +1,19 @@
+<launch>


This file is not created by the setup assistant yet and I can't find them in your main PR either. Would it make sense to add it to the setup assistant templates?

My idea was to keep the resources _moveit_config packages as compatible as possible with the setup assistant, so that changes there can be included without custom modifications.

v4hn · 2020-09-30T09:20:23Z

panda_moveit_config/launch/trajectory_execution.launch.xml

-  <include file="$(find moveit_resources_panda_moveit_config)/launch/$(arg moveit_controller_manager)_moveit_controller_manager.launch.xml" />
+  <include file="$(find moveit_resources_panda_moveit_config)/launch/$(arg moveit_controller_manager)_moveit_controller_manager.launch.xml">
+    <arg name="execution_type" value="$(arg execution_type)" />
+  </include>


👍 I like this approach a lot.

v4hn · 2020-09-30T09:23:34Z

prbt_ikfast_manipulator_plugin/CMakeLists.txt

+add_compile_options(-Wextra)
+add_compile_options(-Wno-unused-parameter)
+add_compile_options(-Wno-unused-variable)
+add_compile_options(-Werror)


It's always good to rely on -Werror to force people to fix their warnings, but I would very much prefer to leave it out in the upstream repository.

There's always some warning popping up that's due to some dependency lagging behind in updating their headers.

as suggested by v4h in moveit/moveit_resources#43 (comment)

henningkayser

I think all major requests have been addressed. It's debatable if we want to add possibly redundant launch files, but to me this seems like a general discussion that's out of scope of this PR. @jschleicher the IKFast plugin is preferred (or critical?) for running demo and unit tests, right? If that's the case it should stay in here IMO, there is just no reasonable alternative.

henningkayser · 2020-10-21T15:11:27Z

@jschleicher could you do another rebase? @davetcoleman, @v4hn I think this is ready to get merged, please confirm if you agree.

* prbt_ikfast_manipulaor_plugin * prbt_support * prbt_pg70_support

to speed up test time by skipping trajectory in fake execution

since we only need urdf model for testing in simulation to get rid of dependency to pilz_testutils

inside from moveit_resources instead of schunk_description

not needed in simulation

to allow skipping execution and jump to last point in fake execution

as suggested in the review by v4hn

This reverts commit 1a467cc.

re-generated from MSA with adapted templates see moveit 99c059f

with updated templates, see moveit commit a4527b

for easier include of move_group.launch into tests. This brings the panda_moveit_config closer to the templates in MSA.

ct2034 · 2020-11-02T09:56:06Z

@henningkayser I hope all your points are addressed now.
Can you merge it? @davetcoleman @v4hn

davetcoleman · 2020-11-02T15:29:32Z

Thanks for this hard work!

* adding RPBT config * removing roslaunch check becasue of removed dependencies * adding pilz pipeline config for panda * add remaining prbt-related packages * prbt_ikfast_manipulaor_plugin * prbt_support * prbt_pg70_support * drop no longer needed dependency * add remaining prbt-related packages * prbt_ikfast_manipulaor_plugin * prbt_support * prbt_pg70_support * add argument to enable last point execution to speed up test time by skipping trajectory in fake execution * last point execution for pg70 gripper * drop system_info since we only need urdf model for testing in simulation to get rid of dependency to pilz_testutils * actually use pg70 package inside from moveit_resources instead of schunk_description * drop depend on prbt_hardware_support not needed in simulation * add execution type for panda config to allow skipping execution and jump to last point in fake execution * fixup 4bc2ce drop system_info * fix roslaunch and CMakeLists checks * also adding capabilities here * drop more dependencies * using pg70 support from moveit_resources * add execution_type to gripper * drop Werror as suggested in the review by v4hn * update deprecated macro to INSTANTIATE_TEST_SUITE_P * Revert "update deprecated macro to INSTANTIATE_TEST_SUITE_P" This reverts commit 1a467cc. * update version numbers to be consitant across the meta-package * version increase fo pilz packages * removing patch files * tiny fix * update panda config from templates re-generated from MSA with adapted templates see moveit 99c059f * update fanuc package from MSA with updated templates, see moveit commit a4527b * drop remapping to joint_states_desired for easier include of move_group.launch into tests. This brings the panda_moveit_config closer to the templates in MSA. Co-authored-by: Joachim Schleicher <J.Schleicher@pilz.de>

ct2034 marked this pull request as ready for review September 21, 2020 12:37

rfeistenauer mentioned this pull request Sep 24, 2020

Removes kinetic from travis ci testing #44

Merged

ct2034 mentioned this pull request Sep 28, 2020

Pilz planner merge moveit/moveit#1893

Merged

13 tasks

davetcoleman reviewed Sep 30, 2020

View reviewed changes

v4hn reviewed Sep 30, 2020

View reviewed changes

jschleicher added a commit to PilzDE/moveit that referenced this pull request Oct 6, 2020

drop -Werror

ce393e2

as suggested by v4h in moveit/moveit_resources#43 (comment)

henningkayser approved these changes Oct 15, 2020

View reviewed changes

ct2034 and others added 21 commits October 23, 2020 12:52

adding RPBT config

25a8fde

removing roslaunch check becasue of removed dependencies

b22e6d1

adding pilz pipeline config for panda

c3ba4f1

add remaining prbt-related packages

d3744e5

* prbt_ikfast_manipulaor_plugin * prbt_support * prbt_pg70_support

drop no longer needed dependency

07ccd47

add remaining prbt-related packages

d143eba

* prbt_ikfast_manipulaor_plugin * prbt_support * prbt_pg70_support

add argument to enable last point execution

320eb2d

to speed up test time by skipping trajectory in fake execution

last point execution for pg70 gripper

c8ad76f

drop system_info

40a00ec

since we only need urdf model for testing in simulation to get rid of dependency to pilz_testutils

actually use pg70 package

3804863

inside from moveit_resources instead of schunk_description

drop depend on prbt_hardware_support

c347c91

not needed in simulation

add execution type for panda config

4b68399

to allow skipping execution and jump to last point in fake execution

fixup 4bc2ce drop system_info

e942e4f

fix roslaunch and CMakeLists checks

3fff73b

also adding capabilities here

6259626

drop more dependencies

c63ee5d

using pg70 support from moveit_resources

28c4513

add execution_type to gripper

1397a4c

drop Werror

8eb0d2f

as suggested in the review by v4hn

update deprecated macro to INSTANTIATE_TEST_SUITE_P

3e83b3d

Revert "update deprecated macro to INSTANTIATE_TEST_SUITE_P"

c41a161

This reverts commit 1a467cc.

jschleicher and others added 3 commits October 23, 2020 12:53

update version numbers to be consitant across the meta-package

211b6dd

version increase fo pilz packages

4287333

removing patch files

124807a

ct2034 force-pushed the adding-prbt branch from 28ede2a to 124807a Compare October 23, 2020 10:53

jschleicher added 4 commits October 30, 2020 13:48

tiny fix

268ea19

update panda config from templates

54b2c08

re-generated from MSA with adapted templates see moveit 99c059f

update fanuc package from MSA

0d98506

with updated templates, see moveit commit a4527b

drop remapping to joint_states_desired

e114eb6

for easier include of move_group.launch into tests. This brings the panda_moveit_config closer to the templates in MSA.

davetcoleman merged commit 919a62f into moveit:master Nov 2, 2020

henningkayser mentioned this pull request Dec 1, 2021

Port PRBT packages #101

Merged

Conversation

ct2034 commented Sep 16, 2020

Uh oh!

davetcoleman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jschleicher commented Sep 30, 2020

Uh oh!

v4hn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

henningkayser left a comment

Choose a reason for hiding this comment

Uh oh!

henningkayser commented Oct 21, 2020

Uh oh!

ct2034 commented Nov 2, 2020

Uh oh!

davetcoleman commented Nov 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants