Decide how to proceed with the IParameterHandler interface

[GiulioRomualdi]

The IParameterHandler interface library is really useful to set and get the parameters however it has an enormous drawback. Given the implementation of the interface, a function that has as input the interface has to be template i.e.

template <class Derived>
bool Foo::initialize(const IParameterHandler<derived>& handler)
{
     // get the parameters
     return true;
}

or https://github.com/dic-iit/bipedal-locomotion-controllers/blob/53b27be78e62fc8e3d5638043f014162fd5b3bee/src/Simulator/include/BipedalLocomotionControllers/Simulator/Simulator.tpp#L18-L26

The IParametersHandler is a template class because it is implemented using the CRTP pattern. The pattern is required to implement static polymorphism and consequentially to make the follwing line of codes compiling

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>
    bool getParameter(const std::string& parameterName, T& parameter) const final
    {
         // custom code
    }
}; 

Indeed class member function template cannot be virtual., and in our case getParameter() is template to be compatible with different types (i.e. iDynTree::VectorDynSize ,int, bool, double, ...).

As discussed with @S-Dafarra having template consumer functions may over-complexify the code. For this reason, I would like to find a solution to the problem. Discussing with @traversaro and @S-Dafarra we found two possible solutions:

1. Chose a specific implementation i.e. the yarp one and pass the parameter with that implementation.

   bool Foo::initialize(const IParameterHandlerYarpImplementation& handler)
   {
        // get the parameters
        return true;
   }

   Thanks to this solution getParameter() method may remain template and in theory, we guarantee the support to the different datatypes. On the other hand, all the libraries will depend on yarp and this is very sad given the effort to make the libraries yarp free.

2. As @traversaro suggested we can decide to support only a finite amount of datatype and implement several versions of getParameter() i.e.

   class IParametersHandler
   {
   public:
       virtual bool getParameter(const std::string& parameterName, int& parameter) const = 0;
       virtual bool getParameter(const std::string& parameterName, bool& parameter) const = 0;
       virtual bool getParameter(const std::string& parameterName, double& parameter) const = 0;
       ...
   }; 
   
   class YarpImplementation : public IParametersHandler
   {
   public:
       bool getParameter(const std::string& parameterName, int& parameter) const final;
       bool getParameter(const std::string& parameterName, bool& parameter) const final;
       bool getParameter(const std::string& parameterName, double& parameter) const final;
   }; 

   Thanks to this solution standard polymorfism can be used and we drop the yarp dependency on all the libraries. On the other hand a new method has to be implemented for supporting a new datatype.

This issue is to discuss the actions that we should take for solving the presented problem.

Related discussions: #26 https://github.com/dic-iit/bipedal-locomotion-controllers/pull/7#discussion_r392261634 https://github.com/dic-iit/bipedal-locomotion-controllers/pull/13#issuecomment-583848797

[S-Dafarra]

As discussed F2F, it is important to define the purpose of the class, in my opinion. I think there are two main applications:

• retrieval of parameters from configuration files
• storing of class parameters

The yarp implementation falls in the first category. The capability of being initialized from a Searchable is quite handy. On the other hand, the main drawback here is that you cannot store and retrieve custom objects, like an iDynTree::Rotation, or a pointer to MyClass for example.

On the other side, the implementation using std::unordered_map<std::string, std::any> falls in the second category since you can potentially store whatever you want, but it doesn't seem to be initializable from a Searchable. The main problems in this context are:

• If you want to compile bipedal-locomotion-controllers without yarp, we cannot add inside this implementation anything yarp related
• When a yarp .ini file is parsed into a Searchable, groups and vectors become indistinguishable as they are both treated as "lists". See also https://github.com/dic-iit/bipedal-locomotion-controllers/pull/18#issue-378217180

Depending on the application, the limitation on the number of supported types can apply or not. Clearly, when parsing a configuration file, given the limited set of things that can be written in a plain text file, it makes sense to support only a set of types. I don't think the same for the second category.

To be honest, I prefer those solutions which fit better in the second category as they ease the definition of interfaces, basically mimicking the role of Searchable in yarp devices while granting the developer freedom about the set of data used in the class.

What remains open is then the parsing of the configuration files. In this regard, the toml option is becoming a little more interesting. For example, the ToruNiina implementation of toml stores the values into a std container (check https://github.com/ToruNiina/toml11#customizing-containers).

Edit: The marzer implementation is probably even cooler. The documentation is great (https://marzer.github.io/tomlplusplus/). In addition, from each value (called node) you can easily get the type, making it super easy to perform a conversion to std::any. See https://marzer.github.io/tomlplusplus/classtoml_1_1node.html#a03e1bbe1a0640953b7105fe40c733118

[GiulioRomualdi]

Another way is to use parameters handler only for passing the parameters from a configuration file and having another class for storing the state and parameters in a class. By the way, I don't know if storing a variable inside an std::any is the right choice since a runtime cast has to be performed every time that variable is required.

Edit: The marzer implementation is probably even cooler. The documentation is great (https://marzer.github.io/tomlplusplus/). In addition, from each value (called node) you can easily get the type, making it super easy to perform a conversion to std::any. See https://marzer.github.io/tomlplusplus/classtoml_1_1node.html#a03e1bbe1a0640953b7105fe40c733118

bool toml::node::is() seems that does not return the type of the object but tells you if the type is the one that you expect

[S-Dafarra]

Another way is to use parameters handler only for passing the parameters from a configuration file and having another class for storing the state and parameters in a class. By the way, I don't know if storing a variable inside an std::any is the right choice since a runtime cast has to be performed every time that variable is required.

That's true, also I don't like the fact that you would need to search the variable every time. It would be interesting to have a reference to a parameter, but any_cast does not seem to allow it. In the end, you will probably always store a copy of the parameter inside the class.

bool toml::node::is() seems that does not return the type of the object but tells you if the type is the one that you expect

Sorry wrong link, I was referring to the method type() (It is only listed).

[GiulioRomualdi]

That's true, also I don't like the fact that you would need to search the variable every time. It would be interesting to have a reference to a parameter, but any_cast does not seem to allow it. In the end, you will probably always store a copy of the parameter inside the class.

Ok, in this case, let's split the problem of having a parameter container inside the class from having a parameter handler for loading the parameter from a configuration file. I would try to avoid having the yarp dependency on all the components. For this reason, we can decide which parameter we support. For instance, we can decide to enable the get only for builtin type (int double ...) and stl containers (i.e. std::vector<>) In this case, it would be amazing adding the support of std::span, so we can pass an std::vector or also an iDynTree::Vector.

C++17 compatible implementation of std::span or std::span

[GiulioRomualdi]

According to this comment https://github.com/dic-iit/bipedal-locomotion-controllers/issues/27#issuecomment-599565826, we will have

bool getParameter(const std::string& key, int& value);
bool getParameter(const std::string& key, double& value);
bool getParameter(const std::string& key, bool& value);
bool getParameter(const std::string& key, std::string& value);

bool getParameter(const std::string& key, std::span<int>& value);
bool getParameter(const std::string& key, std::span<double>& value);
bool getParameter(const std::string& key, std::span<bool>& value);
bool getParameter(const std::string& key, std::span<std::string>& value);

Edit: Or even better bool getParameter(const std::string& key, std::variant<int, double, bool, std::string>& value);

[GiulioRomualdi]

Using std::span may not be the right choice since they are not resizable (of course) and sometimes we don't know the size of the parameter before getting the parameter. i.e. we don't know the number of the actuated joints before loading the configuration file.

For this reason, I would drop the span

[GiulioRomualdi]

Otherwise, the user has to add the size of the parameter (i.e. the size of the vector) I will wait for @traversaro , @S-Dafarra suggestion before proceeding

[S-Dafarra]

Using std::span may not be the right choice since they are not resizable (of course) and sometimes we don't know the size of the parameter before getting the parameter. i.e. we don't know the number of the actuated joints before loading the configuration file. Otherwise, the user has to add the size of the parameter (i.e. the size of the vector) I will wait for @traversaro , @S-Dafarra suggestion before proceeding

You can get the size of a span (https://github.com/robotology/idyntree/blob/master/src/core/include/iDynTree/Core/Span.h#L484) but, as you said, you cannot resize. Indeed, when getting a span is an indication that you don't own the object. It may be ok to have the handler returning a span such that you can store the value with your preferred container. Then, it is clear that the vector you are getting is still owned by the handler. But, how would that work for "setting" a vector?

[S-Dafarra]

Btw, I didn't get this passage:

I would try to avoid having the yarp dependency on all the components. For this reason, we can decide which parameter we support.

I believed that to remove yarp we needed something to parse configuration files. I didn't understand that the problem was in the getParameters method. Once you chose the implementation, that method shouldn't be a problem, also in the current form, right?

[GiulioRomualdi]

@S-Dafarra

You can get the size of a span (https://github.com/robotology/idyntree/blob/master/src/core/include/iDynTree/Core/Span.h#L484) but, as you said, you cannot resize. Indeed, when getting a span is an indication that you don't own the object. It may be ok to have the handler returning a span such that you can store the value with your preferred container.

Is the iDynTree::Span working also with std containers? i.e. std::vector<std::string> If yes I can use the iDynTree span otherwise this is nice.

Then, it is clear that the vector you are getting is still owned by the handler. But, how would that work for "setting" a vector?

For setting it shouldn't be a problem.

bool setParameter(const std::string& key, std::span<int>& value);

I hope it is easy to implement (I have to check) because the parameter is copied inside the handler

I believed that to remove yarp we needed something to parse configuration files. I didn't understand that the problem was in the getParameters method. Once you chose the implementation, that method shouldn't be a problem, also in the current form, right?

If getParameter() and setParameter() are not template the standard inheritance can be used (bye bye CRTP) and the pointer or reference to the interface can be passed as input to the initialize function

bool foo::initialize(const iParameterHandler & handler);

Notice here the template is no more required! Consequentially we can use pimpl again!

[GiulioRomualdi]

At least for the yarp implementation, it should be simple. The Basi implementation has to be rewritten (probably we should drop std::any and add container for each supported datatype)

std::unordered_map<std::string, bool> m_boolContainer;
std::unordered_map<std::string, std::vector<double>> m_doubleVectorContainer;
...

[S-Dafarra]

Is the iDynTree::Span working also with std containers? i.e. std::vector If yes I can use the iDynTree span otherwise this is nice.

That implementation has been strongly "inspired" by https://github.com/Microsoft/GSL/blob/master/include/gsl/span (the same from which https://github.com/tcbrindle/span was born, according to its README). I am pretty sure it works with an std::vector (see https://github.com/robotology/idyntree/blob/master/src/core/tests/SpanUnitTest.cpp#L552). Maybe it is worth checking if it can work with a vector of custom types as well.

For setting it shouldn't be a problem.

Not so fast. If you set a span, you don't transfer the ownership. It is like setting a raw pointer. You need to make sure that who set that parameter will keep it alive as long as some of the objects having the handler require it.

If getParameter() and setParameter() are not template the standard inheritance can be used (bye bye CRTP) and the pointer or reference to the interface can be passed as input to the initialize function

Sorry, you're right. I keep forgetting :sweat_smile: