gazebo_yarp_controlboard: Add coupling icub_left_hand_mk4 to model iCub mk4 left hand #620

mfussi66 · 2022-04-01T15:31:53Z

This PR aims to ~~update the coupling rules of the mk3 hand with the new mk4 version given that the former is now obsolete~~ add new coupling rules for the new iCub hand mk4, while keeping intact the mk3 version until declared obsolete.
The hand mk4 has 5 fingers, causing an increase in the number of joints. In particular, the ring and pinkie fingers move together, since they are physically actuated by one tendon only.

The coupling laws that map the degrees of actuation into model joints are kept to the mk3 version, although they need updating since the geometric quantities have changed.

Reference documentation on the subject:
FingerkinV2.docx

…assignment to new finger

traversaro · 2022-04-04T09:09:14Z

plugins/controlboard/src/ControlBoardDriver.cpp

@@ -326,7 +327,7 @@ bool GazeboYarpControlBoardDriver::gazebo_init()
            }
            else if (coupling_bottle->get(0).asString()=="icub_hand_mk3")
            {
-                BaseCouplingHandler* cpl = new HandMk3CouplingHandler(m_robot,coupled_joints, coupled_joint_names, coupled_joint_limits);
+                BaseCouplingHandler* cpl = new HandMk4CouplingHandler(m_robot,coupled_joints, coupled_joint_names, coupled_joint_limits);


This piece of code uses the HandMk4CouplingHandler class if the coupling name asked is icub_hand_mk3, I am afraid this may be confusing, what do you think? See also the subsquent message yCInfo(GAZEBOCONTROLBOARD) << "using icub_hand_mk3";. Perhaps we could have a specific coupling name for this?

I kept the naming icub_hand_mk3 to be back-compatible on the user-configuration level, but I agree, it might be better to give it a specific name and separate things and maybe keep both classes for the time being.

Yes, I think in any case you would not be able to be back-compatible as the number of joints is different, right?

traversaro · 2022-04-04T09:13:41Z

Some comments on the top of the one that I did online.

Do you have a reference for the coupling laws? For the mantainability it would be great if the coupling laws are documented (i.e. written down) somewhere, and then the gazebo plugins just implement the laws documented somewhere else. This is not a strict requirement if it takes too much time (in #469 we merged the mk3 coupling without any public reference) but it would be nice to have.

This PR aims to update the coupling rules of the mk3 hand with the new mk4 version, given that the former is now obsolete.

I am not sure we did any announcement on that coupling being obsolete in any changelog. Furthermore, that coupling is still used in the model left_hand_mk3 in icub-models (see https://github.com/robotology/icub-models/blob/v1.23.2/iCub/conf_left_hand_mk3/left_hand_phys.ini#L15). I am totally ok on removing the icub_hand_mk3 coupling, just I would do it in a PR different from this one and also removing the left_hand_mk3 model from icub-models, so that we can do at least a release in which we announce that we will remove icub_hand_mk3.

traversaro · 2022-04-04T09:14:54Z

By the way, I was wondering: the coupling is the same for the left and right hand, or is this a left-specific coupling?

pattacini · 2022-04-04T09:22:51Z

Hi @traversaro

Do you have a reference for the coupling laws? For the mantainability it would be great if the coupling laws are documented (i.e. written down) somewhere, and then the gazebo plugins just implement the laws documented somewhere else. This is not a strict requirement if it takes too much time (in #469 we merged the mk3 coupling without any public reference) but it would be nice to have.

This is a good point indeed, but I'd call for someone from the Team DEV (@Andrea-Mor, @andreaderito) to address it, as we're not the original developers.

mfussi66 · 2022-04-04T09:27:04Z

Do you have a reference for the coupling laws? For the mantainability it would be great if the coupling laws are documented (i.e. written down) somewhere, and then the gazebo plugins just implement the laws documented somewhere else. This is not a strict requirement if it takes too much time (in #469 we merged the mk3 coupling without any public reference) but it would be nice to have.

The only reference I have is the document posted in #469, I'll attach it in the PR post, at least until proper documentation on the subject is done, if it's ok.

This PR aims to update the coupling rules of the mk3 hand with the new mk4 version, given that the former is now obsolete.

I am not sure we did any announcement on that coupling being obsolete in any changelog.

My bad! Correcting the PR post now.

I am totally ok on removing the icub_hand_mk3 coupling, just I would do it in a PR different from this one and also removing the left_hand_mk3 model from icub-models, so that we can do at least a release in which we announce that we will remove icub_hand_mk3.

Agreed! I will add a commit to add a new HandMk4 class and keep HandMk3 for the time being.

traversaro · 2022-04-04T09:38:22Z

The only reference I have is the document posted in #469, I'll attach it in the PR post, at least until proper documentation on the subject is done, if it's ok.

Sure, as mention this is just a nice to have, not something that is blocking the PR.

traversaro · 2022-04-04T10:00:57Z

@mfussi66 I updated the CHANGELOG and PR title, let me know if you think this is ok. The only remaining point is #620 (comment), do you have any idea on that? If the coupling is left-specific, probably it would be nice to already use a left-specific name for the coupling.

mfussi66 · 2022-04-04T10:07:36Z

do you have any idea on that? If the coupling is left-specific, probably it would be nice to already use a left-specific name for the coupling.

@traversaro Honestly I did not think about that aspect, my bad. Of course the left and right hands will be mirrored, therefore some quantities' signs might change in the coupling laws.
Maybe it's better to have a left-specific name, at this point even in the PR title and the changelog for added clarity.

So I guess the class name should change too?

traversaro · 2022-04-04T10:09:50Z

do you have any idea on that? If the coupling is left-specific, probably it would be nice to already use a left-specific name for the coupling.

@traversaro Honestly I did not think about that aspect, my bad. Of course the left and right hands will be mirrored, therefore some quantities' signs might change in the coupling laws. Maybe it's better to have a left-specific name, at this point even in the PR title and the changelog for added clarity.

Thanks, yes at least for the parameter name at this point I would then use a left-specific name, for all the rest of the names (for example the class) we can use the existing one as they can always be changed later.

traversaro · 2022-04-04T12:15:07Z

Ok for me! @mfussi66 it is ok for you if I merge with squash to get a clean history?

mfussi66 · 2022-04-04T12:15:50Z

Ok for me! @mfussi66 it is ok for you if I merge with squash to get a clean history?

@traversaro even better!

mfussi66 added 3 commits April 1, 2022 17:09

improved print clarity

f68b2e1

Increased number of joints to account for ring finger, copy coupling …

398a887

…assignment to new finger

Update class name to mk4

2938254

traversaro closed this Apr 4, 2022

traversaro reopened this Apr 4, 2022

Add LUT for pinkie

b08ba9a

traversaro reviewed Apr 4, 2022

View reviewed changes

traversaro mentioned this pull request Apr 4, 2022

Deprecate left_wrist_mk2 and left_hand_mk3 and remove them in next release? robotology/icub-models#147

Open

mfussi66 and others added 2 commits April 4, 2022 11:54

Keep mk3 hand coupling instead of replacing it

416cc2a

Update CHANGELOG.md

bb5d961

traversaro changed the title ~~Replace coupling of Mk3 hand with Mk4 version~~ gazebo_yarp_controlboard: Add coupling icub_hand_mk4 to model iCub mk4 hand Apr 4, 2022

Specify left hand mk4 in changelog

371f7e7

mfussi66 changed the title ~~gazebo_yarp_controlboard: Add coupling icub_hand_mk4 to model iCub mk4 hand~~ gazebo_yarp_controlboard: Add coupling icub_left_hand_mk4 to model iCub mk4 left hand Apr 4, 2022

specify left for hand mk4

ce402cc

mfussi66 requested a review from traversaro April 4, 2022 11:38

traversaro approved these changes Apr 4, 2022

View reviewed changes

traversaro merged commit d9d1ff2 into robotology:devel Apr 4, 2022

traversaro mentioned this pull request Jan 11, 2023

ControlBoardDriverCoupling: add couplings for hand mk5 #641

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gazebo_yarp_controlboard: Add coupling icub_left_hand_mk4 to model iCub mk4 left hand #620

gazebo_yarp_controlboard: Add coupling icub_left_hand_mk4 to model iCub mk4 left hand #620

mfussi66 commented Apr 1, 2022 •

edited by traversaro

Loading

traversaro Apr 4, 2022

mfussi66 Apr 4, 2022

traversaro Apr 4, 2022

traversaro commented Apr 4, 2022 •

edited

Loading

traversaro commented Apr 4, 2022

pattacini commented Apr 4, 2022

mfussi66 commented Apr 4, 2022 •

edited

Loading

traversaro commented Apr 4, 2022

traversaro commented Apr 4, 2022

mfussi66 commented Apr 4, 2022 •

edited

Loading

traversaro commented Apr 4, 2022

traversaro commented Apr 4, 2022

mfussi66 commented Apr 4, 2022

gazebo_yarp_controlboard: Add coupling icub_left_hand_mk4 to model iCub mk4 left hand #620

gazebo_yarp_controlboard: Add coupling icub_left_hand_mk4 to model iCub mk4 left hand #620

Conversation

mfussi66 commented Apr 1, 2022 • edited by traversaro Loading

traversaro Apr 4, 2022

Choose a reason for hiding this comment

mfussi66 Apr 4, 2022

Choose a reason for hiding this comment

traversaro Apr 4, 2022

Choose a reason for hiding this comment

traversaro commented Apr 4, 2022 • edited Loading

traversaro commented Apr 4, 2022

pattacini commented Apr 4, 2022

mfussi66 commented Apr 4, 2022 • edited Loading

traversaro commented Apr 4, 2022

traversaro commented Apr 4, 2022

mfussi66 commented Apr 4, 2022 • edited Loading

traversaro commented Apr 4, 2022

traversaro commented Apr 4, 2022

mfussi66 commented Apr 4, 2022

mfussi66 commented Apr 1, 2022 •

edited by traversaro

Loading

traversaro commented Apr 4, 2022 •

edited

Loading

mfussi66 commented Apr 4, 2022 •

edited

Loading

mfussi66 commented Apr 4, 2022 •

edited

Loading