Skip to content

Remove deprecated ExecuteTrajectoryServiceCapability in Melodic#833

Merged
rhaschke merged 1 commit intomoveit:melodic-develfrom
davetcoleman:melodic-execute-service
May 18, 2018
Merged

Remove deprecated ExecuteTrajectoryServiceCapability in Melodic#833
rhaschke merged 1 commit intomoveit:melodic-develfrom
davetcoleman:melodic-execute-service

Conversation

@davetcoleman
Copy link
Copy Markdown
Member

@davetcoleman davetcoleman commented Apr 11, 2018

Description

Final step for #60

I also created a migration document to help users move to Melodic

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
    Checked - none applicable
  • Include a screenshot if changing a GUI
    No GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)
    Do not cherry pick

@davetcoleman davetcoleman force-pushed the melodic-execute-service branch from addabb3 to 6215881 Compare April 11, 2018 20:03
v4hn
v4hn requested changes Apr 19, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@v4hn v4hn left a comment

Choose a reason for hiding this comment

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

Looks pretty good!
I guess there are a number of similar things around, we might want to clean up for melodic.

I added two minor changes and some requests/comments.

.github/PULL_REQUEST_TEMPLATE.md Outdated
- [ ] **Required**: Code is auto formatted using [clang-format](http://moveit.ros.org/documentation/contributing/code)
- [ ] Extended the tutorials / documentation, if necessary [reference](http://moveit.ros.org/documentation/contributing/)
- [ ] Include a screenshot if changing a GUI
- [ ] Document major API changes in the moveit/MIGRATION.md notes
Copy link
Copy Markdown
Contributor

@v4hn v4hn Apr 19, 2018

Choose a reason for hiding this comment

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

👍 👍 👍
should have been a separate commit though...

MIGRATION Outdated
@@ -0,0 +1,7 @@
# Migration Notes
Copy link
Copy Markdown
Contributor

@v4hn v4hn Apr 19, 2018

Choose a reason for hiding this comment

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

shouldn't this be MIGRATION.md?

Copy link
Copy Markdown
Member Author

@davetcoleman davetcoleman May 14, 2018

Choose a reason for hiding this comment

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

thanks for changing this

MIGRATION Outdated
## ROS Melodic

The move_group capability "ExecuteTrajectoryServiceCapability" has been removed in favor of the improved "ExecuteTrajectoryActionCapability" capability.
Since indigo, both capabilities were supported. If you still load default capabilities in your config/launch/move_group.launch, you can just remove them from the capabilities parameter. The correct default capabilities will be loaded automatically.
Copy link
Copy Markdown
Contributor

@v4hn v4hn Apr 19, 2018

Choose a reason for hiding this comment

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

I added some explanation for users.

Copy link
Copy Markdown
Member Author

@davetcoleman davetcoleman May 14, 2018

Choose a reason for hiding this comment

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

thanks

moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp
@@ -240,14 +240,10 @@ class MoveGroupInterface::MoveGroupInterfaceImpl
{
Copy link
Copy Markdown
Contributor

@v4hn v4hn Apr 19, 2018

Choose a reason for hiding this comment

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

please rename this function too.

Copy link
Copy Markdown
Member Author

@davetcoleman davetcoleman May 14, 2018

Choose a reason for hiding this comment

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

done

moveit_ros/planning_interface/move_group_interface/src/move_group_interface.cpp Outdated
ROS_ERROR_STREAM_NAMED("planning_interface", "Unable to find execution action on topic: "
<< node_handle_.getNamespace() + move_group::EXECUTE_ACTION_NAME
<< " - perhaps you are still using the removed (in Melodic) "
"execute service?");
Copy link
Copy Markdown
Contributor

@v4hn v4hn Apr 19, 2018

Choose a reason for hiding this comment

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

I would remove the last part here.
It is meant as support for the user, but the much more likely reason for this warning is simply that the move_group node is not running/died.

Copy link
Copy Markdown
Member Author

@davetcoleman davetcoleman May 14, 2018

Choose a reason for hiding this comment

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

done

MIGRATION Outdated
@@ -0,0 +1,8 @@
# Migration Notes

API changes in MoveIt releases
Copy link
Copy Markdown
Contributor

@v4hn v4hn Apr 19, 2018

Choose a reason for hiding this comment

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

Also this file should really be documented somewhere everyone will look when transitioning their code.
Otherwise it might as well not exist. ⚠️

rhaschke
rhaschke approved these changes Apr 19, 2018
View reviewed changes
MIGRATION Outdated

- The MoveGroupInterface capability "ExecuteTrajectoryServiceCapability" has been removed in favor of the improved "ExecuteTrajectoryActionCapability" capability. No newline at end of file
The move_group capability "ExecuteTrajectoryServiceCapability" has been removed in favor of the improved "ExecuteTrajectoryActionCapability" capability.
Since indigo, both capabilities were supported. If you still load default capabilities in your config/launch/move_group.launch, you can just remove them from the capabilities parameter. The correct default capabilities will be loaded automatically.
Copy link
Copy Markdown
Contributor

@rhaschke rhaschke Apr 19, 2018

Choose a reason for hiding this comment

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

Indigo (captial)?

@v4hn v4hn mentioned this pull request Apr 25, 2018
Closed
@davetcoleman davetcoleman force-pushed the melodic-execute-service branch 2 times, most recently from 1bfd193 to fb4c6cd Compare May 14, 2018 06:06
@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented May 17, 2018

@davetcoleman Can you please rebase?
@v4hn Can you please approve?

@rhaschke rhaschke added the melodic PRs to be merged into melodic branch label May 17, 2018
@davetcoleman davetcoleman force-pushed the melodic-execute-service branch from fb4c6cd to 45d3c24 Compare May 18, 2018 18:01
@rhaschke rhaschke merged commit 45d3c24 into moveit:melodic-devel May 18, 2018
rhaschke added a commit that referenced this pull request May 18, 2018
@rhaschke
Merge pull request #833 from davetcoleman-melodic-execute-service
ba5c419
@davetcoleman davetcoleman deleted the melodic-execute-service branch May 18, 2018 19:43
@rhaschke rhaschke mentioned this pull request Dec 7, 2018
Merged
steven676 added a commit to UCB-ISG/sawyer_moveit that referenced this pull request Oct 31, 2024
@steven676
Update MoveIt move_group capabilities: MoveGroupExecuteService -> Mov…
06f9f23 
…eGroupExecuteTrajectoryAction

MoveGroupExecuteService has been deprecated since kinetic and was removed
in melodic, so this hasn't worked for quite a while.  See:

* moveit/moveit#144
* moveit/moveit#833

Inspired by ros-industrial/universal_robot#413
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@v4hn v4hn v4hn approved these changes

@rhaschke rhaschke rhaschke approved these changes

Assignees

No one assigned

Labels

melodic PRs to be merged into melodic branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@davetcoleman @rhaschke @v4hn