From 1bad8f1b172d8074272e5bae78a356e3671f7b50 Mon Sep 17 00:00:00 2001 From: Jason Smith Date: Thu, 19 Dec 2019 00:38:48 -0800 Subject: [PATCH] Improve pulse timing and step reliability (#16128) --- Marlin/Configuration_adv.h | 12 +-- Marlin/src/inc/Conditionals_post.h | 14 +-- Marlin/src/inc/SanityCheck.h | 8 -- Marlin/src/module/stepper.cpp | 162 +++++++++++++++++------------ Marlin/src/module/stepper.h | 37 ++++--- 5 files changed, 128 insertions(+), 105 deletions(-) diff --git a/Marlin/Configuration_adv.h b/Marlin/Configuration_adv.h index ce9f86d9c9..fd29c458e1 100644 --- a/Marlin/Configuration_adv.h +++ b/Marlin/Configuration_adv.h @@ -1548,12 +1548,12 @@ /** * Maximum stepping rate (in Hz) the stepper driver allows * If undefined, defaults to 1MHz / (2 * MINIMUM_STEPPER_PULSE) - * 500000 : Maximum for A4988 stepper driver - * 400000 : Maximum for TMC2xxx stepper drivers - * 250000 : Maximum for DRV8825 stepper driver - * 200000 : Maximum for LV8729 stepper driver - * 150000 : Maximum for TB6600 stepper driver - * 15000 : Maximum for TB6560 stepper driver + * 5000000 : Maximum for TMC2xxx stepper drivers + * 1000000 : Maximum for LV8729 stepper driver + * 500000 : Maximum for A4988 stepper driver + * 250000 : Maximum for DRV8825 stepper driver + * 150000 : Maximum for TB6600 stepper driver + * 15000 : Maximum for TB6560 stepper driver * * Override the default value based on the driver type set in Configuration.h. */ diff --git a/Marlin/src/inc/Conditionals_post.h b/Marlin/src/inc/Conditionals_post.h index 717204768c..ced854afae 100644 --- a/Marlin/src/inc/Conditionals_post.h +++ b/Marlin/src/inc/Conditionals_post.h @@ -595,11 +595,7 @@ #elif HAS_DRIVER(A4988) || HAS_DRIVER(A5984) #define MINIMUM_STEPPER_PULSE 1 #elif TRINAMICS - #if ENABLED(LIN_ADVANCE) && (HAS_TMC_STANDALONE_E_DRIVER || (HAS_TMC_E_DRIVER && DISABLED(SQUARE_WAVE_STEPPING))) - #define MINIMUM_STEPPER_PULSE 1 - #else - #define MINIMUM_STEPPER_PULSE 0 - #endif + #define MINIMUM_STEPPER_PULSE 0 #elif HAS_DRIVER(LV8729) #define MINIMUM_STEPPER_PULSE 0 #else @@ -612,14 +608,14 @@ #define MAXIMUM_STEPPER_RATE 15000 #elif HAS_DRIVER(TB6600) #define MAXIMUM_STEPPER_RATE 150000 - #elif HAS_DRIVER(LV8729) - #define MAXIMUM_STEPPER_RATE 200000 #elif HAS_DRIVER(DRV8825) #define MAXIMUM_STEPPER_RATE 250000 - #elif TRINAMICS - #define MAXIMUM_STEPPER_RATE 400000 #elif HAS_DRIVER(A4988) #define MAXIMUM_STEPPER_RATE 500000 + #elif HAS_DRIVER(LV8729) + #define MAXIMUM_STEPPER_RATE 1000000 + #elif TRINAMICS + #define MAXIMUM_STEPPER_RATE 5000000 #else #define MAXIMUM_STEPPER_RATE 250000 #endif diff --git a/Marlin/src/inc/SanityCheck.h b/Marlin/src/inc/SanityCheck.h index 6954aa03b6..616d259a1d 100644 --- a/Marlin/src/inc/SanityCheck.h +++ b/Marlin/src/inc/SanityCheck.h @@ -2551,11 +2551,3 @@ static_assert( _ARR_TEST(3,0) && _ARR_TEST(3,1) && _ARR_TEST(3,2) #error "SHOW_REMAINING_TIME currently requires a Graphical LCD." #endif #endif - -#if ENABLED(LIN_ADVANCE) && MINIMUM_STEPPER_PULSE < 1 - #if HAS_TMC_STANDALONE_E_DRIVER - #error "LIN_ADVANCE with TMC standalone driver on extruder requires MIMIMUM_STEPPER_PULSE >= 1" - #elif HAS_TMC_E_DRIVER && DISABLED(SQUARE_WAVE_STEPPING) - #error "LIN_ADVANCE with TMC driver on extruder requires SQUARE_WAVE_STEPPING or MINIMUM_STEPPER_PULSE >= 1" - #endif -#endif diff --git a/Marlin/src/module/stepper.cpp b/Marlin/src/module/stepper.cpp index f59b7a42a3..9babb13707 100644 --- a/Marlin/src/module/stepper.cpp +++ b/Marlin/src/module/stepper.cpp @@ -338,6 +338,17 @@ xyze_int8_t Stepper::count_direction{0}; #if DISABLED(MIXING_EXTRUDER) #define E_APPLY_STEP(v,Q) E_STEP_WRITE(stepper_extruder, v) #endif +#define TIMER_SETUP_NS (CYCLES_TO_NS(TIMER_READ_ADD_AND_STORE_CYCLES)) + +#define PULSE_HIGH_TICK_COUNT hal_timer_t(NS_TO_PULSE_TIMER_TICKS(_MIN_PULSE_HIGH_NS - _MIN(_MIN_PULSE_HIGH_NS, TIMER_SETUP_NS))) +#define PULSE_LOW_TICK_COUNT hal_timer_t(NS_TO_PULSE_TIMER_TICKS(_MIN_PULSE_LOW_NS - _MIN(_MIN_PULSE_LOW_NS, TIMER_SETUP_NS))) + +#define START_TIMED_PULSE(DIR) (end_tick_count = HAL_timer_get_count(PULSE_TIMER_NUM) + PULSE_##DIR##_TICK_COUNT) +#define AWAIT_TIMED_PULSE() while (HAL_timer_get_count(PULSE_TIMER_NUM) < end_tick_count) { } +#define START_HIGH_PULSE() START_TIMED_PULSE(HIGH) +#define START_LOW_PULSE() START_TIMED_PULSE(LOW) +#define AWAIT_HIGH_PULSE() AWAIT_TIMED_PULSE() +#define AWAIT_LOW_PULSE() AWAIT_TIMED_PULSE() void Stepper::wake_up() { // TCNT1 = 0; @@ -1416,34 +1427,73 @@ void Stepper::stepper_pulse_phase_isr() { // Just update the value we will get at the end of the loop step_events_completed += events_to_do; - // Get the timer count and estimate the end of the pulse - hal_timer_t pulse_end = HAL_timer_get_count(PULSE_TIMER_NUM) + hal_timer_t(MIN_PULSE_TICKS); - - const hal_timer_t added_step_ticks = hal_timer_t(ADDED_STEP_TICKS); - // Take multiple steps per interrupt (For high speed moves) - do { + bool firstStep = true; + xyze_bool_t step_needed{0}; + hal_timer_t end_tick_count; + do { #define _APPLY_STEP(AXIS) AXIS ##_APPLY_STEP #define _INVERT_STEP_PIN(AXIS) INVERT_## AXIS ##_STEP_PIN + // Determine if pulses are needed + #define PULSE_PREP(AXIS) do{ \ + delta_error[_AXIS(AXIS)] += advance_dividend[_AXIS(AXIS)]; \ + step_needed[_AXIS(AXIS)] = (delta_error[_AXIS(AXIS)] >= 0); \ + if (step_needed[_AXIS(AXIS)]) { \ + count_position[_AXIS(AXIS)] += count_direction[_AXIS(AXIS)]; \ + delta_error[_AXIS(AXIS)] -= advance_divisor; \ + } \ + }while(0) + // Start an active pulse, if Bresenham says so, and update position #define PULSE_START(AXIS) do{ \ - delta_error[_AXIS(AXIS)] += advance_dividend[_AXIS(AXIS)]; \ - if (delta_error[_AXIS(AXIS)] >= 0) { \ + if (step_needed[_AXIS(AXIS)]) { \ _APPLY_STEP(AXIS)(!_INVERT_STEP_PIN(AXIS), 0); \ - count_position[_AXIS(AXIS)] += count_direction[_AXIS(AXIS)]; \ } \ }while(0) // Stop an active pulse, if any, and adjust error term #define PULSE_STOP(AXIS) do { \ - if (delta_error[_AXIS(AXIS)] >= 0) { \ - delta_error[_AXIS(AXIS)] -= advance_divisor; \ + if (step_needed[_AXIS(AXIS)]) { \ _APPLY_STEP(AXIS)(_INVERT_STEP_PIN(AXIS), 0); \ } \ }while(0) + // Determine if pulses are needed + #if HAS_X_STEP + PULSE_PREP(X); + #endif + #if HAS_Y_STEP + PULSE_PREP(Y); + #endif + #if HAS_Z_STEP + PULSE_PREP(Z); + #endif + + #if EITHER(LIN_ADVANCE, MIXING_EXTRUDER) + delta_error.e += advance_dividend.e; + if (delta_error.e >= 0) { + count_position.e += count_direction.e; + #if ENABLED(LIN_ADVANCE) + delta_error.e -= advance_divisor; + // Don't step E here - But remember the number of steps to perform + motor_direction(E_AXIS) ? --LA_steps : ++LA_steps; + #else + step_needed[E_AXIS] = delta_error.e >= 0; + #endif + } + #elif HAS_E0_STEP + PULSE_PREP(E); + #endif + + #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) && DISABLED(I2S_STEPPER_STREAM) + if (firstStep) + firstStep = false; + else + AWAIT_LOW_PULSE(); + #endif + // Pulse start #if HAS_X_STEP PULSE_START(X); @@ -1455,24 +1505,10 @@ void Stepper::stepper_pulse_phase_isr() { PULSE_START(Z); #endif - // Pulse Extruders - // Tick the E axis, correct error term and update position - #if EITHER(LIN_ADVANCE, MIXING_EXTRUDER) - delta_error.e += advance_dividend.e; - if (delta_error.e >= 0) { - count_position.e += count_direction.e; - #if ENABLED(LIN_ADVANCE) - delta_error.e -= advance_divisor; - // Don't step E here - But remember the number of steps to perform - motor_direction(E_AXIS) ? --LA_steps : ++LA_steps; - #else // !LIN_ADVANCE && MIXING_EXTRUDER - // Don't adjust delta_error.e here! - // Being positive is the criteria for ending the pulse. - E_STEP_WRITE(mixer.get_next_stepper(), !INVERT_E_STEP_PIN); - #endif - } - #else // !LIN_ADVANCE && !MIXING_EXTRUDER - #if HAS_E0_STEP + #if DISABLED(LIN_ADVANCE) + #if ENABLED(MIXING_EXTRUDER) + if (step_needed[E_AXIS]) E_STEP_WRITE(mixer.get_next_stepper(), !INVERT_E_STEP_PIN); + #elif HAS_E0_STEP PULSE_START(E); #endif #endif @@ -1482,14 +1518,11 @@ void Stepper::stepper_pulse_phase_isr() { #endif // TODO: need to deal with MINIMUM_STEPPER_PULSE over i2s - #if MINIMUM_STEPPER_PULSE && DISABLED(I2S_STEPPER_STREAM) - // Just wait for the requested pulse duration - while (HAL_timer_get_count(PULSE_TIMER_NUM) < pulse_end) { /* nada */ } + #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) && DISABLED(I2S_STEPPER_STREAM) + START_HIGH_PULSE(); + AWAIT_HIGH_PULSE(); #endif - // Add the delay needed to ensure the maximum driver rate is enforced - if (signed(added_step_ticks) > 0) pulse_end += hal_timer_t(added_step_ticks); - // Pulse stop #if HAS_X_STEP PULSE_STOP(X); @@ -1503,31 +1536,26 @@ void Stepper::stepper_pulse_phase_isr() { #if DISABLED(LIN_ADVANCE) #if ENABLED(MIXING_EXTRUDER) + if (delta_error.e >= 0) { delta_error.e -= advance_divisor; E_STEP_WRITE(mixer.get_stepper(), INVERT_E_STEP_PIN); } + #else // !MIXING_EXTRUDER + #if HAS_E0_STEP PULSE_STOP(E); #endif - #endif + + #endif // !MIXING_EXTRUDER #endif // !LIN_ADVANCE - // Decrement the count of pending pulses to do - --events_to_do; + #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) && DISABLED(I2S_STEPPER_STREAM) + if (events_to_do) START_LOW_PULSE(); + #endif - // For minimum pulse time wait after stopping pulses also - if (events_to_do) { - // Just wait for the requested pulse duration - while (HAL_timer_get_count(PULSE_TIMER_NUM) < pulse_end) { /* nada */ } - #if MINIMUM_STEPPER_PULSE - // Add to the value, the time that the pulse must be active (to be used on the next loop) - pulse_end += hal_timer_t(MIN_PULSE_TICKS); - #endif - } - - } while (events_to_do); + } while (--events_to_do); } // This is the last half of the stepper interrupt: This one processes and @@ -1909,13 +1937,19 @@ uint32_t Stepper::stepper_block_phase_isr() { DELAY_NS(MINIMUM_STEPPER_POST_DIR_DELAY); #endif - // Get the timer count and estimate the end of the pulse - hal_timer_t pulse_end = HAL_timer_get_count(PULSE_TIMER_NUM) + hal_timer_t(MIN_PULSE_TICKS); - - const hal_timer_t added_step_ticks = hal_timer_t(ADDED_STEP_TICKS); + //const hal_timer_t added_step_ticks = hal_timer_t(ADDED_STEP_TICKS); // Step E stepper if we have steps + bool firstStep = true; + hal_timer_t end_tick_count; + while (LA_steps) { + #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) && DISABLED(I2S_STEPPER_STREAM) + if (firstStep) + firstStep = false; + else + AWAIT_LOW_PULSE(); + #endif // Set the STEP pulse ON #if ENABLED(MIXING_EXTRUDER) @@ -1925,16 +1959,16 @@ uint32_t Stepper::stepper_block_phase_isr() { #endif // Enforce a minimum duration for STEP pulse ON - #if MINIMUM_STEPPER_PULSE - // Just wait for the requested pulse duration - while (HAL_timer_get_count(PULSE_TIMER_NUM) < pulse_end) { /* nada */ } + #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) + START_HIGH_PULSE(); #endif - // Add the delay needed to ensure the maximum driver rate is enforced - if (signed(added_step_ticks) > 0) pulse_end += hal_timer_t(added_step_ticks); - LA_steps < 0 ? ++LA_steps : --LA_steps; + #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) + AWAIT_HIGH_PULSE(); + #endif + // Set the STEP pulse OFF #if ENABLED(MIXING_EXTRUDER) E_STEP_WRITE(mixer.get_stepper(), INVERT_E_STEP_PIN); @@ -1944,13 +1978,9 @@ uint32_t Stepper::stepper_block_phase_isr() { // For minimum pulse time wait before looping // Just wait for the requested pulse duration - if (LA_steps) { - while (HAL_timer_get_count(PULSE_TIMER_NUM) < pulse_end) { /* nada */ } - #if MINIMUM_STEPPER_PULSE - // Add to the value, the time that the pulse must be active (to be used on the next loop) - pulse_end += hal_timer_t(MIN_PULSE_TICKS); - #endif - } + #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) + if (LA_steps) START_LOW_PULSE(); + #endif } // LA_steps return interval; diff --git a/Marlin/src/module/stepper.h b/Marlin/src/module/stepper.h index 1ab455bd06..b3749d94d3 100644 --- a/Marlin/src/module/stepper.h +++ b/Marlin/src/module/stepper.h @@ -57,6 +57,7 @@ // #ifdef CPU_32_BIT + #define TIMER_READ_ADD_AND_STORE_CYCLES 34UL // The base ISR takes 792 cycles #define ISR_BASE_CYCLES 792UL @@ -85,6 +86,7 @@ #define ISR_STEPPER_CYCLES 16UL #else + #define TIMER_READ_ADD_AND_STORE_CYCLES 13UL // The base ISR takes 752 cycles #define ISR_BASE_CYCLES 752UL @@ -116,43 +118,32 @@ // Add time for each stepper #if HAS_X_STEP - #define ISR_START_X_STEPPER_CYCLES ISR_START_STEPPER_CYCLES #define ISR_X_STEPPER_CYCLES ISR_STEPPER_CYCLES #else - #define ISR_START_X_STEPPER_CYCLES 0UL #define ISR_X_STEPPER_CYCLES 0UL #endif #if HAS_Y_STEP - #define ISR_START_Y_STEPPER_CYCLES ISR_START_STEPPER_CYCLES #define ISR_Y_STEPPER_CYCLES ISR_STEPPER_CYCLES #else #define ISR_START_Y_STEPPER_CYCLES 0UL #define ISR_Y_STEPPER_CYCLES 0UL #endif #if HAS_Z_STEP - #define ISR_START_Z_STEPPER_CYCLES ISR_START_STEPPER_CYCLES #define ISR_Z_STEPPER_CYCLES ISR_STEPPER_CYCLES #else - #define ISR_START_Z_STEPPER_CYCLES 0UL #define ISR_Z_STEPPER_CYCLES 0UL #endif // E is always interpolated, even for mixing extruders -#define ISR_START_E_STEPPER_CYCLES ISR_START_STEPPER_CYCLES #define ISR_E_STEPPER_CYCLES ISR_STEPPER_CYCLES // If linear advance is disabled, the loop also handles them #if DISABLED(LIN_ADVANCE) && ENABLED(MIXING_EXTRUDER) - #define ISR_START_MIXING_STEPPER_CYCLES ((MIXING_STEPPERS) * (ISR_START_STEPPER_CYCLES)) #define ISR_MIXING_STEPPER_CYCLES ((MIXING_STEPPERS) * (ISR_STEPPER_CYCLES)) #else - #define ISR_START_MIXING_STEPPER_CYCLES 0UL #define ISR_MIXING_STEPPER_CYCLES 0UL #endif -// Calculate the minimum time to start all stepper pulses in the ISR loop -#define MIN_ISR_START_LOOP_CYCLES (ISR_START_X_STEPPER_CYCLES + ISR_START_Y_STEPPER_CYCLES + ISR_START_Z_STEPPER_CYCLES + ISR_START_E_STEPPER_CYCLES + ISR_START_MIXING_STEPPER_CYCLES) - // And the total minimum loop time, not including the base #define MIN_ISR_LOOP_CYCLES (ISR_X_STEPPER_CYCLES + ISR_Y_STEPPER_CYCLES + ISR_Z_STEPPER_CYCLES + ISR_E_STEPPER_CYCLES + ISR_MIXING_STEPPER_CYCLES) @@ -170,14 +161,28 @@ // adding the "start stepper pulse" code section execution cycles to account for that not all // pulses start at the beginning of the loop, so an extra time must be added to compensate so // the last generated pulse (usually the extruder stepper) has the right length -#if HAS_DRIVER(LV8729) && MINIMUM_STEPPER_PULSE == 0 - #define MIN_PULSE_TICKS ((((PULSE_TIMER_TICKS_PER_US) + 1) / 2) + ((MIN_ISR_START_LOOP_CYCLES) / uint32_t(PULSE_TIMER_PRESCALE))) +#if MINIMUM_STEPPER_PULSE && MAXIMUM_STEPPER_RATE + constexpr uint32_t _MIN_STEP_PERIOD_NS = 1000000000UL / MAXIMUM_STEPPER_RATE; + constexpr uint32_t _MIN_PULSE_HIGH_NS = 1000UL * MINIMUM_STEPPER_PULSE; + constexpr uint32_t _MIN_PULSE_LOW_NS = _MAX((_MIN_STEP_PERIOD_NS - _MIN(_MIN_STEP_PERIOD_NS, _MIN_PULSE_HIGH_NS)), _MIN_PULSE_HIGH_NS); +#elif MINIMUM_STEPPER_PULSE + // Assume 50% duty cycle + constexpr uint32_t _MIN_STEP_PERIOD_NS = 1000000000UL / MAXIMUM_STEPPER_RATE; + constexpr uint32_t _MIN_PULSE_HIGH_NS = 1000UL * MINIMUM_STEPPER_PULSE; + constexpr uint32_t _MIN_PULSE_LOW_NS = _MIN_PULSE_HIGH_NS; +#elif MAXIMUM_STEPPER_RATE + // Assume 50% duty cycle + constexpr uint32_t _MIN_PULSE_HIGH_NS = 500000000UL / MAXIMUM_STEPPER_RATE; + constexpr uint32_t _MIN_PULSE_LOW_NS = _MIN_PULSE_HIGH_NS; #else - #define MIN_PULSE_TICKS (((PULSE_TIMER_TICKS_PER_US) * uint32_t(MINIMUM_STEPPER_PULSE)) + ((MIN_ISR_START_LOOP_CYCLES) / uint32_t(PULSE_TIMER_PRESCALE))) + #error "Expected at least one of MINIMUM_STEPPER_PULSE or MAXIMUM_STEPPER_RATE to be defined" #endif -// Calculate the extra ticks of the PULSE timer between step pulses -#define ADDED_STEP_TICKS (((MIN_STEPPER_PULSE_CYCLES) / (PULSE_TIMER_PRESCALE)) - (MIN_PULSE_TICKS)) +// TODO: NS_TO_PULSE_TIMER_TICKS has some rounding issues: +// 1. PULSE_TIMER_TICKS_PER_US rounds to an integer, which loses 20% of the count for a 2.5 MHz pulse tick (such as for LPC1768) +// 2. The math currently rounds down to the closes tick. Perhaps should round up. +constexpr uint32_t NS_TO_PULSE_TIMER_TICKS(uint32_t NS) { return PULSE_TIMER_TICKS_PER_US * (NS) / 1000UL; } +#define CYCLES_TO_NS(CYC) (1000UL * (CYC) / ((F_CPU) / 1000000)) // But the user could be enforcing a minimum time, so the loop time is #define ISR_LOOP_CYCLES (ISR_LOOP_BASE_CYCLES + _MAX(MIN_STEPPER_PULSE_CYCLES, MIN_ISR_LOOP_CYCLES))