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

Add coupled_pendulum tutorial and tests #228

Merged
merged 6 commits into from
Feb 7, 2025

Conversation

martinaxgloria
Copy link
Contributor

@martinaxgloria martinaxgloria commented Jan 9, 2025

This PR adds the coupled_pendulum tutorial. Tested:

coupled_pendulum.mp4

It's a draft PR since the corresponding unit test is still missing.

cc @xela-95 @traversaro @Nicogene

CMakeLists.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,38 @@
# Run coupled pendulum tutorial in Gazebo

Copy link
Member

Choose a reason for hiding this comment

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

Somewhere here we should document that this model has a runtime dependency on icub-main.

@traversaro
Copy link
Member

It's a draft PR since the corresponding unit test is still missing.

If you do not have time to add a unit test, we can also just add a tutorial. Let's avoid that further enhancement prevent us to get improvements (see https://en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good).

@martinaxgloria
Copy link
Contributor Author

If you do not have time to add a unit test, we can also just add a tutorial. Let's avoid that further enhancement prevent us to get improvements (see en.wikipedia.org/wiki/Perfect_is_the_enemy_of_good).

Hi @traversaro, sorry for the delay. I'm going to add the unit test by the end of the day, at the latest by tomorrow if you're fine

@traversaro
Copy link
Member

No problem, take your time, I just was going through the open issues and PRs.

@martinaxgloria martinaxgloria marked this pull request as ready for review February 6, 2025 10:44
@martinaxgloria
Copy link
Contributor Author

Hi @traversaro, at the end I added the coupled pendulum within the tests of the controlBoard (both position and position direct) and I also added the test for checking that the tutorial runs without errors. Since the tutorial depends on icub-main, I protected it with an option that is ON if icub-main is found, OFF otherwise. Please, let me know if it is the intended behavior, thanks!

cc @Nicogene @xela-95

@traversaro
Copy link
Member

There is something I do not understand. The CheckPositionTrackingWithTrajectoryGenerationUsingPendulumModel depends as well from ICUB, but it is not disabled by the CMake option. Nevertheless the CI tests (where ICUB is not installed) do not pass. Can you double check what is going on?

Can you also modify the PR title to mention "Add coupled_pendulum tutorial and tests"?

option(ENABLE_COUPLED_PENDULUM_TEST "Enable coupled pendulum tests that require ICUB" ON)
else()
set(ENABLE_COUPLED_PENDULUM_TEST OFF)
endif()
Copy link
Member

Choose a reason for hiding this comment

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

I think this logic can be useful for all tests, so I think we can move it upper in the tests CMakeList.txt (see the comment there).

@xela-95 xela-95 added the enhancement New feature or request label Feb 6, 2025
tests/CMakeLists.txt Outdated Show resolved Hide resolved
@traversaro
Copy link
Member

For what regards testing:

  • The apt test requires to compile icub-main from source, so I would not enable icub-main tests there
  • The conda-forge-ci test using micromamba is mostly superseded by the pixi test, so I would not update it.
  • I think instead it make sense to add the icub-main dependency and the -DGZ_SIM_YARP_PLUGINS_ENABLE_TESTS_WITH_ICUB_MAIN:BOOL=ON option to the pixi project (this will be automatically then used in CI)

If you need any in person clarification feel free to ask me.

@martinaxgloria martinaxgloria changed the title Add coupled_pendulum tutorial Add coupled_pendulum tutorial and tests Feb 6, 2025
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.

Great! Thanks @martinaxgloria ! I have nothing to add to the @traversaro suggestions

@martinaxgloria martinaxgloria force-pushed the feat/addCoupledPendulum branch from e7b9cf0 to b6bb9a7 Compare February 6, 2025 13:29
@martinaxgloria martinaxgloria force-pushed the feat/addCoupledPendulum branch from 4a21ec5 to d5059d4 Compare February 6, 2025 14:01
@martinaxgloria
Copy link
Contributor Author

The CI is still failing due to:

Error:  |yarp.os.YarpPluginSettings| Cannot find "couplingICubEye" plugin (not built in, and no .ini file found for it)Check that YARP_DATA_DIRS leads to at least one directory with plugins/couplingICubEye.ini or share/yarp/plugins/couplingICubEye.ini in it

This is maybe because the couplingICubEye PR was done versus devel, so it doesn't appear within the master branch of icub-main. With @xela-95 we tried to add icub-main dependency in pixi.toml as:

icub-main = { git = "https://github.com/robotology/icub-main.git", rev = "devel"}

but pixi install failed locally with this error:

Error:
  × failed to parse project from /home/mgloria/iit/robotology-superbuild/src/gz-sim-yarp-plugins/pixi.toml: source
  │ dependencies are used in the feature 'default', but the `pixi-build` preview feature is not enabled

cc @traversaro

@traversaro
Copy link
Member

Good point! We still need to release icub-main with coupling support. pixi build support for icub-main would be cool to have at some point, but it will probably take a bit more time. At this point if we are not going to release icub-main in a short time, we can disable the test for now and enable it again once icub-main with coupling support is released?

@martinaxgloria
Copy link
Contributor Author

At this point if we are not going to release icub-main in a short time, we can disable the test for now and enable it again once icub-main with coupling support is released?

I don't know when the new tag will be released (cc @Nicogene @pattacini), but if you all agree for me it's ok to disable it until that moment

@pattacini
Copy link
Member

I don't know when the new tag will be released (cc @Nicogene @pattacini), but if you all agree for me it's ok to disable it until that moment

Discussed F2F w/ @traversaro; let's wait for the Distro 2025.02 to tag icub-main.

@martinaxgloria
Copy link
Contributor Author

Ok, since we are waiting for the distro to tag icub-main we have two possibilities:

  1. keep this PR open (I could put it in draft) until the new tag with the couplingICubEye will be released
  2. disabled the test so that we can close this PR and then open another one to re-enable it

@pattacini
Copy link
Member

  1. disabled the test so that we can close this PR and then open another one to re-enable it

Probably, option 2 is the best.

@traversaro
Copy link
Member

Thanks! Good to go for me once the PR is happy. We will address eventual test problems once we enable the tests.

@xela-95
Copy link
Member

xela-95 commented Feb 7, 2025

Thanks! Merging then 🚀

@xela-95 xela-95 merged commit ea02b61 into robotology:main Feb 7, 2025
13 checks passed
@martinaxgloria martinaxgloria deleted the feat/addCoupledPendulum branch February 7, 2025 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants