-
Notifications
You must be signed in to change notification settings - Fork 23
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
Add iCub 2.5+ models #83
Conversation
README.md
Outdated
| `iCubGazeboV2_5_plus`| simmechanics | v2.5+ with backpack, and inertias of some links modified to run smoothly in Gazebo | | ||
| `iCubGenova01` | simmechanics | v2.5 without backpack | | ||
| `iCubGenova02` | simmechanics | v2.5 with backpack | | ||
| `iCubGenova02_plus` | simmechanics | v2.5+ with backpack | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that "plus" variant has been implemented only on iCubGenova04, would it make sense to have iCubGenova04_plus
instead of iCubGenova02_plus
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected. Thanks!
Not related to this PR as I just noticed that it also applies to the existing mesh https://github.com/robotology-playground/icub-model-generator/blob/master/simmechanics/data/icub2_5/meshes/simmechanics/sim_sea_2-5_head_prt-binary.stl , but I never noticed that in this meshes the iCub head has no left ear! |
I guess that there is a problem with the surfaces of the left ear, probably are not "closed". I can check. |
Ok, anyway it is not related to the PR, I just approved it! |
Hi @fiorisi, Please don't merge yet, I'm still reviewing... A quick question, this new model is for the new ankle tilt and the XSENS IMU on the root link. Is it for any other change on the meshes? Is there any open issue or place with the a description of the changes? |
" - frameName: SCSYS_ROOT_LINK_XSENS_IMU | ||
frameReferenceLink: root_link | ||
exportedFrameName: root_link_imu_frame | ||
") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we now have 2 IMUs, could we rename the previous "imu_frame" in the head to "head_imu_frame"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can do it. I am not sure if then we need to update also some other configuration files (e.g. wbd). If you think that is easy and quick we can change the name of the head imu as you suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there a small impact in iDynTree, quite easy to implement, I let @traversaro confirm...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would need to either have different wbd configuration files for iCub***
and iCub***_plus
, or update the frame name for all the models. To be honest, I am a bit afraid about this small changes that may have unexpected consequences, so I would open a different PR for changing the imu_frame
frame.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would also affect all our controllers that should be updated accordingly @gabrielenava
@S-Dafarra
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @diegoferigo, as suggested by @traversaro we don't change the imu name in this PR. We can open a new one.
simmechanics/CMakeLists.txt
Outdated
linkName: root_link | ||
exportFrameInURDF: No | ||
sensorName: root_link_xsens_imu | ||
sensorType: \"imu\" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, while the additional frame (@XSENS_IMU_FRAME@
) exported for the IMU is required for the dynamic computations in iDynTree, the IMU itself is not considered as a URDF-like "sensor" as per the description in https://github.com/robotology/idyntree/blob/master/doc/model_loading.md and doesn't require an entry in the "sensor" tag section. The following entry...
- frameName: SCSYS_HEAD_MTX_IMU
linkName: head
sensorName: head_imu_acc_1x1
sensorType: "accelerometer"
...
was already there and was intended to provide information on the accelerometers embedded in the head IMU. those information were used for the sensor measurement prediction computation in iDynTree.
This said, we should have for the root_link IMU:
sensorName: root_link_imu_acc
sensorType: "accelerometer"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with nuno on this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
l_hip_yaw | ||
l_knee | ||
l_ankle_pitch | ||
r_hip_pitch |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is r_hip_pitch
inverted instead of l_hip_pitch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The axis that are inverted are magically changing whenever we change the Creo model. 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comes from the CAD model. In theory, it is possible to control the orientation of the axis but is not practical. It is easier to perform the change in the YAML file. Probably, since the upper body is the same for the iCub2.5 and iCub2.5+ we can change only the leg axis.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok.
@@ -8,7 +8,10 @@ originRPY: [0.0,0.0,3.14] | |||
# Meshes options | |||
scale: "0.001 0.001 0.001" | |||
forcelowercase: Yes | |||
filenameformatchangeext: "package://iCub/meshes/simmechanics/%s-binary.stl" | |||
@MESH_FOLDER@ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would actually call this @MESH_FILE_FORMAT@
instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, better!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've requested small changes. Please refer to the individual comments.
simmechanics/CMakeLists.txt
Outdated
sensorBlobs: | ||
- | | ||
<plugin name=\"root_link_xsens_imu_plugin\" filename=\"libgazebo_yarp_imu.so\"> | ||
<yarpConfigurationFile>model://iCub/conf/gazebo_icub_inertial.ini</yarpConfigurationFile> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nunoguedelha do we need also an additional configuration file for the new IMU?
E.g. gazebo_icub_xsens_inertial.ini
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, considering that the parameters will be different:
- period: 10ms -> 2.5ms (because of the rate change to 400Hz)
- different yarp port name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updated!
Thanks @nunoguedelha, I modified the files as you suggested. Can you have a look at my comment about the configuration file? |
Added iCub2.5+ models: for Gazebo and a model for iCubGenova04_plus.