You may note, as I do, that cold/length extrusion handling is redundant, with one instance in prepare_move_to_destination and another in planner.buffer_move. My assumption is that for G-code we want to catch these moves early, before they get into the planner. Then, for moves that don't necessarily go through prepare_move_to_destination the planner version is there to intervene.

At the higher level, in prepare_move_to_destination, the E volumetric and flow factors are not applied, but they are at the planner level. So there is some possibility that a move could get through prepare_move_to_destination but get stopped at planner.buffer_move. So it seems the E factor needs to be applied here as well.

Since the E factor comes into play with each segment of an interpolated move and is used in various places consistently, it seems prudent to pre-calculate this E factor to save on computation and allow its easier use where needed. Just as we have the MMM_TO_MMS_SCALED macro for feedrate scaling, we might want an E_SCALED macro to do the same thing for E moves.

Applied various self-suggestions. Please review!

You may note, as I do, that cold/length extrusion handling is redundant, with one instance in prepare_move_to_destination and another in planner._buffer_line.

Yes, and also that prepare_move_to_destination only checked for positive lengthy extrudes, but would allow any length negative extrude to proceed. This may have originally been by design, but it was inconsistent with the check in planner._buffer_line. Plus, I like the idea of it not running the extruder forever if I accidentally type an extra number in a G0 E command with a negative value.

At the higher level, in prepare_move_to_destination, the E volumetric and flow factors are not applied, but they are at the planner level. So there is some possibility that a move could get through prepare_move_to_destination but get stopped at planner._buffer_line. So it seems the E factor needs to be applied here as well.

I don't think this is the case. The E volumetric and flow factors are not applied in planner._buffer_line until after the PREVENT_LENGTHY_EXTRUDE check. The only difference between the two checks is that the units are in mm in prepare_move_to_destination, but are in steps in planner._buffer_line.

However, a case for when the check might erroneously pass in prepare_move_to_destination is when current_position is manipulated prior to the call to prepare_move_to_destination. For example:

current_position[E_AXIS] += 100000000;
set_destination_from_current();
prepare_move_to_destination();

By modifying current_position, prepare_move_to_destination now has no reference from which to compare destination, so the if (destination[E_AXIS] - current_position[E_AXIS] > EXTRUDE_MAXLENGTH) check will always evaluate to false. I personally feel that this is bad practice as it causes all sorts of problems like this.

Since the E factor comes into play with each segment of an interpolated move and is used in various places consistently, it seems prudent to pre-calculate this E factor to save on computation and allow its easier use where needed. Just as we have the MMM_TO_MMS_SCALED macro for feedrate scaling, we might want an E_SCALED macro to do the same thing for E moves.

And this is why you make the big bucks :-)

would allow any length negative extrude to proceed

That was probably by design to allow for long bowden unloads. I don't mind changing it, but a sanity check comparing this length to the filament unload length should probably be done at compile time to give Bowden users a fair warning.

I don't think this is the case. The E volumetric and flow factors are not applied in planner._buffer_line until after the PREVENT_LENGTHY_EXTRUDE check.

Ah, right you are. n/m that then.

By modifying current_position, prepare_move_to_destination now has no reference from which to compare destination, so the if (destination[E_AXIS] - current_position[E_AXIS] > EXTRUDE_MAXLENGTH) check will always evaluate to false. I personally feel that this is bad practice as it causes all sorts of problems like this.

Agreed. If only we could write Marlin in Haskell.

One of the big caveats I've had with Marlin is that it relies on things that you don't necessarily know at the outset and so many high-level functions have side-effects.

At the moment I am auditing all the movement code so that I know what calls what and when, and that I know all the side-effects. Hopefully that will lead to some minor reform.

So… Maybe Marlin doesn't need two checks for cold and lengthy extrude after all. Or, the check should at least be moved to the planner as an inline function, comparing the given destination against its internal position information. That would bypass problems involving the use of prepare_move_to_destination where current_position has been altered.

What I'll do right now is go through all the code that calls prepare_move_to_destination to see how often current_position is altered. I suppose we only get false negatives at the moment, but I'm sure false positives are just as easy to end up with.

I can find there's no problem calls to prepare_move_to_destination or the functions that call it. Fortunately there are very few places in Marlin where current_position[E_AXIS] is altered directly. It is actually okay if current_position[E_AXIS] is changed ahead of prepare_move_to_destination as long as it and the destination[E_AXIS are appropriately related.

I have added one more commit to this, in which I replace calls to do_blocking_move_to_xy/do_blocking_move_to_z that are paired together unnecessarily, replacing them with a single call to do_blocking_move_to, which will do the correct thing. That should save a few code bytes here and there.

Why not lisp? ;-)

Indeed… Why not Delphi? 🤓

What about do_pause_e_move? It modifies current_position[E_AXIS].