[2.0.x] Fix stepper/planner race condition and Stepper pulse timer (#11081)

* Planner: Removal of some race conditions between Stepper ISR and Planner, some of them pointed out by @AnHardt, some of my own findings
* Fixing timing on stepper pulses adding a compensation for the non-null time required to set ports.

Marlin/src/module/planner.cpp

@@ -758,7 +758,11 @@ void Planner::calculate_trapezoid_for_block(block_t* const block, const float &e
   const bool was_enabled = STEPPER_ISR_ENABLED();
   if (was_enabled) DISABLE_STEPPER_DRIVER_INTERRUPT();
 
-  // Don't update variables if block is busy: It is being interpreted by the planner
+  // Don't update variables if block is busy; it is being interpreted by the planner.
+  // If this happens, there's a problem... The block speed is inconsistent. Some values
+  // have already been updated, but the Stepper ISR is already using the block. Fortunately,
+  // the values being used by the Stepper ISR weren't touched, so just stop here...
+  // TODO: There may be a way to update a running block, depending on the stepper ISR position.
   if (!TEST(block->flag, BLOCK_BIT_BUSY)) {
     block->accelerate_until = accelerate_steps;
     block->decelerate_after = accelerate_steps + plateau_steps;
@@ -862,10 +866,13 @@ void Planner::reverse_pass_kernel(block_t* const current, const block_t * const
         ? max_entry_speed_sqr
         : MIN(max_entry_speed_sqr, max_allowable_speed_sqr(-current->acceleration, next ? next->entry_speed_sqr : sq(MINIMUM_PLANNER_SPEED), current->millimeters));
       if (current->entry_speed_sqr != new_entry_speed_sqr) {
-        current->entry_speed_sqr = new_entry_speed_sqr;
 
-        // Need to recalculate the block speed
+        // Need to recalculate the block speed - Mark it now, so the stepper
+        // ISR does not consume the block before being recalculated
         SBI(current->flag, BLOCK_BIT_RECALCULATE);
+
+        // Set the new entry speed
+        current->entry_speed_sqr = new_entry_speed_sqr;
       }
     }
   }
@@ -925,14 +932,15 @@ void Planner::forward_pass_kernel(const block_t* const previous, block_t* const
       // If true, current block is full-acceleration and we can move the planned pointer forward.
       if (new_entry_speed_sqr < current->entry_speed_sqr) {
 
+        // Mark we need to recompute the trapezoidal shape, and do it now,
+        // so the stepper ISR does not consume the block before being recalculated
+        SBI(current->flag, BLOCK_BIT_RECALCULATE);
+
         // Always <= max_entry_speed_sqr. Backward pass sets this.
         current->entry_speed_sqr = new_entry_speed_sqr; // Always <= max_entry_speed_sqr. Backward pass sets this.
 
         // Set optimal plan pointer.
         block_buffer_planned = block_index;
-
-        // And mark we need to recompute the trapezoidal shape
-        SBI(current->flag, BLOCK_BIT_RECALCULATE);
       }
     }
 
@@ -1019,6 +1027,12 @@ void Planner::recalculate_trapezoids() {
       if (current) {
         // Recalculate if current block entry or exit junction speed has changed.
         if (TEST(current->flag, BLOCK_BIT_RECALCULATE) || TEST(next->flag, BLOCK_BIT_RECALCULATE)) {
+
+          // Mark the current block as RECALCULATE, to protect it from the Stepper ISR running it.
+          // Note that due to the above condition, there's a chance the current block isn't marked as
+          // RECALCULATE yet, but the next one is. That's the reason for the following line.
+          SBI(current->flag, BLOCK_BIT_RECALCULATE);
+
           // NOTE: Entry and exit factors always > 0 by all previous logic operations.
           const float current_nominal_speed = SQRT(current->nominal_speed_sqr),
                       nomr = 1.0 / current_nominal_speed;
@@ -1030,7 +1044,10 @@ void Planner::recalculate_trapezoids() {
               current->final_adv_steps = next_entry_speed * comp;
             }
           #endif
-          CBI(current->flag, BLOCK_BIT_RECALCULATE); // Reset current only to ensure next trapezoid is computed
+
+          // Reset current only to ensure next trapezoid is computed - The
+          // stepper is free to use the block from now on.
+          CBI(current->flag, BLOCK_BIT_RECALCULATE);
         }
       }
 
@@ -1043,6 +1060,12 @@ void Planner::recalculate_trapezoids() {
 
   // Last/newest block in buffer. Exit speed is set with MINIMUM_PLANNER_SPEED. Always recalculated.
   if (next) {
+
+    // Mark the next(last) block as RECALCULATE, to prevent the Stepper ISR running it.
+    // As the last block is always recalculated here, there is a chance the block isn't
+    // marked as RECALCULATE yet. That's the reason for the following line.
+    SBI(next->flag, BLOCK_BIT_RECALCULATE);
+
     const float next_nominal_speed = SQRT(next->nominal_speed_sqr),
                 nomr = 1.0 / next_nominal_speed;
     calculate_trapezoid_for_block(next, next_entry_speed * nomr, (MINIMUM_PLANNER_SPEED) * nomr);
@@ -1053,6 +1076,9 @@ void Planner::recalculate_trapezoids() {
         next->final_adv_steps = (MINIMUM_PLANNER_SPEED) * comp;
       }
     #endif
+
+    // Reset next only to ensure its trapezoid is computed - The stepper is free to use
+    // the block from now on.
     CBI(next->flag, BLOCK_BIT_RECALCULATE);
   }
 }

Marlin/src/module/stepper.h

@@ -53,7 +53,7 @@
 //
 
 #ifndef MINIMUM_STEPPER_PULSE
-  #define MINIMUM_STEPPER_PULSE 0
+  #define MINIMUM_STEPPER_PULSE 0UL
 #endif
 
 #ifndef MAXIMUM_STEPPER_RATE
@@ -86,7 +86,10 @@
   // Stepper Loop base cycles
   #define ISR_LOOP_BASE_CYCLES 4UL
 
-  // And each stepper takes 16 cycles
+  // To start the step pulse, in the worst case takes
+  #define ISR_START_STEPPER_CYCLES 13UL
+
+  // And each stepper (start + stop pulse) takes in worst case
   #define ISR_STEPPER_CYCLES 16UL
 
 #else
@@ -111,51 +114,72 @@
   // Stepper Loop base cycles
   #define ISR_LOOP_BASE_CYCLES 32UL
 
-  // And each stepper takes 88 cycles
+  // To start the step pulse, in the worst case takes
+  #define ISR_START_STEPPER_CYCLES 57UL
+
+  // And each stepper (start + stop pulse) takes in worst case
   #define ISR_STEPPER_CYCLES 88UL
 
 #endif
 
 // Add time for each stepper
 #ifdef 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
 #ifdef 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
 #ifdef 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, then 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)
 
 // Calculate the minimum MPU cycles needed per pulse to enforce, limited to the max stepper rate
 #define _MIN_STEPPER_PULSE_CYCLES(N) MAX((F_CPU) / (MAXIMUM_STEPPER_RATE), ((F_CPU) / 500000UL) * (N))
 #if MINIMUM_STEPPER_PULSE
-  #define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES(MINIMUM_STEPPER_PULSE)
+  #define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES((MINIMUM_STEPPER_PULSE))
 #else
-  #define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES(1)
+  #define MIN_STEPPER_PULSE_CYCLES _MIN_STEPPER_PULSE_CYCLES(1UL)
 #endif
 
-#define MIN_PULSE_TICKS  ((PULSE_TIMER_TICKS_PER_US) * (MINIMUM_STEPPER_PULSE))
+// Calculate the minimum ticks of the PULSE timer that must elapse with the step pulse enabled
-#define ADDED_STEP_TICKS ((MIN_STEPPER_PULSE_CYCLES) / (PULSE_TIMER_PRESCALE) - MIN_PULSE_TICKS)
+// 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
+#define MIN_PULSE_TICKS (((PULSE_TIMER_TICKS_PER_US) * (MINIMUM_STEPPER_PULSE)) + ((MIN_ISR_START_LOOP_CYCLES) / (PULSE_TIMER_PRESCALE)))
+
+// 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))
 
 // 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))