-
Notifications
You must be signed in to change notification settings - Fork 112
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
Adding RPBT config #43
Conversation
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.
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?
--- | ||
SortIncludes: false | ||
DisableFormat: true | ||
... |
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.
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
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 the ikfast code is autogenerated we did not clang-format it. This allows easier diff with upstream ikfast.h and autogenerated solver(s).
@@ -0,0 +1,378 @@ | |||
// -*- coding: utf-8 -*- |
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.
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?
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 |
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.
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.
@@ -1,5 +1,6 @@ | |||
controller_list: | |||
- name: fake_panda_arm_controller | |||
type: $(arg execution_type) |
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 should be specified for the "fake_hand_controller" below as well.
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.
done.
@@ -0,0 +1,19 @@ | |||
<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.
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.
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.
Good idea!
<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> |
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 like this approach a lot.
add_compile_options(-Wextra) | ||
add_compile_options(-Wno-unused-parameter) | ||
add_compile_options(-Wno-unused-variable) | ||
add_compile_options(-Werror) |
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.
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.
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.
ok, done.
as suggested by v4h in moveit/moveit_resources#43 (comment)
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 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.
@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
* 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.
@henningkayser I hope all your points are addressed now. |
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>
For moveit/moveit#1893, we need the part to be part of moveit resources.