Allow dynamic adjustment of pressure advance

[dmbutyugin]

Right now changing the Pressure Advance coefficient during printing can be a little bit problematic. In order to change the PA value, the current implementation flushes the internal move queue, and only then it can change the PA value for the extruder stepper. And if the user realizes after starting the print that they need to adjust the PA, they have to pay close attention to the time they issue SET_PRESSURE_ADVANCE command, as, if unlucky, the flushing can happen when printer is printing outer perimeter or some top surface, and then the momentary pause from flushing the queue will result in a small, but pretty noticeable blob or zit on the print. And besides this obvious inconvenience, the current implementation also makes dynamic adjustments of PA in the slicer impractical.

The new proposed implementation maintains the history of pressure advance values in the extruder stepper kinematics and looks it up during integration. As a result, the following gcode sequence does not result in move queue flushes until the point when the SMOOTH_TIME is changed and the toolhead moves continuously with appropriate extruder stepper position adjustments as the PA changes 0.1 -> 0.01 -> 0.05:

SET_PRESSURE_ADVANCE ADVANCE=0.1
; Home and extrusion moves
G28
G1 X20 Y20 Z10 F6000
G1 E2 F600
G1 X25 Y25 E2.5 F6000

G1 X30 Y30 E3.0

; Update pressure advance for primary extruder
SET_PRESSURE_ADVANCE ADVANCE=0.01
G1 X35 Y35 E3.5

; Update pressure advance for primary extruder
SET_PRESSURE_ADVANCE ADVANCE=0.05
G1 X40 Y40 E4.0 F3000

; Update smooth_time
SET_PRESSURE_ADVANCE SMOOTH_TIME=0.01
G1 X45 Y45 E4.5 F6000

as demonstrated with this chart, obtained with motan tools:

Here, ‘extruder position’ and ‘extruder velocity’ (in blue) are the extruder stepper position and velocity after the PA is applied (so these are the true commanded positions and velocities), and ‘extruder x position’ and ‘extruder x velocity’ (in orange) are the corresponding position and velocity of the extruder from the trapezoidal move planner before any PA (or smoothing) is applied.

As a note, changing SMOOTH_TIME still results in a move queue flush, but the user rarely has to change it, and due to changes in integration timing, it cannot be easily adjusted mid-printing.

The code has been tested by other users and no regressions have been reported.

[KevinOConnor]

Thanks. I agree we should make this change.

Changes to PA can still only occur between moves, so I think we could simplify the change a bit - maybe a pa_move_integrate() could look something like (totally untested):

--- a/klippy/chelper/kin_extruder.c
+++ b/klippy/chelper/kin_extruder.c
@@ -52,17 +52,27 @@ extruder_integrate_time(double base, double start_v, double half_accel
 
 // Calculate the definitive integral of extruder for a given move
 static double
-pa_move_integrate(struct move *m, double pressure_advance
+pa_move_integrate(struct move *m, struct list_head *pa_list
                   , double base, double start, double end, double time_offset)
 {
     if (start < 0.)
         start = 0.;
     if (end > m->move_t)
         end = m->move_t;
-    // Calculate base position and velocity with pressure advance
+    // Determine pressure_advance value
+    double pressure_advance = 0.;
     int can_pressure_advance = m->axes_r.y != 0.;
-    if (!can_pressure_advance)
-        pressure_advance = 0.;
+    if (can_pressure_advance) {
+        struct pa_params *pa = list_last_entry(pa_list, struct pa_params, node);
+        for (;;) {
+            pressure_advance = pa->pressure_advance;
+            if (likely(pa->active_print_time <= m->print_time)
+                || list_is_first(&pa->node, pa_list))
+                break;
+            pa = list_prev_entry(pa, node);
+        }
+    }
+    // Calculate base position and velocity with pressure advance
     base += pressure_advance * m->start_v;
     double start_v = m->start_v + pressure_advance * 2. * m->half_accel;
     // Calculate definitive integral

Using one PA per move was what was done prior to commit 29724a7. Let me know if I'm missing something though.

-Kevin

[dmbutyugin]

Thanks for the suggestion, Kevin. I initially played with m->print_time + start and such, and I ran into some issues with numerical stability, hence the current code. I gave a fairly basic try to your suggestion, and with some tiny fixes it passes the simple tests. I'll give it a bit more thought and testing, and if all goes well, I'll update the PR per your suggestion, and let you know at that point that it's OK to take another look.

[KevinOConnor]

Yeah - numerical stability is actually the main reason I wanted to raise the possibility of an alternative. The Python print_time, m->print_time, and pa->active_print_time should have the same precision, while start, end, and m->move_t have a different precision. I was concerned with the mixing of them due to potential for numerical stability issues (eg, pa->active_print_time - m->print_time >= end).

Cheers,
-Kevin

[KevinOConnor]

Looking a little closer, as a similar comment, I'd suggest extruder_set_pressure_advance() not perform math on print_time - just assign it to pa->active_print_time. That is, it should be possible to enable PA for particular struct move entries and not attempt to enable PA at a particular time (which could increase the chance of numerical stability problems).

Cheers,
-Kevin

[dmbutyugin]

Looking a little closer, as a similar comment, I'd suggest extruder_set_pressure_advance() not perform math on print_time - just assign it to pa->active_print_time.

Yes, that makes sense and that was exactly the setup I gave some testing. I pushed your suggestions and I think it should be good now, there's only one place left in the cleanup where there are some math operations with active_print_time, but I think those ones cannot be avoided, but they should be fine. I'd let this PR be open for another couple of days, and if no issues are discovered, I think we can commit it (probably as a single commit).

[KevinOConnor]

Thanks. Looks good to me. If there are no further comments I'll look to squash and commit in a few days.

-Kevin

[igiannakas]

FYI - I’ve run a test print with the latest commit above. It is working as expected.

[lookypanda]

FYI: also additionally tested this branch on 2-3 different prints and different settings with orca adaptive PA(SoftFever/OrcaSlicer#5609) - no issues, no artifacts.

[KevinOConnor]

Thanks!

-Kevin