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

Implement KinDynComputations::getCentroidalTotalMomentumJacobian() method #706

Merged
merged 7 commits into from
Jun 29, 2020

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Jun 27, 2020

This PR introduces the getCentroidalTotalMomentumJacobian() function in KinDynComputations class.
I also wrote a simple test to spot some bugs in the method

Associated issue robotology/wb-toolbox#20

cc @traversaro @robotology/iit-dynamic-interaction-control

@GiulioRomualdi
Copy link
Member Author

The test that I've developed is failing on macOS. Since I don't have an apple machine I don't know how to find the bug. Any hints?

@GiulioRomualdi
Copy link
Member Author

f9df8a0 fixes the problems on macOS. The PR is now ready to be reviewed.

src/high-level/src/KinDynComputations.cpp Outdated Show resolved Hide resolved
Copy link
Member

@traversaro traversaro left a comment

Choose a reason for hiding this comment

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

A minor comment.

@traversaro
Copy link
Member

Small note: the current devel branch is going to be eventually released as iDynTree 2 due to the breaking changes, but I guess it will not be released before August. If you aim for a quick release of this feature that is back-compatible, feel free to cherry-pick the changes to the master branches, so it can be released in a iDynTree 1.2 release even sooner. However if you don't have any urgency of having this change in a iDynTree released version, feel free to keep it against devel as it is.

@traversaro
Copy link
Member

@Yeshasvitvs as I guess you were interested in this, feel free to check this PR as well!

@GiulioRomualdi
Copy link
Member Author

Small note: the current devel branch is going to be eventually released as iDynTree 2 due to the breaking changes, but I guess it will not be released before August. If you aim for a quick release of this feature that is back-compatible, feel free to cherry-pick the changes to the master branches, so it can be released in a iDynTree 1.2 release even sooner. However if you don't have any urgency of having this change in a iDynTree released version, feel free to keep it against devel as it is.

For the time being, I don't need this feature. Probably @Giulero @gabrielenava or @CarlottaSartore may need it in the next future

@GiulioRomualdi
Copy link
Member Author

@traversaro, regarding the CHANGELOG.md, shall I add the feature in [Unreleased Major] or [Unreleased]?

@traversaro
Copy link
Member

@traversaro, regarding the CHANGELOG.md, shall I add the feature in [Unreleased Major] or [Unreleased]?

Use [Unreleased Major], thanks! [Unreleased] is used for the changes happening on the master branch, and to avoid conflicts anytime I forward merge changes from the master to the devel.

@traversaro
Copy link
Member

Thanks a lot @GiulioRomualdi ! Let me know if you prefer to curate the commit yourself, or if it is ok that I merge with squash.

@GiulioRomualdi
Copy link
Member Author

Merge with squash it's ok with me. Thank you!

@gabrielenava
Copy link
Contributor

gabrielenava commented Jun 29, 2020

For the time being, I don't need this feature. Probably @Giulero @gabrielenava or @CarlottaSartore may need it in the next future

In the iRonCub team we use an homemade centroidal momentum matrix block which is actually overcomplicated for the task, but for the moment it works. Also at the moment we use iDynTree in master branch, but in case of necessity it is ok for us to migrate to devel if no big changes are required at the Simulink level.

So it is ok to wait for August for what concerns iRonCub team.

@traversaro traversaro merged commit c29e23f into robotology:devel Jun 29, 2020
@traversaro
Copy link
Member

Thanks @GiulioRomualdi !

@GiulioRomualdi GiulioRomualdi deleted the feature/cmm branch June 29, 2020 08:56
@CarlottaSartore
Copy link

For the time being, I don't need this feature. Probably @Giulero @gabrielenava or @CarlottaSartore may need it in the next future

I do not see a problem for me to stay in devel rather than Master! Thank you @GiulioRomualdi

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.

5 participants