ami-iit / bipedal-locomotion-framework

Suite of libraries for achieving bipedal locomotion on humanoid robots
https://ami-iit.github.io/bipedal-locomotion-framework/
BSD 3-Clause "New" or "Revised" License
132 stars 36 forks source link

Modify the unicycle planner #844

Closed LoreMoretti closed 3 weeks ago

LoreMoretti commented 2 months ago

This PR modifies the current implementation of the blf unicycle planner which is now named Unicycle Trajectory Planner.

This planner is in charge of generating the:

  1. footsteps
  2. DCM trajectory
  3. CoM trajectory

over the trajectory time horizon, which is a configuration parameter of the planner itself.

This PR should be merged before #845.

LoreMoretti commented 2 months ago

Not sure why I am getting the following build error:


In file included from /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/Planners/src/UnicyclePlanner.cpp:15:
/home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/Planners/include/BipedalLocomotion/Planners/UnicyclePlanner.h:199:52: error: ‘UnicycleController’ has not been declared
  199 |     const std::string& unicycleControllerAsString, UnicycleController& unicycleController);
      |                                                    ^~~~~~~~~~~~~~~~~~
/home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/Planners/src/UnicyclePlanner.cpp:106:52: error: ‘UnicycleController’ has not been declared
  106 |     const std::string& unicycleControllerAsString, UnicycleController& unicycleController)
      |                                                    ^~~~~~~~~~~~~~~~~~

since UnicycleController is contained in the header file included by src/Planners/include/BipedalLocomotion/Planners/UnicyclePlanner.h:21

cc @traversaro @GiulioRomualdi @S-Dafarra

traversaro commented 2 months ago

Not sure why I am getting the following build error:


In file included from /home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/Planners/src/UnicyclePlanner.cpp:15:
/home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/Planners/include/BipedalLocomotion/Planners/UnicyclePlanner.h:199:52: error: ‘UnicycleController’ has not been declared
  199 |     const std::string& unicycleControllerAsString, UnicycleController& unicycleController);
      |                                                    ^~~~~~~~~~~~~~~~~~
/home/runner/work/bipedal-locomotion-framework/bipedal-locomotion-framework/src/Planners/src/UnicyclePlanner.cpp:106:52: error: ‘UnicycleController’ has not been declared
  106 |     const std::string& unicycleControllerAsString, UnicycleController& unicycleController)
      |                                                    ^~~~~~~~~~~~~~~~~~

since UnicycleController is contained in the header file included by src/Planners/include/BipedalLocomotion/Planners/UnicyclePlanner.h:21

cc @traversaro @GiulioRomualdi @S-Dafarra

The fact that you are including a file UnicyclePlanner.h inside a UnicyclePlanner.h is quite confusing, and I guess also the preprocessor is getting confused. What I guess is happening is that BLF's UnicyclePlanner.h is recursively included instead of unicycle-footstep-planner one (that is the reason why the include headers are contained in a dedicated directory and not directly installed <install_prefix>/include, but I guess that that ship has sailed for unicycle-footstep-planner.

To be sure about this, you can check the include directories being passed to the compiler (and hence to the preprocessor) by compiling with make VERBOSE=1 or similar. Where are you obtaining this error?

LoreMoretti commented 2 months ago

Where are you obtaining this error?

I have this error in the CI.

On my machine the compilation runs fine. Also, in VSCode, I see all the include statements resolved correctly.

To be sure about this, you can check the include directories being passed to the compiler (and hence to the preprocessor) by compiling with make VERBOSE=1 or similar.

Is there a way to do it in the CI?

LoreMoretti commented 2 months ago

The fact that you are including a file UnicyclePlanner.h inside a UnicyclePlanner.h is quite confusing, and I guess also the preprocessor is getting confused. What I guess is happening is that BLF's UnicyclePlanner.h is recursively included instead of unicycle-footstep-planner one (that is the reason why the include headers are contained in a dedicated directory and not directly installed /include, but I guess that that ship has sailed for unicycle-footstep-planner.

@traversaro, I see your point.

