Skip to content

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

Merged
davetcoleman merged 2 commits intomoveit:indigo-develfrom
wkentaro:feature/execute-action
Aug 18, 2016
Merged

Add .action file for execution trajectory in a ROS action#24
davetcoleman merged 2 commits intomoveit:indigo-develfrom
wkentaro:feature/execute-action

Conversation

@wkentaro
Copy link
Copy Markdown
Contributor

@wkentaro wkentaro commented Jul 18, 2016 •
edited
Loading

For moveit/moveit_ros#719
Being moved from #24

@wkentaro wkentaro mentioned this pull request Jul 18, 2016
Closed
@v4hn
Copy link
Copy Markdown
Contributor

v4hn commented Jul 18, 2016

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

@wkentaro wkentaro force-pushed the feature/execute-action branch from fd41eeb to 5e694f7 Compare July 19, 2016 05:06
@wkentaro wkentaro force-pushed the feature/execute-action branch from 5e694f7 to ce3fdd3 Compare July 19, 2016 05:07
@wkentaro
Copy link
Copy Markdown
Contributor Author

wkentaro commented Jul 19, 2016

Fixed.

@wkentaro wkentaro mentioned this pull request Aug 17, 2016
Merged
davetcoleman
davetcoleman reviewed Aug 17, 2016
View reviewed changes
action/ExecutePath.action Outdated
# 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
Copy Markdown
Member

@davetcoleman davetcoleman Aug 17, 2016

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
Copy Markdown
Contributor Author

wkentaro commented Aug 18, 2016

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

@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Aug 18, 2016

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
Copy Markdown
Contributor Author

wkentaro commented Aug 18, 2016

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 pushed a commit that referenced this pull request Aug 18, 2016
@wkentaro @davetcoleman
Add .action file for execution trajectory in a ROS action (#24)
e11f494
@davetcoleman
Copy link
Copy Markdown
Member

davetcoleman commented Aug 18, 2016

cherry-picked to J

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

wkentaro commented Aug 18, 2016

Thanks!

@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented Aug 19, 2016 •
edited
Loading

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
Copy Markdown
Member

davetcoleman commented Aug 19, 2016

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
Copy Markdown
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.

@wkentaro wkentaro mentioned this pull request Aug 20, 2016
Merged
@rhaschke
Copy link
Copy Markdown
Contributor

rhaschke commented Aug 20, 2016

@130s Name fixed with #27.

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@wkentaro @v4hn @davetcoleman @rhaschke @130s