Add and enforce min_entry_speed_sqr

[mh-dm]

Description

Remove nominal_length, add min_entry_speed_sqr, initial clean up of reverse_pass_kernel() and forward_pass_kernel().

Added min_entry_speed_sqr to avoid a specific potential source of judder due to working with discrete steps rather than continuous real-valued physics. The first step of any segment runs at initial_rate. If it is too low compared to acceleration_steps_per_s2 it will result in too much accumulated acceleration_time (see stepper.cpp) which will mean the following step will be at a much higher speed, and the speed change will significantly surpass the set acceleration_steps_per_s2 limit.

Making sure we can actually accelerate at around acceleration_steps_per_s2 is probably one reason for why we have minimum_planner_speed_sqr in the first place. This change makes sure we follow that limit and keeps the second step under at most 2x the acceleration_steps_per_s2 limit.

Removed nominal_length flag as a current->entry_speed_sqr == current->max_entry_speed_sqr check basically already tells us the same information. Done now to make it easier to reason about what the planner actually does. Slightly reduces the number of max_allowable_speed_sqr() calls as tiny benefit.

Benefits

Pulled this commit out of #27035 to enable submitting PR #26881 that removes the hard MINIMAL_STEP_RATE limit of 120. This 120 step rate limit acted like a kludge that prevented the worst kinds of acceleration spikes.

Related Issues

This change was more fully tested only on top of #27013 so I think that PR should be submitted first. #27013 fixes nearly all acceleration spikes so it makes acceleration spikes easier to see.

[tombrazier]

Making sure we can actually accelerate at around acceleration_steps_per_s2 is probably one reason for why we have minimum_planner_speed_sqr in the first place.

That is exactly why we have minimum_planner_speed_sqr. I implemented it to replace a hard coded constant when I discovered an earlier version of the speed jump bug.

[tombrazier]

I think the work you're doing here is really impressive @mh-dm. I have often wondered whether the forward pass / reverse pass code could be improved. I have stayed away from it, though, because it hurts my brain to really fully understand it. Optimisations like nominal_length and block_buffer_planned make it even harder to understand fully.

You have convinced me that nominal_length is, indeed, redundant. If that flag is true then the first time a block is processed by reverse_pass_kernel() the value of current->entry_speed_sqr will be set equal to current->max_entry_speed_sqr. As you observe, we already test for that condition. The exception is the case where the forward pass subsequently decreases current->entry_speed_sqr but in that case block_buffer_planned will be advanced and the block won't be seen again by the reverse pass.

Now that I have really forced my brain to think through all of this, though, I believe the entire forward pass is redundant. Check out my branch https://github.com/tombrazier/Marlin/tree/noforwardpass which adds a variable real_max_entry_speed_sqr to planner blocks that is only calculated once and successfully predicts the behaviour of the forward pass. I have run this on an hour long print without getting the "Failed!" message once.

The value I am storing in real_max_entry_speed_sqr could, instead, be stored in max_entry_speed_sqr and this would automatically apply the effect of the forward pass in the backward pass. Then the backward pass itself can call recalculate_trapezoids().

What do you think?

[mh-dm]

I think the work you're doing here is really impressive @mh-dm.

Thank you!

but in that case block_buffer_planned will be advanced

Since you mentioned it: Similarly to nominal_length, block_buffer_planned is also unnecessary. #27035 removes it.

adds a variable real_max_entry_speed_sqr to planner blocks that is only calculated once and successfully predicts the behavior of the forward pass.

I need to unpack this. I looked your code and this is the most relevant bit:

block->real_max_entry_speed_sqr = _MIN(block->max_entry_speed_sqr, max_allowable_speed_sqr(-previous->acceleration, previous->real_max_entry_speed_sqr, previous->millimeters));

What this is telling me is that your proposed pre-computed entry speed limit is either the junction limit or the speed achieved by full acceleration from the [previous junction limit or the speed limit achieved by full acceleration ... (and so on)]. This is definitely often true, and you have data that points that way. But is it true always?

Trying to find a counterexample.. We'd need a segment that somehow ends up at an entry speed below the junction speed but then out of that would be a hard acceleration... Hmm..

Say everything gets planned for a hard acceleration ramp into a hard deceleration to safe_exit_speed_sqr (speed over time looks like a triangle). Say the stepper now starts the segment at the peak (which is accelerating+decelerating) which means its exit speed becomes fixed (and lower). A new segment gets planned that would in theory allow a higher peak speed and the backward pass happily updates all entry speeds as if that's actually achievable. But in practice it's not because the stepper is executing that peak segment already and it's executing it as accelerating+decelerating. But the pre-computed entry speed limit assumes full acceleration is always possible so it won't be able to correct a too optimistic backward pass. The result is a sudden speed change from the mismatch.

I think what you're proposing would work if the planner was fully precomputing (like klipper) and not racing the stepper.

Why didn't this show up in your testing? Multiple possible reasons:

1. You need motion with a lot of small segments and high speeds so that these sorts of multi-segment peaks can happen.
2. For fast machines the planner is basically always ahead and since the default buffer size is 8/16 you'd need that many fast consecutive small segments to reliably hit the issue.
3. You're comparing against the existing code and the existing code already has at least one bug that presents exactly like this. This bug is fixed by Speed up (~1.5x) and fix planner, including judder due to a planner-stepper race #27035. You can see the sudden speed change at the beginning of the logic analyzer capture.

Finally, if you look at #27035 the forward pass is half-removed as it was integrated into recalculate_trapezoids and most of the time does nothing.

[tombrazier]

Oh you're right. That is a rare counter case to my idea.

[tombrazier]

I have reviewed this PR and I believe I understand every line of it and I think it does exactly what @mh-dm says. I also think it will open the way for #26881.