Skip to content
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 setting of cartesian speed and acceleration #53

Merged
merged 2 commits into from
Apr 4, 2024

Conversation

Zarnack
Copy link
Contributor

@Zarnack Zarnack commented Mar 25, 2024

Description

This PR fixes the issue #45 by changing the max_cartesian_speed setter to set the max_velocity_scaling_factor of the cartesian_path_request.
This PR also adds a max_cartesian_acceleration setter and getter to set the max_acceleration_scaling_factor of the cartesian_path_request.

Testing

Tested with a real and simulated franka panda robot via the move_to_pose command with the cartesian flag set to true.
I used the BiTRRT algorithm from the OMPL and the PTP algorithm from the PILZ trajectory generator.

@amalnanavati
Copy link
Contributor

The pattern used for all other cartesian path parameters is that the property setters/getters set them in self.__move_action_goal.request and then the _plan_cartesian_path function copies them into self.__cartesian_path_request. See here for multiple such examples.

Thus, I'd recommend reverting back to the original property setters/getters, and instead copying those values from self.__move_action_goal.request to self.__cartesian_path_request in _plan_cartesian_path. That way, we continue to maintain the same functionality regardless of whether one is using the MoveGroup action or just calling the planning service in isolation.

@Zarnack
Copy link
Contributor Author

Zarnack commented Mar 26, 2024

I do agree and rebased the commit. But to avoid any further confusion the setter and getter for max_cartesian_speed should be removed and the corresponding value in the MotionPlanRequest be set to a default value (0.0) as it is currently not used within moveit2.

--> According to the message description of MotionPlanRequest.msg the parameter cartesian_speed_limited_link and max_cartesian_speed requires the default_planner_request_adapters/SetMaxCartesianEndEffectorSpeed which only exists for moveit1.

…ing_factor from move_action goal to cartesian_path_request
@amalnanavati
Copy link
Contributor

Removing the setter/getter of those properties, as they are not yet used in MoveIt2, seems reasonable to me.

(Although to be clear, I am just a contributor to this library, not an author/maintainer, so I'd recommend waiting for @AndrejOrsula to weigh in before making that change, since that would be a breaking change.)

@AndrejOrsula
Copy link
Owner

Thank you @Zarnack for this contribution and for explaining why max_cartesian_speed does not currently function. @amalnanavati thanks as well for mentioning the pattern.

But to avoid any further confusion the setter and getter for max_cartesian_speed should be removed and the corresponding value in the MotionPlanRequest be set to a default value (0.0) as it is currently not used within moveit2.

This is a valid point. It can be removed if it is not used. Would you be able to add this change to this PR @Zarnack? Including a comment describing the reason would be appreciated. This change would then be included in the next major release.

as the corresponding value of the MotionPlanRequest.msg
(respectively the self.__move_action_goal.request object) is currently
not used in moveit2.
--> The 'max_cartesian_speed' variable requires the
'default_planner_request_adapters/SetMaxCartesianEndEffectorSpeed'
which currently only exists in moveit1.
@Zarnack
Copy link
Contributor Author

Zarnack commented Apr 3, 2024

Done and tested in simulation.

Copy link
Owner

@AndrejOrsula AndrejOrsula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot!

@AndrejOrsula AndrejOrsula changed the base branch from master to devel April 4, 2024 09:40
@AndrejOrsula AndrejOrsula merged commit fe26066 into AndrejOrsula:devel Apr 4, 2024
2 checks passed
@AndrejOrsula AndrejOrsula mentioned this pull request Apr 25, 2024
AndrejOrsula pushed a commit that referenced this pull request May 10, 2024
* added re-use of max_velocity_scaling_factor and max_acceleration_scaling_factor from move_action goal to cartesian_path_request

* Removed getter and setter of 'max_cartesian_speed' to avoid confusion
as the corresponding value of the MotionPlanRequest.msg
(respectively the self.__move_action_goal.request object) is currently
not used in moveit2.
--> The 'max_cartesian_speed' variable requires the
'default_planner_request_adapters/SetMaxCartesianEndEffectorSpeed'
which currently only exists in moveit1.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants