robotology / idyntree

Multibody Dynamics Library designed for Free Floating Robots
BSD 3-Clause "New" or "Revised" License
156 stars 65 forks source link

Checking time data for `CubicSpline` in the wrong place #1075

Closed Woolfrey closed 4 months ago

Woolfrey commented 1 year ago

The iDynTree::CubicSpline does not check if the time data is in increasing order when calling the setData() function.

I wrote my own Cartesian trajectory generator that builds on the iDynTree::CubicSpline class which throws an error in the constructor:

if(not this->spline[i].setData(iDynTree::VectorDynSize(times), iDynTree::VectorDynSize(points[i])))
{
     errorMessage += "Unable to set the spline data.";
     throw std::runtime_error(errorMessage);
}

This is meant to stop the robot from moving if bad data is given.

However, during testing last week the robot lost control. We discovered that it was because a trajectory had been created where the time data was not increasing. iDynTree::CubicSpline only checks the time data when calling evaluatePoint() which I call in the middle of a control loop:

[ERROR][CUBICSPLINE] The input points are expected to be consecutive, strictly increasing in the time variable.  

It would be good to check the time data in the setData() function so that the user is informed immediately of any problems.

traversaro commented 1 year ago

Thanks for the issue @Woolfrey ! @GiulioRomualdi @S-Dafarra do you have any opinion on this?

S-Dafarra commented 1 year ago

I guess it is something I did not consider back in time. I suspect I wanted to make the setting of the data quick, while the computations are made when all the data is available (interpolation points + boundary conditions). I guess we could call computePhasesDuration immediately after setting the data.

In any case, I guess that you can work around this by adding a evaluatePoint(0) when you want to check the data, since this will trigger the checks.