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

Add capability of execute known trajectory with a ROS action #60

Merged
merged 10 commits into from Aug 19, 2016

Conversation

Projects
None yet
4 participants
@wkentaro
Contributor

wkentaro commented Aug 17, 2016

{
std::string response = "Cannot execute trajectory since ~allow_trajectory_execution was set to false";
action_res.error_code.val = moveit_msgs::MoveItErrorCodes::CONTROL_FAILED;
execute_action_server_->setAborted(action_res, response);

This comment has been minimized.

@gavanderhoorn

gavanderhoorn Aug 17, 2016

Member

Is the lack of a return here on purpose?

@gavanderhoorn

gavanderhoorn Aug 17, 2016

Member

Is the lack of a return here on purpose?

This comment has been minimized.

@wkentaro

wkentaro Aug 17, 2016

Contributor

Thanks! I fixed it.

@wkentaro

wkentaro Aug 17, 2016

Contributor

Thanks! I fixed it.

Show outdated Hide outdated ...s/move_group/src/default_capabilities/execute_path_action_capability.cpp
setExecutePathState(IDLE);
}
void MoveGroupExecutePathAction::executePathCallback_Execute(const moveit_msgs::ExecutePathGoalConstPtr& goal, moveit_msgs::ExecutePathResult &action_res)

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

120 char line limit

@davetcoleman

davetcoleman Aug 17, 2016

Member

120 char line limit

Show outdated Hide outdated ...s/move_group/src/default_capabilities/execute_path_action_capability.cpp
{
action_res.error_code.val = moveit_msgs::MoveItErrorCodes::CONTROL_FAILED;
}
ROS_INFO_STREAM("Execution completed: " << es.asString());

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

would prefer _NAMED for the console log, see bottom of http://moveit.ros.org/documentation/contributing/code.html

@davetcoleman

davetcoleman Aug 17, 2016

Member

would prefer _NAMED for the console log, see bottom of http://moveit.ros.org/documentation/contributing/code.html

Show outdated Hide outdated moveit_ros/planning_interface/move_group_interface/src/move_group.cpp
@@ -142,7 +147,7 @@ class MoveGroup::MoveGroupImpl
(node_handle_, move_group::PLACE_ACTION, false));
waitForAction(place_action_client_, wait_for_server, move_group::PLACE_ACTION);
execute_service_ = node_handle_.serviceClient<moveit_msgs::ExecuteKnownTrajectory>(move_group::EXECUTE_SERVICE_NAME);
// execute_service_ = node_handle_.serviceClient<moveit_msgs::ExecuteKnownTrajectory>(move_group::EXECUTE_SERVICE_NAME);

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

so this new ExecutePathAction replaced the old ExecuteKnownTrajectory service, correct? but we need a backwards compatible way of phasing out the old one - this won't work

@davetcoleman

davetcoleman Aug 17, 2016

Member

so this new ExecutePathAction replaced the old ExecuteKnownTrajectory service, correct? but we need a backwards compatible way of phasing out the old one - this won't work

Show outdated Hide outdated moveit_ros/planning_interface/move_group_interface/src/move_group.cpp
req.trajectory = plan.trajectory_;
req.wait_for_execution = wait;
if (execute_service_.call(req, res))
// moveit_msgs::ExecuteKnownTrajectory::Request req;

This comment has been minimized.

@davetcoleman

davetcoleman Aug 17, 2016

Member

this comments should be removed

@davetcoleman

davetcoleman Aug 17, 2016

Member

this comments should be removed

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 17, 2016

Member

Thanks for porting this to the new repo, I'm excited about this new feature

Member

davetcoleman commented Aug 17, 2016

Thanks for porting this to the new repo, I'm excited about this new feature

@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 18, 2016

Contributor

Thanks for the reviews. I fixed them.

Contributor

wkentaro commented Aug 18, 2016

Thanks for the reviews. I fixed them.