Alternatively, I can rename the blf UnicyclePlanner to UnicycleTrajectoryPlanner (note that a UnicycleTrajectoryGenerator is being provided by #845), to avoid conflicts with the UnicyclePlanner of unicycle-footstep-planner.

What do you think?

cc @GiulioRomualdi @S-Dafarra

traversaro commented 2 months ago

The fact that you are including a file UnicyclePlanner.h inside a UnicyclePlanner.h is quite confusing, and I guess also the preprocessor is getting confused. What I guess is happening is that BLF's UnicyclePlanner.h is recursively included instead of unicycle-footstep-planner one (that is the reason why the include headers are contained in a dedicated directory and not directly installed /include, but I guess that that ship has sailed for unicycle-footstep-planner.

@traversaro, I see your point.

Alternatively, I can rename the blf UnicyclePlanner to UnicycleTrajectoryPlanner (note that a UnicycleTrajectoryGenerator is being provided by #845), to avoid conflicts with the UnicyclePlanner of unicycle-footstep-planner.

What do you think?

cc @GiulioRomualdi @S-Dafarra

If it is not too ugly, I think that is worth to do to avoid future similar incompatibilities.

traversaro commented 2 months ago

Where are you obtaining this error?

I have this error in the CI.

On my machine the compilation runs fine. Also, in VSCode, I see all the include statements resolved correctly.

To be sure about this, you can check the include directories being passed to the compiler (and hence to the preprocessor) by compiling with make VERBOSE=1 or similar.

Is there a way to do it in the CI?

Actually if the problem is there, I guess it could be that the unicycle-footstep-planner used in the CI is too old? See https://github.com/LoreMoretti/bipedal-locomotion-framework/blob/feature/unicyclePlanner/.github/workflows/ci.yml#L26C24-L26C64 ?

S-Dafarra commented 2 months ago

Where are you obtaining this error?

I have this error in the CI. On my machine the compilation runs fine. Also, in VSCode, I see all the include statements resolved correctly.

To be sure about this, you can check the include directories being passed to the compiler (and hence to the preprocessor) by compiling with make VERBOSE=1 or similar.

Is there a way to do it in the CI?

Actually if the problem is there, I guess it could be that the unicycle-footstep-planner used in the CI is too old? See https://github.com/LoreMoretti/bipedal-locomotion-framework/blob/feature/unicyclePlanner/.github/workflows/ci.yml#L26C24-L26C64 ?

Yes, that's too old!

LoreMoretti commented 2 months ago

Yes, that's too old!

Ok, I will update it. Thanks!

Though, I will try anyway to perform the renaming from UnicyclePlanner to UnicycleTrajectoryPlanner which should make things clearer.

LoreMoretti commented 2 months ago
  1. Regarding the conda CI, it seems like some Unexpected Error occurs due to time out when downloading conda packages. Maybe just re-running the CI will fix it.

  2. Instead regarding the following CI error:

    [ 75%] Linking CXX executable ../../../bin/UnicycleTrajectoryPlannerUnitTests
    /usr/bin/ld: ../../../lib/libBipedalLocomotionFrameworkUnicycle.so.0.18.100: undefined reference to `yarp::os::ResourceFinder::getResourceFinderSingleton()'
    /usr/bin/ld: ../../../lib/libBipedalLocomotionFrameworkUnicycle.so.0.18.100: undefined reference to `yarp::os::ResourceFinder::findFileByName(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&)'
    collect2: error: ld returned 1 exit status
    make[2]: *** [src/Planners/tests/CMakeFiles/UnicycleTrajectoryPlannerUnitTests.dir/build.make:108: bin/UnicycleTrajectoryPlannerUnitTests] Error 1
    make[1]: *** [CMakeFiles/Makefile2:4054: src/Planners/tests/CMakeFiles/UnicycleTrajectoryPlannerUnitTests.dir/all] Error 2
    make[1]: *** Waiting for unfinished jobs....

I think (at least on my machine I was able to do it) it can be fixed by adding BipedalLocomotion::ParametersHandlerYarpImplementation to the PUBLIC_LINK_LIBRARIES used to build the Unicycle library with this CMakeLists.txt, as it follows:

if (FRAMEWORK_COMPILE_Unicycle)

  set(H_PREFIX include/BipedalLocomotion/Planners)

  add_bipedal_locomotion_library(
    NAME Unicycle
    PUBLIC_HEADERS ${H_PREFIX}/UnicycleTrajectoryPlanner.h
    SOURCES src/UnicycleTrajectoryPlanner.cpp
    PUBLIC_LINK_LIBRARIES BipedalLocomotion::ParametersHandler BipedalLocomotion::ParametersHandlerYarpImplementation BipedalLocomotion::System BipedalLocomotion::Contacts Eigen3::Eigen
    PRIVATE_LINK_LIBRARIES BipedalLocomotion::TextLogging iDynTree::idyntree-core UnicyclePlanner BipedalLocomotion::ContinuousDynamicalSystem BipedalLocomotion::ManifConversions
    INSTALLATION_FOLDER Planners)

endif()

Though, I do not fully understand why.

Indeed, I thought that this was not needed since I already see among the PUBLIC_LINK_LIBRARIES the BipedalLocomotion::ParametersHandler one. And, looking at the BipedalLocomotion::ParametersHandler CMakeLists.txt, I see the following lines:

set(H_PREFIX include/BipedalLocomotion/ParametersHandler)

add_bipedal_locomotion_library(
  NAME                  ParametersHandler
  PUBLIC_HEADERS        ${H_PREFIX}/IParametersHandler.h ${H_PREFIX}/StdImplementation.h ${H_PREFIX}/StdImplementation.tpp
  SOURCES                src/StdImplementation.cpp
  PUBLIC_LINK_LIBRARIES BipedalLocomotion::GenericContainer BipedalLocomotion::TextLogging
  SUBDIRECTORIES        tests YarpImplementation TomlImplementation)

In particular I see that among the SUBDIRECTORIES there is also the YarpImplementation, which made me think that also the library BipedalLocomotion::ParametersHandlerYarpImplementation gets automatically included at the Linking Stage.

Am I missing something?

cc @traversaro

S-Dafarra commented 2 months ago

In particular I see that among the SUBDIRECTORIES there is also the YarpImplementation, which made me think that also the library BipedalLocomotion::ParametersHandlerYarpImplementation gets automatically included at the Linking Stage.

Am I missing something?

The SUBDIRECTORIES field adds directories to the Cmake project, but it does not add the libraries in the list of linked libraries.

LoreMoretti commented 2 months ago

Indeed I was wrong in this comment.

I think the library I am missing to link is YARP::YARP_os. The reason why adding instead BipedalLocomotion::ParametersHandlerYarpImplementation was working is because this library has YARP::YARP_os among its PUBLIC_LINK_LIBRARIES.

LoreMoretti commented 1 month ago

Hi @GiulioRomualdi could we merge this PR?

GiulioRomualdi commented 3 weeks ago

CI failure not related to the PR