pantor / ruckig

Motion Generation for Robots and Machines. Real-time. Jerk-constrained. Time-optimal.
https://ruckig.com
MIT License
688 stars 162 forks source link

Segmentation fault in VelocityStep1::addProfile #116

Closed fricher closed 2 years ago

fricher commented 2 years ago

Hello @pantor

I'm having segfaults (see backtrace below) on master: image

It might be related to the changes your made in 22cbb5da0eb9bd4017596ec3ba8197353d4c021a as I don't recall having these crashes before.

As always, thanks for your help !

pantor commented 2 years ago

Hey @fricher, any chance you can copy the InputParameter here so that I can easily reproduce your issue?

fricher commented 2 years ago

There you go:

ruckig::Ruckig<6> ruckig(0.008);
ruckig::InputParameter<6> input;
ruckig::Trajectory<6> traj;

input.control_interface = ruckig::ControlInterface::Velocity;
input.synchronization = ruckig::Synchronization::Phase;
input.duration_discretization = ruckig::DurationDiscretization::Continuous;
input.current_position = {-0.54516231864478149, -1.4206629551476477, 0.32727720821160067, -1.2563817016634644, -0.51079124473334669, 2.4607439315667303};
input.current_velocity = {6.1348323207600686, -0.0040477518609957240, 0.0088178054233364854, 3.4194572623820010, 5.7548168684509085, -1.5675191369761612};
input.current_acceleration = {0.0000000000000000, 0.19725113486681556, -0.23382835447542066, 27.722541632719086, -1.7945991957745204, -22.971380686605098};
std::ranges::fill(input.target_position, 0);
input.target_velocity = {6.1348323207600686, 0.014259490365703332, -0.012798219033181931, 5.1559110036108979, 5.5636136585722848, -3.6173923596604807};
std::ranges::fill(input.target_acceleration, 0);
input.max_velocity = {6.4577182323790190, 5.4105206811824216, 7.1558499331767509, 9.5993108859688121, 9.4247779607693793, 17.453292519943297};
input.max_acceleration = {26.895523773232618, 22.532200643246796, 29.810223624063148, 39.985493163190093, 39.618974020271281, 72.710416638083771};
input.max_jerk = {224.22244900371152, 187.84978739214969, 248.46507231391274, 333.30552725335713, 330.26865435488696, 606.01322287747109};

ruckig.calculate(input, traj);
fricher commented 2 years ago

Hello @pantor ! I spent a bit more time on this, I think it comes down to this check in velocity-step1.cpp if (std::abs(v0) < DBL_EPSILON && std::abs(vf) < DBL_EPSILON && std::abs(a0) < DBL_EPSILON && std::abs(af) < DBL_EPSILON) In our case, v0 == vf and a0 == af and profile.check_for_velocity() returns too many valid profiles. I changed the check to: if (std::abs(vf-v0) < DBL_EPSILON && std::abs(af-a0) < DBL_EPSILON)

--- a/src/velocity-step1.cpp
+++ b/src/velocity-step1.cpp
@@ -76,7 +76,7 @@ bool VelocityStep1::get_profile(const Profile& input, Block& block) {
     Profile profile = input;
     valid_profile_counter = 0;

-    if (std::abs(v0) < DBL_EPSILON && std::abs(vf) < DBL_EPSILON && std::abs(a0) < DBL_EPSILON && std::abs(af) < DBL_EPSILON) {
+    if (std::abs(vf-v0) < DBL_EPSILON && std::abs(af-a0) < DBL_EPSILON) {
         time_none(profile, _aMax, _aMin, _jMax);
         if (valid_profile_counter > 0) { return Block::calculate_block(block, valid_profiles, valid_profile_counter); }

Is this an acceptable solution ?

fricher commented 2 years ago

Hello again, I'm also having some issues with bad optional accesses. It's happening when we try to access blocks[limiting_dof.value()].a->profile in include/ruckig/calculator_target.hpp:160; Sometimes, possible_t_sync is inf and a and b are nullopt. Should we just return false in this case (as all following possible_t_sync will also be inf) ?

--- a/include/ruckig/calculator_target.hpp
+++ b/include/ruckig/calculator_target.hpp
@@ -140,6 +140,11 @@ private:
                     break; // inner dof loop
                 }
             }
+
+            if(!std::isfinite(possible_t_sync)) {
+                return false;
+            }
+
             if (is_blocked || possible_t_sync < t_min.value_or(0.0)) {
                 continue;
             }
pantor commented 2 years ago

Thanks for following up! Can you check out my recent changes on master? It should improve the stability of velocity control quite a bit.

pantor commented 2 years ago

And even if it fixes your issues, can you provide an example that results in bad optional access? I just can't see why that happens.

fricher commented 2 years ago

Thanks a lot, it works a lot better!

I still noticed a synchronization error with these params:

    inp.control_interface = ruckig::ControlInterface::Velocity;
    inp.duration_discretization = ruckig::DurationDiscretization::Discrete;
    inp.current_position = {0, -0.936615802324139767875977, 1.57079632679489655799898, 0, 1.57079632679489655799898, 0};
    inp.current_velocity = {0, -0.0577630321017372216907404, 0, 0, 0, 0};
    inp.current_acceleration = {0, -1.91986217719376273116438, 0, 0, 0, 0};
    inp.target_position = {0, 0, 0, 0, 0, 0};
    inp.target_velocity = {0, -0.100000000000000005551115, 0, 0, 0, 0};
    inp.target_acceleration = {0, 0, 0, 0, 0, 0};
    inp.max_velocity = {
        5.23598775598298882272275, 3.92699081698724139499745, 3.92699081698724139499745, 6.64970445009839572492183, 5.42797397370236467395443, 8.58701991981210177584671};
    inp.max_acceleration = {
        4.36332312998582416696536, 4.36332312998582416696536, 4.36332312998582416696536, 4.36332312998582416696536, 4.36332312998582416696536, 4.36332312998582416696536};
    inp.max_jerk = {
        43.6332312998582381169399, 43.6332312998582381169399, 43.6332312998582381169399, 43.6332312998582381169399, 43.6332312998582381169399, 43.6332312998582381169399};

It only happens with DurationDiscretization::Discrete. Continuous is fine.

I guess it comes from this line:

possible_t_syncs[i] = std::ceil(possible_t_syncs[i] / delta_time) * delta_time;

In this particular case, possible_t_sync[1] actually gets lower (0.044000000000000004 -> 0.043999999999999997) and so blocks[1].is_blocked(...) returns true.

Checking the difference between possible_t_syncs[i] and std::ceil(possible_t_syncs[i] / delta_time) * delta_time and assigning it to possible_t_syncs[i] only if it's >= DBL_EPSILON seems to do the trick but I'm not sure if it's the best way.

As always thanks for your help !

pantor commented 2 years ago

I've now replaced the mentioned line with a new method that should be less prone to numeric issues. Does that work for you?

fricher commented 2 years ago

👍