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

Switch UR10, UR10e and UR16e to effort controllers #525

Conversation

ipa-kut
Copy link

@ipa-kut ipa-kut commented Jul 7, 2020

This fix addresses #521 which in turn addresses #386, and was worked on during WRID2020

Based on a rough understanding of this comment, only the three URs mentioned in the title have been updated.

PID values for the effort controllers were taken from here

After making these changes, the gazebo simulation of the robot was observed to start in a "weird" pose :
UR16e:
initial_pose_16e

UR10 and UR10e:
initial_pos_10e

However, the controllers initially seem to initially work well, and the robots could be jogged using the RQT Joint trajectory controller plugin

@fmauch
Copy link
Contributor

fmauch commented Jul 7, 2020

As this PR is branching of melodic-devel-staging none of the models use the updated cog from #504.

Eventually, this should be changed for all 7 robot variants, though.

@ipa-kut
Copy link
Author

ipa-kut commented Jul 7, 2020

I will change this for the remaining URs now.

As this PR is branching of melodic-devel-staging none of the models use the updated cog from #504.

Would it be better to wait until #504 is merged, to rebase this and test again?

@fmauch
Copy link
Contributor

fmauch commented Jul 7, 2020

I will change this for the remaining URs now.

As this PR is branching of melodic-devel-staging none of the models use the updated cog from #504.

Would it be better to wait until #504 is merged, to rebase this and test again?

I think, this can be implemented independently from #504 but for fine tuning it might be interesting whether it makes a difference whether we use the cylinder approximation for the inertia or we use the matrices provided by the robot controller. So, testing and fine-tuning this might require a rebase on #504. However, as the results from those tests will also impact #504, we should not wait for that to be merged, but develop them in parallel. I think, this won't be too much of an issue as there are no conflicts between those two.

Copy link
Member

@ipa-nhg ipa-nhg left a comment

Choose a reason for hiding this comment

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

Tested with #526 and it works.

Great contribution @ipa-kut, thanks! 👍

@RobertWilbrandt
Copy link
Contributor

The changes from this should now be merged through #619. Thanks @ipa-kut for getting this started!

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