PA smoothing of base position is unmotivated and has multiple negative consequences
Closed this issue · 4 comments
I first reported this for mainline Klipper in 2021: Klipper3d/klipper#4442. Since then, a number of people have become interested in the matter, and I think we've come to better understand the issue.
To recap, the original "smell" was that the behavior you get with PA of 0.00001 is vastly different from with PA=0, because whenever PA is "on" (nonzero), PA smoothing is in effect, and it not only smooths the amount of advancement made by the extruder to compensate for pressure needed to extrude at higher velocity, but also smooths out the base motion of the extruder.
On the upstream report, myself and others did a lot of math explaining why this is bad theoretically, but since then the biggest practical effects identified have been:
-
Smoothing applies to retraction. This interferes with extrusion just before a retract or after an unretract, and is likely responsible for a number of issues people see with poor restart after retraction, especially if the problem is speed/accel-dependent.
-
When E base position is smoothed around corners and the cornering time is significantly less than the smooth time, the extrusion rate does not lower like it should at the corner, but is only slightly lowered in a large neighborhood of the corner. This means the corner itself bulges, with slightly too little material before/after the corner. This can be partly compensated by using a larger PA value to negate the corner bulge, but that's wrong and will make non-corner velocity changes extrude wrong. I believe this is the main mechanism by which PA is believed to be "acceleration-dependent" and "speed-dependent" - the fudge factor here depends on the relationship between speed, accel, and smooth time.
There was no progress on reaching consensus to upstream a fix for this, but there were two implementations of the concept:
-
an original hack by myself that just ran the integration twice, once with PA=0, and subtracted that one (the pure smoothed base position) out to get a smoothing of just the advance, then added the unsmoothed base position, and
-
a more invasive patch by @Piezoid to completely remove base position integration. This saves compute time rather than adding to it, but is hard to rebase against upstream changes. I'm not sure where the original is but my inclusion of it was here: richfelker/klipper@84431c3
I would like to see DangerKlipper lead the way in getting rid of base position smoothing by PA. Dmitry's work in bleeding edge has already factored out the integration of advance vs base position, such that the following minimal patch disables use of the smoothed base position:
diff --git a/klippy/chelper/kin_extruder.c b/klippy/chelper/kin_extruder.c
index 50f33f96..cf3735d4 100644
--- a/klippy/chelper/kin_extruder.c
+++ b/klippy/chelper/kin_extruder.c
@@ -226,6 +226,10 @@ extruder_calc_position(struct stepper_kinematics *sk, struct move *m
pa_range_integrate(m, axis, move_time, sm,
&e_pos.axis[i], &pa_vel.axis[i]);
}
+ // get rid of (bad! bad! bad!) smoothing of base position
+ e_pos.axis[i] = num_pulses
+ ? shaper_calc_position(m, axis, move_time, sp)
+ : m->start_pos.axis[i] + m->axes_r.axis[i] * move_dist;
}
}
double position = e_pos.x + e_pos.y + e_pos.z;
Clearly this is not the right way to do it, since it throws away a bunch of nontrivial computation.
If we want to leave smoothing of the base position as optional behavior, I believe the right thing to do is to separate the integration into two calls, where integrates the base position (only called if desired) and the other integrates the advance ("velocity") and returns it (no more need for returning a second value via a function pointer). But I'm not sure if this code is used anywhere else, like in IS smoothing; if so, more care is needed when ripping it in two.
If we don't want to keep smoothing of the base position at all, the integration can be simplified to only integrate the advance ("velocity") term.
If there is consensus on how to proceed, I'm willing to work on the actual code changes and PR.
@richfelker hey! this is awesome, thank you! I think we are going to start diverging a little more drastically from mainline, so we're not afraid of slightly more invasive changes. I am open to completely removing smoothing on the base position in bleeding-edge and taking a look at some PA performance from testers, and provided we don't see regression in prints, send it on master. cc @rogerlz since he's the other decider on direction with DK
I am all in for it
Just pulled this update from bleeding-edge and have been testing on my high speed bowden printer. I have been seeing improvement to PA results. I was seeing issues of PA never being able to remove corner blobbing, and having significant under extrusion following the corner at the optimal PA value. I think this has improved those issues slightly in some situations, more testing needed. I also run infill at 200ish mm/s 80k accel 20 scv and was seeing gaps between the infill and the wall due to under extrusion at the infill end junctions. First thing I noticed after installing this is that under extrusion seems non-existent now.