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

Synchronization bug in ruckig/include/ruckig/calculator_target.hpp #124

Closed Snowrock closed 2 years ago

Snowrock commented 2 years ago

In following code, there maybe a bug cause changing of InputParameter::synchronization never take effect:

    Result calculate(const InputParameter<DOFs>& inp, Trajectory<DOFs>& traj, double delta_time, bool& was_interrupted) {
       //...
       if (found_time_synchronization && std::all_of(inp_per_dof_synchronization.begin(), inp_per_dof_synchronization.end(), [](Synchronization s){ return s == Synchronization::Phase || s == Synchronization::None; })) {
                    return Result::Working;
                }
       //...
    }

When the Ruckig::degrees_of_freedom is changed to smaller than its initial value, if we want to switch the InputParameter::synchronization to Phase, it will never take effect. As std::all_of(...) will check all the element of inp_per_dof_synchronization even the dof is not actived.

So I try to fix it as below, please check it!

    Result calculate(const InputParameter<DOFs>& inp, Trajectory<DOFs>& traj, double delta_time, bool& was_interrupted) {
        //...
        bool synchronization_phase_none = true;
        for (size_t dof = 0; dof < inp_per_dof_synchronization.size(); ++dof)
        {
            if (!inp.enabled[dof] || inp_per_dof_synchronization[dof] == Synchronization::Phase || inp_per_dof_synchronization[dof] == Synchronization::None){
                continue;
            }
            synchronization_phase_none = false;
        }
        if (found_time_synchronization && synchronization_phase_none){
            return Result::Working;
        }
        //....
    }
pantor commented 2 years ago

Thanks for raising this issue @Snowrock! Unfortunately, Ruckig::degrees_of_freedom was never intended to be changed after constructing the Ruckig object and should therefore be const. Therefore, if you want to change the DoFs, you would need to initialize a new instance with Ruckig(DoFs, delta_time);.

Btw, #118 is also relevant for this. I'm in favor of closing this issue and keep #118.

Snowrock commented 2 years ago

【邮件自动回复】您好,邮件已收到,我将尽快给您进一步回复。 祝您:工作愉快,生活美满!

pantor commented 2 years ago

I've now made Ruckig::degrees_of_freedom const, so this issue should not occur anymore. Please initialize a new Ruckig instance with the correct DoFs instead. Thanks!

Snowrock commented 1 year ago

【邮件自动回复】您好,邮件已收到,我将尽快给您进一步回复。 祝您:工作愉快,生活美满!