Update motion_control.c

[kimstik]

bug: cubic spline step is up to 2*BEZIER_MAX_STEP

[terjeio]

AFAIKT your fix increases it to 4*BEZIER_MAX_STEP. Have you checked running code?

[kimstik]

AFAIKT your fix increases it to 4*BEZIER_MAX_STEP. Have you checked running code?

I got this result by testing:

#define BEZIER_MIN_STEP 0.002f
#define BEZIER_MAX_STEP 0.1f
#define BEZIER_SIGMA 0.1f

mc_cubic_b_spline() with args:
  position = (0,0,0)
  first    = (0,1,0)
  second   = (1,0,0)
  target   = (1,1,1)

gives:
t 0.000000 step=0.200  line to (0.104000,0.392000,0.000000)
t 0.200000 step=0.200  line to (0.352000,0.496000,0.000000)
t 0.400000 step=0.200  line to (0.648000,0.504000,0.000000)
t 0.600000 step=0.200  line to (0.896000,0.608000,0.000000)
t 0.800000 step=0.200  line to (1.000000,1.000000,0.000000)
multiplications:281 additions:224

with BEZIER_MAX_STEP set to 0.1, 10 interpolation points are expected over t=0.0...1.0, but only 5 were generated.

relevant link is : https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.1.x/Marlin/src/module/planner_bezier.cpp#L159

[kimstik]

where you see 4*BEZIER_MAX_STEP ? Do you got it by running code?

[kimstik]

BTW I like "interp(position.z, target.z, t)" in the Marlin. Not ideal but it is much better then no Z interp.

[terjeio]

with BEZIER_MAX_STEP set to 0.1, 10 interpolation points are expected over t=0.0...1.0, but only 5 were generated.

According to the description how many steps to expect depends on the curve straightness.

When I run the code with double precision math I get four segments, not five. And with your change I get five vs. ten. And to add to float impreciseness dist1() returns an approximation. So IMO the algorithm is kind of "fuzzy".

[kimstik]

According to the description

description: "MAX_STEP is taken at the first iteration."
The first iteration with my correction uses MAX_STEP; otherwise, it uses 2*MAX_STEP. This behavior has been fixed in Marlin (for float).
The cause was an incorrect entry into the "enlarge step" logic because, on the first iteration,
step = BEZIER_MAX_STEP
and the condition (new_t - t <= BEZIER_MAX_STEP) may be triggered.
The condition (new_t - t < BEZIER_MAX_STEP) would be more appropriate here (as per Marlin).

4 steps sounds buggy as per "2*MAX_STEP" rule from description. In this case 5 steps is minimum, not 4.

I haven’t checked the double version yet..
I’ll need to look at it carefully

[terjeio]

The condition (new_t - t < BEZIER_MAX_STEP) would be more appropriate here (as per Marlin).

My while loop produces the same steps as the Marlin for loop, so the linked Marlin code has the same bug? Or is Marlin using a different float library that has produces slightly different results that affects the loop exit condition?

[kimstik]

My while loop produces the same steps as the Marlin for loop, so the linked Marlin code has the same bug? Or is Marlin using a different float library that has produces slightly different results that affects the loop exit condition?

Marlin code looks correct. (https://github.com/MarlinFirmware/Marlin/blob/bugfix-2.1.x/Marlin/src/module/planner_bezier.cpp#L159)

    if (!did_reduce) for (;;) {
      if (new_t - t > MAX_STEP) break;

is matches to the:
if (!did_reduce) while (new_t - t < BEZIER_MAX_STEP) {...

actual grblHAL not matches Marlin:
if (!did_reduce) while (new_t - t <= BEZIER_MAX_STEP) {...

The difference between your while loop and Marlin’s is that in your case, one (wrong) iteration may pass when (new_t - t) is exactly equal to BEZIER_MAX_STEP.
The bad thing is that this can happen randomly because of floating-point precision.

Sorry I didn’t try to clarify it from the start. I thought it was pretty obvious.
You’ve shown that with “double” we obtain 4 segments, whereas with the maximum step size of 0.2, it’s impossible to get fewer than 5 segments. Is that correct?
Feel free to correct me if I’m wrong. Thank you.