-
Notifications
You must be signed in to change notification settings - Fork 938
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
Remove deprecated ExecuteTrajectoryServiceCapability in Melodic #833
Remove deprecated ExecuteTrajectoryServiceCapability in Melodic #833
Conversation
addabb3
to
6215881
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.
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
@@ -6,6 +6,7 @@ Please explain the changes you made, including a reference to the related issue | |||
- [ ] **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 |
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.
👍 👍 👍
should have been a separate commit though...
MIGRATION
Outdated
@@ -0,0 +1,7 @@ | |||
# Migration Notes |
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 this be MIGRATION.md
?
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.
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. |
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 added some explanation for users.
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.
thanks
@@ -240,14 +240,10 @@ class MoveGroupInterface::MoveGroupInterfaceImpl | |||
{ |
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.
please rename this function too.
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
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?"); |
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 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.
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
MIGRATION
Outdated
@@ -0,0 +1,8 @@ | |||
# Migration Notes | |||
|
|||
API changes in MoveIt releases |
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.
Also this file should really be documented somewhere everyone will look when transitioning their code.
Otherwise it might as well not exist.
MIGRATION
Outdated
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. |
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.
Indigo (captial)?
1bfd193
to
fb4c6cd
Compare
@davetcoleman Can you please rebase? |
- Add MIGRATION.md file
fb4c6cd
to
45d3c24
Compare
Description
Final step for #60
I also created a migration document to help users move to Melodic
Checklist
Checked - none applicable
No GUI
Do not cherry pick