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

updating iCub3 with new arm covers and corresponding options #202

Merged
merged 3 commits into from
Jul 21, 2021

Conversation

AlexAntn
Copy link
Contributor

This PR sees the update of the iCub3 with its new upperarm covers.

This change now accounts for skin width.

Other small changes in the model options were introduced (collision/contact sensors for the upper arms; small tweak on upper joint limit for the roll to be on par with real robot).

Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

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

LGTM, we can merge it if the generated model has been tested

@AlexAntn
Copy link
Contributor Author

I have tested the model, it is working properly in Gazebo

@@ -3,10 +3,10 @@ torso_yaw,-43,43,2,0,120,
torso_pitch,-18,45,2,0,120,
torso_roll,-23,23,2,0,120,
r_shoulder_pitch,-88,13,2,0,120,
r_shoulder_roll,12,160,2,0,120,
r_shoulder_roll,12,163,2,0,120,
Copy link
Member

Choose a reason for hiding this comment

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

Did we documented somewhere this limits? It would be great if we could have some official documentation, such as https://icub-tech-iit.github.io/documentation/icub_kinematics/icub-joints-limits/icub-joints-limits/ , so that here we just reference those.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we did yet, since there was still going to be an update of the limits for the real robot, but we can update them I think

Copy link
Member

@traversaro traversaro Jul 8, 2021

Choose a reason for hiding this comment

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

If they are not updated, I am ok in merging this PR before the documentation changed, but I would open an issue on the update of joint limits, otherwise we risk forgetting about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Regarding this point I think we need to discuss with @pattacini about these limits. Were they set for iCub3 anywhere, since the joint calibration? Where should we get these values from?

Copy link
Member

@pattacini pattacini Jul 12, 2021

Choose a reason for hiding this comment

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

@traversaro's suggestions were:

  1. to update our official documentation website with the iCub 3 joint limits.
  2. to create in robots-configuration an issue in order to keep track of the need for checking/implementing these new limits on the physical robot.

I think you can go ahead certainly with 2; it'd be great to have also 1 done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have opened a new issue in robots-configuration, that we can see here: robotology/robots-configuration#280

this should allow us to keep track of this, in order to update them in the future

Copy link
Member

Choose a reason for hiding this comment

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

Thanks!

Comment on lines 750 to 756
<origin xyz="0.030743125543818514 0.141531075879301 0.06954757274337188" rpy="-0.0715644603308 0.0619699233851 -1.84623504024"/>
<geometry>
<mesh>
<scale>0.001 0.001 0.001</scale>
<uri>model://iCub/meshes/simmechanics/sim_icub3_r_upperarm_prt.stl</uri>
</mesh>
</geometry>
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure that it is necessary to duplicate this infor from the model manually? If you see how we inject collision info for r_foot_rear, this does not seems to be necessary, as in the collision tag only the elements that are added need to be included. This avoids the need for manually updating the origin tag if the transform location changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed we don't need the duplicated section, I have removed it, tested it and it works properly, I have committed and updated the PR (waiting for tests)

@traversaro
Copy link
Member

Friendly ping @AlexAntn .

@traversaro
Copy link
Member

@AlexAntn is this PR ready to be merged? For the the future, I don't get notifications for new commits, so feel free to comment whenever the PR is ready for review/merge.

Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

See inline comments.

@pattacini
Copy link
Member

Hi @AlexAntn

We are going to regenerate the URDF of iCub 3 quite soon.
It'd be great if we could include also these new additions to the model.

@pattacini
Copy link
Member

@traversaro do you deem we can merge this PR or are there still open points?

@traversaro
Copy link
Member

@traversaro do you deem we can merge this PR or are there still open points?

My comments were solved, but from #202 (comment) it seemed that you wanted to wait before merging this. If this is not the case anymore, we can merge.

@pattacini
Copy link
Member

My comments were solved, but from #202 (comment) it seemed that you wanted to wait before merging this. If this is not the case anymore, we can merge.

We can merge the PR so that we will inherit these additions into the URDF next time.

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