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

Added center_of_gravity to SetPayload service #2

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

fmauch
Copy link
Contributor

@fmauch fmauch commented Jun 23, 2020

"Newer" version of URScript (>= 1.7) have the CoG as second optional
parameter to the script function setting up the payload.

As we'd like to add this service to the ur_robot_driver I'd like to add this suggestion here. As far as I know, this is being used by both, the ur_driver and the ur_modern_driver, which is why I left the payload field untouched.

I had the feeling that duplicating this message into another message package is not the right way to go.

First of all, I wanted to start the discussion here. Once we figured out the final solution, I'd propose to also include this into kinetic-devel, as the ur_robot_driver is also working with kinetic.

This is ros-industrial/universal_robot#470 reopened on the new ur_msgs repository

@gavanderhoorn
Copy link
Member

Travis failed, but with a bit of a strange error.

I've triggered another run.

@fmauch
Copy link
Contributor Author

fmauch commented Jun 23, 2020

I was having similar trouble recently trying to run industrial-ci locally. This also happened for ros-industrial/universal_robot#517.

@gavanderhoorn
Copy link
Member

I was having similar trouble recently trying to run industrial-ci locally.

I'm seeing some buildfarm builds also failing with similar errors.

Wondering whether it's a global issue.

This also happened for ros-industrial/universal_robot#517.

The failures for Melodic are expected: ur_msgs is not available as a .deb for that ROS version, so as the package is not in the repository any more, and there are no install candidates (until ros/rosdistro#25555 gets merged and built), CI will fail.

@fmauch
Copy link
Contributor Author

fmauch commented Jun 23, 2020

The failures for Melodic are expected: ur_msgs is not available as a .deb for that ROS version, so as the package is not in the repository any more, and there are no install candidates (until ros/rosdistro#25555 gets merged and built), CI will fail.

Yes, I wasn't looking thoroughly enough, sorry...

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 23, 2020

Seeing as this (edit: the CI failures) appears to be a problem with the ros-testing repository (at least for now), I'm going to ignore that Travis result and merge this (as the main repo build succeeded).

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @fmauch.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 23, 2020

As far as I know, this is being used by both, the ur_driver and the ur_modern_driver, which is why I left the payload field untouched.

Just making sure: why do you say "I left the payload field untouched"?

Would you want to change anything?

First of all, I wanted to start the discussion here. Once we figured out the final solution, I'd propose to also include this into kinetic-devel, as the ur_robot_driver is also working with kinetic.

And is this still relevant?

@fmauch
Copy link
Contributor Author

fmauch commented Jun 23, 2020

As far as I know, this is being used by both, the ur_driver and the ur_modern_driver, which is why I left the payload field untouched.

Just making sure: why do you say "I left the payload field untouched"?

Would you want to change anything?

Actually, I don't quite remember. Maybe I thought, it would make sense to rename it to "mass", as the whole "payload" object consists of a mass and a CoG.

First of all, I wanted to start the discussion here. Once we figured out the final solution, I'd propose to also include this into kinetic-devel, as the ur_robot_driver is also working with kinetic.

And is this still relevant?

The question is how all those recent changes will be ported to kinetic, anyway. Once we switch do the new structure, we either will have to distinguish between a kinetic and melodic version (of the ur_robot_driver) or we would have to port everything back to kinetic. But currently, I'd say it's fine to just go ahead with melodic.

@gavanderhoorn
Copy link
Member

Actually, I don't quite remember. Maybe I thought, it would make sense to rename it to "mass", as the whole "payload" object consists of a mass and a CoG.

We could do this.

The Melodic transition is certainly the right time to change these message definitions.

The question is how all those recent changes will be ported to kinetic, anyway. Once we switch do the new structure, we either will have to distinguish between a kinetic and melodic version (of the ur_robot_driver) or we would have to port everything back to kinetic. But currently, I'd say it's fine to just go ahead with melodic.

My plan is to not touch kinetic-devel at all. It will stay the way it is, identical to the content of the .debs of 1.2.7.

melodic-devel will most likely work on Kinetic as well (it may need the --inorder on the xacro commands, but that will need to be checked). For users still on Kinetic, the suggestion would be to use melodic-devel and build it from source.

Users on Melodic (and Noetic) can install the binaries (starting with 1.3.0).

@gavanderhoorn gavanderhoorn merged commit 4c111fc into ros-industrial:melodic-devel Jun 23, 2020
@gavanderhoorn
Copy link
Member

Thanks for the contribution again @fmauch.

Renaming the payload field to mass could be done in a follow-up PR.

@fmauch fmauch deleted the set_payload_cog branch June 23, 2020 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants