Implement ParametersHandler library #13

GiulioRomualdi · 2020-02-06T18:29:46Z

This PR implements a library for retrieving parameters from configuration files. Thanks to this PR we can drop the dependency from YARP for all the consumer libraries.

The classes implemented in the library are structured as follows:

IParametersHandler<Derived> is a base template class that implements getParameter() and getGroup() method.
YarpImplementation is the specific implementation of IParametersHandler based on yarp.

The Curiously Recurring Template Pattern has been used to implement a static polymorphism. It is required because a class member function template cannot be virtual. (i.e. the following example will not compile)

class IParametersHandler
{
public:
    template<typename T>
    void bool getParameters(const std::string&, T&);
};

In the end, thanks to this library the controllers/estimators/planners will not require Yarp anymore for retrieving the parameters from configuration files. The consumer library will link ParametersHandler (it is YARP independent) and the entire configuration process will be transparent to the middleware used to retrieve the parameters (i.e. YARP/ROS ...)

@prashanthr05 @traversaro

traversaro

A minor comment. However, how is this supposed to be used? The code that consumes the parameters need to be templated on the template of IParametersHandler to be used?

src/ParametersHandler/YarpImplementation/tests/ParametesHandlerYarpTest.cpp

GiulioRomualdi · 2020-02-07T09:18:59Z

A minor comment. However, how is this supposed to be used? The code that consumes the parameters need to be templated on the template of IParametersHandler to be used?

Yes this is a drawback.
The consumer library will have a template member function like.

template <typename T>
bool initialize(std::unique_ptr<IParametersHandler<T>> params);

traversaro · 2020-02-07T09:33:24Z

I see, the main problem that I see is that would be tricky with this to implement plugins (opened using dlopen) and mantain a stable ABI, but both are things that we don't need right now, so not a big problem. I do not fully understood why it was not possible to use classical runtime polymorphism with a virtual interface with a method, however this is something that can be always changed in the future, so I think it is ok to merge this as it is.

S-Dafarra

Nice!

I would suggest adding the following, but it is not blocking:

an example with a custom parser
setter methods
printing methods
an alias (see Implement ParametersHandler library #13 (comment))

src/ParametersHandler/CMakeLists.txt

S-Dafarra · 2020-02-07T10:30:43Z

src/ParametersHandler/YarpImplementation/CMakeLists.txt

+    )
+
+  # add an executable to the project using the specified source files.
+  add_library(${LIBRARY_TARGET_NAME} SHARED ${${LIBRARY_TARGET_NAME}_SRC} ${${LIBRARY_TARGET_NAME}_HDR})


Hence there is no way to compile STATIC right?

When I try to compile STATIC I got the following error.
Probably there is something wrong with YarpUtilities

../../../../lib/libBipedalLocomotionControllersParametersHandlerYarpImplementation.a(YarpImplementation.cpp.o): In function `bool BipedalLocomotionControllers::YarpUtilities::getVectorFromSearchable<std::vector<bool, std::allocator<bool> > >(yarp::os::Searchable const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<bool, std::allocator<bool> >&)': /home/gromualdi/robot-code/bipedal-locomotion-controllers/src/YarpUtilities/include/BipedalLocomotionControllers/YarpUtilities/Helper.tpp:168: multiple definition of `bool BipedalLocomotionControllers::YarpUtilities::getVectorFromSearchable<std::vector<bool, std::allocator<bool> > >(yarp::os::Searchable const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<bool, std::allocator<bool> >&)' CMakeFiles/ParametesHandlerYarpTest.dir/ParametesHandlerYarpTest.cpp.o:/home/gromualdi/robot-code/bipedal-locomotion-controllers/src/YarpUtilities/include/BipedalLocomotionControllers/YarpUtilities/Helper.tpp:168: first defined here collect2: error: ld returned 1 exit status src/ParametersHandler/YarpImplementation/tests/CMakeFiles/ParametesHandlerYarpTest.dir/build.make:126: recipe for target 'bin/ParametesHandlerYarpTest' failed make[2]: *** [bin/ParametesHandlerYarpTest] Error 1 CMakeFiles/Makefile2:1138: recipe for target 'src/ParametersHandler/YarpImplementation/tests/CMakeFiles/ParametesHandlerYarpTest.dir/all' failed make[1]: *** [src/ParametersHandler/YarpImplementation/tests/CMakeFiles/ParametesHandlerYarpTest.dir/all] Error 2 Makefile:140: recipe for target 'all' failed make: *** [all] Error 2 gromualdi@iiticublap109:~

Probably it also needs to be compiled STATIC. @traversaro once suggested me to avoid using SHARED when adding a library (simply put nothing). Then, in the main CMakeLists you add https://github.com/robotology/how-to-export-cpp-library/blob/master/CMakeLists.txt#L98. See https://cmake.org/cmake/help/v3.0/variable/BUILD_SHARED_LIBS.html

When I try to follow what you suggested in #13 (comment) I had the following error:

Scanning dependencies of target YarpUtilities [ 8%] Building CXX object src/YarpUtilities/CMakeFiles/YarpUtilities.dir/src/Helper.cpp.o [ 16%] Linking CXX static library ../../lib/libBipedalLocomotionControllersYarpUtilities.a [ 16%] Built target YarpUtilities Scanning dependencies of target YarpUtilitiesTestMain [ 25%] Building CXX object src/YarpUtilities/tests/CMakeFiles/YarpUtilitiesTestMain.dir/YarpUtilitiesTestMain.cpp.o [ 33%] Linking CXX static library ../../../lib/libYarpUtilitiesTestMain.a [ 33%] Built target YarpUtilitiesTestMain [ 41%] Linking CXX executable ../../../bin/YarpUtilitiesTest ../../../lib/libBipedalLocomotionControllersYarpUtilities.a(Helper.cpp.o): In function `bool BipedalLocomotionControllers::YarpUtilities::getVectorFromSearchable<std::vector<bool, std::allocator<bool> > >(yarp::os::Searchable const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<bool, std::allocator<bool> >&)': /home/gromualdi/robot-code/bipedal-locomotion-controllers/src/YarpUtilities/include/BipedalLocomotionControllers/YarpUtilities/Helper.tpp:168: multiple definition of `bool BipedalLocomotionControllers::YarpUtilities::getVectorFromSearchable<std::vector<bool, std::allocator<bool> > >(yarp::os::Searchable const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<bool, std::allocator<bool> >&)' CMakeFiles/YarpUtilitiesTest.dir/YarpUtilitiesTest.cpp.o:/home/gromualdi/robot-code/bipedal-locomotion-controllers/src/YarpUtilities/include/BipedalLocomotionControllers/YarpUtilities/Helper.tpp:168: first defined here collect2: error: ld returned 1 exit status src/YarpUtilities/tests/CMakeFiles/YarpUtilitiesTest.dir/build.make:125: recipe for target 'bin/YarpUtilitiesTest' failed make[2]: *** [bin/YarpUtilitiesTest] Error 1 CMakeFiles/Makefile2:1055: recipe for target 'src/YarpUtilities/tests/CMakeFiles/YarpUtilitiesTest.dir/all' failed make[1]: *** [src/YarpUtilities/tests/CMakeFiles/YarpUtilitiesTest.dir/all] Error 2 Makefile:140: recipe for target 'all' failed

I should open a issue

Yes, you can avoid facing this now.

Ah, some good old One Definition Rule (ODR, https://en.wikipedia.org/wiki/One_Definition_Rule) violations. I do not know the details, but the tipical trick is to mark the offending function as inline or avoid explicit instantiation.

Other useful references:

https://en.cppreference.com/w/cpp/language/definition

https://gieseanw.wordpress.com/2018/10/30/oops-i-violated-odr-again/

...arametersHandler/include/BipedalLocomotionControllers/ParametersHandler/IParametersHandler.h

GiulioRomualdi · 2020-02-07T13:06:55Z

@S-Dafarra

an example with a custom parser

What do you mean?

setter methods

In case of YarpInteface the member attribute is a

const yarp::os::Searchable& m_searchable

If a setter method is added the const identifier should be dropped

printing methods

Implemented in 63d472b. The parameters can be printed as:

std::cout << handler << std::endl;

an alias (see #13 (comment))

I'm investigating on this

S-Dafarra · 2020-02-07T13:14:45Z

an example with a custom parser

What do you mean?

I was thinking of adding a test that could work as a tutorial for implementing a parser different from Yarp. For example, how would it work if the parameters are stored in a map?

If a setter method is added the const identifier should be dropped

Is the const identifier really necessary? I was thinking that it may be needed to overwrite a parameter or add other parameters to a group before passing it to a controller (like you did in walking-controllers for example).

GiulioRomualdi · 2020-02-07T13:33:51Z

I was thinking of adding a test that could work as a tutorial for implementing a parser different from Yarp. For example, how would it work if the parameters are stored in a map?

A map of std::map<std::string,std::any> can be used. I can try

GiulioRomualdi · 2020-02-07T13:34:23Z

Is the const identifier really necessary? I was thinking that it may be needed to overwrite a parameter or add other parameters to a group before passing it to a controller (like you did in walking-controllers for example).

I don't think it is necessary

S-Dafarra · 2020-02-07T13:35:52Z

A map of std::mapstd::string,std::any can be used. I can try

From where do you think I got the idea? 😁

GiulioRomualdi · 2020-02-07T13:48:41Z

Is the const identifier really necessary? I was thinking that it may be needed to overwrite a parameter or add other parameters to a group before passing it to a controller (like you did in walking-controllers for example).

I don't think it is necessary

Unfortunately, the object searchable does not have the method for adding new parameters

S-Dafarra · 2020-02-07T13:57:59Z

Unfortunately, the object searchable does not have the method for adding new parameters

What about using a Bottle instead?

GiulioRomualdi · 2020-02-07T14:43:03Z

Unfortunately, the object searchable does not have the method for adding new parameters

What about using a Bottle instead?

Passing a Bottle means that we cannot use the class with a ResourceFinder that it is actually our case. However, sometimes wrapping a Bottle in case findGroup() is called on a Searchable

S-Dafarra · 2020-02-07T14:46:41Z

Passing a Bottle means that we cannot use the class with a ResourceFinder that it is actually our case.

Can you use a Bottle as a class parameter and input a Searchable?

GiulioRomualdi · 2020-02-07T14:50:08Z

Passing a Bottle means that we cannot use the class with a ResourceFinder that it is actually our case.

Can you use a Bottle as a class parameter and input a Searchable?

You mean a yarp::os::Bottle and not a yarp::os::Bottle& right?

S-Dafarra · 2020-02-07T14:51:36Z

You mean a yarp::os::Bottle and not a yarp::os::Bottle& right?

The first (the object not the reference).

GiulioRomualdi · 2020-02-07T14:53:35Z

You mean a yarp::os::Bottle and not a yarp::os::Bottle& right?

The first (the object not the reference).

Seems that we cannot initialize a Bottle using a Searchable
no matching function for call to ‘yarp::os::Bottle::Bottle(const yarp::os::Searchable&)’

S-Dafarra · 2020-02-07T15:20:02Z

Seems that we cannot initialize a Bottle using a Searchable
no matching function for call to ‘yarp::os::Bottle::Bottle(const yarp::os::Searchable&)’

Bummer...I am afraid that also without setters it may be pretty restricting though 🤔

S-Dafarra · 2020-02-07T16:00:29Z

Seems that we cannot initialize a Bottle using a Searchable
no matching function for call to ‘yarp::os::Bottle::Bottle(const yarp::os::Searchable&)’

Ok, I think I may have found a solution. Apparently you can load a ResourceFinder object inside a Bottle via the toString() method. In fact the toString documentation of ResourceFinder says:

Return a standard text representation of the content of the object.
The representation is readable by the Bottle and Property classes.

Hence, you can do

bottle.fromString(searchable.toString());

GiulioRomualdi · 2020-02-07T16:32:18Z

In a68d06c I implemented a simple test with an homemade parameters container based on std::map<std::string, std::any>. Everything seems ok, however when I try to run the test with valgrind I had several memory leaks.

The only way to solve the problem is to edit the following line

std::unique_ptr<IParametersHandler<BasicImplementation>> parameterHandler = std::make_unique<BasicImplementation>(parameters);

in

std::unique_ptr<BasicImplementation> parameterHandler = std::make_unique<BasicImplementation>(parameters);

Am I missing something on CRPT? Do you have any idea?
cc @nunoguedelha

This is the valgrind error

colour-valgrind ./bin/ParametersHandlerTest 
==29368== Memcheck, a memory error detector
==29368== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==29368== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==29368== Command: ./bin/ParametersHandlerTest
==29368== 
======================================================================================================
===============================================================================
All tests passed (10 assertions in 1 test case)

======================================================================================================
==29368== 
==29368== HEAP SUMMARY:
==29368==     in use at exit: 4,112 bytes in 90 blocks
==29368==   total heap usage: 2,430 allocs, 2,340 frees, 387,468 bytes allocated
==29368== 
==29368== LEAK SUMMARY:
==29368==    definitely lost: 280 bytes in 7 blocks
==29368==    indirectly lost: 3,832 bytes in 83 blocks
==29368==      possibly lost: 0 bytes in 0 blocks
==29368==    still reachable: 0 bytes in 0 blocks
==29368==         suppressed: 0 bytes in 0 blocks
==29368== Rerun with --leak-check=full to see details of leaked memory
==29368== 
==29368== For counts of detected and suppressed errors, rerun with: -v
==29368== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
gromualdi@iiticublap109:~/robot-code/bipedal-locomotion-controllers/build (feature/parameters_handler)$ colour-valgrind --leak-check=full ./bin/ParametersHandlerTest 
==29389== Memcheck, a memory error detector
==29389== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==29389== Using Valgrind-3.13.0 and LibVEX; rerun with -h for copyright info
==29389== Command: ./bin/ParametersHandlerTest
==29389== 
======================================================================================================
===============================================================================
All tests passed (10 assertions in 1 test case)

======================================================================================================
==29389== 
==29389== HEAP SUMMARY:
==29389==     in use at exit: 4,112 bytes in 90 blocks
==29389==   total heap usage: 2,430 allocs, 2,340 frees, 387,468 bytes allocated
==29389== 
==29389== 16 bytes in 1 blocks are definitely lost in loss record 1 of 15
==29389==    at 0x4C3017F: operator new(unsigned long) (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==29389==    by 0x11AF39: __gnu_cxx::new_allocator<std::__detail::_Hash_node_base*>::allocate(unsigned long, void const*) (new_allocator.h:111)
==29389==    by 0x119D90: std::allocator_traits<std::allocator<std::__detail::_Hash_node_base*> >::allocate(std::allocator<std::__detail::_Hash_node_base*>&, unsigned long) (alloc_traits.h:436)
==29389==    by 0x1185FE: std::__detail::_Hashtable_alloc<std::allocator<std::__detail::_Hash_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, true> > >::_M_allocate_buckets(unsigned long) (hashtable_policy.h:2107)
==29389==    by 0x116AEE: std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_allocate_buckets(unsigned long) (hashtable.h:354)
==29389==    by 0x1147C5: void std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_assign<std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_assign(std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> > const&)::{lambda(std::__detail::_Hash_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, true> const*)#1}>(std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> > const&, std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_M_assign(std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> > const&)::{lambda(std::__detail::_Hash_node<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, true> const*)#1} const&) (hashtable.h:1085)
==29389==    by 0x112CC6: std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> >::_Hashtable(std::_Hashtable<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any>, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> >, std::__detail::_Select1st, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::__detail::_Mod_range_hashing, std::__detail::_Default_ranged_hash, std::__detail::_Prime_rehash_policy, std::__detail::_Hashtable_traits<true, false, true> > const&) (hashtable.h:1247)
==29389==    by 0x1123AC: std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::any, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> > >::unordered_map(std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::any, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> > > const&) (unordered_map.h:180)
==29389==    by 0x1123EE: BasicImplementation::BasicImplementation(std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::any, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> > > const&) (ParametersHandlerTest.cpp:28)
==29389==    by 0x112FC2: std::_MakeUniq<BasicImplementation>::__single_object std::make_unique<BasicImplementation, std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::any, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> > >&>(std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::any, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::any> > >&) (unique_ptr.h:825)
==29389==    by 0x11261F: BasicImplementation::getGroup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (ParametersHandlerTest.cpp:71)
==29389==    by 0x11415B: BipedalLocomotionControllers::ParametersHandler::IParametersHandler<BasicImplementation>::getGroup(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) const (IParametersHandler.tpp:23)
==29389==

GiulioRomualdi · 2020-02-07T19:23:38Z

@S-Dafarra

Seems that we cannot initialize a Bottle using a Searchable
no matching function for call to ‘yarp::os::Bottle::Bottle(const yarp::os::Searchable&)’

Ok, I think I may have found a solution. Apparently you can load a ResourceFinder object inside a Bottle via the toString() method. In fact the toString documentation of ResourceFinder says:

Return a standard text representation of the content of the object.
The representation is readable by the Bottle and Property classes.

Hence, you can do
bottle.fromString(searchable.toString());

setParameter() method implemented in 35eb9e0

Co-Authored-By: Silvio Traversaro <silvio.traversaro@iit.it>

…sHandler::YarpImplementation - Implement YarpImplementation::setParameter() function

…as shared library

GiulioRomualdi · 2020-02-08T18:51:12Z

The memory leak shown in #13 (comment) has been fixed in 729ca1e

GiulioRomualdi · 2020-02-09T14:01:35Z

@traversaro

I do not fully understood why it was not possible to use classical runtime polymorphism with a virtual interface with a method, however this is something that can be always changed in the future, so I think it is ok to merge this as it is.

I would like to use classical polymorphism i.e.

class IParametersHandler
{
public:
    template <typename T>
    virtual bool getParameter(const std::string& parameterName, T& parameter) const = 0;
}; 

class YarpImplementation : public IParametersHandler
{
public:
    template <typename T>
    virtual bool getParameter(const std::string& parameterName, T& parameter) const
    {
         // custom code
    }
};

However seems that a a class member function template cannot be virtual. getParameter() should be template becaouse I would avoid to implement different functions for dofferent parameters type (int, double, iDynTree::VectorDynSize, iDynTree::VectorFixSize etc...).

If you have any idea of how to solve the problem without using CRTP let me know. I would prefer to not use CRTP because the consumer libraries must have template member functions. Consequentially pimpl paradigm cannot be used in the consumer library.

class ConsmerLibrary
{
    ...
public:
    template <typename T>
    bool initialize(std::unique_ptr<IParameterHandler<T>> handler);
    ...
}

traversaro · 2020-02-10T12:03:31Z

@GiulioRomualdi , thanks, everything is clear to me after your explanation, clearly you need to use CRTP if you want to support arbitrary data types that support a given "Concept", i.e. a group of methods. I suspect the proper solution then is to agree on canonical types for vector and matrices, and just use them also in the IParameterHandler interface, but for now what you did seems fine.

GiulioRomualdi · 2020-02-10T12:12:28Z

Ok if @S-Dafarra agrees, I merge the PR

S-Dafarra · 2020-02-10T13:27:24Z

src/ParametersHandler/tests/ParametersHandlerTest.cpp

+
+    template <typename T> void setParameter(const std::string& parameterName, const T& parameter)
+    {
+        m_map.insert({parameterName, std::make_any<T>(parameter)});


I know it's just a test, but can you use the operator[] to deal also with the case where the parameter already exists?

Yes! Done in #13

S-Dafarra · 2020-02-10T13:29:06Z

src/ParametersHandler/tests/ParametersHandlerTest.cpp

+        std::vector<std::string> element;
+        REQUIRE(groupHandler->getParameter("Donald's nephews", element));
+        REQUIRE(element == std::vector<std::string>{"Huey", "Dewey", "Louie"});
+    }


Can you test also the print function as in https://github.com/dic-iit/bipedal-locomotion-controllers/pull/13/files#diff-2187c40c7243c514d7eca0ce41d286eeR72-R75?

Done in 987775d

S-Dafarra · 2020-02-10T13:30:25Z

src/ParametersHandler/tests/ParametersHandlerTest.cpp

+        return std::make_unique<BasicImplementation>(map);
+    }
+
+    friend std::ostream& operator<<(std::ostream& os, const BasicImplementation& handler)


Why is the friend required? Wouldn't it be easier with a toString() method?

As discussed F2F the toString() function has been implemented in #13

S-Dafarra

Awesome!

GiulioRomualdi added this to the Iteration 36 - Release of the release milestone Feb 6, 2020

GiulioRomualdi requested review from S-Dafarra and MiladShafiee February 6, 2020 18:29

GiulioRomualdi self-assigned this Feb 6, 2020

GiulioRomualdi added the enhancement New feature or request label Feb 6, 2020

traversaro requested changes Feb 7, 2020

View reviewed changes

src/ParametersHandler/YarpImplementation/tests/ParametesHandlerYarpTest.cpp Outdated Show resolved Hide resolved

traversaro approved these changes Feb 7, 2020

View reviewed changes

S-Dafarra approved these changes Feb 7, 2020

View reviewed changes

GiulioRomualdi force-pushed the feature/parameters_handler branch from 448f528 to f718bdf Compare February 7, 2020 14:02

GiulioRomualdi mentioned this pull request Feb 8, 2020

Unable to compile YarpUtilities as static library #14

Closed

GiulioRomualdi added 2 commits February 8, 2020 19:23

Implement the first vetsion of the ParametersHandler

5679bcb

Enable complitation of the ParameterHandler library

8102760

GiulioRomualdi and others added 11 commits February 8, 2020 19:23

Overload operator<<() in ParametersHandler library

0b2face

Bugfix in header installation in ParametersHandler library

c3996c6

FIx typo ParametesHandlerYarpTest.cpp

2cb0382

Co-Authored-By: Silvio Traversaro <silvio.traversaro@iit.it>

Fix Typos in YarpImplementation/tests for the ParametersHandler library

8848762

Implement simple test for parameters handler library

302cdf4

Add setParameter() method in IParametersHandler class

c97ba90

- Use yarp::os::Property instead of yarp::os::Searchable in Parameter…

89b179f

…sHandler::YarpImplementation - Implement YarpImplementation::setParameter() function

Update ParametersHandlerYarpTest.cpp file

ac47432

Update ParametersHandlerTest.cpp

33c7d16

Update the ParametersHandlerYarpTest in ParametersHandler library

e07dd3c

Avoid forcing to compile ParametersHandlerYarpImplementation library …

6f2e259

…as shared library

GiulioRomualdi force-pushed the feature/parameters_handler branch from c7dbe38 to 6f2e259 Compare February 8, 2020 18:26

Add Destructors in ParametersHandler library

729ca1e

GiulioRomualdi added 2 commits February 8, 2020 20:08

Add unique_ptr public alias in IParametersHandler class

02efab5

Fix typo in CMakeLists.txt file of ParametersHandler YarpImplementation

8d7ae30

S-Dafarra reviewed Feb 10, 2020

View reviewed changes

GiulioRomualdi added 5 commits February 10, 2020 16:40

Implemented toString() method in ParametersHandler library

7c70533

Improve documentation in ParametersHandler library

3c23233

Use operator[] in setParameter function in ParametersHandlerTest

d52881f

Test printing function in ParametersHandlerTest.cpp

987775d

Update the CHANGELOG.md

74bdb96

S-Dafarra approved these changes Feb 10, 2020

View reviewed changes

GiulioRomualdi merged commit 5c6a94e into master Feb 10, 2020

GiulioRomualdi deleted the feature/parameters_handler branch February 10, 2020 16:20

This was referenced Mar 13, 2020

Find a way for passing parameters for testing the components #26

Closed

Decide how to proceed with the IParameterHandler interface #27

Closed

Implement ParametersHandler library #13

Implement ParametersHandler library #13

Conversation

GiulioRomualdi commented Feb 6, 2020

traversaro left a comment

Choose a reason for hiding this comment

GiulioRomualdi commented Feb 7, 2020 • edited

traversaro commented Feb 7, 2020

S-Dafarra left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

S-Dafarra Feb 7, 2020 • edited

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

GiulioRomualdi commented Feb 7, 2020

S-Dafarra commented Feb 7, 2020

GiulioRomualdi commented Feb 7, 2020 • edited

GiulioRomualdi commented Feb 7, 2020 • edited

S-Dafarra commented Feb 7, 2020

GiulioRomualdi commented Feb 7, 2020

S-Dafarra commented Feb 7, 2020

GiulioRomualdi commented Feb 7, 2020

S-Dafarra commented Feb 7, 2020 • edited

GiulioRomualdi commented Feb 7, 2020 • edited

S-Dafarra commented Feb 7, 2020

GiulioRomualdi commented Feb 7, 2020

S-Dafarra commented Feb 7, 2020

S-Dafarra commented Feb 7, 2020

GiulioRomualdi commented Feb 7, 2020

GiulioRomualdi commented Feb 7, 2020

GiulioRomualdi commented Feb 8, 2020

GiulioRomualdi commented Feb 9, 2020

traversaro commented Feb 10, 2020

GiulioRomualdi commented Feb 10, 2020

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

S-Dafarra left a comment

Choose a reason for hiding this comment

GiulioRomualdi commented Feb 7, 2020 •

edited

S-Dafarra left a comment •

edited

S-Dafarra Feb 7, 2020 •

edited

GiulioRomualdi commented Feb 7, 2020 •

edited

GiulioRomualdi commented Feb 7, 2020 •

edited

S-Dafarra commented Feb 7, 2020 •

edited

GiulioRomualdi commented Feb 7, 2020 •

edited