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

Add iCubGazeboV2_7 model #139

Merged
merged 6 commits into from
Nov 9, 2020
Merged

Add iCubGazeboV2_7 model #139

merged 6 commits into from
Nov 9, 2020

Conversation

traversaro
Copy link
Member

Fix for robotology/icub-models#49 . For now, the iCubGazeboV2_7 is just a iCubGazeboV2_5 with an additional frame and sensor to represent the XSens IMU mounted in the waist.

cc @GiulioRomualdi

<child link="root_link_imu_frame"/>
</joint>
- |
<gazebo reference=\"root_link\">
Copy link
Collaborator

@prashanthr05 prashanthr05 Sep 28, 2020

Choose a reason for hiding this comment

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

This should be base_link.

Please see the fix in prashanthr05/icub-models@d7e09b4

and a detailed comment in https://github.com/dic-iit/element_floating-base-estimation/issues/61#issuecomment-651695040

Posting the comment here if someone cannot access the private repository,

We need to understand how the Gazebo IMU considers the sensor transform to generate the measurements. Given the CAD frame associated with the root IMU, the measurements from the Gazebo should reflect the gravity only along the zy-plane, more inclined towards the y-axis.

Related to this,
In simulation, it was noticed that the measurements were not being published in the sensor local frame and it was always published with respect to the inertial frame. This was because the root_link_imu_acc sensor was referenced to root_link which is a massless link attached to the actual link base_link (base_link is the link actually identified by Gazebo). So this had lead to the measurements/orientations being published with respect to the root_link frame which had almost the same orientation as the Gazebo world except for the heading.

This bug was identified and fixed in,
prashanthr05/icub-models@d7e09b4

Now the root_link_imu_acc publishes measurements (accelerometer and gyroscope) in its local frame.

cc @MiladShafiee also faced similar issues in simulation. Seems like the gazebo measurements coming from the port /icubSim/xsens_inertial is wrong.
It streams orientation rpy =0.0, 0.0, 0.0 in the case of static home position, instead of streaming the rootlink_R_imu orientation.

However, the orientation still seems to be a problem.
The documentation here https://osrf-distributions.s3.amazonaws.com/gazebo/api/dev/classgazebo_1_1sensors_1_1ImuSensor.html#a66db2fd83d1d832184d888adc367115b says,

ignition::math::Quaterniond Orientation ( ) const
get orientation of the IMU relative to a reference pose Initially, the reference pose is the boot up pose of the IMU, but user can call either SetReferencePose to define current pose as the reference frame, or call SetWorldToReferencePose to define transform from world frame to reference frame.
returns the orientation quaternion of the IMU relative to the imu reference pose.

This might be related to gazebosim/gazebo-classic#1959

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhh, I am a bit confused here. base_link and root_link share the same orientation, so I don't understand why there should be a difference. I typically prefer to attach the sensor to root_link as the fixed link lumping works in a different way in iDynTree/URDF/Rviz, in which "root_link" is the real link, while "base_link" is the massless additional frame.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

@prashanthr05 prashanthr05 Sep 29, 2020

Choose a reason for hiding this comment

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

Mhh, I am a bit confused here. base_link and root_link share the same orientation, so I don't understand why there should be a difference. I typically prefer to attach the sensor to root_link as the fixed link lumping works in a different way in iDynTree/URDF/Rviz, in which "root_link" is the real link, while "base_link" is the massless additional frame.

Possibly because the sensor is not considering the massless link as its parent and the relative pose we specify is not used, thereby making the world become its parent?

Copy link
Collaborator

@prashanthr05 prashanthr05 Sep 29, 2020

Choose a reason for hiding this comment

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

However, note that the problem is already there for the iCubGazeboV2_5_plus in

Indeed, I think I copied the changes to iCubGazeboV2_5 on my fork from the iCubGazeboV2_5_plus. (or maybe @GiulioRomualdi did, and I started making modifications over it)

Copy link
Member Author

Choose a reason for hiding this comment

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

Mhh, I am a bit confused here. base_link and root_link share the same orientation, so I don't understand why there should be a difference. I typically prefer to attach the sensor to root_link as the fixed link lumping works in a different way in iDynTree/URDF/Rviz, in which "root_link" is the real link, while "base_link" is the massless additional frame.

Possibly because the sensor is not considering the massless link as its parent and the relative pose we specify is not used, thereby making the world become its parent?

I think there is a bug in the URDF to SDF conversion on this, see #140 .

Copy link
Collaborator

@prashanthr05 prashanthr05 Sep 29, 2020

Choose a reason for hiding this comment

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

So this might explain the problem with orientation as well described in #139 (comment). Possibly not.

Copy link
Member Author

Choose a reason for hiding this comment

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

So this might explain the problem with orientation as well described in #139 (comment). Possibly not.

Why not? I think it is exactly this the problem.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because even after changing the sensor's parent link to a lumped mass link (base_link), I remember having the problem with orientation measurements from the sensor. So I was thinking if it was more related to the initialization of IMU pose?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sorry, I referred indeed to the problem that was fixed by switching to using base_link as the reference. That seems indeed another problem that maybe is related to gazebosim/gazebo-classic#1959 .

@Nicogene
Copy link
Member

Nicogene commented Nov 9, 2020

I rebased this PR and fixed the model generation, is it ready to be merged?

@traversaro
Copy link
Member Author

I rebased this PR and fixed the model generation, is it ready to be merged?

Ok for me, once the CI is happy.

@Nicogene
Copy link
Member

Nicogene commented Nov 9, 2020

@prashanthr05 is it ok also for you?

@prashanthr05
Copy link
Collaborator

prashanthr05 commented Nov 9, 2020

@prashanthr05 is it ok also for you?

Ok for me too.

I think if the problem of Gazebo IMU with root_link/base_link confusion still persists, we can open a new PR since that might require changes in other models as well.

@Nicogene Nicogene merged commit c4c9f5a into master Nov 9, 2020
@Nicogene Nicogene deleted the add/icubv2_7 branch November 9, 2020 10:32
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