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 RobotModeDataMsg #395

Merged

Conversation

felixvd
Copy link
Contributor

@felixvd felixvd commented Dec 19, 2018

Copy link
Member

@miguelprada miguelprada left a comment

Choose a reason for hiding this comment

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

There are some additional fields in the Robot Mode Data, according to the overview of the client interfaces.

In particular, I'm interested in the speed fraction/scaling value (name is different depending on version). I'm going to guess that value indicates the position of the slider in PolyScope, but I haven't been able to check this. If that is the case, it could be interesting to add it since application logic may use that to throttle down the commands based on its value.

Any reason for not having included it in the message?

@felixvd
Copy link
Contributor Author

felixvd commented Feb 26, 2019

The historical reason is that I merged the fields from this PR and didn't want to do unnecessary development that no one might have a need for. I have also had issues with the driver when the slider is not at 100% (e.g. robots would overshoot and not stop at the target position), so adding more fields may open up more construction sites for little benefit. I suspect that in the case you describe, users will either use the URscript commands or send a slower trajectory through ROS.

A practical reason is that the fields that this PR publishes are common to all UR firmware versions, so there is no need to do additional checks.

Either way, new fields can be added afterwards if the need arises.

@miguelprada
Copy link
Member

I think the slider being not at 100% is a common source of errors, indeed. Anyways, it may make more sense to tackle that problem by adding a check at the driver level and displaying a warning, rather than publishing the value.

I'm fine merging this as it is. @gavanderhoorn, @ipa-nhg?

miguelprada
miguelprada previously approved these changes Feb 26, 2019
@gavanderhoorn
Copy link
Member

Sorry for dismissing your review @miguelprada, but I felt the additional comment in the .msg file would be good to add.

@miguelprada
Copy link
Member

Sorry for dismissing your review @miguelprada, but I felt the additional comment in the .msg file would be good to add.

👍

Not sure why this cannot be merged as is. I'll merge kinetic-devel and squash-merge.

@miguelprada miguelprada merged commit 5cfc5a4 into ros-industrial:kinetic-devel Feb 26, 2019
@miguelprada
Copy link
Member

Thanks @felixvd!

@felixvd
Copy link
Contributor Author

felixvd commented Feb 27, 2019

Thanks for the quick action after my late reply!

ipa-nhg pushed a commit to ipa-nhg/universal_robot that referenced this pull request Jul 2, 2019
* Add RobotModeDataMsg

* msg: clarify that not all fields from RobotMode are published
ipa-nhg pushed a commit to ipa-nhg/universal_robot that referenced this pull request Jul 2, 2019
* Add RobotModeDataMsg

* msg: clarify that not all fields from RobotMode are published
gavanderhoorn pushed a commit to ros-industrial/ur_msgs that referenced this pull request Jun 22, 2020
* Add RobotModeDataMsg

* msg: clarify that not all fields from RobotMode are published
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.

None yet

3 participants