From 15548958e98ddae4de263d77aa68aa025226e2d2 Mon Sep 17 00:00:00 2001 From: Yuri D'Elia Date: Sun, 21 Jun 2020 15:19:31 +0200 Subject: [PATCH 1/5] 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 2/5] 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 3/5] 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 4/5] 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 5/5] 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;