Decide how to proceed with the IParameterHandler interface about bipedal-locomotion-framework HOT 12 CLOSED

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 #18 (comment)

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

from bipedal-locomotion-framework.

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

from bipedal-locomotion-framework.

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).

from bipedal-locomotion-framework.

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

from bipedal-locomotion-framework.

According to this comment #27 (comment), 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);

from bipedal-locomotion-framework.

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

from bipedal-locomotion-framework.

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

from bipedal-locomotion-framework.

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?

from bipedal-locomotion-framework.

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?

from bipedal-locomotion-framework.

@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!

from bipedal-locomotion-framework.

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;
...

from bipedal-locomotion-framework.

Is the iDynTree::Span working also with std containers? i.e. std::vectorstd::string 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 😅

from bipedal-locomotion-framework.