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 #470

Closed
wants to merge 2 commits into from

Conversation

fmauch
Copy link
Contributor

@fmauch fmauch commented Nov 27, 2019

"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.

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

We at Shadow Robot are on our own fork/branch until this is merged! The same goes for the dependent PR (UniversalRobots/Universal_Robots_ROS_Driver#50). Let me know if we can help in any way.

@fmauch
Copy link
Contributor Author

fmauch commented Jun 15, 2020

This is currently the last puzzle piece that prevents ur_robot_driver to use this repository instead of my fork as a dependency.

Thanks @gavanderhoorn for putting so much effort into everything description-related recently so we have the chance of getting close to migrating away from my fork.

@gavanderhoorn
Copy link
Member

Yes, as soon as the description stuff is out of the way, we'll merge the other outstanding PRs into melodic-devel-staging (this one, the IK related ones, the MoveIt config ones, etc).

@gavanderhoorn
Copy link
Member

ur_msgs will be moved to its own repository (#448), so I'm still deciding whether we'll merge this here, or after ur_msgs has been extracted.

@fmauch
Copy link
Contributor Author

fmauch commented Jun 15, 2020

Both would be working for me.

@gavanderhoorn
Copy link
Member

@fmauch: could I ask you to resubmit this PR to ros-industrial/ur_msgs? I've decided that it's easier to maintain a sane history if we first migrate the package and then merge any new PRs there.

@gavanderhoorn
Copy link
Member

Closing in favour of a PR against the new repository.

@jwhendy
Copy link
Contributor

jwhendy commented Oct 15, 2020

In the process of migrating an old universal_robot setup, I'm trying to adapt to the new driver.

I found the merging into master but relying on a custom fork unusual for ROS, where I'd expect that to just wait until all the pieces were in place (and, thus, melodic-devel or its equivalent should JustWork with other melodic-devel branches). Is there an ETA on when all this gets tidied up?

@gavanderhoorn
Copy link
Member

I'm not entirely sure I understand what you ask.

There is no master branch here.

Could you clarify?

@jwhendy
Copy link
Contributor

jwhendy commented Oct 17, 2020

Sorry, I thought this PR within the UR ecosystem would be understood. #50 is linked above. It's merged into master there since Jul 20, which creates a "gotcha" when you go to build. What I would assume to be a stable branch is relying on the fork being PR'd here. I wondered if there was an estimate on when this long-standing PR might be integrated to resolve that.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Oct 17, 2020

I still don't understand.

ur_msgs is a released package on Melodic, and it includes the changes discussed here.

What I would assume to be a stable branch is relying on the fork being PR'd here.

I'm not sure I understand what fork you are referring to.

And you refer to #50, but should that be UniversalRobots/Universal_Robots_ROS_Driver#50?

@jwhendy
Copy link
Contributor

jwhendy commented Oct 17, 2020

Correct, as stated it's "the #50 linked above," not the one from this repo. I'll take this question there.

The evidence I observed: build instructions pointing to a fork to get around build errors, with that workaround being necessary "until the changes are merged upstream." Thus, I thought the reason for this due to upstream (here).

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.

4 participants