Fix CLASSIC_JERK to consistently respect jerk limits.

[mh-dm]

Description

This fixes 3 bugs/issues in jerk limiting:

1. Computed v_entry speed was wrong (missing a 'current_speed_factor') when the current block nominal speed was > previous nominal speed. Thus the resulting junction speed was wrong.
2. When switching directions (say nearly 180 deg) the computed jerk would be completely overridden by applying separate 'safe' exit / entry speeds. This results in basically doubling the jerk limit for sharp back and forth movements. Presumably this was done to address slowdowns, but after fixing 1) this special handling is no longer needed.
3. Jerk is a change in speed (i.e. ABS(v_exit - v_entry)) and not _MAX(v_exit, -v_entry), even in axis reversal.

Other notes:

• Less special casing, shorter, cleaner code.
• Slightly faster jerk computation as we stop having to always compute the 'safe' speed (jerk to halt speed).
• Refactored the code that computes the 'safe' speed.

Benefits

The overall result is smoother motion when printing models that involve lots of small back and forth moves, like on thin strips in top layers. Alternatively, jerk limits can be slightly increased for faster (or sometimes higher quality) prints while keeping overall vibrations at same level.

Reduced chance of surprise layer shifts. Say you tune jerk and acceleration using some non-extreme model (say a calibration cube). Later, when printing something with a lot of sharp back and forth movements you get layer shifts.

Configurations

Affects using CLASSIC_JERK. As part of a journey to getting an Ender-3 to print really fast I've tested (on top of) this change quite a bit.

Related Issues

A bit hard find related issues as most people would tune down jerk and acceleration if the movement sounded too jerky and acceleration defaults are often conservative. I did find one case of surprise Layer shifting #23800 that mentioned "Y axis only has very small movements".

Here's a before/after comparison showing improvement in vibrations. I set X and Y jerk to 10mm/s (the default in marlin). X and Y acceleration are 10k and 5k respectively, with print/travel acceleration really high (20k) to make sure said limits apply. Using 100mm/s and 300mm/s print and travel speeds. (On a heavily customized Ender-3 with hardware allows said high settings.)

classic-jerk-before.mp4

classic-jerk-after.mp4

Test part was specifically designed/sliced to cause many small back and forth movements and to calibrate acceleration/jerk settings.

I was going to say that the print quality is exactly the same, down to the blobs on the outer walls that are there due to the random seam setting in the slicer. In the photo, before is bottom, after is top. But no, the print quality is slightly better as there are slightly fewer blobs. Note that model is sliced at high enough resolution to be processing intensive - any code speed up means the printer doesn't have to stop to process the next segment.
To make sure the jerk code is properly benchmarked, the part includes multiple rounded outer walls of varying radii which are very sensitive to the jerk setting. If not high enough (or if the junction speed is wrongly computed) the printer would have to slow down and speed up significantly for every single small segment of a curve.

[mh-dm]

Note that this is a recreation of #26518 that couldn't be reopened which was copied into #26521 which has some review comments and discussion.

[thinkyhead]

I did a quick rebase on this to preserve all commits from #26521. Update your working copy with…

git checkout bugfix-2.1.x-jerk
git fetch origin
git reset --hard origin/bugfix-2.1.x-jerk

[mh-dm]

Anything else I can help with here?

[thinkyhead]

Anything else I can help with here?

It would be good to get a cycle-to-cycle comparison between the old code and the updated code. Some of the logic in the old code was specifically to avoid doing costly division, especially important with 16MHz AVR where every cycle counts. If the new code is slower then we need to consider restoring some of those optimizations.

[mh-dm]

The updated code is definitely a bit faster. Old code computed both safe speed and jerk limited speed. Updated code only computes one or the other. I've also made sure to minimize number of divisions.

cycle-to-cycle comparison

That said: 1) it would be nice to have numbers on how much faster it is. 2) There is a further optimization that can be applied to even more reduce number of divisions (to at most 1), but it complicates the code a bit and does more multiplications. It may or may not make sense to do it.
Is there a guide on how to run such a benchmark? I'm using SKR1.3/1.4 lpc1768/9.

[thinkyhead]

The updated code is definitely a bit faster.

I can believe that. Smaller and simpler code usually is faster. Especially when it ends up doing less multiplication and division overall. Division is one of our biggest enemies.

Is there a guide on how to run such a benchmark? I'm using SKR1.3/1.4 lpc1768/9.

There isn't. You'd basically need to make a global cycle counter and accumulate the number of cycles used by _populate_block (or just a small part of it). Something like this:

long long code_cycles = 0;
. . .
hal_timer_t start_pulse_count = HAL_timer_get_count(MF_TIMER_PULSE);
// some code to time
code_cycles += HAL_timer_get_count(MF_TIMER_PULSE) - start_pulse_count;

