From 15548958e98ddae4de263d77aa68aa025226e2d2 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sun, 21 Jun 2020 15:19:31 +0200 Subject: [PATCH 01/15] Remove cumulative error in segments without cruising (take 1) PR #2591 made LA compression always account for retractions instead of discarding the current compression steps. While this fixed overextrusion in short segments followed by wipes, it uncovered another issue in how the compression steps are spread during the trapezoid calculations leading to gaps in segments followed by retractions (as highlighted by /some/ prints in #2693). LA1.5 always computes the required target compression steps for a segment at nominal speed. Because of how the extra steps are allocated using multiples of the accelerating frequency, if the segment is truncated before cruising is reached, an additional cycle of steps can be inserted before deceleration starts. Deceleration is also not guaranteed to be symmetric where up to _two_ cycles can be skipped depending on the stepping cycle, leading to a situation where a symmetric acceleration/deceleration block will lead up to a cycle of accumulated compression. While forcing an the extra step during deceleration is possible by tweaking the error term (eISR_Err), this doesn't guarantee balance in all cases. The underlying issue is that the function is aiming a compression which cannot be reached (nominal speed), and not at the effective max speed reached in the trapezoid, thus moving the average result higher over time. We fix this by calculating the effective maximum speed (and compression) reached during the trapezoid, which stops compression on the required cycle irregardless of the error term, balancing the result. This is the first unoptimized POC: this is not for production: a lot of calculations are redundand and could work directly in steps/s^2. --- Firmware/planner.cpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index c0f465c2a..049dd2022 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -225,12 +225,15 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit uint32_t accel_decel_steps = accelerate_steps + decelerate_steps; // Size of Plateau of Nominal Rate. uint32_t plateau_steps = 0; + // Maximum effective speed reached in the trapezoid (mm/s) + float max_speed; // Is the Plateau of Nominal Rate smaller than nothing? That means no cruising, and we will // have to use intersection_distance() to calculate when to abort acceleration and start braking // in order to reach the final_rate exactly at the end of this block. if (accel_decel_steps < block->step_event_count.wide) { plateau_steps = block->step_event_count.wide - accel_decel_steps; + max_speed = block->nominal_speed; } else { uint32_t acceleration_x4 = acceleration << 2; // Avoid negative numbers @@ -263,12 +266,18 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit decelerate_steps = block->step_event_count.wide; accelerate_steps = block->step_event_count.wide - decelerate_steps; } + + // TODO: not for production + float dist = intersection_distance(entry_speed, exit_speed, block->acceleration, block->millimeters); + max_speed = sqrt(2 * block->acceleration * dist + entry_speed*entry_speed); } #ifdef LIN_ADVANCE uint16_t final_adv_steps = 0; + uint16_t max_adv_steps = 0; if (block->use_advance_lead) { final_adv_steps = exit_speed * block->adv_comp; + max_adv_steps = max_speed * block->adv_comp; } #endif @@ -284,6 +293,7 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit block->final_rate = final_rate; #ifdef LIN_ADVANCE block->final_adv_steps = final_adv_steps; + block->max_adv_steps = max_adv_steps; #endif } CRITICAL_SECTION_END; @@ -1137,9 +1147,8 @@ Having the real displacement of the head, we can calculate the total movement le #ifdef LIN_ADVANCE if (block->use_advance_lead) { // the nominal speed doesn't change past this point: calculate the compression ratio for the - // segment and the required advance steps + // segment (the required advance steps are computed during trapezoid planning) block->adv_comp = extruder_advance_K * e_D_ratio * cs.axis_steps_per_unit[E_AXIS]; - block->max_adv_steps = block->nominal_speed * block->adv_comp; float advance_speed; if (e_D_ratio > 0) From 753e651af3026fba344169e45f677476741cf485 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sun, 21 Jun 2020 16:01:13 +0200 Subject: [PATCH 02/15] Remove cumulative error in segments without cruising (take 2) Reduce per-trapezoid calculations --- Firmware/planner.cpp | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index 049dd2022..8687055b1 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -267,9 +267,7 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit accelerate_steps = block->step_event_count.wide - decelerate_steps; } - // TODO: not for production - float dist = intersection_distance(entry_speed, exit_speed, block->acceleration, block->millimeters); - max_speed = sqrt(2 * block->acceleration * dist + entry_speed*entry_speed); + max_speed = sqrt(acceleration_x2 * accelerate_steps + initial_rate_sqr) / block->speed_factor; } #ifdef LIN_ADVANCE From 7c140bc497d50d1ebda861d79c7fb5fe64d5c673 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sun, 21 Jun 2020 16:19:46 +0200 Subject: [PATCH 03/15] Remove cumulative error in segments without cruising (take 3) Remove another division by precomputing the division directly in adv_comp. --- Firmware/planner.cpp | 104 ++++++++++++++++++++++--------------------- 1 file changed, 53 insertions(+), 51 deletions(-) diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index 8687055b1..54bc692d8 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -225,15 +225,15 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit uint32_t accel_decel_steps = accelerate_steps + decelerate_steps; // Size of Plateau of Nominal Rate. uint32_t plateau_steps = 0; - // Maximum effective speed reached in the trapezoid (mm/s) - float max_speed; + // Maximum effective speed reached in the trapezoid (step/min) + float max_rate; // Is the Plateau of Nominal Rate smaller than nothing? That means no cruising, and we will // have to use intersection_distance() to calculate when to abort acceleration and start braking // in order to reach the final_rate exactly at the end of this block. if (accel_decel_steps < block->step_event_count.wide) { plateau_steps = block->step_event_count.wide - accel_decel_steps; - max_speed = block->nominal_speed; + max_rate = block->nominal_rate; } else { uint32_t acceleration_x4 = acceleration << 2; // Avoid negative numbers @@ -267,15 +267,15 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit accelerate_steps = block->step_event_count.wide - decelerate_steps; } - max_speed = sqrt(acceleration_x2 * accelerate_steps + initial_rate_sqr) / block->speed_factor; + max_rate = sqrt(acceleration_x2 * accelerate_steps + initial_rate_sqr); } #ifdef LIN_ADVANCE uint16_t final_adv_steps = 0; uint16_t max_adv_steps = 0; if (block->use_advance_lead) { - final_adv_steps = exit_speed * block->adv_comp; - max_adv_steps = max_speed * block->adv_comp; + final_adv_steps = final_rate * block->adv_comp; + max_adv_steps = max_rate * block->adv_comp; } #endif @@ -1142,51 +1142,6 @@ Having the real displacement of the head, we can calculate the total movement le block->acceleration_rate = (long)((float)block->acceleration_st * (16777216.0 / (F_CPU / 8.0))); -#ifdef LIN_ADVANCE - if (block->use_advance_lead) { - // the nominal speed doesn't change past this point: calculate the compression ratio for the - // segment (the required advance steps are computed during trapezoid planning) - block->adv_comp = extruder_advance_K * e_D_ratio * cs.axis_steps_per_unit[E_AXIS]; - - float advance_speed; - if (e_D_ratio > 0) - advance_speed = (extruder_advance_K * e_D_ratio * block->acceleration * cs.axis_steps_per_unit[E_AXIS]); - else - advance_speed = cs.max_jerk[E_AXIS] * cs.axis_steps_per_unit[E_AXIS]; - - // to save more space we avoid another copy of calc_timer and go through slow division, but we - // still need to replicate the *exact* same step grouping policy (see below) - if (advance_speed > MAX_STEP_FREQUENCY) advance_speed = MAX_STEP_FREQUENCY; - float advance_rate = (F_CPU / 8.0) / advance_speed; - if (advance_speed > 20000) { - block->advance_rate = advance_rate * 4; - block->advance_step_loops = 4; - } - else if (advance_speed > 10000) { - block->advance_rate = advance_rate * 2; - block->advance_step_loops = 2; - } - else - { - // never overflow the internal accumulator with very low rates - if (advance_rate < UINT16_MAX) - block->advance_rate = advance_rate; - else - block->advance_rate = UINT16_MAX; - block->advance_step_loops = 1; - } - - #ifdef LA_DEBUG - if (block->advance_step_loops > 2) - // @wavexx: we should really check for the difference between step_loops and - // advance_step_loops instead. A difference of more than 1 will lead - // to uneven speed and *should* be adjusted here by furthermore - // reducing the speed. - SERIAL_ECHOLNPGM("LA: More than 2 steps per eISR loop executed."); - #endif - } -#endif - // Start with a safe speed. // Safe speed is the speed, from which the machine may halt to stop immediately. float safe_speed = block->nominal_speed; @@ -1312,6 +1267,53 @@ Having the real displacement of the head, we can calculate the total movement le // Precalculate the division, so when all the trapezoids in the planner queue get recalculated, the division is not repeated. block->speed_factor = block->nominal_rate / block->nominal_speed; + +#ifdef LIN_ADVANCE + if (block->use_advance_lead) { + // calculate the compression ratio for the segment (the required advance steps are computed + // during trapezoid planning) + float adv_comp = extruder_advance_K * e_D_ratio * cs.axis_steps_per_unit[E_AXIS]; // (step/(mm/s)) + block->adv_comp = adv_comp / block->speed_factor; // step/(step/min) + + float advance_speed; + if (e_D_ratio > 0) + advance_speed = (extruder_advance_K * e_D_ratio * block->acceleration * cs.axis_steps_per_unit[E_AXIS]); + else + advance_speed = cs.max_jerk[E_AXIS] * cs.axis_steps_per_unit[E_AXIS]; + + // to save more space we avoid another copy of calc_timer and go through slow division, but we + // still need to replicate the *exact* same step grouping policy (see below) + if (advance_speed > MAX_STEP_FREQUENCY) advance_speed = MAX_STEP_FREQUENCY; + float advance_rate = (F_CPU / 8.0) / advance_speed; + if (advance_speed > 20000) { + block->advance_rate = advance_rate * 4; + block->advance_step_loops = 4; + } + else if (advance_speed > 10000) { + block->advance_rate = advance_rate * 2; + block->advance_step_loops = 2; + } + else + { + // never overflow the internal accumulator with very low rates + if (advance_rate < UINT16_MAX) + block->advance_rate = advance_rate; + else + block->advance_rate = UINT16_MAX; + block->advance_step_loops = 1; + } + + #ifdef LA_DEBUG + if (block->advance_step_loops > 2) + // @wavexx: we should really check for the difference between step_loops and + // advance_step_loops instead. A difference of more than 1 will lead + // to uneven speed and *should* be adjusted here by furthermore + // reducing the speed. + SERIAL_ECHOLNPGM("LA: More than 2 steps per eISR loop executed."); + #endif + } +#endif + calculate_trapezoid_for_block(block, block->entry_speed, safe_speed); if (block->step_event_count.wide <= 32767) From 51a539608cafafb41fe8ee248c41a91c70f7b145 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Mon, 22 Jun 2020 00:19:47 +0200 Subject: [PATCH 04/15] Reset LA_phase at each trapezoid stage There used to be a single stage where an extruder reversal could occur, but since PR #2591 reversals can happen up to two times per trapezoid. Reset LA_phase when ADV_INIT is set, since it is re-inizialized only when needed a few lines afterward. This improves performance by avoiding to check the phase continuosly to the end of the trapezoid. Likewise, always set ADV_INIT during the first cruising step, also to force a LA_phase reset. --- Firmware/stepper.cpp | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index de250ec97..57520070f 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -848,12 +848,10 @@ FORCE_INLINE void isr() { #ifdef LIN_ADVANCE if(current_block->use_advance_lead) { - if (!nextAdvanceISR) { - // Due to E-jerk, there can be discontinuities in pressure state where an - // acceleration or deceleration can be skipped or joined with the previous block. - // If LA was not previously active, re-check the pressure level - la_state = ADV_INIT; - } + // Due to E-jerk, there can be discontinuities in pressure state where an + // acceleration or deceleration can be skipped or joined with the previous block. + // If LA was not previously active, re-check the pressure level + la_state = ADV_INIT; } #endif } @@ -865,6 +863,7 @@ FORCE_INLINE void isr() { #ifdef LIN_ADVANCE // avoid multiple instances or function calls to advance_spread if (la_state & ADV_INIT) { + LA_phase = -1; if (current_adv_steps == target_adv_steps) { // nothing to be done in this phase la_state = 0; From 173aa2debad36da6590c53ad0a4114bfcda6a710 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Mon, 22 Jun 2020 00:54:50 +0200 Subject: [PATCH 05/15] Fix bogus timer check preventing fast LA steps to be scheduled Simplify and fix the broken timer check when scheduling advance ticks. This dates back to the original LA15 PR, an old bug... --- Firmware/stepper.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index 57520070f..b1c3d2861 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -1016,7 +1016,7 @@ FORCE_INLINE void advance_isr_scheduler() { // Schedule the next closest tick, ignoring advance if scheduled too // soon in order to avoid skewing the regular stepper acceleration - if (nextAdvanceISR != ADV_NEVER && (nextAdvanceISR + TCNT1 + 40) < nextMainISR) + if (nextAdvanceISR != ADV_NEVER && (nextAdvanceISR + 40) < nextMainISR) OCR1A = nextAdvanceISR; else OCR1A = nextMainISR; From a36efcb347bc3c1312f401ecaac383073d3856ba Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Mon, 22 Jun 2020 15:03:49 +0200 Subject: [PATCH 06/15] Remove cumulative error in segments without cruising (take 4) Avoid sqrt when possible --- Firmware/planner.cpp | 35 +++++++++++++++++++++++------------ 1 file changed, 23 insertions(+), 12 deletions(-) diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index 54bc692d8..b5b251ec4 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -225,15 +225,24 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit uint32_t accel_decel_steps = accelerate_steps + decelerate_steps; // Size of Plateau of Nominal Rate. uint32_t plateau_steps = 0; - // Maximum effective speed reached in the trapezoid (step/min) - float max_rate; + +#ifdef LIN_ADVANCE + uint16_t final_adv_steps = 0; + uint16_t max_adv_steps = 0; + if (block->use_advance_lead) { + final_adv_steps = final_rate * block->adv_comp; + } +#endif // Is the Plateau of Nominal Rate smaller than nothing? That means no cruising, and we will // have to use intersection_distance() to calculate when to abort acceleration and start braking // in order to reach the final_rate exactly at the end of this block. if (accel_decel_steps < block->step_event_count.wide) { plateau_steps = block->step_event_count.wide - accel_decel_steps; - max_rate = block->nominal_rate; +#ifdef LIN_ADVANCE + if (block->use_advance_lead) + max_adv_steps = block->nominal_rate * block->adv_comp; +#endif } else { uint32_t acceleration_x4 = acceleration << 2; // Avoid negative numbers @@ -267,17 +276,19 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit accelerate_steps = block->step_event_count.wide - decelerate_steps; } - max_rate = sqrt(acceleration_x2 * accelerate_steps + initial_rate_sqr); - } - #ifdef LIN_ADVANCE - uint16_t final_adv_steps = 0; - uint16_t max_adv_steps = 0; - if (block->use_advance_lead) { - final_adv_steps = final_rate * block->adv_comp; - max_adv_steps = max_rate * block->adv_comp; - } + if (block->use_advance_lead) { + if(!accelerate_steps || !decelerate_steps) { + // accelerate_steps=0: deceleration-only ramp, max_rate is effectively unused + // decelerate_steps=0: acceleration-only ramp, max_rate _is_ final_rate + max_adv_steps = final_adv_steps; + } else { + uint16_t max_rate = sqrt(acceleration_x2 * accelerate_steps + initial_rate_sqr); + max_adv_steps = max_rate * block->adv_comp; + } + } #endif + } CRITICAL_SECTION_START; // Fill variables used by the stepper in a critical section // This block locks the interrupts globally for 4.38 us, From 1206fc316402012c4607865d32cbc8ad470b71aa Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Mon, 22 Jun 2020 15:34:34 +0200 Subject: [PATCH 07/15] Avoid useless cast --- Firmware/planner.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index b5b251ec4..bc991fdf8 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -283,7 +283,7 @@ void calculate_trapezoid_for_block(block_t *block, float entry_speed, float exit // decelerate_steps=0: acceleration-only ramp, max_rate _is_ final_rate max_adv_steps = final_adv_steps; } else { - uint16_t max_rate = sqrt(acceleration_x2 * accelerate_steps + initial_rate_sqr); + float max_rate = sqrt(acceleration_x2 * accelerate_steps + initial_rate_sqr); max_adv_steps = max_rate * block->adv_comp; } } From 50a09824fd8b3d33d62940b6836db350ea8670d5 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Tue, 23 Jun 2020 15:20:46 +0200 Subject: [PATCH 08/15] Avoid scheduling useless eISR ticks When switching to a new trapezoid step with the right pressure, cancel any pending eISR right away. Similarly do not schedule another eISR if the pressure will be reached by the end of the eISR. This was done in the past to preserve the current LA_phase. This is not needed anymore, since it will be reset at each trapezoid step when LA is re-initialized. --- Firmware/stepper.cpp | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index b1c3d2861..0c7ecae5d 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -864,9 +864,11 @@ FORCE_INLINE void isr() { // avoid multiple instances or function calls to advance_spread if (la_state & ADV_INIT) { LA_phase = -1; + if (current_adv_steps == target_adv_steps) { - // nothing to be done in this phase + // nothing to be done in this phase, cancel any pending eisr la_state = 0; + nextAdvanceISR = ADV_NEVER; } else { eISR_Err = current_block->advance_rate / 4; @@ -933,20 +935,21 @@ FORCE_INLINE void advance_isr() { current_adv_steps -= e_step_loops; else current_adv_steps = 0; - nextAdvanceISR = eISR_Rate; } else if (current_adv_steps < target_adv_steps) { // compression e_steps += e_step_loops; if (e_steps) WRITE_NC(E0_DIR_PIN, e_steps < 0? INVERT_E0_DIR: !INVERT_E0_DIR); current_adv_steps += e_step_loops; - nextAdvanceISR = eISR_Rate; } - else { + + if (current_adv_steps == target_adv_steps) { // advance steps completed nextAdvanceISR = ADV_NEVER; - LA_phase = -1; - e_step_loops = 1; + } + else { + // schedule another tick + nextAdvanceISR = eISR_Rate; } } From f1efce7e525a651f0fcd08005e310ff7f21e6c8b Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Tue, 23 Jun 2020 16:40:39 +0200 Subject: [PATCH 09/15] Handle LA termination with double/quad stepping properly Before PR #2591 LA was automatically capped during cruising or deceleration. However we now rely on reaching the current pressure state exactly to stop. When dual/quad stepping inside the eISR we might incur in oscillating behavior if we do not handle it correctly. This might be the cause behind #2757 This now changes e_step_loops to be a phase-local variable, so we now reset it each phase too (instead of per-segment). --- Firmware/stepper.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index 0c7ecae5d..13724cc9e 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -348,10 +348,7 @@ FORCE_INLINE void stepper_next_block() #ifdef LIN_ADVANCE if (current_block->use_advance_lead) { - e_step_loops = current_block->advance_step_loops; target_adv_steps = current_block->max_adv_steps; - } else { - e_step_loops = 1; } e_steps = 0; nextAdvanceISR = ADV_NEVER; @@ -871,7 +868,10 @@ FORCE_INLINE void isr() { nextAdvanceISR = ADV_NEVER; } else { + // reset error and iterations per loop for this phase eISR_Err = current_block->advance_rate / 4; + e_step_loops = current_block->advance_step_loops; + if ((la_state & ADV_ACC_VARY) && e_extruding && (current_adv_steps > target_adv_steps)) { // LA could reverse the direction of extrusion in this phase LA_phase = 0; @@ -929,15 +929,22 @@ FORCE_INLINE void isr() { FORCE_INLINE void advance_isr() { if (current_adv_steps > target_adv_steps) { // decompression + if (e_step_loops != 1) { + uint16_t d_steps = current_adv_steps - target_adv_steps; + if (d_steps < e_step_loops) + e_step_loops = d_steps; + } e_steps -= e_step_loops; if (e_steps) WRITE_NC(E0_DIR_PIN, e_steps < 0? INVERT_E0_DIR: !INVERT_E0_DIR); - if(current_adv_steps > e_step_loops) - current_adv_steps -= e_step_loops; - else - current_adv_steps = 0; + current_adv_steps -= e_step_loops; } else if (current_adv_steps < target_adv_steps) { // compression + if (e_step_loops != 1) { + uint16_t d_steps = target_adv_steps - current_adv_steps; + if (d_steps < e_step_loops) + e_step_loops = d_steps; + } e_steps += e_step_loops; if (e_steps) WRITE_NC(E0_DIR_PIN, e_steps < 0? INVERT_E0_DIR: !INVERT_E0_DIR); current_adv_steps += e_step_loops; From c08f37da9661feb946a6602dae49bab3a76071cb Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Wed, 24 Jun 2020 16:56:25 +0200 Subject: [PATCH 10/15] Use nominal rate for phase calculations The local interval calculated by advance_spread() might oscillate too much in narrow intervals. --- Firmware/stepper.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index 13724cc9e..80c5be294 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -883,11 +883,13 @@ FORCE_INLINE void isr() { advance_spread(main_Rate); if (LA_phase >= 0) { if (step_loops == e_step_loops) - LA_phase = (eISR_Rate > main_Rate); + LA_phase = (current_block->advance_rate > main_Rate); else { // avoid overflow through division. warning: we need to _guarantee_ step_loops // and e_step_loops are <= 4 due to fastdiv's limit - LA_phase = (fastdiv(eISR_Rate, step_loops) > fastdiv(main_Rate, e_step_loops)); + auto adv_rate_n = fastdiv(current_block->advance_rate, step_loops); + auto main_rate_n = fastdiv(main_Rate, e_step_loops); + LA_phase = (adv_rate_n > main_rate_n); } } } From fb5f09da6d365cb03db7abc01b18532740db3e6b Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Thu, 16 Jul 2020 17:47:48 +0200 Subject: [PATCH 11/15] Fix incorrect precedence for retraction phase The logic was inverted, causing the fastest isr to always retract instead of counter-balance the acceleration properly. --- Firmware/stepper.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index 80c5be294..c5a6b69c9 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -883,13 +883,13 @@ FORCE_INLINE void isr() { advance_spread(main_Rate); if (LA_phase >= 0) { if (step_loops == e_step_loops) - LA_phase = (current_block->advance_rate > main_Rate); + LA_phase = (current_block->advance_rate < main_Rate); else { // avoid overflow through division. warning: we need to _guarantee_ step_loops // and e_step_loops are <= 4 due to fastdiv's limit auto adv_rate_n = fastdiv(current_block->advance_rate, step_loops); auto main_rate_n = fastdiv(main_Rate, e_step_loops); - LA_phase = (adv_rate_n > main_rate_n); + LA_phase = (adv_rate_n < main_rate_n); } } } From c54474f2db5cf9f04e0308c3c6694fff671a7d65 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Thu, 16 Jul 2020 13:26:15 +0200 Subject: [PATCH 12/15] Guard against planning/numerical errors in short segments Turns out for high-res curved models the numerical error and the SLOWDOWN handling in the planner can cause enough variance in the calculated pressure to trigger LA to continuosly, making matters worse. Clamp LA again, but only during extrusion, so that the runaway error is limited by the current segment length. --- Firmware/stepper.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index c5a6b69c9..949ffd587 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -809,8 +809,11 @@ FORCE_INLINE void isr() { acceleration_time += timer; #ifdef LIN_ADVANCE if (current_block->use_advance_lead) { - if (step_events_completed.wide <= (unsigned long int)step_loops) + if (step_events_completed.wide <= (unsigned long int)step_loops) { la_state = ADV_INIT | ADV_ACC_VARY; + if (e_extruding && current_adv_steps > target_adv_steps) + target_adv_steps = current_adv_steps; + } } #endif } @@ -832,6 +835,8 @@ FORCE_INLINE void isr() { if (step_events_completed.wide <= (unsigned long int)current_block->decelerate_after + step_loops) { target_adv_steps = current_block->final_adv_steps; la_state = ADV_INIT | ADV_ACC_VARY; + if (e_extruding && current_adv_steps < target_adv_steps) + target_adv_steps = current_adv_steps; } } #endif @@ -849,6 +854,8 @@ FORCE_INLINE void isr() { // acceleration or deceleration can be skipped or joined with the previous block. // If LA was not previously active, re-check the pressure level la_state = ADV_INIT; + if (e_extruding) + target_adv_steps = current_adv_steps; } #endif } From 9b8f642b28eb5441a8342c73043d1570c0544bd2 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sun, 19 Jul 2020 17:41:38 +0200 Subject: [PATCH 13/15] Account for flow adjustments in LA The e/D ratio should be calculated using the extrusion length. As such, purify the e_D_ratio from the current extruder multiplier in order to account correctly for flow adjustments. --- Firmware/Marlin.h | 2 +- Firmware/planner.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Firmware/Marlin.h b/Firmware/Marlin.h index 5c03552bf..697f2f72c 100755 --- a/Firmware/Marlin.h +++ b/Firmware/Marlin.h @@ -299,7 +299,7 @@ extern float feedrate; extern int feedmultiply; extern int extrudemultiply; // Sets extrude multiply factor (in percent) for all extruders extern int extruder_multiply[EXTRUDERS]; // sets extrude multiply factor (in percent) for each extruder individually -extern float volumetric_multiplier[EXTRUDERS]; // reciprocal of cross-sectional area of filament (in square millimeters), stored this way to reduce computational burden in planner +extern float extruder_multiplier[EXTRUDERS]; // reciprocal of cross-sectional area of filament (in square millimeters), stored this way to reduce computational burden in planner extern float current_position[NUM_AXIS] ; extern float destination[NUM_AXIS] ; extern float min_pos[3]; diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index bc991fdf8..a994c74fd 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -1098,7 +1098,7 @@ Having the real displacement of the head, we can calculate the total movement le if (block->use_advance_lead) { // all extrusion moves with LA require a compression which is proportional to the // extrusion_length to distance ratio (e/D) - e_D_ratio = (e - position_float[E_AXIS]) / + e_D_ratio = ((e - position_float[E_AXIS]) / extruder_multiplier[extruder]) / sqrt(sq(x - position_float[X_AXIS]) + sq(y - position_float[Y_AXIS]) + sq(z - position_float[Z_AXIS])); From a08ca19adeec3da70725e5b9555bf37bd3e276ba Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Mon, 20 Jul 2020 12:42:28 +0200 Subject: [PATCH 14/15] Make flow correction optional, disabled by default If you're using flow to correct for an incorrect source diameter, which is probably the main usage when using the LCD, then LA shouldn't be adjusted. It's still unclear what the effect of M221 in gcode should be regarding overall extrusion width. If M221 means "thicker lines", then LA should also be adjusted accordingly. This stems from the fact that the source diameter/length needs to be known in order to determine a compression factor which is independent of the extrusion width, but the FW only ever sees one value currently (the extrusion length) which combines both. This makes it impossible for the FW to adjust for one OR the other scenario, depending on what you expect for M221 to mean. --- Firmware/Configuration_adv.h | 1 + Firmware/planner.cpp | 16 ++++++++++++---- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/Firmware/Configuration_adv.h b/Firmware/Configuration_adv.h index 5386c2815..7deff3c12 100644 --- a/Firmware/Configuration_adv.h +++ b/Firmware/Configuration_adv.h @@ -288,6 +288,7 @@ #define LA_K_DEF 0 // Default K factor (Unit: mm compression per 1mm/s extruder speed) #define LA_K_MAX 10 // Maximum acceptable K factor (exclusive, see notes in planner.cpp:plan_buffer_line) #define LA_LA10_MIN LA_K_MAX // Lin. Advance 1.0 threshold value (inclusive) + //#define LA_FLOWADJ // Adjust LA along with flow/M221 for uniform width //#define LA_NOCOMPAT // Disable Linear Advance 1.0 compatibility //#define LA_LIVE_K // Allow adjusting K in the Tune menu //#define LA_DEBUG // If enabled, this will generate debug information output over USB. diff --git a/Firmware/planner.cpp b/Firmware/planner.cpp index a994c74fd..4789b0336 100644 --- a/Firmware/planner.cpp +++ b/Firmware/planner.cpp @@ -1096,12 +1096,20 @@ Having the real displacement of the head, we can calculate the total movement le && delta_mm[E_AXIS] >= 0 && abs(delta_mm[Z_AXIS]) < 0.5; if (block->use_advance_lead) { +#ifdef LA_FLOWADJ + // M221/FLOW should change uniformly the extrusion thickness + float delta_e = (e - position_float[E_AXIS]) / extruder_multiplier[extruder]; +#else + // M221/FLOW only adjusts for an incorrect source diameter + float delta_e = (e - position_float[E_AXIS]); +#endif + float delta_D = sqrt(sq(x - position_float[X_AXIS]) + + sq(y - position_float[Y_AXIS]) + + sq(z - position_float[Z_AXIS])); + // all extrusion moves with LA require a compression which is proportional to the // extrusion_length to distance ratio (e/D) - e_D_ratio = ((e - position_float[E_AXIS]) / extruder_multiplier[extruder]) / - sqrt(sq(x - position_float[X_AXIS]) - + sq(y - position_float[Y_AXIS]) - + sq(z - position_float[Z_AXIS])); + e_D_ratio = delta_e / delta_D; // Check for unusual high e_D ratio to detect if a retract move was combined with the last // print move due to min. steps per segment. Never execute this with advance! This assumes From feafc5e5abdc1acee8683411c83c4595885332f5 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Mon, 27 Jul 2020 19:12:56 +0200 Subject: [PATCH 15/15] Alternative schedule for LA ticks Remove most of the original complexity from advance_spread. Instead of accumulating time to be scheduled, plan ahead of time each eISR tick using the next main interval + an accumulator (eISR_Err), which keeps everything much simpler. The distribution of the advance ticks is now using the real LA frequency, which leaves a bit more time between the last LA tick and the main stepper isr. We take advantage of the accumulator to force a LA tick right after the first main tick, which removes a +/- 1 scheduling error at higher step rates. When decompressing, we force 2 steps instead, so that the direction reversal happens immediately (first tick zeros esteps, second inverts the sign), removing another +/- 1 error at higher step rates. --- Firmware/stepper.cpp | 39 ++++++++++++++++----------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/Firmware/stepper.cpp b/Firmware/stepper.cpp index 949ffd587..8b21ee678 100644 --- a/Firmware/stepper.cpp +++ b/Firmware/stepper.cpp @@ -125,7 +125,7 @@ volatile signed char count_direction[NUM_AXIS] = { 1, 1, 1, 1}; static uint16_t main_Rate; static uint16_t eISR_Rate; - static uint16_t eISR_Err; + static uint32_t eISR_Err; static uint16_t current_adv_steps; static uint16_t target_adv_steps; @@ -733,38 +733,30 @@ FORCE_INLINE uint16_t fastdiv(uint16_t q, uint8_t d) FORCE_INLINE void advance_spread(uint16_t timer) { - if(eISR_Err > timer) + eISR_Err += timer; + + uint8_t ticks = 0; + while(eISR_Err >= current_block->advance_rate) + { + ++ticks; + eISR_Err -= current_block->advance_rate; + } + if(!ticks) { - // advance-step skipped - eISR_Err -= timer; eISR_Rate = timer; nextAdvanceISR = timer; return; } - // at least one step - uint8_t ticks = 1; - uint32_t block = current_block->advance_rate; - uint16_t max_t = timer - eISR_Err; - while (block < max_t) - { - ++ticks; - block += current_block->advance_rate; - } - if (block > timer) - eISR_Err += block - timer; - else - eISR_Err -= timer - block; - - if (ticks <= 4) - eISR_Rate = fastdiv(timer, ticks); + if (ticks <= 3) + eISR_Rate = fastdiv(timer, ticks + 1); else { // >4 ticks are still possible on slow moves - eISR_Rate = timer / ticks; + eISR_Rate = timer / (ticks + 1); } - nextAdvanceISR = eISR_Rate / 2; + nextAdvanceISR = eISR_Rate; } #endif @@ -876,11 +868,12 @@ FORCE_INLINE void isr() { } else { // reset error and iterations per loop for this phase - eISR_Err = current_block->advance_rate / 4; + eISR_Err = current_block->advance_rate; e_step_loops = current_block->advance_step_loops; if ((la_state & ADV_ACC_VARY) && e_extruding && (current_adv_steps > target_adv_steps)) { // LA could reverse the direction of extrusion in this phase + eISR_Err += current_block->advance_rate; LA_phase = 0; } }