Add test for forcetorque plugin #30

lucapa17 · 2023-08-09T14:58:21Z

traversaro · 2023-08-10T07:31:20Z

Thanks @lucapa17, some preliminary comments:

As this is now a public repo that many people can look into, it is useful to have understandable titles and description for the PRs. In this case probably just "Add test for forcetorque plugin" is sufficient, and in the description you can add "Fix Write tests for forcetorque plugin #20" so that the issue is closed automatically once the PR is merged (see https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue)
I saw that you added a comment on how to run the test. However, this is not a convenient way to run the tests, especially if you have many tests, as tipically developers want to run all the tests in the project whenever they do modifications. This is tipically done via the ctest command (see https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html).
Related to the previous point, if you check the CI output, you can see that the output during the tests step is:

Run cd build
Test project /home/runner/work/gz-yarp-plugins/gz-yarp-plugins/build
No tests were found!!!"

so something is not working as expected.

traversaro · 2023-08-10T07:42:24Z

tests/forcetorque/CMakeLists.txt

+project(GTestSetup)
+
+# Find Gazebo
+find_package(gz-sim7 REQUIRED)
+set(GZ_SIM_VER ${gz-sim7_VERSION_MAJOR})


These lines are not requested, and can be removed to reduce the noise in the code. project is only required in the root CMakeLists.txt of the project, and find_package needs only to be invoked once.

Suggested change

project(GTestSetup)

# Find Gazebo

find_package(gz-sim7 REQUIRED)

set(GZ_SIM_VER ${gz-sim7_VERSION_MAJOR})

traversaro · 2023-08-10T07:43:08Z

tests/forcetorque/CMakeLists.txt

+set(gtest_force_shared_crt ON CACHE BOOL "" FORCE)
+FetchContent_MakeAvailable(googletest)
+
+enable_testing()


This only works when called in the root CMakeLists.txt, so I suggest to remove it here and add it there.

Suggested change

enable_testing()

traversaro · 2023-08-10T07:44:04Z

tests/forcetorque/README.md

+## ForceTorque plugin Test
+
+~~~
+cd build
+export GZ_SIM_SYSTEM_PLUGIN_PATH=`pwd`
+cd tests/forcetorque
+./ForceTorqueTest
+~~~


As mentioned in the comment to the PR as an whole, I suggest to remove this and just document in the README how to run tests.

Suggested change

## ForceTorque plugin Test

~~~

cd build

export GZ_SIM_SYSTEM_PLUGIN_PATH=`pwd`

cd tests/forcetorque

./ForceTorqueTest

~~~

traversaro · 2023-08-10T07:48:04Z

CMakeLists.txt

@@ -15,4 +15,5 @@ set(GZ_SIM_VER ${gz-sim7_VERSION_MAJOR})

 add_subdirectory(libraries)
 add_subdirectory(plugins)
+add_subdirectory(tests)


I suggest the following modification, based on the docs in https://cmake.org/cmake/help/book/mastering-cmake/chapter/Testing%20With%20CMake%20and%20CTest.html :

Suggested change

add_subdirectory(tests)

option(BUILD_TESTING "Create tests using CMake" OFF)

enable(CTest)

if(BUILD_TESTING)

add_subdirectory(tests)

endif()

Explanation:

Instead of calling enable_testing directly, the CMake docs suggest to just include the CTest module

The CTest module defines the BUILD_TESTING variable, and if that variable is set to OFF, no tests are enabled. For this reason, if BUILD_TESTING is OFF, we do not add the tests subdirectory. Furthermore, tipically in our project we set the default value of BUILD_TESTING to OFF, so that users that do not need to compile the tests do not spend time compiling them.

traversaro · 2023-08-10T08:58:30Z

Can you check out the conflicts with the main branch?

traversaro · 2023-08-10T09:25:39Z

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:

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

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 Add install step #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).
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 · 2023-08-10T12:19:12Z

tests/forcetorque/ForceTorqueTest.cc

+ gz::sim::TestFixture fixture("../../../tests/forcetorque/model.sdf");
+
+ int iterations = 1000;
+ fixture.Server()->Run(true, iterations, false);


