robotology / gz-sim-yarp-plugins

YARP plugins for Modern Gazebo (gz-sim).
BSD 3-Clause "New" or "Revised" License
8 stars 0 forks source link

Add test for forcetorque plugin #30

Closed lucapa17 closed 11 months ago

lucapa17 commented 11 months ago

Fix #20

traversaro commented 11 months ago

Thanks @lucapa17, some preliminary comments:

traversaro commented 11 months ago

Can you check out the conflicts with the main branch?

traversaro commented 11 months ago

The test is failing, there are two errors:

First Error

unning main() from /home/runner/work/gz-yarp-plugins/gz-yarp-plugins/build/_deps/googletest-src/googletest/src/gtest_main.cc
[==========] Running 1 test from 1 test suite.
[----------] Global test environment set-up.
[----------] 1 test from ForceTorqueTest
[ RUN      ] ForceTorqueTest.PluginTest
[Err] [SystemLoader.cc:118] Failed to load system plugin: (Reason: Could not find shared library)
- Requested plugin name: [GazeboYarpForceTorque]
- Requested library name: [plugins/forcetorque/libGazeboYarpForceTorque.so]
- Library search paths:
  - /home/runner/.gz/sim/plugins/
  - /usr/lib/x86_64-linux-gnu/gz-sim-7/plugins/
  - /home/runner/.ignition/gazebo/plugins/
[Err] [SystemLoader.cc:118] Failed to load system plugin: (Reason: Could not find shared library)
- Requested plugin name: [GazeboYarpRobotInterface]
- Requested library name: [plugins/robotinterface/libGazeboYarpRobotInterface.so]
- Library search paths:
  - /home/runner/.gz/sim/plugins/
  - /usr/lib/x86_64-linux-gnu/gz-sim-7/plugins/
  - /home/runner/.ignition/gazebo/plugins/

