Improve pulse timing and step reliability (#16128)

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.
  */

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

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

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,47 +1427,50 @@ 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)
 
-    // Pulse start
+    // Determine if pulses are needed
     #if HAS_X_STEP
-      PULSE_START(X);
+      PULSE_PREP(X);
     #endif
     #if HAS_Y_STEP
-      PULSE_START(Y);
+      PULSE_PREP(Y);
     #endif
     #if HAS_Z_STEP
-      PULSE_START(Z);
+      PULSE_PREP(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) {
@@ -1465,14 +1479,36 @@ void Stepper::stepper_pulse_phase_isr() {
           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);
+        #else
+          step_needed[E_AXIS] = delta_error.e >= 0;
         #endif
       }
-    #else // !LIN_ADVANCE && !MIXING_EXTRUDER
-      #if HAS_E0_STEP
+    #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);
+    #endif
+    #if HAS_Y_STEP
+      PULSE_START(Y);
+    #endif
+    #if HAS_Z_STEP
+      PULSE_START(Z);
+    #endif
+
+    #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 // !LIN_ADVANCE
 
-    // Decrement the count of pending pulses to do
-    --events_to_do;
+      #endif  // !MIXING_EXTRUDER
+    #endif // !LIN_ADVANCE
 
-    // 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
-    }
+    #if (MINIMUM_STEPPER_PULSE || MAXIMUM_STEPPER_RATE) && DISABLED(I2S_STEPPER_STREAM)
+      if (events_to_do) START_LOW_PULSE();
+    #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;

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))