This is a bit difficult to read, see https://abseil.io/tips/94 for a description way.

Suggested change

fixture.Server()->Run(true, iterations, false);

fixture.Server()->Run(/*_blocking=*/true, iterations, /*_paused=*/false);

traversaro · 2023-08-10T12:24:05Z

tests/forcetorque/ForceTorqueTest.cc

+ p.open("/read"); 
+ yarp::os::Network::connect("/forcetorque/measures:o","/read");
+ yarp::os::Bottle b; 
+ p.read(b); 


macOS and Windows tests are failing. Perhaps we should add a check that the read is successful, checking the return value of this function (see https://www.yarp.it/latest/classyarp_1_1os_1_1Port.html#a7b4eb018faba3837d50fffee0ea722ef). If it fails, it is possible that it is not able to read in time anything, as nothing is published between the connect and the read. Perhaps we can try to migrate this code to multipleanalogsensorsclient and then look again at this code? Tryng to fix it now before migrating to multipleanalogsensorsclient may be a waste of time.

traversaro · 2023-08-11T09:27:45Z

tests/forcetorque/ForceTorqueTest.cc

+ double timestamp;
+
+ isixaxis->getSixAxisForceTorqueSensorName(0, sensorName);
+ isixaxis->getSixAxisForceTorqueSensorMeasure(0, measure, timestamp);


I suggest:

Suggested change

isixaxis->getSixAxisForceTorqueSensorMeasure(0, measure, timestamp);

size_t maxNrOfReadingAttempts = 20;

bool readSuccessful = false;

for (size_t i=0; (i < maxNrOfReadingAttempts) && !readSuccessful ; i++)

{

readSuccessful = isixaxis->getSixAxisForceTorqueSensorMeasure(0, measure, timestamp);

std::this_thread::sleep_for(std::chrono::milliseconds(100));

}

ASSERT_TRUE(readSuccessful);

if the failure continue, you can inspect also the value returned by getSixAxisForceTorqueSensorStatus to understand why the output is not the expected one.

traversaro · 2023-08-11T09:29:20Z

In 6697372 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 · 2023-08-11T09:33:28Z

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 .

traversaro · 2023-08-11T09:42:28Z

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 · 2023-08-11T09:43:55Z

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 · 2023-08-11T09:52:44Z

tests/forcetorque/ForceTorqueTest.cc

@@ -36,7 +36,15 @@ TEST(ForceTorqueTest, PluginTest)
 double timestamp;

 isixaxis->getSixAxisForceTorqueSensorName(0, sensorName);
- isixaxis->getSixAxisForceTorqueSensorMeasure(0, measure, timestamp);
+ //isixaxis->getSixAxisForceTorqueSensorMeasure(0, measure, timestamp);


Suggested change

//isixaxis->getSixAxisForceTorqueSensorMeasure(0, measure, timestamp);

If we are not using it, let's remove it.

traversaro · 2023-08-11T09:53:22Z

tests/forcetorque/ForceTorqueTest.cc

+ readSuccessful = isixaxis->getSixAxisForceTorqueSensorMeasure(0, measure, timestamp);
+ std::this_thread::sleep_for(std::chrono::milliseconds(100));
+ }
+ ASSERT_TRUE(readSuccessful);


Please avoid mixing tabs and spaces, as it may result in inconsistent rendring (the width of tabs is not constant).

Suggested change

ASSERT_TRUE(readSuccessful);

ASSERT_TRUE(readSuccessful);

traversaro · 2023-08-11T10:53:01Z

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 · 2023-08-11T12:42:16Z

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 · 2023-08-11T13:10:05Z

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:

We can't just have a test on Windows fail, as then it would hide the failure could hide other failures that could emerge in future PRs. If tests on Windows are know to fail, we should suppress running tests on Windows. We can do that by adding the if: contains(matrix.os, 'windows') after the line https://github.com/robotology/gz-yarp-plugins/blob/FT-tests/.github/workflows/conda-forge.yml#L69.
As we changed the location of the plugins in the build directory, we should change all the sdf files in tutorials from <plugin name="GazeboYarpForceTorque" filename="plugins/forcetorque/libGazeboYarpForceTorque.so"> to <plugin name="GazeboYarpForceTorque" filename="GazeboYarpForceTorque"> (also for other non-FT sensors plugins), otherwise all the tutorial would be broken by this PR.

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

