-
Notifications
You must be signed in to change notification settings - Fork 117
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
Port PRBT packages #101
Port PRBT packages #101
Conversation
* 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>
There is no compiled code in the package, so drop all compiler references. The acceptance_test requires real hardware and doesn't make sense for MoveIt. The compiled URDF test is inappropriate as well, because the repository should only provide descriptions/configurations for testing in main moveit. The actual test was already disabled because it relies on moveit datatypes, but the package.xml/find_package entries remained.
I don't know how urgent your pilz port is, but I would strongly suggest to solve the issue you link as part of the migration work. |
fde9080
to
ae5d4d1
Compare
That's exactly what I'm thinking as well. I just wanted to have something running for testing first, because the migration branch was already a bit dusty... I was hoping that we could get Pilz into Galactic before we branch off. |
I don't think we should block the port of Pilz into moveit2 on migrating away from this. I do agree that long term we should migrate all the tests away from dead robots (including the PR2) like this one but that should be separate work. Another thing we should solve is the circular dependency for pre-release testing that having tests depend on this repo creates. That is another thing that has come up in this conversation that I think should be solved separately. I don't see the problem generally of having moveit_test_configs or something similar in moveit itself for the tests. I don't want to block this work over that discussion either. |
With some back and forth discussions I think that we should get the PRBT setup merged anyway, just so that the tests can stay enabled while we merge the planner before the upcoming Galactic branch-off. We still need to get the robot migrated to something else, possibly the Fanuc as suggested here. |
@@ -0,0 +1,5 @@ | |||
cartesian_limits: |
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 confused why there are changes to the fanuc packages in this PR.
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.
Cartesian (max) speed is needed for calculating LINear and CIRCular movements. They are configured as cartesian_limits.yaml
and should be auto-generated by MSA. Not sure, why they were missing in the fanuc 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.
They were missing here since we haven't supported it for MoveIt 2 yet. I think it makes sense to just add the yaml files for now, I just dropped the changes to xml launch files for Panda and Fanuc.
301d1a1
to
5e7f217
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.
@henningkayser if this is your plan for most lines added, no one is even getting close to this one.
What do you mean? The majority of lines is from #43 which is simply cherry-picked for this PR. The huge number comes from the meshes that are being added for the PRBT. |
It was a bad attempt at a joke. I know that most of the lines are just the one generated cpp file. When reviewing this I've just been skipping over reading that because I know where it came from. |
* Fixup prbt_moveit_config demo launch * Fix package.xml dependencies * Remove dependency to moveit_common * Fix deprecated headers * Revert xml launch changes to Panda, Fanuc * Fix double format in yaml files * Filter clang-tidy rules
5e7f217
to
fa39b74
Compare
This is needed for moveit/moveit2#452 to get the tests to pass. I don't think that we should merge this right away, or at least we should consider already deprecating this setup since it will not be supported in the future. See moveit/moveit#2947