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 note in ExecuteKnownTrajectory service to recommend ExecuteTrajectory action. #29

Merged
merged 1 commit into from
Aug 31, 2016

Conversation

130s
Copy link
Contributor

@130s 130s commented Aug 30, 2016

Not entirely sure if this suggestion is the right move, but looking at places like this comment we want to switch to the new action.

@130s
Copy link
Contributor Author

130s commented Aug 30, 2016

Apparently I was wrong; We still want both moveit/moveit#60 (comment)

So instead of removing, I added a notice in ExecuteKnownTrajectory service to recommend the newly added action.

@130s 130s changed the title Remove ExecuteKnownTrajectory service that is replaced by ExecuteTrajectory action. Add note in ExecuteKnownTrajectory service to recommend ExecuteTrajectory action. Aug 30, 2016
@v4hn
Copy link
Contributor

v4hn commented Aug 30, 2016

-1
Yes, we wanted to retain the obsolete service, until lunar I believe, to be backward compatible.
But even if you add the comment, this would change the checksum of the action specification and is not compatible. ROS compares the checksums of publishers and subscribers before allowing a connection.

@rhaschke
Copy link
Contributor

Closing, due to @v4hn's comment. Deprecation message is included in the main code.

@rhaschke rhaschke closed this Aug 30, 2016
@130s
Copy link
Contributor Author

130s commented Aug 30, 2016

Good point for the checksum change. I'm just not sure if updating ONLY comment would change the md5sum, so I asked a question for confirmation (I included there a quick test result I locally did that didn't change checksum).

@130s
Copy link
Contributor Author

130s commented Aug 30, 2016

There seems to be a wiki page that says comment are excluded for md5 generation.

As a non-expert MoveIt! coder, the description in msg/srv/action files is very helpful. There's also an open issue #2 for in-file description enhancement.

So reopening.

@130s 130s reopened this Aug 30, 2016
@v4hn
Copy link
Contributor

v4hn commented Aug 30, 2016

Thanks for researching this! Looks like it was a myth - or maybe they changed it at some point?
In this case your addition totally makes sense.

I would prefer to formulate the comment a bit more strict. How about

This service is deprecated and will go away at some point. For new development use the ExecuteTrajectory action.

?

@130s
Copy link
Contributor Author

130s commented Aug 31, 2016

Ok, commit updated.

@v4hn
Copy link
Contributor

v4hn commented Aug 31, 2016

+1 Looks good to me.

@130s 130s merged commit 649eb10 into moveit:indigo-devel Aug 31, 2016
@130s 130s deleted the rm/replacedservice branch August 31, 2016 18:55
@130s
Copy link
Contributor Author

130s commented Aug 31, 2016

Cherrypicked to jade-devel 40b691e

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.

3 participants