Show outdated Hide outdated ...s/move_group/src/default_capabilities/execute_path_action_capability.cpp
void MoveGroupExecutePathAction::executePathCallback_Execute(const moveit_msgs::ExecutePathGoalConstPtr& goal,
moveit_msgs::ExecutePathResult &action_res)
{
ROS_INFO("Execution request received for ExecutePath action.");

This comment has been minimized.

@davetcoleman

davetcoleman Aug 18, 2016

Member

missed this earlier: needs _NAMED("move_group"

@davetcoleman

davetcoleman Aug 18, 2016

Member

missed this earlier: needs _NAMED("move_group"

This comment has been minimized.

@wkentaro

wkentaro Aug 18, 2016

Contributor

Oops. Fixed.

@wkentaro

wkentaro Aug 18, 2016

Contributor

Oops. Fixed.

moveit_msgs::ExecuteKnownTrajectory::Response res;
req.trajectory = plan.trajectory_;
req.wait_for_execution = wait;
if (execute_service_.call(req, res))

This comment has been minimized.

@davetcoleman

davetcoleman Aug 18, 2016

Member

Since we are no longer using execute_service_ within move_group, I think we can remove it from this class (but keep the capability in general for backwards compatibility)

e.g. we don't need execute_service_ as a MoveGroup member variable and we don't need to initialize it

@davetcoleman

davetcoleman Aug 18, 2016

Member

Since we are no longer using execute_service_ within move_group, I think we can remove it from this class (but keep the capability in general for backwards compatibility)

e.g. we don't need execute_service_ as a MoveGroup member variable and we don't need to initialize it

This comment has been minimized.

@wkentaro

wkentaro Aug 18, 2016

Contributor

Fixed.

@wkentaro

wkentaro Aug 18, 2016

Contributor

Fixed.

This comment has been minimized.

@rhaschke

rhaschke Aug 19, 2016

Contributor

-1
See main comment.

@rhaschke

rhaschke Aug 19, 2016

Contributor

-1
See main comment.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 18, 2016

Member

When we merge this, is it ok if we squash-merge to cleanup the multiple commits?

Member

davetcoleman commented Aug 18, 2016

When we merge this, is it ok if we squash-merge to cleanup the multiple commits?

@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 18, 2016

Contributor

When we merge this, is it ok if we squash-merge to cleanup the multiple commits?

Yeah, no problem.

Contributor

wkentaro commented Aug 18, 2016

When we merge this, is it ok if we squash-merge to cleanup the multiple commits?

Yeah, no problem.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 18, 2016

Member

just merged the moveit_msgs change and retriggered this travis build

Member

davetcoleman commented Aug 18, 2016

just merged the moveit_msgs change and retriggered this travis build

@@ -142,7 +147,6 @@ class MoveGroup::MoveGroupImpl
(node_handle_, move_group::PLACE_ACTION, false));
waitForAction(place_action_client_, wait_for_server, move_group::PLACE_ACTION);
execute_service_ = node_handle_.serviceClient<moveit_msgs::ExecuteKnownTrajectory>(move_group::EXECUTE_SERVICE_NAME);

This comment has been minimized.

@davetcoleman

davetcoleman Aug 18, 2016

Member

You can also remove #include <moveit_msgs/ExecuteKnownTrajectory.h>

@davetcoleman

davetcoleman Aug 18, 2016

Member

You can also remove #include <moveit_msgs/ExecuteKnownTrajectory.h>

This comment has been minimized.

@wkentaro

wkentaro Aug 18, 2016

Contributor

Thanks. Fixed.

@wkentaro

wkentaro Aug 18, 2016

Contributor

Thanks. Fixed.

@rhaschke rhaschke self-assigned this Aug 18, 2016

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 18, 2016

Contributor

@davetcoleman Please wait before merging. I want to have a look too.
We definitely need a transition process, deprecating the old execute_service as mentioned before (ros-planning/moveit_ros#719 (comment)).

Contributor

rhaschke commented Aug 18, 2016

@davetcoleman Please wait before merging. I want to have a look too.
We definitely need a transition process, deprecating the old execute_service as mentioned before (ros-planning/moveit_ros#719 (comment)).

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 18, 2016

Member

@rhaschke sounds great. this current PR doesn't break any old functionality AFAIK, but I think you're saying that what is missing is a warning message and inline comments telling users not to use the old excute_service. Then we can remove it completely in ROS-L

Member

davetcoleman commented Aug 18, 2016

@rhaschke sounds great. this current PR doesn't break any old functionality AFAIK, but I think you're saying that what is missing is a warning message and inline comments telling users not to use the old excute_service. Then we can remove it completely in ROS-L

@rhaschke rhaschke merged commit faaf0fc into ros-planning:indigo-devel Aug 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@wkentaro wkentaro deleted the wkentaro:execute-path-action branch Aug 19, 2016

@rhaschke

This comment has been minimized.

Show comment
Hide comment
@rhaschke

rhaschke Aug 19, 2016

Contributor

@wkentaro @davetcoleman: Sorry, I accidentally pressed merged, while I was looking for the commandline instructions. I reverted for now, because I have some more comments.

This current PR doesn't break any old functionality AFAIK

NO! There are lots of moveit_config packages out there, which do not yet configure the new capability.
Hence, they will not be able to execute trajectories anymore.

The MoveGroupInterface should try to connect to the ExecuteAction first, and if that fails, resort to the ExecuteService (printing a deprecation warning).

Furthermore, you should adapt the template of moveit_setup_assistant.

@wkentaro I'm afraid that you need to reopen as new PR. I apologize.

Contributor

rhaschke commented Aug 19, 2016

@wkentaro @davetcoleman: Sorry, I accidentally pressed merged, while I was looking for the commandline instructions. I reverted for now, because I have some more comments.

This current PR doesn't break any old functionality AFAIK

NO! There are lots of moveit_config packages out there, which do not yet configure the new capability.
Hence, they will not be able to execute trajectories anymore.

The MoveGroupInterface should try to connect to the ExecuteAction first, and if that fails, resort to the ExecuteService (printing a deprecation warning).

Furthermore, you should adapt the template of moveit_setup_assistant.

@wkentaro I'm afraid that you need to reopen as new PR. I apologize.

@wkentaro wkentaro restored the wkentaro:execute-path-action branch Aug 19, 2016

@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 19, 2016

Contributor

Reopened here #85

Contributor

wkentaro commented Aug 19, 2016

Reopened here #85

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 19, 2016

Member

@rhaschke good points

Member

davetcoleman commented Aug 19, 2016

@rhaschke good points

@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 20, 2016

Contributor

Fixed in #85

Contributor

wkentaro commented Aug 20, 2016

Fixed in #85

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment