Access to optimization parameters

[disRecord]

There are many important private parameters (e.g. dt_constraint_range_of_motion_ or duration_base_polynomial_) in towr::Parameters class which values are hardcoded. So it is impossible to change their value without recompilation of library. Shouldn't they be public?

There is the same question about SetGaits() in towr::GaitGenerator. Because this function is protected it is impossible to create custom gaits and change number of steps.

[awinkler]

Yeah, that makes sense, let me look into how to make that more convenient :+1:

[disRecord]

I provided possible changes in pull request above. (I hope it is not double work). I tested it with towr_app, it seems nothing is broken.

But I bumped it following issue. When library is compiled in DEBUG mode, optimize gait is set for Hyq robot after a few iterations application fails:

   1  0.0000000e+00 2.88e+03 2.66e+01   0.1 9.69e+01    -  1.10e-01 9.82e-03f  1
towr_ros_app: /home/oleg/rosws/towrws/src/towr/towr/src/phase_durations.cc:86: virtual void towr::PhaseDurations::SetVariables(const VectorXd&): Assertion `durations_.back()>0' failed.

It is not related with my code because it happens even if undo my commits.

[awinkler]

Thanks! Yeah, the issue is known and I added an explanation for this assert for developers digging into the code, see: https://github.com/ethz-adrl/towr/blob/d30c4a06a86e8378ce5dea813071d0ef6d04070b/towr/src/phase_durations.cc#L82

[disRecord]

I also thought about storing gait parameters in some kind of map.

In current implementation if new constraint type is added corresponding type and parameters should be added to Parameters. It's an additional and maybe unnecessary dependency.

Also if you decide to load parameters from ROS Parameter Server or from XML/YAML you have to write something like this

if (ros::param::getCached("~duration_base_polynomial", dvalue)) {
    params.duration_base_polynomial_ = dvalue;
}
if (ros::param::getCached("~dt_constraint_base_motion", dvalue)) {
    params.dt_constraint_base_motion_ = dvalue;
}

and so on for each parameter.

Whereas map version can be loaded much simple:

XmlRpcValue xmlparams;
ros::param::get("~towr_parameters", xmlparams);
for(auto param_it = xmlparams.begin(); param_it != xmlparams.end(); param_it++) {
    params.setParameter(param_it->first, static_cast<double>(param_it->second));
}

Still I do not think that such changes should be introduced immediately. But they can be considered as a way to improve towr API.

[awinkler]

Yes, that could be helpful, I agree. That could possibly replace/enhance the current terminal UI. II'll think about this in future releases, like you mentioned, mostly trying to keep dependencies low at this point.