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

Add .action file for execution trajectory in a ROS action #24

Merged
merged 2 commits into from
Aug 18, 2016

Conversation

wkentaro
Copy link
Contributor

@wkentaro wkentaro commented Jul 18, 2016

For moveit/moveit_ros#719
Being moved from #24

@v4hn
Copy link
Contributor

v4hn commented Jul 18, 2016

-1 on the name.
I agree with @davetcoleman's comment.
Please rename the action to "ExecutePath.action".

@wkentaro
Copy link
Contributor Author

Fixed.

# The full starting state of the robot at the start of the trajectory
moveit_msgs/RobotState trajectory_start

# The trace of the trajectory recorded during execution
Copy link
Member

Choose a reason for hiding this comment

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

could you explain "trace" better... in what cases would it differ from the input trajectory and at what discretization level are you expecting this returned trajectory to be at?

@wkentaro
Copy link
Contributor Author

@davetcoleman
It seems that the two results are not necessary.
So I removed them in the last commit.

@davetcoleman
Copy link
Member

I'm not sure if they are useful or not, but it wasn't obvious to me what they would be used for. Seeing how the old ExecuteKnownTrajectory.srv doesn't have anything similar, it makes sense to not have them

@wkentaro
Copy link
Contributor Author

I agree. I think the results can be added afterward when it is required.

@davetcoleman davetcoleman merged commit cd1971b into moveit:indigo-devel Aug 18, 2016
@davetcoleman
Copy link
Member

cherry-picked to J

@wkentaro wkentaro deleted the feature/execute-action branch August 18, 2016 07:21
@wkentaro
Copy link
Contributor Author

Thanks!

@rhaschke
Copy link
Contributor

rhaschke commented Aug 19, 2016

Please rename the action to ExecutePath.action.

Hm. Even better would have been ExecuteTrajectory.action. All other stuff related to joint-space trajectories is called like this. Also, a RobotTrajectory is passed in the message.
Looks like path is usually used in the context of Cartesian trajectories.

@v4hn @davetcoleman: What do you think?

Should we also rename the existing ExecuteKnownTrajectory.srv to ExecuteTrajectory.srv for unification?

@davetcoleman
Copy link
Member

I see your point, trajectory makes more sense than path. +1

I don't see nearly enough advantage to change the long existing service
name though.

On Aug 19, 2016 12:45 PM, "Robert Haschke" notifications@github.com wrote:

Please rename the action to ExecutePath.action.

Hm. Even better would have been ExecuteTrajectory.action. All other stuff
related to joint-space trajectories is called like this. Also, a
RobotTrajectory is passed in the message...

@v4hn https://github.com/v4hn @davetcoleman
https://github.com/davetcoleman: What do you think?

Should we also rename the existing ExecuteKnownTrajectory.srv to
ExecuteTrajectory.srv for unification?


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

@130s
Copy link
Contributor

130s commented Aug 19, 2016

Are we going to make a change discussed after this PR was merged any sooner?
I like to fix c5c4c61#commitcomment-18705574 and make a release asap (within a day or so), so it'd be great if both changes can be released together.

@rhaschke
Copy link
Contributor

@130s Name fixed with #27.

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.

5 participants