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

Revert #255 #274

Merged
merged 1 commit into from
Apr 8, 2019
Merged

Revert #255 #274

merged 1 commit into from
Apr 8, 2019

Conversation

knorth55
Copy link
Contributor

@knorth55 knorth55 commented Mar 18, 2019

related to #245 and #255

In #245 and #255, they try to work PR2 on Gazebo in Indigo version and make this change.
However, this modification doesn't make sense, and it doesn't work on Kinetic and Melodic, which uses higher version Gazebo.
I revert the PR and I checked it's working on Kinetic Gazebo.

@knorth55 knorth55 changed the title fix gripepr thread_pitch fix gripper thread_pitch Mar 18, 2019
@knorth55
Copy link
Contributor Author

I update to revert d5c8f47 and it works in Kinetic !
pr2-kinetic-gripper-gazebo

@knorth55 knorth55 changed the title fix gripper thread_pitch Revert #255 Mar 18, 2019
@k-okada k-okada self-requested a review March 26, 2019 08:00
Copy link
Contributor

@k-okada k-okada left a comment

Choose a reason for hiding this comment

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

LGTM, please backport to kinetic and also add a more detailed explanation.

@knorth55 knorth55 mentioned this pull request Mar 26, 2019
@knorth55
Copy link
Contributor Author

I add detailed explanation in this PR.

@knorth55
Copy link
Contributor Author

@k-okada can you review this again?

@v4hn
Copy link
Member

v4hn commented Mar 29, 2019

Hi @knorth55

However, this modification doesn't make sense, and it doesn't work on Kinetic and Melodic, which uses higher version Gazebo.

Please give more details in pull-requests.
"doesn't make sense".. Why?
"doesn't work on kinetic and melodic".. What do you observe there?

Any idea why this factor of 10 seemed necessary in indigo and "doesn't work" in kinetic?

@k-okada given that both the original and the new patch come from your lab, I will leave this to you to resolve.

@knorth55
Copy link
Contributor Author

knorth55 commented Mar 29, 2019

"doesn't make sense".. Why?

Originally, this parameter is set as 3141.6, which comes from PR2 hardware design.
Willow garage set this parameter at first (we can see it from git log), but some people change the parameter to 314.16 in #255.
They change this parameter from 3141.6 -> 314.16 to run PR2 gazebo indigo, but this parameter is defined from PR2 hardware, so 314.16 doesn't make sense.
However, PR2 gazebo in Indigo does not work (it is reported in #245) with this original correct parameter 3141.6, and they solved the problem by changing this parameter to 314.16.
This is my guess, but this problem is actually a bug in Gazebo, and it looks running correctly on Gazebo with the wrong parameter 314.16 somehow.

"doesn't work on kinetic and melodic".. What do you observe there?

However, when I tried to close PR2 gripper on PR2 Gazebo Kinetic, the gripper does not close.
We read git log, and this is caused the parameter difference between


and
<thread_pitch>-3141.6</thread_pitch>
.
So I change the parameter back to correct one 314.16 -> 3141.6, and now PR2 gazebo on kinetic works.

@knorth55
Copy link
Contributor Author

knorth55 commented Mar 31, 2019

@v4hn I updated the comment to add more detailed information.
My point is that

@knorth55
Copy link
Contributor Author

knorth55 commented Apr 5, 2019

@v4hn kindly ping.

@k-okada k-okada merged commit 01642f2 into PR2:melodic-devel Apr 8, 2019
@knorth55 knorth55 deleted the fix-gripper branch April 8, 2019 07:18
@knorth55 knorth55 mentioned this pull request Apr 8, 2019
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.

3 participants