[fbeV1] simple floating base estimation algorithm #1

prashanthr05 · 2019-04-25T14:13:57Z

TODO: to organize the repository structure in a way that the implementation of this version of base estimator is separately accessible and coherent with the other whole-body-estimators to be implemented in the future.

Also need to take into account the current refactoring phase of the estimator-framework.

traversaro

Some comments.

traversaro · 2019-04-26T07:28:48Z

include/icubFloatingBaseEstimatorV1.h

+################################################################################
+#                                                                              #
+# Copyright (C) 2018 Fondazione Istituto Italiano di Tecnologia (IIT)          #
+# All Rights Reserved.                                                         #


Please use the usual license LGPL2+ , see https://github.com/robotology/blockfactory for an example of nice C++ license headers.

Addressed in 714c6cc

traversaro · 2019-04-26T07:28:57Z

include/icubFloatingBaseEstimatorV1.h

+#include <iDynTree/KinDynComputations.h>
+#include <iDynTree/ModelIO/ModelLoader.h>
+#include <iDynTree/yarp/YARPConversions.h>
+#include <iDynTree/Core//EigenHelpers.h>


Suggested change

#include <iDynTree/Core//EigenHelpers.h>

#include <iDynTree/Core/EigenHelpers.h>

Fixed in 714c6cc

traversaro · 2019-04-26T07:31:03Z

include/icubFloatingBaseEstimatorV1.h

+    namespace dev {
+
+
+    class icubFloatingBaseEstimatorV1 : public yarp::dev::DeviceDriver,


It would be great if we could avoid explicit reference to iCub in the device name, in the end even if there are a few icub-ism I think it is quite a general legged robot estimator, so a more general name could come handy for the future.

Addressed in 714c6cc

traversaro · 2019-04-26T07:32:28Z

CMakeLists.txt

+target_link_libraries(icubFloatingBaseEstimatorV1 ${YARP_LIBRARIES}
+                                                  ${iDynTree_LIBRARIES}
+                                                  floatingBaseEstimationRPC-service
+                                                  ${codyco-modules_LIBRARIES})


Which libraries are you using from codyco-modules? The idea was to move all the useful devices from codyco-modules here, so it would be great if we could avoid adding this dependency.

Fixed in 714c6cc

traversaro · 2019-04-26T07:33:57Z

Note that to mark draft PR, instead of marking them as WIP you can use draft PR, see https://github.blog/2019-02-14-introducing-draft-pull-requests/ .

traversaro · 2019-04-26T07:35:28Z

I am fine in copy&pasting the code from walking-controllers if it is to proceed forward fast, but it would be great if we have a strategy (i.e. moving this utilities somewhere else) to avoid this duplication.

DanielePucci · 2019-04-26T07:57:31Z

@prashanthr05 @traversaro
We may discuss this today F2F

prashanthr05 · 2019-04-26T11:20:51Z

With @traversaro, we agreed that this repository should be a common-place for any estimator software developments like base-estimator (v1, v2, etc), wholebodydynamics device...

We discussed f2f a proper repository structure to have devices like base-estimator-v1, wholebodydynamics separate but also room for any common libraries that might be used.
Also, at the moment it should be noted that some libraries used by the estimator device are also used by walking-controllers.
- The actual solution would be to have a common repository for these library factors.
- The temporary solution would be to not have same library names for the factors present in both walking-controllers and whole-body-estimators in order to avoid any shared library conflicts.

I shall remodify the repository structure as desired.

Meanwhile in another PR, I will work on porting the wholebodydynamics to this repository from codyco-modules and avoid any dependencies on codyco-modules from this repository.

cc @DanielePucci

prashanthr05 · 2019-06-13T11:36:43Z

cc @traversaro I think this PR is ready for review.

GiulioRomualdi · 2019-07-01T10:46:00Z

devices/baseEstimatorV1/app/robots/iCubGenova04/wholebodydynamics-external.xml

@@ -0,0 +1,81 @@
+<!--


Probably you don't need this file because on the real platform wholebodydynamics runs independently from the base estimation device

Addressed in 82d978f

traversaro · 2019-07-01T10:50:33Z

CMakeLists.txt

+find_package(YARP REQUIRED)
+if(${YARP_VERSION} VERSION_LESS ${YARP_REQUIRED_VERSION})
+    message(FATAL_ERROR "YARP version ${YARP_VERSION} not sufficient, at least version ${YARP_REQUIRED_VERSION} is required.")
+endif()


Just to understand, why don't you simply write:

find_package(YARP 3.0.1 REQUIRED)

?

True. Mindless ctrl+c ctrl+v.

Addressed in 82d978f

traversaro · 2019-07-01T10:51:32Z

README.md

+Notice: `sudo` is not necessary if you specify the `CMAKE_INSTALL_PREFIX`. In this case it is necessary to add in the `.bashrc` or `.bash_profile` the following lines:
+``` sh
+export WBDEstimator_INSTALL_DIR=/path/where/you/installed
+export LD_LIBRARY_PATH=${LD_LIBRARY_PATH}:${WBDEstimator_INSTALL_DIR}/lib/yarp


In the future perhaps we can remove the need for this line by adding rpath handling.

I think RPATH handling is already done in the CMakeLists. It's just that I didn't fully understand the implications of RPATH.

I would like to understand,
does adding

add_install_rpath_support(BIN_DIRS "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_BINDIR}" LIB_DIRS "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}" INSTALL_NAME_DIR "${CMAKE_INSTALL_PREFIX}" DEPENDS ENABLE_RPATH USE_LINK_PATH)

in your CMakeLists.txt add the generated dynamic libraries to the LD_LIBRARY_PATH automatically?

Alright the documentation in add_install_rpath_support details this.

Addressed in 82d978f

traversaro · 2019-07-01T10:53:36Z

cmake/AddUninstallTarget.cmake

+endif()
+add_custom_target(${_uninstall} COMMAND "${CMAKE_COMMAND}" -P "${_filename}")
+set_property(TARGET ${_uninstall} PROPERTY FOLDER "CMakePredefinedTargets")
+


For AddUninstallTarget and AddInstallRPATHSupport, can you get the latest versions from https://github.com/robotology/ycm/tree/master/modules ? A few problems have been fixed recently.

Addressed in 82d978f

traversaro · 2019-07-01T10:54:09Z

devices/baseEstimatorV1/CMakeLists.txt

+find_package(iDynTree REQUIRED)
+if(${iDynTree_VERSION} VERSION_LESS ${iDynTree_REQUIRED_VERSION})
+    message(FATAL_ERROR "iDyntree version ${iDynTree_VERSION} not sufficient, at least version ${iDynTree_REQUIRED_VERSION} is required.")
+endif()


As before, why not simply find_package(iDynTree 0.11 REQUIRED)?

Addressed in 82d978f

traversaro · 2019-07-01T10:54:28Z

devices/baseEstimatorV1/CMakeLists.txt

+
+set(iDynTree_REQUIRED_VERSION 0.11.0)
+
+set(CMAKE_INCLUDE_CURRENT_DIR TRUE)


Is this necessary?

Removed in 82d978f

traversaro · 2019-07-01T11:46:49Z

Sorry for the delay!

prashanthr05 · 2019-07-01T12:11:57Z

@traversaro Thanks Silvio!

@GiulioRomualdi is currently testing this device with torque-walking, if he gives a thumbs up with the tests, then we can merge it to devel and master.

GiulioRomualdi · 2019-07-01T13:27:25Z

Tested in simulation, it works as expected.

GiulioRomualdi · 2019-07-01T13:56:08Z

I don't know if it is a bug or something wanted but every time I run the code the device returns the following warnings

[DEBUG]Reading file /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml 
[WARNING]Invalid syntax while loading /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml at line 1 . No DTD found. Assuming version yarprobotinterfaceV1.0 
[DEBUG]yarprobotinterface: using xml parser for DTD v1.x 
[DEBUG]Reading file /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml 
[WARNING]Invalid syntax while loading /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml at line 9 . "robot" element should contain the "portprefix" attribute. Using "name" attribute

prashanthr05 · 2019-07-01T14:06:23Z

I don't know if it is a bug or something wanted but every time I run the code the device returns the following warnings

[DEBUG]Reading file /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml 
[WARNING]Invalid syntax while loading /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml at line 1 . No DTD found. Assuming version yarprobotinterfaceV1.0 
[DEBUG]yarprobotinterface: using xml parser for DTD v1.x 
[DEBUG]Reading file /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml 
[WARNING]Invalid syntax while loading /home/gromualdi/robot-code/robotology-superbuild/build/install/share/yarp/robots/iCubGazeboV2_5/launch-fbe-analogsens.xml at line 9 . "robot" element should contain the "portprefix" attribute. Using "name" attribute

This bug is because the latest YARP requires the DTD version to be 3.0x. And also few of the soft requirements of the yarprobotinreface xml syntax are being violated.

I will fix it in a commit and push, then we can merge.

prashanthr05 · 2019-07-01T14:35:53Z

@traversaro you could merge it now, thanks!

traversaro · 2019-07-03T09:56:24Z

You are the mantainer, so feel free to merge it!

prashanthr05 · 2019-07-03T15:09:52Z

@traversaro @GiulioRomualdi Merged, thank you guys!

[fbeV1] simple floating base estimation algorithm

8012719

traversaro requested changes Apr 26, 2019

View reviewed changes

[fbev1] refactor repo structure

714c6cc

prashanthr05 mentioned this pull request Jun 3, 2019

Move WalkingLogger and Utils to a common location robotology/walking-controllers#34

Closed

traversaro mentioned this pull request Jun 9, 2019

Move wholebodydynamics YARP device to separate repository robotology-legacy/codyco-modules#158

Closed

[fbeV1] update docs

76d8f27

prashanthr05 changed the title ~~[WIP] [fbeV1] simple floating base estimation algorithm~~ [fbeV1] simple floating base estimation algorithm Jun 13, 2019

GiulioRomualdi reviewed Jul 1, 2019

View reviewed changes

traversaro requested changes Jul 1, 2019

View reviewed changes

[fbeV1] address reviews

82d978f

traversaro approved these changes Jul 1, 2019

View reviewed changes

[fbeV1] fix yarprobotinterface warnings

fd3c170

prashanthr05 merged commit eb39eb2 into robotology:devel Jul 3, 2019

prashanthr05 deleted the simple-base-estimator branch July 3, 2019 15:10

	#include <iDynTree/Core//EigenHelpers.h>
	#include <iDynTree/Core/EigenHelpers.h>

		namespace dev {


		class icubFloatingBaseEstimatorV1 : public yarp::dev::DeviceDriver,


		set(iDynTree_REQUIRED_VERSION 0.11.0)

		set(CMAKE_INCLUDE_CURRENT_DIR TRUE)

[fbeV1] simple floating base estimation algorithm #1

[fbeV1] simple floating base estimation algorithm #1

Conversation

prashanthr05 commented Apr 25, 2019

traversaro left a comment

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

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

Choose a reason for hiding this comment

traversaro commented Apr 26, 2019

traversaro commented Apr 26, 2019

DanielePucci commented Apr 26, 2019

prashanthr05 commented Apr 26, 2019

prashanthr05 commented Jun 13, 2019

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

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

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

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 Jul 1, 2019

prashanthr05 commented Jul 1, 2019

GiulioRomualdi commented Jul 1, 2019

GiulioRomualdi commented Jul 1, 2019

prashanthr05 commented Jul 1, 2019 • edited Loading

prashanthr05 commented Jul 1, 2019

traversaro commented Jul 3, 2019

prashanthr05 commented Jul 3, 2019

prashanthr05 commented Jul 1, 2019 •

edited

Loading