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

Fix handling of limbs versions in iKin #839

Merged
merged 6 commits into from
Dec 5, 2022
Merged

Conversation

pattacini
Copy link
Member

@pattacini pattacini commented Oct 24, 2022

This PR follows up on comments reported in #838 for which limbs version 2.10 is handled equally to 2.1 in iKin, for example.

To remedy this, a dedicated class iKinLimbVersion is introduced and thus all the related components have been updated.

This is an old flaw that emerges now as we have crafted the new iCub version 2.10.

The gaze and cartesian controller client/server versions have been upgraded to 2.0 to notify a breaking change in the way getInfo() operates for the hardware-version1.

Careful tests need to be done.

Footnotes

  1. Other code that needs to be upgraded:

@pattacini pattacini self-assigned this Oct 24, 2022
@pattacini pattacini force-pushed the fix/ikin-versioning branch from 13c8d30 to e733b15 Compare October 24, 2022 22:00
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.

At first glance seems ok 👍🏻

@pattacini pattacini force-pushed the fix/ikin-versioning branch from e733b15 to 86c3f06 Compare October 25, 2022 08:29
@pattacini
Copy link
Member Author

Before doing specific tests on the robot, I'll be verifying this solution in https://github.com/pattacini/icub-gazebo-grasping-sandbox.

@pattacini pattacini force-pushed the fix/ikin-versioning branch 4 times, most recently from e6179ba to 27348b6 Compare October 28, 2022 11:00
@pattacini
Copy link
Member Author

pattacini commented Nov 13, 2022

Before doing specific tests on the robot, I'll be verifying this solution in https://github.com/pattacini/icub-gazebo-grasping-sandbox.

After various fixes, I managed to make it work. See the result below:

2022-11-13.16-29-12.mp4

For the record, here's enclosed the patch file to adapt the sandbox: master...pattacini_icub-gazebo-grasping-sandbox_master.patch.txt 📝

Verifying that the sandbox works as intended is not sufficient:

  • The next move is to assess the PR on a physical robot.
  • Then, we're required to update the configuration files for the iKinGazeCtrl module plus the other 2 downstream projects.

@pattacini
Copy link
Member Author

pattacini commented Nov 13, 2022

  • Then, we're required to update the configuration files for the iKinGazeCtrl module plus the other 2 downstream projects.

See:

@xEnVrE
Copy link
Contributor

xEnVrE commented Nov 13, 2022

In realsense-holder-calibration, that is linked in https://icub-tech-iit.github.io/documentation/upgrade_kits/realsense_holder/support/#calibration-of-the-realsense-holder, we also have a snippet of code of the form

icub_head_center_ = iCub::iKin::iCubHeadCenter("right_" + head_version);

Does that need the same kind of patch as in stereo-vision?

@pattacini
Copy link
Member Author

pattacini commented Nov 13, 2022

Does that need the same kind of patch as in stereo-vision?

At first sight, I think it's ok as that module employs the conf param eye_version, which contains the "v" char and there doesn't seem to be any version comparison.
See:

At any rate, better off making a deeper inspection.

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.

SInce it is a breaking change I would increment the patch number, in this way we could set the new icub-main version as minimum required for the downstream code that uses iKin.

@pattacini
Copy link
Member Author

SInce it is a breaking change I would increment the patch number, in this way we could set the new icub-main version as minimum required for the downstream code that uses iKin.

Actually, I was thinking to increment the major number, right because it's a breaking change.

@pattacini
Copy link
Member Author

pattacini commented Nov 14, 2022

Actually, I was thinking to increment the major number, right because it's a breaking change.

Done via b6a0792, @Nicogene!

@pattacini pattacini requested a review from Nicogene November 14, 2022 09:37
@pattacini pattacini force-pushed the fix/ikin-versioning branch 2 times, most recently from a0b43f3 to 75c32bb Compare November 15, 2022 15:53
@pattacini pattacini force-pushed the fix/ikin-versioning branch from fe326a4 to b9a1ed0 Compare December 5, 2022 14:25
@pattacini
Copy link
Member Author

@mfussi66 and @AntonioConsilvio checked the correct behavior of the Red Ball demo!

demoredballrendered.mp4

@pattacini pattacini merged commit 1be6920 into devel Dec 5, 2022
@pattacini pattacini deleted the fix/ikin-versioning branch December 5, 2022 15:50
@pattacini
Copy link
Member Author

  • Then, we're required to update the configuration files for the iKinGazeCtrl module plus the other 2 downstream projects.

See:

I've merged the PR's above too.

cc @Nicogene

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