-
Notifications
You must be signed in to change notification settings - Fork 48
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
HandMk5CouplingHandler: refactor it in order to make it configurable #662
Conversation
In my opinion, I would leave them all just for clarity also considering that in the code they are easily handled within a for cycle and just once at configuration time. I will review the PR within today and tomorrow. |
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 @Nicogene. I have added some comments for requested changes.
ad0b3a8
to
c494009
Compare
As suggested by @xEnVrE, with the usage of a map, we got rid of some functions that are now useless |
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 @Nicogene, I added some comments for requested changes. It seems that at the moment the code was not using the values for the middle and the ring and still using those of the index for them.
@xEnVrE for the pinkie/pinky I am a little confused what is the correct name, in the documentation there is a pinkie In any case it is ok for me if we prefer pinky |
Then there is a bit of a mix. In this part of the documentation:
|
5874f8c
to
794a672
Compare
794a672
to
196eef9
Compare
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.
LGTM! Thanks @Nicogene
Important
We need to wait for icub-tech-iit/ergocub-software#169 to be ready for merging I guess.
@traversaro I think we can ignore the failures, they seems strange conda/windows failures that seems unrelated
I saw it also in other repositories ( |
if (disable_wrapper && !m_parameters.check("yarpDeviceName")) | ||
{ | ||
yError() << "GazeboYarpControlBoard : missing yarpDeviceName parameter for one device in robot " << m_robotName; | ||
return; | ||
} | ||
|
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.
For the future, please avoid functional and whitespace changes in the same commit, thanks!
@Nicogene can you add a note in the CHANGELOG, also considering that (if I am not wrong) this change breaks old mk5 simulations, so there is a strict dependency between future gyp releases and ergocub-software releases. |
See #664 . If you see this kind of environmental regression, please report them in new issues. Unfortunately in GitHub actions only the last person that modified the ci files is getting notification of CI failures (see #664), so it is not possible for me or any other mantainer to get all possible notifications. |
It fixes #661
This is a first draft, I thoght that the configuration file will become:
Right now I am asking to specify all the coefficient for all the fingers, but @xEnVrE said that only thumb index and pinky is required, maybe I can ask only those?
cc @pattacini @traversaro