And then you can report the value after running some moves with...

extern long long code_cycles;
SERIAL_ECHOLNPGM("Cycles:", code_cycles);

...which could be tossed into a G-code that doesn't do much, like GcodeSuite::G4.

[robherc]

2. There is a further optimization that can be applied to even more reduce number of divisions (to at most 1), but it complicates the code a bit and does more multiplications. It may or may not make sense to do it.

#1 wokwi might work exceptionally well as a benchmark here, as you can use carious methods in their simulator to see cycle-by-cycle what your code is doing in the MCU core

#2 on systems without coprocessors, div instructions tend to take ~7 cycles to complete, while the vast majority of architectures can complete mul instructions in a single cycle, so the savings can be significant (which is why many libraries use a database of multipliers to use in place of division...multiply by a number that makes a power-of-two division give you the same/very close result; then logical-shift left)

[thinkyhead]

on systems without coprocessors, div instructions tend to take ~7 cycles to complete, while the vast majority of architectures can complete mul instructions in a single cycle

Our specific concern is with AVR. According to this post on AVR freaks…

A quick test with avr-gcc 5.4.0 shows this:
An int16 to float conversion adds 170 bytes of code and uses 38 cycles.
A float divided by float constant adds 110 bytes of code and uses 89 cycles.
An int16 divided by an int16 constant adds 88 bytes of code and takes 46 cycles.

…and that's just for operations involving constants, which are optimized.

[mh-dm]

#2 on systems without coprocessors, div instructions tend to take ~7 cycles to complete, while the vast majority of architectures can complete mul instructions in a single cycle, so the savings can be significant (which is why many libraries use a database of multipliers to use in place of division...multiply by a number that makes a power-of-two division give you the same/very close result; then logical-shift left)

Division optimized code looks like:

      float limiting_jerk = 1.0f, limiting_maxj = 1.0f;
      LOOP_LOGICAL_AXES(i) {
        const float jerk = ABS(current_speed[i]),   // cs : Starting from zero, change in speed for this axis
                    maxj = (max_jerk[i] + (i == X_AXIS || i == Y_AXIS ? extra_xyjerk : 0.0f)); // mj : The max jerk setting for this axis
        if (jerk * limiting_maxj > maxj * limiting_jerk) {
          limiting_jerk = jerk;
          limiting_maxj = maxj;
        }
      }
      vmax_junction = block->nominal_speed * limiting_maxj / limiting_jerk;

For the common XYZE scenario we're looking at probably 1.5 to 2 divisions on average vs 1 division and 4 extra multiplications plus ~1.5-2 extra register copies.

Edit: I think I'd rather leave it for a follow up that can also de-duplicate code between the two from halt or from previous segment.

[robherc]

A quick test with avr-gcc 5.4.0 shows this: An int16 to float conversion adds 170 bytes of code and uses 38 cycles. A float divided by float constant adds 110 bytes of code and uses 89 cycles. An int16 divided by an int16 constant adds 88 bytes of code and takes 46 cycles.

For the common XYZE scenario we're looking at probably 1.5 to 2 divisions on average vs 1 division and 4 extra multiplications plus ~1.5-2 extra register copies.

Well, according to my "best-case" numbers earlier with DIV taking 7 cycles, your new numbers look like a slight improvement probably...but according to the numbers thinkyhead just copied here, it's apparently a landslide victory. lol.

[mh-dm]

Regardless, I'm happy to follow up later with another PR with the optimization (and refactor).

[mh-dm]

While I was benchmarking the code I found ways to further optimize the consistent jerk calculation, going from ~728 to ~616 cycles (SKR 1.3). I've included that optimization in #26551.
I've tested the division optimized code above and it's slightly slower, from ~616 to ~673 cycles, probably those extra multiplications added up.

For posterity, a couple surprising things with the benchmarking:

1. SERIAL_ECHOLNPGM("Cycles:", code_cycles) has a catch if using multiple serial ports since the output will go to the non-monitoring port.
2. HAL_timer_get_count(MF_TIMER_PULSE) - start_pulse_count is strangely often negative when there's lots of fast small steps in the gcode :. I assume that's because we get a stepper motion ISP right in the middle of the calculation. So these need filtering out and just for benchmarking I've also changed the slicer to further reduce small segments. With that the individual cycle counts were consistent enough to trust the averages.

[vovodroid]

HAS_LINEAR_E_JERK is always false for CLASSIC_JERK:

#if ALL(HAS_JUNCTION_DEVIATION, LIN_ADVANCE)
  #define HAS_LINEAR_E_JERK 1
#endif