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

merge branch boneil_point_streaming manually #88

Open
wants to merge 1 commit into
base: indigo-devel
Choose a base branch
from

Conversation

jim-rees
Copy link

original author: Brian O'Neil

This is an update of pull request #37, "On-the-fly point streaming and Cartesian jog". I've merged it to apply to indigo-devel, and have tested it and am using it. This does not include the cartesian jog part.

original author: Brian O'Neil
@shaun-edwards
Copy link
Member

@jim-rees, thanks for this. I'm working my way through PRs on the Motoman repo over the next few weeks.

One quick questions, does this PR support multiple joint groups (i.e. for a dual arm set up)?

@jim-rees
Copy link
Author

jim-rees commented May 9, 2016

I have not tested with dual arm, but I don't see any reason why it shouldn't.

void JointTrajectoryInterface::jointCommandCB(
const trajectory_msgs::JointTrajectoryConstPtr &msg)
{
//ROS_INFO("Yessir?");
Copy link
Member

Choose a reason for hiding this comment

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

Can this be deleted?

@shaun-edwards
Copy link
Member

@jim-rees, sorry this has taken a little while. I'm working through a bunch of PRs on the motorman repo (in the hopes of clearing them out). I had a few comments as well as one general question:

It's been a while since I have looked at the industrial_robot_client classes, but why does the state case TransferStates::POINT_STREAMING: come up multiple times? Should the state machine be handled in one spot?

@jim-rees
Copy link
Author

I've had a report that this does not work with dual arm robots, because they require motoman_msgs/DynamicJointTrajectory messages which this doesn't implement.

@jim-rees
Copy link
Author

Regarding the state machines, there are two, one in MotomanJointTrajectoryStreamer::streamingThread() and one in JointTrajectoryStreamer::streamingThread(). I don't know why there are two. As far as I can tell, only the one in MotomanJointTrajectoryStreamer is actually used. I do not know why there are two classes, one overriding the other. I was hoping someone could tell me, because I find it overly confusing.

@shaun-edwards
Copy link
Member

@jim-rees, thanks for the info. I'm working on merging some other PRs before this one. Can you address some of my comments above? If we can get everything resolved, I think we should merge this into a jade development branch (thoughts?)

@shaun-edwards shaun-edwards mentioned this pull request Jun 5, 2016
@curranw
Copy link

curranw commented May 11, 2017

This seems like a useful feature, and based on the conversation, it seems implemented. However, the joint_command subscription in the JointTrajectoryInterface seems to be a placeholder. Is this fully implemented?

@jim-rees
Copy link
Author

JointTrajectoryInterface is not fully implemented because it is not fully used. See my comments above about the two classes. The Yessir? code is never called. I did not remove this code because I don't know why it's there.

I have not worked on this code in over a year so my memory may be a bit hazy but I will try to answer questions if anyone wants to resurrect this.

@gavanderhoorn gavanderhoorn added the backlog Will be addressed at a later time label May 1, 2018
tdl-ua added a commit to tbs-ualberta/motoman that referenced this pull request May 16, 2018
…al#88

* Addressed comments from @shaun-edwards:
  - Comment 1: Made JointTrajectoryInterface::jointCommandCB() pure
    virtual so that JointTrajectoryInterface::jointCommandCB()
    implementation can be deleted.
  - Comment 2: Message was changed to ROS_DEBUG
* Some other minor changes (e.g., commenting)
gmeurant pushed a commit to gmeurant/motoman that referenced this pull request Feb 1, 2022
…al#88

* Addressed comments from @shaun-edwards:
  - Comment 1: Made JointTrajectoryInterface::jointCommandCB() pure
    virtual so that JointTrajectoryInterface::jointCommandCB()
    implementation can be deleted.
  - Comment 2: Message was changed to ROS_DEBUG
* Some other minor changes (e.g., commenting)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Will be addressed at a later time
Development

Successfully merging this pull request may close these issues.

4 participants