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

ControlBoardDriver: add iJointCoupling interface #221

Merged
merged 11 commits into from
Dec 16, 2024
Merged

Conversation

Nicogene
Copy link
Member

@Nicogene Nicogene commented Dec 2, 2024

This PR adds the iJointCoupling interface, that allows to handle coupled systems using YARP device driver such as:

@Nicogene

This comment was marked as outdated.

@Nicogene Nicogene force-pushed the feat/addJointCoupling branch from 4ffd1ad to 27b52f9 Compare December 12, 2024 14:53
@Nicogene
Copy link
Member Author

Yesterday @traversaro @xela-95 and I had an alignment, and we decided to deeply refactor the plugin in order to make cleaner the introduction of the coupling.

I managed to have a first version that compiles, it is already a success given how many things I changed :D

I will now test it using the current configuration files of ergocub (no coupling involved), in case of positive outcome I will move to test it with the coupling of the hands mk5.x.

@traversaro
Copy link
Member

If you want a more self-contained test with a smaller model w.r.t. to ergocub, it seems that also the single_pendulum tests is currently failing.

@Nicogene
Copy link
Member Author

If you want a more self-contained test with a smaller model w.r.t. to ergocub, it seems that also the single_pendulum tests is currently failing.

Thanks for the heads up! I used that test for debugging it and I managed to find that specific error, one controlboard test is still failing I am not sure if it is due to my changes

@Nicogene
Copy link
Member Author

I managed to fix:

  • limits
  • names

for the actuated axes, I am still getting a segmentation fault on ControlBoardCommonsTest

At first glance, ergocub seems controllable with my changes
image

@traversaro
Copy link
Member

for the actuated axes, I am still getting a segmentation fault on ControlBoardCommonsTest

Are you sure that you do not have some stale or dirty file in your system, for example some old installation? The ControlBoardCommonsTest test passes fine in CI.

@Nicogene
Copy link
Member Author

The coupling seems to work just fine

image

@traversaro
Copy link
Member

traversaro commented Dec 16, 2024

Great! Not blocking, but most of the (implicit) documentation for plugins is given in the tutorial folder in form of examples for each plugin. To have an example for the coupling functionality, we could port https://github.com/robotology/gazebo-yarp-plugins/tree/master/tutorial/model/coupled_pendulum (and obviously it will start depending on icub-main). That will provide also a simple smoke test for the coupling functionality in

.

@Nicogene
Copy link
Member Author

I noticed that enabling the control of the hands make drop the RTF of a ~ 20%, this can be noticed also in the screenshots I posted:

I have also checked the RTF on main and it is ~ 78% so it seems not that my changes slowed down the simulation, probably the more joints to control make the simulation slower

cc @xela-95 @traversaro

@Nicogene
Copy link
Member Author

Great! Not blocking, but most of the (implicit) documentation for plugins is given in the tutorial folder in form of examples for each plugin. To have an example for the coupling functionality, we could port robotology/gazebo-yarp-plugins@master/tutorial/model/coupled_pendulum (and obviously it will start depending on icub-main). That will provide also a simple smoke test for the coupling functionality in


.

Ok for me! But we need first to merge the coupling device PR in icub-main:

@Nicogene Nicogene marked this pull request as ready for review December 16, 2024 11:22
@Nicogene Nicogene requested review from xela-95 and removed request for xela-95 December 16, 2024 11:37
@traversaro
Copy link
Member

Great! Not blocking, but most of the (implicit) documentation for plugins is given in the tutorial folder in form of examples for each plugin. To have an example for the coupling functionality, we could port robotology/gazebo-yarp-plugins@master/tutorial/model/coupled_pendulum (and obviously it will start depending on icub-main). That will provide also a simple smoke test for the coupling functionality in

.

Ok for me! But we need first to merge the coupling device PR in icub-main:

* [Add `couplingICubEye` device icub-main#997](https://github.com/robotology/icub-main/pull/997)

Sure! We can also just add tutorial and then add the tests once robotology/icub-main#997 is merged.

@Nicogene
Copy link
Member Author

Nicogene commented Dec 16, 2024

Great! Not blocking, but most of the (implicit) documentation for plugins is given in the tutorial folder in form of examples for each plugin. To have an example for the coupling functionality, we could port robotology/gazebo-yarp-plugins@master/tutorial/model/coupled_pendulum (and obviously it will start depending on icub-main). That will provide also a simple smoke test for the coupling functionality in

.

Ok for me! But we need first to merge the coupling device PR in icub-main:

* [Add `couplingICubEye` device icub-main#997](https://github.com/robotology/icub-main/pull/997)

Sure! We can also just add tutorial and then add the tests once robotology/icub-main#997 is merged.

I would prefer to add the tutorial after merging robotology/icub-main#997 because I am not 100% about the configuration file for that coupling, I can add the tutorial but it is possible that the configuration files has to be revised after testing the coupling of the eyes.

Our plan is to test the coupling devices of icub once this PR is merged (cc @martinaxgloria)

@traversaro
Copy link
Member

Ok, no problem for me!

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.

Minor comments, the one that I care more is to avoid to change the content of m_pluginParameters

Copy link
Member

@xela-95 xela-95 left a comment

Choose a reason for hiding this comment

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

Thanks a lot @Nicogene for the great work! It looks good to me! 🚀

Maybe it could be useful to add the coupled pendulum tutorial as suggested by @traversaro both as an example for the user and as a smoke test for the coupling feature. But this can be done in another PR.

@Nicogene Nicogene force-pushed the feat/addJointCoupling branch 3 times, most recently from 8caedce to 84f634b Compare December 16, 2024 15:56
@Nicogene Nicogene force-pushed the feat/addJointCoupling branch from 84f634b to 90d2bd3 Compare December 16, 2024 16:03
@Nicogene Nicogene requested a review from traversaro December 16, 2024 16:03
@Nicogene
Copy link
Member Author

I should have addressed all your requested changes, and I re-tested the coupling after the changes

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.

Great, thanks a lot!

@traversaro
Copy link
Member

@xela-95 the PR for me is ready to merge once CI is happy. I guess we can merge with squash as the history is not curated.

@xela-95 xela-95 merged commit d6382c2 into main Dec 16, 2024
11 checks passed
@Nicogene Nicogene deleted the feat/addJointCoupling branch December 17, 2024 14:54
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.

3 participants