lucapa17 added 2 commits August 9, 2023 15:36

FT test

5589283

Create README.md

374f667

traversaro reviewed Aug 10, 2023

View reviewed changes

changes in CMakeLists files

f01adbc

lucapa17 changed the title ~~Ft tests~~ Add test for forcetorque plugin Aug 10, 2023

Merge branch 'main' into FT-tests

dbcc747

set env variable GZ_SIM_SYSTEM_PLUGIN_PATH in CMakeList file for FT test

3d7a04e

traversaro reviewed Aug 10, 2023

View reviewed changes

lucapa17 and others added 4 commits August 11, 2023 09:21

use multipleanalogsensorsclient instead of yarp port for test

9f4a0ec

add YARP_DATA_DIRS

222a77c

increase timeout and running time

fd28795

Fix use of cache in apt workflow

6697372

traversaro reviewed Aug 11, 2023

View reviewed changes

Update apt.yml

2864978

Update apt.yml

f305c97

check successful read of getSixAxisForceTorqueSensorMeasure

5a0f38c

traversaro reviewed Aug 11, 2023

View reviewed changes

Test with getSixAxisForceTorqueSensorStatus

2b18712

Delete README.md

1b97714

GiacomoBisio added 3 commits August 11, 2023 15:31

Changes in tutorial sdf files

9f13744

Suppress tests on Windows

234c357

Update conda-forge.yml

0715cd4

GiacomoBisio requested a review from traversaro August 11, 2023 14:24

traversaro approved these changes Aug 11, 2023

View reviewed changes

lucapa17 merged commit f498935 into main Aug 11, 2023
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for forcetorque plugin #30

Add test for forcetorque plugin #30

lucapa17 commented Aug 9, 2023 •

edited

Loading

traversaro commented Aug 10, 2023

traversaro Aug 10, 2023

traversaro Aug 10, 2023

traversaro Aug 10, 2023

traversaro Aug 10, 2023

traversaro commented Aug 10, 2023

traversaro commented Aug 10, 2023 •

edited

Loading

traversaro Aug 10, 2023

traversaro Aug 10, 2023

traversaro Aug 11, 2023 •

edited

Loading

traversaro commented Aug 11, 2023

traversaro commented Aug 11, 2023

apt

traversaro commented Aug 11, 2023

apt

traversaro commented Aug 11, 2023

conda-windows

traversaro Aug 11, 2023

traversaro Aug 11, 2023

traversaro commented Aug 11, 2023

GiacomoBisio commented Aug 11, 2023

traversaro commented Aug 11, 2023

-add_subdirectory(tests)
+option(BUILD_TESTING "Create tests using CMake" OFF)
+enable(CTest)
+if(BUILD_TESTING)
+ add_subdirectory(tests)
+endif()

	fixture.Server()->Run(true, iterations, false);
	fixture.Server()->Run(/_blocking=/true, iterations, /_paused=/false);

Add test for forcetorque plugin #30

Add test for forcetorque plugin #30

Conversation

lucapa17 commented Aug 9, 2023 • edited Loading

traversaro commented Aug 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro commented Aug 10, 2023

traversaro commented Aug 10, 2023 • edited Loading

First Error

Second Error

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro Aug 11, 2023 • edited Loading

Choose a reason for hiding this comment

traversaro commented Aug 11, 2023

conda-windows

apt

traversaro commented Aug 11, 2023

apt

traversaro commented Aug 11, 2023

apt

traversaro commented Aug 11, 2023

conda-windows

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traversaro commented Aug 11, 2023

GiacomoBisio commented Aug 11, 2023

traversaro commented Aug 11, 2023

lucapa17 commented Aug 9, 2023 •

edited

Loading

traversaro commented Aug 10, 2023 •

edited

Loading

traversaro Aug 11, 2023 •

edited

Loading