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

Created CMM Simulink Block #194

Merged

Conversation

CarlottaSartore
Copy link
Contributor

As commented in issue #20 , This PR contains the implementation of the Simulink block calling the iDynTree::KinDynComputations::getCentroidalTotalMomentumJacobian() function implemented by @GiulioRomualdi .

C.C. @traversaro @gabrielenava

@CarlottaSartore
Copy link
Contributor Author

I saw that the CI failed, I have tried again the compilation from a brand new build and everything worked fine.

I checked the output of the CI tests and I saw that the problem is in the iDynTree dependency i.e.

../toolbox/library/src/CMM.cpp:176:18: error: 'using element_type = class iDynTree::KinDynComputations {aka class iDynTree::KinDynComputations}' has no member named 'getCentroidalTotalMomentumJacobian'; did you mean 'getCentroidalTotalMomentum'?
     ok = kinDyn->getCentroidalTotalMomentumJacobian(pImpl->CentroidalTotalMomentumMatrix);
                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
                  getCentroidalTotalMomentum
[26/30] Building CXX object toolbox/library/CMakeFiles/WBToolbox.dir/src/DiscreteFilter.cpp.o

Which branch is the CI using ? is it possible that the error is due to "wrong" iDynTree version?

In my configuration I have iDynTree branch devel commit 03a667417dcc4f5ac2ec881bad99cc24cea24947.

@diegoferigo
Copy link
Member

I think that the images in https://github.com/robotology-playground/docker_images should be updated, if we're lucky we just need to re-run its CI. Let's wait and then retrigger the tests of this PR.

Copy link
Member

@diegoferigo diegoferigo left a comment

Choose a reason for hiding this comment

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

Thanks @CarlottaSartore for your contribution! It looks good to me apart for some nits. Just checking, did you export the mdl and slx for the correct Matlab version we support?

@@ -38,6 +38,7 @@ list(APPEND WBT_BLOCKS "Jacobian")
list(APPEND WBT_BLOCKS "ForwardKinematics")
list(APPEND WBT_BLOCKS "RelativeTransform")
list(APPEND WBT_BLOCKS "CentroidalMomentum")
list(APPEND WBT_BLOCKS "CMM")
Copy link
Member

Choose a reason for hiding this comment

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

I would extend CMM to CentroidalTotalMomentumMatrix pretty much everywhere (class names, files names, etc)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perfect, I will apply this change right away!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 95fa7bc

Comment on lines 63 to 65



Copy link
Member

Choose a reason for hiding this comment

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

I don't think that the clang-format of this repo allows multiple blank lines. Are you sure you're using it? If not, can you update the style of all the C++ file with that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes sure !

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in commit 95fa7bc

@CarlottaSartore
Copy link
Contributor Author

CarlottaSartore commented Jul 22, 2020

@diegoferigo Thank you for your review!

did you export the mdl and slx for the correct Matlab version we support?

For exporting the slx I have used the export_library script (i.e. it is 2014b), instead, the mdl version is 2019b, which Matlab version is the default one? 2014b ?

@traversaro
Copy link
Member

Just to understand, @CarlottaSartore you have addressed all the reviews of @diegoferigo, right?

@CarlottaSartore
Copy link
Contributor Author

Just to understand, @CarlottaSartore you have addressed all the reviews of @diegoferigo, right?

Yes, the only thing missing, to the best of my knowledge, regards the version of Matlab as per the following comment:

For exporting the slx I have used the export_library script (i.e. it is 2014b), instead, the mdl version is 2019b, as Matlab version is the default one? 2014b ?

The CI tests are not passing but it could due to #194 (comment)

@diegoferigo
Copy link
Member

diegoferigo commented Oct 7, 2020

For exporting the slx I have used the export_library script (i.e. it is 2014b), instead, the mdl version is 2019b, which Matlab version is the default one? 2014b ?

I think that executing the export_library CMake target should suffice, it already takes care of using the right version of Matlab:

# The logic about library development is the following:
#
# * Developers should edit the library (e.g. changing block's masks) stored in the
# matlab/library/WBToolboxLibrary_repository.mdl file.
# * Developers should export the changes using the export_library custom target.
# * Users will use the files installed from the exported/ folder.

For what concerns CI, I triggered the generation of the updated Docker images, let's see if they succeed. The pipeline is a bit rusty lately, I had not much time to dedicate and there was no one else that was maintaining it. Let's see if the building process succeeds, it's gonna take a while.

@diegoferigo
Copy link
Member

diegoferigo commented Oct 8, 2020

@CarlottaSartore Can you please rebase this PR on top of current devel? CI should be fixed now.

@CarlottaSartore CarlottaSartore force-pushed the feature/CreateCMMSimulinkBlock branch from 05a59e0 to 16aae9b Compare October 9, 2020 10:01
@CarlottaSartore
Copy link
Contributor Author

@diegoferigo I have updated all the file with the clang-format (using QT), I have rebase on top of devel and fix one leftover. of the reanming in the simulink library.
The version of the WBToolboxLibrary.slx (inside the exported folder) is R2014b and has been created using the export_library script.

@diegoferigo
Copy link
Member

diegoferigo commented Oct 9, 2020

Perfect, thanks for your contribution @CarlottaSartore, it looks good! A last nit, can you please squash together the two commits with the slx file? Minimizing the changes to binary files is very important to avoid the size of the git project to diverge :) Otherwise, I can squash and merge this PR. Let me know what you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants