Add basic test for YarpRobotLoggerDevice #862
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
There is CI failure: I guess the problem is to put all |
Argh, that cleanup went out of hand, I will probably cleanup the PR in the next days. |
Fixed and rebased, CI should be happy. |
|
Thank you!! This was an important missing point! |
|
A few jobs fail, I will check them when I have a laptop. |
S-Dafarra
reviewed
Jul 4, 2024
Comment on lines
37
to
70
| std::string toString(const matioCpp::VariableType& type) | ||
| { | ||
| switch(type) | ||
| { | ||
| case matioCpp::VariableType::Element: | ||
| return "Element"; | ||
| case matioCpp::VariableType::Vector: | ||
| return "Vector"; | ||
| case matioCpp::VariableType::MultiDimensionalArray: | ||
| return "MultiDimensionalArray"; | ||
| case matioCpp::VariableType::Struct: | ||
| return "Struct"; | ||
| case matioCpp::VariableType::CellArray: | ||
| return "CellArray"; | ||
| case matioCpp::VariableType::StructArray: | ||
| return "StructArray"; | ||
| case matioCpp::VariableType::Unsupported: | ||
| return "Unsupported"; | ||
| default: | ||
| return "Unknown VariableType"; | ||
| } | ||
| } |
There was a problem hiding this comment.
This does not seem to be used. In any case, what was the use case here? I may also add it to matioCpp
There was a problem hiding this comment.
Ah right, debugging leftover. Yes, I can add some helped in matio-cpp from my first time experience.
S-Dafarra
reviewed
Jul 4, 2024
devices/YarpRobotLoggerDevice/tests/YarpRobotLoggerDeviceTest.cpp
Outdated
Show resolved
Hide resolved
|
Windows build is failing with: |
|
Ubuntu 20.04 ci is failing with: |
Probably fixed by @S-Dafarra suggestion in #862 (comment) . |
S-Dafarra
reviewed
Jul 5, 2024
| std::string envVarListSeparator = ":"; | ||
| #define blf_putenv putenv | ||
| #endif | ||
| std::string original_yarp_data_dirs = getenv("YARP_DATA_DIRS"); |
There was a problem hiding this comment.
Maybe the problem is here. It returns a nullptr if the env variable is not found
S-Dafarra
reviewed
Jul 5, 2024
devices/YarpRobotLoggerDevice/tests/YarpRobotLoggerDeviceTest.cpp
Outdated
Show resolved
Hide resolved
|
Ok, all the failures are in the non-conda case, as in that case |
|
I replicated the failure locally by unsetting |
This was fixed in 7931de9 . |
|
Last failure: We need to modify the logic to account for the different location of the |
|
The name of the file looks weird. Maybe the issue is in https://github.com/robotology/robometry/blob/bb2ea36c90141b9a6ac2be903619786734f95a52/src/librobometry/src/BufferManager.cpp#L325 which converts the format specified in |
Nevermind, I guess it was just a copy paste issue 😂 |
|
There are some failures on valgrind also, but they are probably related to YARP: |
GiulioRomualdi
approved these changes
Aug 19, 2024
I had the same failure on conda-forge, I solved de-vendoring catch2, but I guess also updating the FetchContent's catch2 should solve the problem. |
|
Hi @traversaro could you please rebase it on top of master? |
Sorry, I missed this comment, I guess you merged yourself? |
|
yes don't worry :) |
|
Unfortunately the CI fails with |
|
I think the test passes fine, see: That error message is due to some code that access directly the server without respecting the The test fails to a leak in |
|
There is still a macos error: but this is a conda-forge problem tracked in conda-forge/idyntree-feedstock#105 . |
|
Cool, now a totally unrelated test is failing with a leak, probably due to tbb. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Discussing with @GiulioRomualdi, it turns out that the
YarpRobotLoggerDeviceis an important and critical piece of software, that has almost zero test coverage, making it complex to make (or review) any change to it.As an initial (and by far non complete) solution for this, in this PR I add a basic test for the
YarpRobotLoggerDevice, that logs the encoders and the imu of a fake robot, and check that the value saved in the .mat files are indeed correct.In the future, the test can be made more complex to test more functionality of the
YarpRobotLoggerDevice(log of YARP ports, textual logging), but I guess we can start simple.For what regards the technicalities, this has been implemented with this key ideas:
yarpserverto run thanks to the use ofyarp::os::NetworkBase::setLocalMode(true)libYARP_robotinterfacelibrary, that permits to do from C++ what typically is done by launching theyarprobotinterfacefrom the command line. In this way, we can start and stop the yarprobotinterface automatically, and also access all the devices that the yarprobotinterface spawns.fakeMotionControlandfakeIMUdevices have been used to build a "virtual" robot with 4 joints and 1 imu. In the future we could even run a robot in Gazebo with the same technique (see Write the code that extracts a list of devices from a given simulation instance from gzyarp::Handler and pass it to BipedalLocomotion::RobotInterface::YarpRobotControl::setDriver() and BipedalLocomotion::RobotInterface::YarpSensorBridge::setDriversList robotology/gz-sim-yarp-plugins#149), but for this specific test there was no need for a fully fledged simulation of the robot.cc @nicktrem @xela-95 that could be interested to this PR as well.