Closed traversaro closed 4 years ago
cc @S-Dafarra @gabrielenava @diegoferigo @nunoguedelha @fjandrad @prashanthr05 @Yeshasvitvs @lrapetti
Why not whole-body-controllers
?
At least at the superbuild level, whole-body-controllers
is only installed if ROBOTOLOGY_USES_MATLAB
is enabled, while an (hidden) requirement that I was thinking of was that this configuration files need to be installed in the superbuild if ROBOTOLOGY_USES_DYNAMICS
is enabled, but ROBOTOLOGY_USES_MATLAB
is not.
However, indeed an option is to install all the time whole-body-controllers
, and just install the configuration files if MATLAB support is not enabled.
Indeed, those configuration files are mainly used for whole-body-controllers
related demos. Actually, that repo does not need to be compiled (as far as I remember) so it would not be an issue to download it anyway. It also contains scripts and functions that probably can run in Octave as well. Then, we can think of splitting it into several parts.
I too think whole-body-controllers
is a good choice as the configuration files are mostly used while working with the controllers, both during building and then for demos.
Of the options proposed above, I like two of them:
Store these files icub-gazebo-wholebody
after properly rename the repo. Actually, I think here we may open a Pandora's box: in fact, It seems to me that at the moment there is a bit of redundancy between icub-gazebo-wholebody
and icub-gazebo
. Maybe we can exploit this moment to properly cleanup the two repos, e.g. port all the models in icub-gazebo
, and leave worlds
, home positions
and similar configuration files in icub-gazbeo-wholebody
, and change the name of the repo in something different.
If all the users of the home-positions
don't mind of creating a dependency to whole-body-controllers
, we can put them in whole-body-controllers
. Hoping that this won't result in people copy-pasting the home positions in other repos in order to skip the dependency.
in fact, It seems to me that at the moment there is a bit of redundancy between icub-gazebo-wholebody and icub-gazebo.
Mhh, let's discuss about this. I do not think there is a lot of redundancy between the two repos: if icub-models could cover all the missing features w.r.t. to icub-gazebo, I think that icub-gazebo repo should disappear.
If all the users of the home-positions don't mind of creating a dependency to whole-body-controllers, we can put them in whole-body-controllers. Hoping that this won't result in people copy-pasting the home positions in other repos in order to skip the dependency.
I think most of the users of this actually use the robotology-superbuild, so as long as we ensure that the positions are still installed, I do not think there will be a lot of change.
Is the offer of moving the home position in whole-body-controllers
still valid @traversaro? I am working on the new WBC
release, we can include also this moving.
Yes please, that would be really helpful. Unless there is something that is required also for walking, but probably that could be moved in the walking-controllers repo @GiulioRomualdi .
Hi guys @robotology/iit-dic , any news on this? codyco-modules is ideally going to be removed soon, so it would be great to either port this files, or drop their use.
Given the lack of replies, I assume that no one is interested/using those configuration files, and proceed to remove them from the files installed in the robotology-superbuild by dropping codyco-modules in the next weeks.
Note that probably the docs in whole-body-controllers and walking-teleoperation would need to be updated after the removal of these files, as they still references them in their docs:
cc @gabrielenava @kouroshD
Note that probably the docs in whole-body-controllers and walking-teleoperation would need to be updated after the removal of these files, as they still references them in their docs:
@traversaro which part of the documentation in walking-teleoperation needs updates?
Note that probably the docs in whole-body-controllers and walking-teleoperation would need to be updated after the removal of these files, as they still references them in their docs:
@traversaro which part of the documentation in walking-teleoperation needs updates?
The linked walking-teleoperation wiki page refers to the homePoseBalancing.ini
file that is amont the one installed by codyco-modules
and discussed in this issue.
The linked walking-teleoperation wiki page refers to the homePoseBalancing.ini file that is amont the one installed by codyco-modules and discussed in this issue.
When you apply the intended changes, please ping us, so I will update the documentation.
The linked walking-teleoperation wiki page refers to the homePoseBalancing.ini file that is amont the one installed by codyco-modules and discussed in this issue.
When you apply the intended changes, please ping us, so I will update the documentation.
Due to the lack of feedback, the currently planned change is to remove those files, as mentioned in https://github.com/robotology/codyco-modules/issues/273#issuecomment-586681614 . For this reason, if you are not using those files I suggest to change the documentation now, without to wait for the actual removal of the files.
Just to understand, where is the file homePoseBalancing.ini
going to be moved?
Just to understand, where is the file
homePoseBalancing.ini
going to be moved?
As mentioned in https://github.com/robotology/codyco-modules/issues/273#issuecomment-586681614, given the lack of replies I assumed that no one is currently using the homePoseBalancing.ini
files, and so the current plan is to delete them. If instead someone is actually using them, then let's reopen the discussion about there to move them.
I probably overlooked this because of the similarity with https://github.com/robotology/codyco-modules/issues/290. Sorry about that.
The file homePoseBalancing.ini
is largely used when using the robot as mentioned above. I guess we failed in finding in agreement about where to put it right?
The file
homePoseBalancing.ini
is largely used when using the robot as mentioned above.
Ok, then I updated again the title of the issue.
I guess we failed in finding in agreement about where to put it right?
As of https://github.com/robotology/codyco-modules/issues/273#issuecomment-547353474, I think the agreement to put the models in whole-body-controllers
was reached, however no one replied to the comments until I threatened to delete the configuration files. :)
@gabrielenava do you still agree in keeping this files in whole-body-controllers
. Among the users of these files, there is any volunteer for moving these files to whole-body-controllers
@robotology/iit-dic ?
@gabrielenava do you still agree in keeping this files in whole-body-controllers.
I still agree.
Among the users of these files, there is any volunteer for moving these files to whole-body-controllers @robotology/iit-dic ?
If necessary I can do it on Wednesday (even if it should be a simple task).
Great, thanks @gabrielenava ! Wednesday is ok, but there is nothing urgent.
Done with this commit: https://github.com/robotology/whole-body-controllers/commit/36d905658c657681208dfb106dcf53703d9738d3
Installation tested with the robotology superbuild and seems working fine. It is in devel
branch for now, but PR for merging in master is already opened.
NB: to keep the commit history readable, modifications of the CMakeLists.txt
and of the README
files are in a separate commit: https://github.com/robotology/whole-body-controllers/commit/144332b9f58461e2dfca96eed1f80de2d8cf7fa1
Related issue in wbc: https://github.com/robotology/whole-body-controllers/issues/80 .
I tried to updated the name of the folder where the files are installed from wbc
to yarp
as discussed here, but I got an error while compiling the superbuild:
-- Build files have been written to: /home/gnava/Software/github/robotology/robotology-superbuild/build/robotology/whole-body-controllers
[ 94%] Performing build step for 'whole-body-controllers'
[ 94%] Performing install step for 'whole-body-controllers'
Install the project...
-- Install configuration: "Release"
Disabled exporting of autogenerated c++ code from Simulink.
CMake Error at utilities/homePositions/robots/bigman/cmake_install.cmake:49 (file):
file cannot create directory: /bigman. Maybe need administrative
privileges.
Call Stack (most recent call first):
utilities/homePositions/robots/cmake_install.cmake:42 (include)
utilities/homePositions/cmake_install.cmake:42 (include)
utilities/cmake_install.cmake:42 (include)
cmake_install.cmake:46 (include)
Makefile:73: recipe for target 'install' failed
make[3]: *** [install] Error 1
CMakeFiles/whole-body-controllers.dir/build.make:72: recipe for target 'robotology/whole-body-controllers/CMakeFiles/YCMStamp/whole-body-controllers-install' failed
make[2]: *** [robotology/whole-body-controllers/CMakeFiles/YCMStamp/whole-body-controllers-install] Error 2
CMakeFiles/Makefile2:2041: recipe for target 'CMakeFiles/whole-body-controllers.dir/all' failed
make[1]: *** [CMakeFiles/whole-body-controllers.dir/all] Error 2
Makefile:140: recipe for target 'all' failed
make: *** [all] Error 2
Maybe I cannot use yarp
as specified folder name in this command?
If you want to avoid the need of adding the wbc
install folder into the YARP_DATA_DIRS
, you can also depend on ICUBcontrib
as walking-controllers
does. Then you can install the robot configuration files in the ICUBcontrib
folder (https://github.com/robotology/walking-controllers/blob/master/app/CMakeLists.txt#L33).
In my opinion though, I don't see the advantage of having an additional dependency against modifying an env variable. In this latter case, it would be enough to modify to the README.
I just realized that if yarp
has his own ${YARP_ROBOTS_INSTALL_DIR}
then what I wrote above can be done also without depending on ICUBcontrib
:sweat_smile:
I just realized that if yarp has his own ${YARP_ROBOTS_INSTALL_DIR} then what I wrote above can be done also without depending on ICUBcontrib
This worked thanks @S-Dafarra! Updated with this commit: https://github.com/robotology/whole-body-controllers/commit/55928fb5def6ccf82a81d6354adaac3ae9948164
Maybe I cannot use yarp as specified folder name in this command?
Exactly, you definitely cannot. Actually I was suggesting something much simpler, just to define:
set(WBC_ROBOTS_INSTALL_DIR share/yarp/robots)
in the main CMakeLists.txt. As these are YARP-related configuration files of robots, I do not think there is anything strange or risk in installing them in <prefix>/share/yarp
. These means that if you install just this projects on its own standalone prefix, you would still need to add <prefix>/share/yarp
to YARP_DATA_DIRS
, but at least when you install the project in the same prefix of YARP
, you do not need any additonal env variable value.
I just realized that if yarp has his own ${YARP_ROBOTS_INSTALL_DIR} then what I wrote above can be done also without depending on ICUBcontrib
This worked thanks @S-Dafarra! Updated with this commit: robotology/whole-body-controllers@55928fb
Ouch, too late. This works fine if you install the project in the same prefix path of YARP, but in some cases, for example if you used YARP installed from binaries in the system directories, it will not work. I personally suggest to use the solution in my other comment (that will give you the same result if YARP is installed in the same prefix, and something sane in any case). Actually I am not sure if ${YARP_ROBOTS_INSTALL_DIR}
is an absolute path or a relative one. If it is a relative path, that is by far the best solution, as its content should be just share/yarp/robots
.
In my opinion though, I don't see the advantage of having an additional dependency against modifying an env variable. In this latter case, it would be enough to modify to the README.
This is not strictly related to this repo, but a non-trivial problem is the need to have a env variable for each project, for example in context such as the superbuild. Having a lot of env variables is quite error prone, while having just a few of them is more manageable.
This is not strictly related to this repo, but a non-trivial problem is the need to have a env variable for each project, for example in context such as the superbuild
How does the superbuild handle it? When we tried yesterday it worked out-of-the-box, also with the wbc
prefix, without the need of modifying the env variable :thinking:
When we tried yesterday it worked out-of-the-box, also with the wbc prefix, without the need of modifying the env variable thinking
I have no idea how that would be working if you did not modified the YARP_DATA_DIRS env variables. If you want to debug, it would be interesting to run the yarp resource <stuffThatYouAreSearching.ini --verbose
and see the output. In which directory where you running the tests?
I checked out on https://github.com/robotology/whole-body-controllers/commit/36d905658c657681208dfb106dcf53703d9738d3, installed and uninstalled codyco-modules
. Then I run it on the home folder and I got this output:
sdafarra@iiticublap104:~$ yarp resource --find homePoseBalancing.ini --verbose
||| configuring
||| configuring
||| default config file specified as homePoseBalancing.ini
||| checking [/home/sdafarra/homePoseBalancing.ini] (pwd)
||| checking [/home/sdafarra/.config/yarp/robots/icubGazeboSim] (robot YARP_CONFIG_HOME)
||| checking [/home/sdafarra/.local/share/yarp/robots/icubGazeboSim] (robot YARP_DATA_HOME)
||| checking [/etc/xdg/xdg-ubuntu/yarp/robots/icubGazeboSim] (robot YARP_CONFIG_DIRS)
||| checking [/usr/share/upstart/xdg/yarp/robots/icubGazeboSim] (robot YARP_CONFIG_DIRS)
||| checking [/etc/xdg/yarp/robots/icubGazeboSim] (robot YARP_CONFIG_DIRS)
||| checking [robots/icubGazeboSim] (robot YARP_DATA_DIRS)
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/yarp/robots/icubGazeboSim] (robot YARP_DATA_DIRS)
||| found /home/sdafarra/Software/robotology-superbuild/build/install/share/yarp/robots/icubGazeboSim
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/iCub/robots/icubGazeboSim] (robot YARP_DATA_DIRS)
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/ICUBcontrib/robots/icubGazeboSim] (robot YARP_DATA_DIRS)
||| found /home/sdafarra/Software/robotology-superbuild/build/install/share/ICUBcontrib/robots/icubGazeboSim
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/codyco/robots/icubGazeboSim] (robot YARP_DATA_DIRS)
||| found /home/sdafarra/Software/robotology-superbuild/build/install/share/codyco/robots/icubGazeboSim
||| checking [config/path.d] (robot path.d YARP_DATA_DIRS)
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/yarp/config/path.d] (robot path.d YARP_DATA_DIRS)
||| found /home/sdafarra/Software/robotology-superbuild/build/install/share/yarp/config/path.d
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/wbc/robots/icubGazeboSim] (robot yarp.d)
||| found /home/sdafarra/Software/robotology-superbuild/build/install/share/wbc/robots/icubGazeboSim
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/yarp/robots/icubGazeboSim/homePoseBalancing.ini] (robot)
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/ICUBcontrib/robots/icubGazeboSim/homePoseBalancing.ini] (robot)
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/codyco/robots/icubGazeboSim/homePoseBalancing.ini] (robot)
||| checking [/home/sdafarra/Software/robotology-superbuild/build/install/share/wbc/robots/icubGazeboSim/homePoseBalancing.ini] (robot)
||| found /home/sdafarra/Software/robotology-superbuild/build/install/share/wbc/robots/icubGazeboSim/homePoseBalancing.ini
||| finding file [from]
"/home/sdafarra/Software/robotology-superbuild/build/install/share/wbc/robots/icubGazeboSim/homePoseBalancing.ini"
~I guess that it looked into every subfolder of share
.~ Actually it did not, but this is my setup.sh
sdafarra@iiticublap104:~$ cat $ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX/share/robotology-superbuild/setup.sh
# Automatically generated setup file for robotology-superbuild
export ROBOTOLOGY_SUPERBUILD_SOURCE_DIR=/home/sdafarra/Software/robotology-superbuild
export ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX=/home/sdafarra/Software/robotology-superbuild/build/install
# Extend PATH (see https://en.wikipedia.org/wiki/PATH_(variable) )
export PATH=$PATH:$ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX/bin
# YARP related env variables (see http://www.yarp.it/yarp_data_dirs.html )
export YARP_DATA_DIRS=$YARP_DATA_DIRS:$ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX/share/yarp:$ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX/share/iCub:$ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX/share/ICUBcontrib
# Extend CMAKE_PREFIX_PATH (see https://cmake.org/cmake/help/v3.8/variable/CMAKE_PREFIX_PATH.html )
export CMAKE_PREFIX_PATH=${CMAKE_PREFIX_PATH}:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}
# Extend path for shared libraries (see http://tldp.org/HOWTO/Program-Library-HOWTO/shared-libraries.html)
export LD_LIBRARY_PATH=$LD_LIBRARY_PATH:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/lib
# Setup the path of blockfactory plugins
export BLOCKFACTORY_PLUGIN_PATH=$ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX/lib/blockfactory
# ROBOTOLOGY_USES_GAZEBO-specific lines
# Gazebo related env variables (see http://gazebosim.org/tutorials?tut=components#EnvironmentVariables )
[ -f /usr/share/gazebo/setup.sh ] && source /usr/share/gazebo/setup.sh
export GAZEBO_PLUGIN_PATH=${GAZEBO_PLUGIN_PATH}:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/lib
export GAZEBO_MODEL_PATH=${GAZEBO_MODEL_PATH}:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/share/gazebo/models:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/share/iCub/robots:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/share
export GAZEBO_RESOURCE_PATH=${GAZEBO_RESOURCE_PATH}:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/share/gazebo/worlds
# ROBOTOLOGY_ENABLE_DYNAMICS-specific lines
export YARP_DATA_DIRS=$YARP_DATA_DIRS:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/share/codyco
# Configure the Matlab path
export MATLABPATH=${MATLABPATH}:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/mex:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/share/WBToolbox:${ROBOTOLOGY_SUPERBUILD_INSTALL_PREFIX}/share/WBToolbox/images
My YARP_DATA_DIRS
are
:/home/sdafarra/Software/robotology-superbuild/build/install/share/yarp:/home/sdafarra/Software/robotology-superbuild/build/install/share/iCub:/home/sdafarra/Software/robotology-superbuild/build/install/share/ICUBcontrib:/home/sdafarra/Software/robotology-superbuild/build/install/share/codyco
I also modified the CMakeLists
to print the content of YARP_ROBOTS_INSTALL_DIR
and I got:
-- Found YARP: /home/sdafarra/Software/robotology-superbuild/build/install/lib/cmake/YARP (found version "3.3.1+3-20200117.4+git5a112f9")
CMake Deprecation Warning at /home/sdafarra/Software/robotology-superbuild/build/install/lib/cmake/YARP/YARPConfig.cmake:315 (message):
The YARP_MODULE_PATH variable is deprecated. CMake find package modules
are now in YCM.
Call Stack (most recent call first):
CMakeLists.txt:9223372036854775807 (_yarp_module_path_is_deprecated)
CMakeLists.txt:15 (set)
YARP_ROBOTS_INSTALL_DIR = share/yarp/robots
-- Configuring done
-- Generating done
-- Build files have been written to: /home/sdafarra/Software/robotology-superbuild/build/robotology/whole-body-controllers
Hence the path is relative I would say. This output arises me a couple of questions:
ResourceFinder
~look into every folder inside share
~ somehow finds it, why not just install them in wbc
as it was before? In this way, it is also clear by who those files are installed.YARP_MODULE_PATH
is deprecated, what is the corresponding macro of yarp_install
? Hence the path is relative I would say.
Great!
If that path is relative and ResourceFinder look into every folder inside share, why not just install them wbc as it was before? In this way, it is also clear by who those files are installed.
It that is true, I would agree, but that seems to be different from the search policy described in https://www.yarp.it/yarp_data_dirs.html#datafiles_searchpolicy . What I am afraid is that what we are seeing is due to a bug in a YARP, that if fixed would break our workflow. I tried to check the code in https://github.com/robotology/yarp/blob/v3.3.2/src/libYARP_os/src/yarp/os/ResourceFinder.cpp#L600 , but it would take more time. For me we can also install in share/wbc/robots
, but then I am afraid that it will stop working and we have no idea how to fix it. : )
If YARP_MODULE_PATH is deprecated, what is the corresponding macro of yarp_install?
I think it is sufficient to just do find_package(YARP)
, and that already defines yarp_install
.
I got it. I checked into the folder /home/sdafarra/Software/robotology-superbuild/build/install/share/yarp/config/path.d
(you can see it in the ResourceFinder
output) and inside there is a wbc.ini
file whose content is
###### This file is automatically generated by CMake.
[search wbc]
path "/home/sdafarra/Software/robotology-superbuild/build/install/share/wbc"
I guess this is the result of
yarp_configure_external_installation(wbc)
In fact, look at the first lines make install
Install the project...
-- Install configuration: "Release"
-- Up-to-date: /home/sdafarra/Software/robotology-superbuild/build/install/share/yarp/config/path.d/wbc.ini
............
Seems to be too elaborate to be a bug :sweat_smile:
I think I understood what is happening: the wbc
installed a config file in yarp.d
(see the discussion in https://www.yarp.it/yarp_data_dirs.html#datafiles_extendsearch). However, once upon a time when the YARP_DATA_DIRS
env variable was defined, that disable the use of the files installed in yarp.d
, and that is the reason why I never relied on that, see https://github.com/robotology/yarp/issues/336#issuecomment-167793620 for the related YARP issue. It is possible that over time the issue has been fixed, even if not intentionally, and the yarp.d
is now working fine even if YARP_DATA_DIRS
is defined, but the problem is that without a test it could break again in the future.
Edit: @S-Dafarra you beat me by seconds!
What would you suggest to do then?
What would you suggest to do then?
Do whatever you find more reasonable for you, the regression in https://github.com/robotology/yarp/issues/336#issuecomment-167793620 I think is still there, but getting crazy for a super corner case does not make a lot of sense. We can try to install in wbc
, not update the YARP_DATA_DIRS in the superbuild, and investigate later if we have problems.
We can try to install in wbc, not update the YARP_DATA_DIRS in the superbuild, and investigate later if we have problems.
I agree with this. I reverted the last commit and in the process I fixed the warning discussed here.
Here is the final commit: https://github.com/robotology/whole-body-controllers/commit/623ac9da9aff8414a520b0287db06a7c19624978
@gabrielenava What is the timeline for getting those files in the stable branch of whole-body-controllers?
cc @fjandrad @prashanthr05
@traversaro according to my scheduling the release should be done this Friday, but I may need an extra week.
@traversaro according to my scheduling the release should be done this Friday, but I may need an extra week.
Ack. @prashanthr05 @fjandrad given this, we can also just disable the installation of wholebodydynamics from codyco-modules for now, and then completly remove codyco-modules when WBC is released.
@gabrielenava has this feature been released?
After the last modifications done in the weekend, the PR is ready to be merged: https://github.com/robotology/whole-body-controllers/issues/70
what is missing: time :-D this morning was my thesis time, but in the afternoon I will proceed with merging. There are some comments/issues to properly document before and after the merging, so I need some time to do it properly :-)
whole-body-controllers 2.5 released: https://github.com/robotology/whole-body-controllers/commit/f4bfb2127524c44cbba32556ada0aaaab4abeaab
So can we close the issue?
CC @traversaro @S-Dafarra
Besides some estimator-related devices (that will be migrated to the
whole-body-estimators
repository) the only currently used part of this repo is the yarpmotorgui configuration files available incodyco-modules/src/modules/torqueBalancing/app/robots
. We need to migrate them to some other repo. Possible options are:robots-configurations
: I would avoid this asrobots-configuration
is a repo already shared by a lot of people with different interest, so it may be a good idea to not add more complexity to it.dic-robots-configuration
orwhole-body-robots-configurations
, that just contains this kind of robot-specific configuration files, but it is mantained by DIC and only contains configuration files for robots that we mantainicub-gazebo-wholebody
repo to explain that it contains configuration files for a lot of software, not only for Gazebo. Furthermore, we should modify the superbuild to ensure that this repo is installed also whenROBOTOLOGY_USES_GAZEBO
is set to off.