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

Reopen #85: Add capability to execute trajectory with a ROS action #94

Merged
merged 4 commits into from Aug 23, 2016

Conversation

Projects
None yet
4 participants
@rhaschke
Contributor

rhaschke commented Aug 20, 2016

This reopens #85/#60 against kinetic. As proposed in ros-planning/moveit_msgs#27 (comment), I suggest to drop that new feature for Indigo, because we require user intervention to switch moveit_config's launch files to the new capability.

Additional to @wkentaro great work (from #85), I cleaned up the waiting for required action servers and services to allow to issue the deprecation warning once on startup.

cleanup instantiation of MoveGroupInterface
- unified names of execute_trajectory service and action capabilities
- consider overall (wall) timeout for waiting for action servers
- output deprecation warning once during instantiation
@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 22, 2016

Contributor

@rhaschke Thanks! The code looks good to me.

Contributor

wkentaro commented Aug 22, 2016

@rhaschke Thanks! The code looks good to me.

Show outdated Hide outdated ...ce/move_group_interface/include/moveit/move_group_interface/move_group.h
const ros::Duration &wait_for_server = ros::Duration(0, 0));
const ros::WallDuration &wait_for_servers = ros::WallDuration());
ROS_DEPRECATED MoveGroup(const Options &opt, const boost::shared_ptr<tf::Transformer> &tf = boost::shared_ptr<tf::Transformer>(),
const ros::Duration &wait_for_servers = ros::Duration());

This comment has been minimized.

*/
MoveGroup(const Options &opt, const boost::shared_ptr<tf::Transformer> &tf = boost::shared_ptr<tf::Transformer>(),
const ros::Duration &wait_for_server = ros::Duration(0, 0));
const ros::WallDuration &wait_for_servers = ros::WallDuration());

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

i assume you are switching to Wall for better rosbag and simulation support?

@davetcoleman

davetcoleman Aug 22, 2016

Member

i assume you are switching to Wall for better rosbag and simulation support?

This comment has been minimized.

@rhaschke

rhaschke Aug 22, 2016

Contributor

It's the other way around, isn't it? Here we want to use real time, not simulated time. Hence we should use WallTime.

@rhaschke

rhaschke Aug 22, 2016

Contributor

It's the other way around, isn't it? Here we want to use real time, not simulated time. Hence we should use WallTime.

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

For cases where you want access to the actual wall-clock time even if running inside simulation, roslib provides Wall versions of all its time constructs

Yep, I was confused. Then I have the converse question - won't this break running MoveIt! with Gazebo, etc?

@davetcoleman

davetcoleman Aug 22, 2016

Member

For cases where you want access to the actual wall-clock time even if running inside simulation, roslib provides Wall versions of all its time constructs

Yep, I was confused. Then I have the converse question - won't this break running MoveIt! with Gazebo, etc?

This comment has been minimized.

@rhaschke

rhaschke Aug 23, 2016

Contributor

Nope. This timeout is about getting the action server fired up. No ROS-time involved at all. We definitely should use wall time, otherwise (on a crazy fast machine) 10s might become a single second...

@rhaschke

rhaschke Aug 23, 2016

Contributor

Nope. This timeout is about getting the action server fired up. No ROS-time involved at all. We definitely should use wall time, otherwise (on a crazy fast machine) 10s might become a single second...

This comment has been minimized.

@davetcoleman

davetcoleman Aug 23, 2016

Member

good explanation, its clear to me now

@davetcoleman

davetcoleman Aug 23, 2016

Member

good explanation, its clear to me now

Show outdated Hide outdated moveit_ros/planning_interface/move_group_interface/src/move_group.cpp
execute_action_client_.reset(new actionlib::SimpleActionClient<moveit_msgs::ExecuteTrajectoryAction>
(node_handle_, move_group::EXECUTE_ACTION_NAME, false));
// TODO: standard waitForAction after deprecation period (L-turtle)
// waitForAction(execute_action_client_, move_group::EXECUTE_ACTION_NAME, timeout_for_servers, allotted_time);

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

+1 this is very nice

@davetcoleman

davetcoleman Aug 22, 2016

Member

+1 this is very nice

Show outdated Hide outdated moveit_ros/planning_interface/move_group_interface/src/move_group.cpp
// TODO: after deprecation period, i.e. for L-turtle, remove the following stuff
// SNIP

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

move this SNIP-SNAP into helper function to hide all this extra compatibility logic?

@davetcoleman

davetcoleman Aug 22, 2016

Member

move this SNIP-SNAP into helper function to hide all this extra compatibility logic?

This comment has been minimized.

@rhaschke

rhaschke Aug 22, 2016

Contributor

Good idea.

@rhaschke

rhaschke Aug 22, 2016

Contributor

Good idea.

@@ -155,44 +193,35 @@ class MoveGroup::MoveGroupImpl
}
template<typename T>
void waitForAction(const T &action, const ros::Duration &wait_for_server, const std::string &name)
void waitForAction(const T &action, const std::string &name, const ros::WallTime &timeout, double allotted_time)

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

why did you move the name var to the middle of the function header instead of at the back? seems like this is an overly-intrusive re-ordering

@davetcoleman

davetcoleman Aug 22, 2016

Member

why did you move the name var to the middle of the function header instead of at the back? seems like this is an overly-intrusive re-ordering

This comment has been minimized.

@rhaschke

rhaschke Aug 22, 2016

Contributor

I just wanted to have the two time variables together. I have no specific preference for the variable ordering. As it's a function private to this source file, it doesn't actually matter.

@rhaschke

rhaschke Aug 22, 2016

Contributor

I just wanted to have the two time variables together. I have no specific preference for the variable ordering. As it's a function private to this source file, it doesn't actually matter.

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

i agree time vars should be grouped. since its private you're right, it doesn't matter

@davetcoleman

davetcoleman Aug 22, 2016

Member

i agree time vars should be grouped. since its private you're right, it doesn't matter

Show outdated Hide outdated moveit_ros/planning_interface/move_group_interface/src/move_group.cpp
{
while (node_handle_.ok() && !action->isServerConnected())
{
ros::WallDuration(0.001).sleep();
ros::WallDuration(0.1).sleep();

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

this is a really long sleep!

@davetcoleman

davetcoleman Aug 22, 2016

Member

this is a really long sleep!

This comment has been minimized.

@rhaschke

rhaschke Aug 22, 2016

Contributor

The overall timeout is in the range of seconds (here even infinity). Hence, tiny sleeps are not very useful. I guess, the round-trip time of a isServerConnected() call is larger.

@rhaschke

rhaschke Aug 22, 2016

Contributor

The overall timeout is in the range of seconds (here even infinity). Hence, tiny sleeps are not very useful. I guess, the round-trip time of a isServerConnected() call is larger.

This comment has been minimized.

@v4hn

v4hn Aug 22, 2016

Member

please undo this.
only recently @alainsanguinetti and I agreed to change these.
See ros-planning/moveit_ros#701 (comment)

@v4hn

v4hn Aug 22, 2016

Member

please undo this.
only recently @alainsanguinetti and I agreed to change these.
See ros-planning/moveit_ros#701 (comment)

This comment has been minimized.

@rhaschke

rhaschke Aug 22, 2016

Contributor

OK. Done.

@rhaschke

rhaschke Aug 22, 2016

Contributor

OK. Done.

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

This is a clear use-case for a comment explaining how the magic number was set, something like:

"From testing, 0.001 has been found to capture the connection time expected from the callback queue"

@davetcoleman

davetcoleman Aug 22, 2016

Member

This is a clear use-case for a comment explaining how the magic number was set, something like:

"From testing, 0.001 has been found to capture the connection time expected from the callback queue"

This comment has been minimized.

@v4hn

v4hn Aug 22, 2016

Member

this is not related to this request.
@alainsanguinetti could you open a request that adds the comment @davetcoleman asks for?

@v4hn

v4hn Aug 22, 2016

Member

this is not related to this request.
@alainsanguinetti could you open a request that adds the comment @davetcoleman asks for?

req.trajectory = plan.trajectory_;
req.wait_for_execution = wait;
if (execute_service_.call(req, res))
if (!execute_action_client_)

This comment has been minimized.

@davetcoleman

davetcoleman Aug 22, 2016

Member

add comment about removing this in ROS-L

@davetcoleman

davetcoleman Aug 22, 2016

Member

add comment about removing this in ROS-L

This comment has been minimized.

@rhaschke

rhaschke Aug 23, 2016

Contributor

Done.

@rhaschke

rhaschke Aug 23, 2016

Contributor

Done.

rhaschke added some commits Aug 22, 2016

addressed Dave's comments
- use MOVEIT_DEPRECATED instead of ROS_DEPRECATED
- use separated function waitForExecuteActionOrService()
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 23, 2016

Member

+1, does @v4hn want to look at this?

Member

davetcoleman commented Aug 23, 2016

+1, does @v4hn want to look at this?

@v4hn

This comment has been minimized.

Show comment
Hide comment
@v4hn

v4hn Aug 23, 2016

Member
Member

v4hn commented Aug 23, 2016

@davetcoleman davetcoleman merged commit 0d470d6 into ros-planning:kinetic-devel Aug 23, 2016

1 check passed

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

davetcoleman added a commit that referenced this pull request Aug 23, 2016

Reopen #85: Add capability to execute trajectory with a ROS action (#94)
* Add capability to execute trajectory as a ROS action

* cleanup instantiation of MoveGroupInterface

- unified names of execute_trajectory service and action capabilities
- consider overall (wall) timeout for waiting for action servers
- output deprecation warning once during instantiation

* addressed Dave's comments

- use MOVEIT_DEPRECATED instead of ROS_DEPRECATED
- use separated function waitForExecuteActionOrService()

* reduced sleeps to 0.001s again
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 23, 2016

Member

cherry-picked to J, not I

Member

davetcoleman commented Aug 23, 2016

cherry-picked to J, not I

@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 24, 2016

Contributor

@davetcoleman
Oops, you squashed the commits of mine and @rhaschke . And my own commit has gone.
Actually, I'm working as a Google Summer of Code student, and I need to report my commits that are merged. So I hope it would be non-squashed commits.
Could u please fix this?

Contributor

wkentaro commented Aug 24, 2016

@davetcoleman
Oops, you squashed the commits of mine and @rhaschke . And my own commit has gone.
Actually, I'm working as a Google Summer of Code student, and I need to report my commits that are merged. So I hope it would be non-squashed commits.
Could u please fix this?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 24, 2016

Member

@wkentaro I'm really sorry, I didn't think about that. I checked the 4 commits and they were messy / not feature-grouped:

Add capability to execute trajectory as a ROS action
cleanup instantiation of MoveGroupInterface …
addressed Dave's comments …
reduced sleeps to 0.001s again

so did my standard squash-merge without thinking about attribution. I'm not sure there is an easy way to fix this other than reverting this change with a commit, then re-committing on top of that with a commit with your name on it. We'd have to do this twice for both branches. Is there any way you can still show to GSoC that this was in fact your work?

Member

davetcoleman commented Aug 24, 2016

@wkentaro I'm really sorry, I didn't think about that. I checked the 4 commits and they were messy / not feature-grouped:

Add capability to execute trajectory as a ROS action
cleanup instantiation of MoveGroupInterface …
addressed Dave's comments …
reduced sleeps to 0.001s again

so did my standard squash-merge without thinking about attribution. I'm not sure there is an easy way to fix this other than reverting this change with a commit, then re-committing on top of that with a commit with your name on it. We'd have to do this twice for both branches. Is there any way you can still show to GSoC that this was in fact your work?

@rhaschke rhaschke deleted the rhaschke:execute-action branch Aug 24, 2016

@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 24, 2016

Contributor

so did my standard squash-merge without thinking about attribution. I'm not sure there is an easy way to fix this other than reverting this change with a commit, then re-committing on top of that with a commit with your name on it. We'd have to do this twice for both branches. Is there any way you can still show to GSoC that this was in fact your work?

I'm not sure how the GSoC admins evaluate the my final submission, because they don't say about it in the final evaluation instruction.
(If they read all PRs carefully, as well as this comment, it would be fine.)
I'll refer this PR as one of my works, and I hope they don't evaluate only with the number of commits whose author is me.

Contributor

wkentaro commented Aug 24, 2016

so did my standard squash-merge without thinking about attribution. I'm not sure there is an easy way to fix this other than reverting this change with a commit, then re-committing on top of that with a commit with your name on it. We'd have to do this twice for both branches. Is there any way you can still show to GSoC that this was in fact your work?

I'm not sure how the GSoC admins evaluate the my final submission, because they don't say about it in the final evaluation instruction.
(If they read all PRs carefully, as well as this comment, it would be fine.)
I'll refer this PR as one of my works, and I hope they don't evaluate only with the number of commits whose author is me.

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Aug 25, 2016

Member

I did GSoC two years ago! From my experience I think you'll be fine

Member

davetcoleman commented Aug 25, 2016

I did GSoC two years ago! From my experience I think you'll be fine

@wkentaro

This comment has been minimized.

Show comment
Hide comment
@wkentaro

wkentaro Aug 25, 2016

Contributor

I see. That's fine then!

2016年8月25日木曜日、Dave Colemannotifications@github.comさんは書きました:

I did GSoC two years ago! From my experience I think you'll be fine


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#94 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEHFk9Vn116VYqzBl4NWJ7X3qkBLppQdks5qjTprgaJpZM4JpGvb
.

和田 健太郎 / Kentaro Wada
http://wkentaro.com

Contributor

wkentaro commented Aug 25, 2016

I see. That's fine then!

2016年8月25日木曜日、Dave Colemannotifications@github.comさんは書きました:

I did GSoC two years ago! From my experience I think you'll be fine


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#94 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AEHFk9Vn116VYqzBl4NWJ7X3qkBLppQdks5qjTprgaJpZM4JpGvb
.

和田 健太郎 / Kentaro Wada
http://wkentaro.com

rhaschke added a commit that referenced this pull request Aug 26, 2016

v4hn added a commit that referenced this pull request Nov 11, 2016

Merge pull request #144 from rhaschke/execute-action
cherry-pick #94 from kinetic-devel: execute action capability
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment