Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discuss the drawback of using const IParametersHandler in the Advanceable class #285

Open
GiulioRomualdi opened this issue Apr 26, 2021 · 5 comments
Labels
help wanted Extra attention is needed

Comments

@GiulioRomualdi
Copy link
Member

In #267 I added

bool initialize(std::shared_ptr<const BipedalLocomotion::ParametersHandler::IParametersHandler>)

method.

The signature of the method is in theory fine since a block should not change the handler in the initialization phase, however, in practice, this creates several issues.

For instance, in centroidal-mpc-walking I tried to instantiate the LeggedOdometry however I'm not able to do it because the LeggedOdometry::initialize() wants a non-const IParameterHandler.
https://github.com/dic-iit/bipedal-locomotion-framework/blob/4ad31d8d62ffa0e3da5b7fbef77a945124c7bb4e/src/Estimators/include/BipedalLocomotion/FloatingBaseEstimators/FloatingBaseEstimator.h#L160-L161

Here two possible solutions can be implemented:

  1. Change the signature of
    https://github.com/dic-iit/bipedal-locomotion-framework/blob/4ad31d8d62ffa0e3da5b7fbef77a945124c7bb4e/src/Estimators/include/BipedalLocomotion/FloatingBaseEstimators/FloatingBaseEstimator.h#L160-L161
    Into bool initialize(std::weak_ptr<const BipedalLocomotion::ParametersHandler::IParametersHandler> handler, std::shared_ptr<iDynTree::KinDynComputations> kindyn);
  2. Implement the copy constructor/copy assignment operator for IParameterHandler.

This first solution is temporary since we may need to expand the content of a ParameterHandler and if the object is const this is not possible.
On the other hand, I don't know how to implement the second solution. Since there is no way to get the type of a stored parameter is almost impossible to copy the content of a parameter handler into another one. (e.g. I don't know how to copy the content of a YarpParameterHandler in a StdParameterHandler and vice-versa)

cc @prashanthr05 @S-Dafarra and @traversaro

@GiulioRomualdi GiulioRomualdi added the help wanted Extra attention is needed label Apr 26, 2021
@S-Dafarra
Copy link
Member

I would go for the first solution. I would prefer if the user of the parameters handler does not modify anything.

@GiulioRomualdi
Copy link
Member Author

I would go for the first solution. I would prefer if the user of the parameters handler does not modify anything.

I agree with that however we may need to change the content of the handler in the future. For instance, several classes require the sampling_time and the user may specify it only once. e.g.

sampling_time 0.01

[IK]
some_useful_parameter "parameter"

if the ik requires the sampling time in c++ we need to expand the parameter handler used in the ik with sampling_time. This is currently not possible

void initilizeIK(std::shared_ptr<const IParameterHandler> handler, double samplingTime)
{
    auto newHandler = std::make_shared<StdParameterHandler>();
    newHandler->set(handler); //<---------------- This is not possible
    newHandler->setParameter("sampling_time", samplingTime);
   ...........
}

@S-Dafarra
Copy link
Member

S-Dafarra commented Apr 26, 2021

Ok, we need a method to clone then 🤔 I think that is doable for both the implementations we have at the moment.
Something like

virtual std::shared_ptr<IParameterHandler> IParameterHandler::clone() const = 0;

@GiulioRomualdi
Copy link
Member Author

You are right!

@GiulioRomualdi
Copy link
Member Author

#288 implements what @S-Dafarra suggested in #285 (comment)

Before closing the issue we should address point 1 of #285 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants