Add legs FT IMU frames

[traversaro]

See #113 .

[traversaro]

Now the cI tests seems to be correctly working. There is a test failure:

Test project /home/travis/build/robotology/icub-model-generator/build

      Start  1: iCubGenova03ConsistencyCheck

 1/11 Test  #1: iCubGenova03ConsistencyCheck ..........   Passed    0.02 sec

      Start  2: iCubNancy01ConsistencyCheck

 2/11 Test  #2: iCubNancy01ConsistencyCheck ...........   Passed    0.02 sec

      Start  3: iCubParis01ConsistencyCheck

 3/11 Test  #3: iCubParis01ConsistencyCheck ...........   Passed    0.02 sec

      Start  4: iCubParis02ConsistencyCheck

 4/11 Test  #4: iCubParis02ConsistencyCheck ...........   Passed    0.02 sec

      Start  5: iCubLisboa01ConsistencyCheck

 5/11 Test  #5: iCubLisboa01ConsistencyCheck ..........   Passed    0.02 sec

      Start  6: iCubGazeboV2_5ConsistencyCheck

 6/11 Test  #6: iCubGazeboV2_5ConsistencyCheck ........***Failed    0.02 sec

icub-model-test : base_link test performed correctly 

icub-model-test error: l_sole_x is 0.0072817, while r_sole_x is 0.0073878 (diff : 0.0001061 )

      Start  7: iCubGazeboV2_5_plusConsistencyCheck

 7/11 Test  #7: iCubGazeboV2_5_plusConsistencyCheck ...   Passed    0.03 sec

      Start  8: iCubGenova01ConsistencyCheck

 8/11 Test  #8: iCubGenova01ConsistencyCheck ..........***Failed    0.02 sec

icub-model-test : base_link test performed correctly 

icub-model-test error: l_sole_x is 0.0072817, while r_sole_x is 0.0073878 (diff : 0.0001061 )

      Start  9: iCubGenova02ConsistencyCheck

 9/11 Test  #9: iCubGenova02ConsistencyCheck ..........   Passed    0.03 sec

      Start 10: iCubGenova04ConsistencyCheck

10/11 Test #10: iCubGenova04ConsistencyCheck ..........***Failed    0.02 sec

icub-model-test : base_link test performed correctly 

icub-model-test error: l_sole_x is 0.0072817, while r_sole_x is 0.0073878 (diff : 0.0001061 )

      Start 11: iCubDarmstadt01ConsistencyCheck

11/11 Test #11: iCubDarmstadt01ConsistencyCheck .......***Failed    0.02 sec

icub-model-test : base_link test performed correctly 

icub-model-test error: l_sole_x is 0.0072817, while r_sole_x is 0.0073878 (diff : 0.0001061 )

the error is valid (a 0.1 mm difference between r_sole and l_sole frames, probably hinting to some strange asymmetry in the model), but given the small difference we can just increase the tolerance in https://github.com/robotology/icub-model-generator/blob/bada6e53f0c874d80812a0b3cd146145d1ea85c9/tests/icub-model-test.cpp#L195 for now and open a issue for tracking this problem later.

[prashanthr05]

See https://github.com/dic-iit/component_wholebody-teleoperation/issues/190#issuecomment-546413251

ping @traversaro

[traversaro]

See dic-iit/component_wholebody-teleoperation#190 (comment)

ping @traversaro

Can you copy your message here? This is a public repo, and not everyone reading this issue/PR will have access to the repo that you linked, thanks!

[prashanthr05]

@traversaro Definitely.

@traversaro said,

Related icub-model-generator PR: #122 (comment) . If we tested the new models and this assimetry is not a problem, we can also merge the PR, increasing the tolerance of the test.

@prashanthr05 said,

Would walking and Yoga be a good test for checking the model asymmetry on the robot?

@fjandrad said,

Mmm I don't expect 0.1 mm to be an issue, but it is odd that it exist at all since we did not touch does frames right? @prashanthr05 . Anyway if you can run yoga and walking it's enough. Now it would be better if we can test directly a coherence of some estimation that actually uses the information from these IMU. But I guess that comes later. With yoga and walking we at least check we didn't break compatibility with working models.

@traversaro said,

Mmm I don't expect 0.1 mm to be an issue, but it is odd that it exist at all since we did not touch does frames right? @prashanthr05 .

I reported the problem to the mechanical team and they think it is probably a problem in the CAD models.

With yoga and walking we at least check we didn't break compatibility with working models.

Exactly, this is my first concern, if that is ok we can merge and then solve the 0.1 issue later.

@prashanthr05 said,

Tested both Yoga (on icub30) and walking(on icub-head) with the URDF model using the feet FT IMUs. The controllers seemed to be working fine.
We can open an issue about the asymmetry and merge the currently open PR.
However, I noticed that with the current PR, the root_link_imu_acc is still not added to the URDF model. (Reasons maybe, the imu is available only on iCub Greeny).

[traversaro]

CI is finally green, @prashanthr05 can you please approve? Thanks!

[traversaro]

Small note: as discussed in robotology/icub-models#20 (comment), merging this PR may change the orientation and location of the upper body frames. cc @kouroshD @GiulioRomualdi @S-Dafarra @Yeshasvitvs @lrapetti @MiladShafiee

[prashanthr05]

Small note: as discussed in robotology/icub-models#20 (comment), merging this PR may change the orientation and location of the upper body frames. cc @kouroshD @GiulioRomualdi @S-Dafarra @Yeshasvitvs @lrapetti @MiladShafiee

I hope none of the frame transformations are hard-coded anywhere? If that's the case, it's really bad practice and I strongly suggest to change it. Possible hard-codes should only be the frame names!

if this shouldn't be a blocking issue, I will proceed and approve the PR.

[traversaro]

I hope none of the frame transformations are hard-coded anywhere? If that's the case, it's really bad practice and I strongly suggest to change it. Possible hard-codes should only be the frame names!

We had a few discussion about this on the xsens-based retargeting, I think @kouroshD @Yeshasvitvs @lrapetti can provide more links or explain f2f .

[S-Dafarra]

I hope none of the frame transformations are hard-coded anywhere? If that's the case, it's really bad practice and I strongly suggest to change it. Possible hard-codes should only be the frame names!

They are not hardcoded, but you need to know how a frame is oriented to define desired values for example. An issue is given by the desired orientation of the torso in the walking (https://github.com/robotology/walking-controllers/blob/master/app/robots/iCubGenova04/dcm_walking/hand_retargeting/qpInverseKinematics.ini#L7) or how the oculus device gives the references. I am afraid this could be a major issue.

[traversaro]

They are not hardcoded, but you need to know how a frame is oriented to define desired values for example.

I think for the xsens-based retargeting there are indeed hardcoded in the human link <---> robot link transform.

[lrapetti]

They are not hardcoded, but you need to know how a frame is oriented to define desired values for example.

I think for the xsens-based retargeting there are indeed hardcoded in the human link <---> robot link transform.

Yes, they are "hardcoded" in the model teleoperation_iCub_model_V_2_5.urdf. This is not going to be an immediate problem since we will keep using that model and what are computed for the teleoperation are the joint angles. However, the model would not be updated, so as @traversaro is pointing out it would be better to find a more automate solution.

[yeshasvitirupachuri]

https://github.com/dic-iit/component_wholebody-teleoperation/issues/172#issuecomment-484034702

Relevant issue for automatic updated robot model generation from human to robot links

[traversaro]

dic-iit/component_wholebody-teleoperation#172 (comment)

Relevant issue for automatic updated robot model generation from human to robot links

Hi @Yeshasvitvs , as that issue is not public, I suggest to copy any info that you may find relevant directly here, thanks.

[traversaro]

Yes, they are "hardcoded" in the model teleoperation_iCub_model_V_2_5.urdf. This is not going to be an immediate problem since we will keep using that model and what are computed for the teleoperation are the joint angles.

Unfortunately that model (if I am not wrong) still uses the meshes from the icub-models's iCubGazeboV2_5, so if the link frames of that model change, the meshes will also change, and so the visualization for that model can break.

[lrapetti]

Unfortunately that model (if I am not wrong) still uses the meshes from the icub-models's iCubGazeboV2_5, so if the link frames of that model change, the meshes will also change, and so the visualization for that model can break.

yes, it is as you said, but as I was saying the visualization is achieved trough yarprobotstatepublisher, and the modified model is used just to compute the joint angles (for the visualization we can use the regular model, this works as long as the definition of the joints is the same one)

[traversaro]

Unless there are objections, I plan to merge this on the 20th of November. This will trigger an update of the icub-models repo.

[DanielePucci]

CC
@traversaro

[traversaro]

Unless there are objections, I plan to merge this on the 20th of November. This will trigger an update of the icub-models repo.

Obviously I had forgot about this. @prashanthr05 @MiladShafiee for the future, please ping me whenever you see that something is not happening as announced, thanks!

[traversaro]

To be honest, this PR still needs to be reviewed, so if you could review it @prashanthr05 @MiladShafiee @DanielePucci then we can merge, thanks!

[DanielePucci]

Please @prashanthr05, @nunoguedelha and @MiladShafiee provide code review. This is important, if we could do it today it would be great

[traversaro]

Just to clarify, even a single review is ok for the merge.

[prashanthr05]

I approve of the PR hoping that the consequent induced changes in the frame transformations on the upper body will not disrupt any workflow and keeping the changes minimal for specific user-cases like task-space control in walking described by @S-Dafarra here and in retargeting described by @lrapetti here, or more.

[prashanthr05]

I approved this PR based on our reviews of previous PRs (which were closed, changes accounted for in this PR) that lead to these changes. The major review was the error tolerance in the tests, that was causing the Travis to fail and also reverting back to stable versions of dependencies. However, since an issue remains open indicating these changes, we can proceed with the merge.

Previous PRs:

• Add legs FT IMU frames #111
• Add legs FT IMU frames - address review #113

[MiladShafiee]

Just to clarify, even a single review is ok for the merge.

I can review it and would you please guide me what should I check mostly? 😄

[traversaro]

Just to clarify, even a single review is ok for the merge.

I can review it and would you please guide me what should I check mostly? smile

Thanks, @prashanthr05 already reviewed. For the future, a good guide to code reviews is available at https://google.github.io/eng-practices/review/reviewer/ .

[nunoguedelha]

As a side note, I reviewed the changes anyway.

[nunoguedelha]

As second side, for the next times, it's recommended, when possible, to have the PR reviewed and approved by people that didn't participate in the changes.

[traversaro]

As second side, for the next times, it's recommended, when possible, to have the PR reviewed and approved by people that didn't participate in the changes.

I totally agree.