The problem is that the test is not finding the plugin, as no one is adding the folder in which the plugin can be found to the GZ_SIM_SYSTEM_PLUGIN_PATH environment variable. Indeed, it is non trivial to do so as now the plugins are spread in many different directories. To solve this, I suggest to:

  1. Add in the root CMakeLists.txt the following lines (taken from https://github.com/robotology/how-to-export-cpp-library/blob/afb21efb655e7b1cc11636c3d42aad9e02fb626f/CMakeLists.txt#L41 and https://github.com/robotology/gazebo-yarp-plugins/blob/9ce261e8c0f3433d48de8032820afabc0b33afcb/CMakeLists.txt#L22):

    set(CMAKE_RUNTIME_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR}")
    set(CMAKE_LIBRARY_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}")
    set(CMAKE_ARCHIVE_OUTPUT_DIRECTORY "${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_LIBDIR}")

    this will ensure that all plugins are placed in build/lib on Linux/macOS and build/bin on Windows. That means that for using the plugins from the build directory will boil down to just add build/lib or or build/bin to GZ_SIM_SYSTEM_PLUGIN_PATH

  2. As after the change in point 1. the plugins will not be found in a nested directory, change all occurences in sdf files of filename="plugins/forcetorque/libGazeboYarpForceTorque.so" or similar to filename="GazeboYarpForceTorque" . This would be also helpful to achieve https://github.com/robotology/gz-yarp-plugins/issues/28, as when the plugins are installed they are not installed in a plugins/forcetorque or similar directory, and to support Windows and macOS (where the plugin do not end with .so).

  3. Once 1. and 2. are done, we will then just need to tell to the test to add the directory with the plugins to the correct env variable, this can be done by adding (see https://github.com/gazebosim/gz-sim/blob/579013fe2841de68c0901192780ab8ee86cc9bb1/src/CMakeLists.txt#L274):

    set(_env_vars)
    list(APPEND _env_vars "GZ_SIM_SYSTEM_PLUGIN_PATH=$<TARGET_FILE_DIR:GazeboYarpForceTorque>")
    
    set_tests_properties(ForceTorqueTest PROPERTIES
    ENVIRONMENT "${_env_vars}")

Second Error

Error:  |yarp.os.Port| YARP not initialized; create a yarp::os::Network object before using ports

Whenever you use yarp ports, you need to have somewhere a yarp::os::Network object defined, just adding yarp::os::Network yarp should solve the problem.

traversaro commented 11 months ago

In https://github.com/robotology/gz-yarp-plugins/pull/30/commits/6697372d52e9154b88b126ef746ec5b242df857f I added a fix for actually use the cache, this should speedup the execution of apt-based workflow.

For what regards the failures, there are the following two failures:

conda-windows

It still fails with error:

 [INFO] |yarp.dev.PolyDriver|multipleanalogsensorsclient| Created device <multipleanalogsensorsclient>. See C++ class MultipleAnalogSensorsClient for documentation.
D:\a\gz-yarp-plugins\gz-yarp-plugins\tests\forcetorque\ForceTorqueTest.cc(44): error: The difference between measure(2) and -9.8*10 is 98, which exceeds 1e-2, where
measure(2) evaluates to 0,
-9.8*10 evaluates to -98, and
1e-2 evaluates to 0.01.
[INFO] |yarp.os.impl.PortCoreInputUnit|/ForceTorqueTest

It seems that you tried to fix this by increasting the delay. This is not ideal as it also slows down the tests that do not need to wait more. What I suggest instead is to check the return values of the getSixAxisForceTorqueSensorMeasure method, and only consider the output once the value is successful, see https://github.com/robotology/gz-yarp-plugins/pull/30/files#r1291112238 .

apt

In here probably we just need to set the YARP_DATA_DIRS when we install YARP, let me try to do that.

traversaro commented 11 months ago

apt

In here probably we just need to set the YARP_DATA_DIRS when we install YARP, let me try to do that.

Done in https://github.com/robotology/gz-yarp-plugins/pull/30/commits/2864978fcecd41dbd73b30c51a571ded4b8252ba, inspired by https://github.com/robotology/yarp-devices-ros2/blob/ecc88be70dac7fa3f44076b0f85bfca471754642/.github/workflows/conda-ci.yml#L103 .

traversaro commented 11 months ago

apt

In here probably we just need to set the YARP_DATA_DIRS when we install YARP, let me try to do that.

Done in 2864978, inspired by https://github.com/robotology/yarp-devices-ros2/blob/ecc88be70dac7fa3f44076b0f85bfca471754642/.github/workflows/conda-ci.yml#L103 .

It worked fine, probably we can just remove the attemps to set YARP_DATA_DIRS in the code.

traversaro commented 11 months ago

conda-windows

It still fails with error:

 [INFO] |yarp.dev.PolyDriver|multipleanalogsensorsclient| Created device <multipleanalogsensorsclient>. See C++ class MultipleAnalogSensorsClient for documentation.
D:\a\gz-yarp-plugins\gz-yarp-plugins\tests\forcetorque\ForceTorqueTest.cc(44): error: The difference between measure(2) and -9.8*10 is 98, which exceeds 1e-2, where
measure(2) evaluates to 0,
-9.8*10 evaluates to -98, and
1e-2 evaluates to 0.01.
[INFO] |yarp.os.impl.PortCoreInputUnit|/ForceTorqueTest

It seems that you tried to fix this by increasting the delay. This is not ideal as it also slows down the tests that do not need to wait more. What I suggest instead is to check the return values of the getSixAxisForceTorqueSensorMeasure method, and only consider the output once the value is successful, see https://github.com/robotology/gz-yarp-plugins/pull/30/files#r1291112238 .

Apparently also macOS sometimes fails. We can try to implement the logic in https://github.com/robotology/gz-yarp-plugins/pull/30/files#r1291112238 and see if that improves.

traversaro commented 11 months ago

Ok, it seems fine. If Windows is the only missing platform, for now probably we can skip the test and open an issue to track the test failures on Windows.

GiacomoBisio commented 11 months ago

Ok, it seems fine. If Windows is the only missing platform, for now probably we can skip the test and open an issue to track the test failures on Windows.

Ok, we added a new issue #31. So, can we merge?

traversaro commented 11 months ago

Ok, it seems fine. If Windows is the only missing platform, for now probably we can skip the test and open an issue to track the test failures on Windows.

Ok, we added a new issue #31. So, can we merge?

There are a few things we probably still need to fix before merging, to ensure that the main branch is a good step, basically:

After that, I think we are ready to Squash and merge (again, squash to avoid having a messy history).