Fixed the wrong type for time parameterization in ompl #55

jodle001 · 2023-11-10T19:37:30Z

#54 related to this issue I created. Here is the change.

Yadunund

Thanks for the PR!

Another big change to the configs in moveit 2.7+ is how kinematics is loaded. Now the kinematics yaml needs to be namespaced under robot_description_kinematics so loading the yaml like we do now will not work.

One easy way to resolve this is to rely on moveit_config_utils to load all params under correct namespaces without having to update the kinematics.yaml file.

Do you mind also updating the abb_moveit.launch.py file to rely on moveit_config_utils instead?

In the meanwhile I will create + push a humble branch so that users on Humble have a compatible version of this repo to work with.

jodle001 · 2023-11-14T17:58:47Z

Okay I have pushed the changes you requested @Yadunund. Please review these changes to abb_moveit.launch.py to see if this is what you were asking for. Thank you for the feedback.

jodle001 · 2023-11-14T18:03:57Z

I have also noticed that a lot of the same files are referenced in the abb_control.launch.py file. What I have done before is had a separate file called moveit_utils.py or something like that, which has an object that builds the MoveItConfigsBuilder, then both files (abb_control and abb_moveit) can just call that utils file object. This would reduce redundant code. But this was not requested so I didn't do that for this commit.

Yadunund

Hi @jodle001

Thanks for making the requested changes and sorry for the delay in following up (I was away for a bit). Left a couple of minor comments to address before we can merge.
In addition, please also add an <exec_depend>moveit_configs_utils</exec_depend> tag to the package.xml for the newly introduced dependency.

abb_bringup/launch/abb_moveit.launch.py

Removed hard coded config package Co-authored-by: Yadu <[email protected]>

added argument names for clarity. Co-authored-by: Yadu <[email protected]>

Added moveit_configs_utils dependency to package.xml

jodle001 · 2024-01-02T18:13:30Z

I have made the changes requested, let me know if there is anything else that you would like to have changed.

jodle001 · 2024-01-02T19:59:54Z

Added one more change to publish robot description and robot semantic.

Yadunund · 2024-01-03T06:53:51Z

Thanks for addressing the additional feedback so quickly. Do you mind fixing the formatting of the code so that Format CI job passes? I think the Build and Test CI failing is due to bug upstream that was patched but a Rolling Sync has not happened in a while so the binaries are still outdated -> we can ignore this CI failure for now.

jodle001 · 2024-01-03T15:28:21Z

Okay I believe the formatting changes have been made to pass your test.

Yadunund · 2024-01-03T16:55:19Z

Format CI is still failing.
Maybe you could try setting up the pre-commit check? Once installed, it should format the code before your commit.

jodle001 · 2024-01-03T17:07:47Z

Format CI is still failing. Maybe you could try setting up the pre-commit check? Once installed, it should format the code before your commit.

Sorry about that, I set that up and used the local command to fix the formatting. Thank you for your help.

Yadunund · 2024-01-03T17:15:12Z

abb_bringup/launch/abb_moveit.launch.py

+                f"{moveit_config_file.perform(context)}",
+            )
+        )
+        .planning_pipelines(


Sorry one last thing came to mind. I don't think we should restrict the planning_pipelines to just ompl. If we remove this entry, the builder will load default configs for ompl, stomp, chompand pilz which would be nicer for users to play around with in the rviz palnning panel.

Just a quick question, I removed the lines here:

But I was getting this error without adding a pilz_cartesian_limits.yaml file to the moveit_config package:

[ERROR] [launch]: Caught exception in launch (see debug for traceback): "File /opt/robotic_platform/install/abb_irb1200_5_90_moveit_config/share/abb_irb1200_5_90_moveit_config/config/pilz_cartesian_limits.yaml doesn't exist"

So I added this to the abb_irb1200_5_90_moveit_config package in the config directory:

cartesian_limits: max_trans_vel: 1.0 max_trans_acc: 2.25 max_trans_dec: -5.0 max_rot_vel: 1.57

But even with this, If you select the pilz planner it doesn't seem to successfully plan a path. The other planners I see available are ompl, and CHOMP and they can both successfully plan a path.

Not sure how you would like me to proceed.

I think the problem is because the pilz_indsutrial_motion_planner is loaded based on the deafult config file in moveit_config_utils but there isn't a default pilz_cartesian_limits.yaml file. Hence you had to manually include on in the config folder here. I think if you use moveit_setup_assistant, it does generate this file in the local config.
Perhaps you can open an issue ticket in upstream moveit to include a default pilz_cartesian_limits.yaml file in moveit_config_utils?

But even with this, If you select the pilz planner it doesn't seem to successfully plan a path. The other planners I see available are ompl, and CHOMP and they can both successfully plan a path.

I think this is because we have not set any acceleration limits in the joint_limits.yaml file which are needed by the planner. You should be seeing a message printout indicating this

[move_group-1] [INFO] [1704356296.402945412] [moveit_move_group_capabilities_base.move_group_capability]: Using planning pipeline 'pilz_industrial_motion_planner' [move_group-1] [ERROR] [1704356296.402995752] [moveit.ros_planning.planning_pipeline]: Exception caught: 'acceleration limit not set for group manipulator'

If you edit the joint_limits.yaml file by setting has_acceleration_limts: true for each joint a specify some max_acceleration like 5.0, you should be apply to plan PTP motions with pilz_industrial_motion_planner.

I think it's best to leave the acceleration limits as false for now until someone can share accurate values for this specific robot.

…tesian_limits yaml.

Yadunund

Thanks for iterating on this PR!

Yadunund · 2024-01-13T03:52:32Z

abb_bringup/launch/abb_moveit.launch.py

        "moveit_controller_manager": "moveit_simple_controller_manager/MoveItSimpleControllerManager",
    }

-    trajectory_execution = {
-        # MoveIt does not handle controller switching automatically


This behavior was lost in the changes as by default, moveit_manage_controllers is set to True by the builder. I'll open a PR to update.

* Fixed the wrong type for time parameterization in ompl * Updated to use the MoveItConfigsBuilder * Optimized the imports. * Update abb_bringup/launch/abb_moveit.launch.py Removed hard coded config package Co-authored-by: Yadu <[email protected]> * Update abb_bringup/launch/abb_moveit.launch.py added argument names for clarity. Co-authored-by: Yadu <[email protected]> * Update package.xml Added moveit_configs_utils dependency to package.xml * Formatting adn also publishing robot_description and semantic * Fixed formatting to pass format test per requeset by Yadunund. * Used pre-commit to fix formatting locally. * Removed planning pipeline limits to ompl planner, also added pilz_cartesian_limits yaml. --------- Co-authored-by: Yadu <[email protected]>

* Fixed the wrong type for time parameterization in ompl * Updated to use the MoveItConfigsBuilder * Optimized the imports. * Update abb_bringup/launch/abb_moveit.launch.py Removed hard coded config package * Update abb_bringup/launch/abb_moveit.launch.py added argument names for clarity. * Update package.xml Added moveit_configs_utils dependency to package.xml * Formatting adn also publishing robot_description and semantic * Fixed formatting to pass format test per requeset by Yadunund. * Used pre-commit to fix formatting locally. * Removed planning pipeline limits to ompl planner, also added pilz_cartesian_limits yaml. --------- Co-authored-by: Jacob Odle <[email protected]>

Fixed the wrong type for time parameterization in ompl

2e23a07

Yadunund requested changes Nov 14, 2023

View reviewed changes

jodle001 added 2 commits November 14, 2023 12:51

Updated to use the MoveItConfigsBuilder

66196fa

Optimized the imports.

014959d

jodle001 requested a review from Yadunund November 15, 2023 15:15

Yadunund requested changes Jan 2, 2024

View reviewed changes

abb_bringup/launch/abb_moveit.launch.py Outdated Show resolved Hide resolved

abb_bringup/launch/abb_moveit.launch.py Outdated Show resolved Hide resolved

abb_bringup/launch/abb_moveit.launch.py Outdated Show resolved Hide resolved

jodle001 and others added 3 commits January 2, 2024 13:09

Update abb_bringup/launch/abb_moveit.launch.py

e5fe1b2

Removed hard coded config package Co-authored-by: Yadu <[email protected]>

Update abb_bringup/launch/abb_moveit.launch.py

396f044

added argument names for clarity. Co-authored-by: Yadu <[email protected]>

Update package.xml

7eb9379

Added moveit_configs_utils dependency to package.xml

jodle001 requested a review from Yadunund January 2, 2024 18:13

Formatting adn also publishing robot_description and semantic

eadbc1d

Fixed formatting to pass format test per requeset by Yadunund.

e447429

Used pre-commit to fix formatting locally.

6f303b5

Yadunund reviewed Jan 3, 2024

View reviewed changes

Removed planning pipeline limits to ompl planner, also added pilz_car…

cee3f4a

…tesian_limits yaml.

jodle001 requested a review from Yadunund January 3, 2024 18:09

Yadunund approved these changes Jan 4, 2024

View reviewed changes

Yadunund merged commit fd9a44c into PickNikRobotics:rolling Jan 4, 2024
2 of 3 checks passed

Yadunund reviewed Jan 13, 2024

View reviewed changes

Yadunund mentioned this pull request Jan 13, 2024

Cleanup moveit_config #57

Merged

Yadunund mentioned this pull request Mar 17, 2024

Moveit robot does not appear #63

Closed

Yadunund mentioned this pull request Apr 13, 2024

Controller failed during execution #64

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed the wrong type for time parameterization in ompl #55

Fixed the wrong type for time parameterization in ompl #55

jodle001 commented Nov 10, 2023

Yadunund left a comment •

edited

Loading

jodle001 commented Nov 14, 2023 •

edited

Loading

jodle001 commented Nov 14, 2023

Yadunund left a comment

jodle001 commented Jan 2, 2024

jodle001 commented Jan 2, 2024

Yadunund commented Jan 3, 2024

jodle001 commented Jan 3, 2024

Yadunund commented Jan 3, 2024

jodle001 commented Jan 3, 2024

Yadunund Jan 3, 2024

jodle001 Jan 3, 2024

Yadunund Jan 4, 2024 •

edited

Loading

Yadunund left a comment

Yadunund Jan 13, 2024

Fixed the wrong type for time parameterization in ompl #55

Fixed the wrong type for time parameterization in ompl #55

Conversation

jodle001 commented Nov 10, 2023

Yadunund left a comment • edited Loading

Choose a reason for hiding this comment

jodle001 commented Nov 14, 2023 • edited Loading

jodle001 commented Nov 14, 2023

Yadunund left a comment

Choose a reason for hiding this comment

jodle001 commented Jan 2, 2024

jodle001 commented Jan 2, 2024

Yadunund commented Jan 3, 2024

jodle001 commented Jan 3, 2024

Yadunund commented Jan 3, 2024

jodle001 commented Jan 3, 2024

Yadunund Jan 3, 2024

Choose a reason for hiding this comment

jodle001 Jan 3, 2024

Choose a reason for hiding this comment

Yadunund Jan 4, 2024 • edited Loading

Choose a reason for hiding this comment

Yadunund left a comment

Choose a reason for hiding this comment

Yadunund Jan 13, 2024

Choose a reason for hiding this comment

Yadunund left a comment •

edited

Loading

jodle001 commented Nov 14, 2023 •

edited

Loading

Yadunund Jan 4, 2024 •

edited

Loading