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 robot_status support #143

Conversation

matEhickey
Copy link

Hello Thomas,
Here is my first pull-request, so don't hesitate to ask me to modify things.
The modifications here only consist of a RobotStatus http://docs.ros.org/kinetic/api/industrial_msgs/html/msg/RobotStatus.html publisher, allowing ur_modern_driver to come closer to ros-industrial specs.
It's now publishing every 100ms, but may need to be modified.
I also have a problem, I'm now unable to properly close the script, or to get error on close, because of the python-urx Robot module (I suspect the connection).

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Oct 19, 2017

Hi @matEhickey. Very nice to see that you considered opening a PR for what we discussed on ROS Answers. 👍

This functionality is definitely something we want to add to ur_modern_driver, but I feel we could adapt your approach a bit and make it much less of an invasive change.

As far as I can tell, all (most?) the info you are now getting access to by using urx is available already in the RobotState class:

https://github.com/ThomasTimm/ur_modern_driver/blob/6c3cade12fb47f41b816d05377feaea993742a28/include/ur_modern_driver/robot_state.h#L196-L202

Those functions are backed by the same datastream that urx uses to get you that info. To avoid introducing the urx (and Python) dependency, it would be great if you could change your implementation to use those functions instead.

If I understand your current PR, I think you should be able to get away with just adding a new ros::Publisher to the RosWrapper for the industrial_msgs::RobotStatus msg, and then populating and publishing a RobotStatus msg with it in RosWrapper::publishMbMsg() (somewhere around here). Note that urx's robot.secmon is called robot_.sec_interface in the RosWrapper class, so it's essentially the same.

It's not Python, but I believe you'll find the implementation to be significantly fewer lines.

That is not necessarily less work for you, but it is for us maintainers in the future, as we then don't have to also maintain a Python script and dependency (ie: keep track of urx).

@matEhickey
Copy link
Author

Hello, I just did the change. Thanks for the directions.
Do you need me to re-begin a pull request to preserve the commit history on ur_common file ? (as I modified it, but restore it on the newest commit )

@gavanderhoorn
Copy link
Member

Thanks, much appreciated 👍.

re: history: I would suggest you squash all your changes into a single commit (don't forget to update the commit msg), and force push that. That will automatically overwrite the commits in this PR.

No need to open a new one.

@matEhickey matEhickey force-pushed the branchAddRobotStatus branch from 108bcd7 to ffae9c1 Compare October 19, 2017 14:56
@matEhickey
Copy link
Author

I think that it's good now, can you confirm me ?

@gavanderhoorn
Copy link
Member

The commit history now looks ok.

minor: you might want to check the email address that was used for the commit. It currently just says mathias.

@gavanderhoorn
Copy link
Member

I'll do a proper review later.

@matEhickey matEhickey force-pushed the branchAddRobotStatus branch from ffae9c1 to 0e37bc1 Compare October 19, 2017 15:08
@matEhickey
Copy link
Author

I think the email address is now set, but I'm not sure

@gavanderhoorn
Copy link
Member

This was (still is) a very nice PR @matEhickey. It got ported to the new "kinetic refactor" by @simonschmeisser in the @Zagitta fork and merged into that in 24eef75.

As we're not merging new features in the master branch, I'll close this PR, but know that your work is part of the kinetic-devel version.

So thanks again 👍 for the PR.

@matEhickey
Copy link
Author

👍

@matEhickey matEhickey deleted the branchAddRobotStatus branch November 6, 2018 19:51
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.

2 participants