Proper AVR preemptive interrupt handling (#10501)

Co-Authored-By: ejtagle <ejtagle@hotmail.com>

Marlin/planner.cpp

@@ -1436,7 +1436,7 @@ void Planner::_buffer_steps(const int32_t (&target)[XYZE]
   const float v_allowable = max_allowable_speed(-block->acceleration, MINIMUM_PLANNER_SPEED, block->millimeters);
   // If stepper ISR is disabled, this indicates buffer_segment wants to add a split block.
   // In this case start with the max. allowed speed to avoid an interrupted first move.
-  block->entry_speed = TEST(TIMSK1, OCIE1A) ? MINIMUM_PLANNER_SPEED : min(vmax_junction, v_allowable);
+  block->entry_speed = STEPPER_ISR_ENABLED() ? MINIMUM_PLANNER_SPEED : min(vmax_junction, v_allowable);
 
   // Initialize planner efficiency flags
   // Set flag if block will always reach maximum junction speed regardless of entry/exit speeds.

Marlin/stepper.cpp

@@ -360,14 +360,33 @@ void Stepper::set_directions() {
  *  4000   500  Hz - init rate
  */
 ISR(TIMER1_COMPA_vect) {
+  /**
+   * On AVR there is no hardware prioritization and preemption of
+   * interrupts, so this emulates it. The UART has first priority
+   * (otherwise, characters will be lost due to UART overflow).
+   * Then: Stepper, Endstops, Temperature, and -finally- all others.
+   *
+   * This ISR needs to run with as little preemption as possible, so
+   * the Temperature ISR is disabled here. Now only the UART, Endstops,
+   * and Arduino-defined interrupts can preempt.
+   */
+  const bool temp_isr_was_enabled = TEMPERATURE_ISR_ENABLED();
+  DISABLE_TEMPERATURE_INTERRUPT();
+  DISABLE_STEPPER_DRIVER_INTERRUPT();
+  sei();
+
   #if ENABLED(LIN_ADVANCE)
     Stepper::advance_isr_scheduler();
   #else
     Stepper::isr();
   #endif
-}
 
-#define _ENABLE_ISRs() do { cli(); if (thermalManager.in_temp_isr) CBI(TIMSK0, OCIE0B); else SBI(TIMSK0, OCIE0B); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
+  // Disable global interrupts and reenable this ISR
+  cli();
+  ENABLE_STEPPER_DRIVER_INTERRUPT();
+  // Reenable the temperature ISR (if it was enabled)
+  if (temp_isr_was_enabled) ENABLE_TEMPERATURE_INTERRUPT();
+}
 
 void Stepper::isr() {
 
@@ -378,7 +397,7 @@ void Stepper::isr() {
 
   #if DISABLED(LIN_ADVANCE)
     // Disable Timer0 ISRs and enable global ISR again to capture UART events (incoming chars)
-    CBI(TIMSK0, OCIE0B); // Temperature ISR
+    DISABLE_TEMPERATURE_INTERRUPT();
     DISABLE_STEPPER_DRIVER_INTERRUPT();
     sei();
   #endif
@@ -409,7 +428,6 @@ void Stepper::isr() {
       step_remaining -= ocr_val;
       _NEXT_ISR(ocr_val);
       NOLESS(OCR1A, TCNT1 + 16);
-      _ENABLE_ISRs(); // re-enable ISRs
       return;
     }
 
@@ -433,7 +451,6 @@ void Stepper::isr() {
     }
     current_block = NULL;                       // Prep to get a new block after cleaning
     _NEXT_ISR(200);                             // Run at max speed - 10 KHz
-    _ENABLE_ISRs();
     return;
   }
 
@@ -462,14 +479,12 @@ void Stepper::isr() {
         if (current_block->steps[Z_AXIS] > 0) {
           enable_Z();
           _NEXT_ISR(2000); // Run at slow speed - 1 KHz
-          _ENABLE_ISRs(); // re-enable ISRs
           return;
         }
       #endif
     }
     else {
       _NEXT_ISR(2000); // Run at slow speed - 1 KHz
-      _ENABLE_ISRs(); // re-enable ISRs
       return;
     }
   }
@@ -773,9 +788,6 @@ void Stepper::isr() {
     current_block = NULL;
     planner.discard_current_block();
   }
-  #if DISABLED(LIN_ADVANCE)
-    _ENABLE_ISRs(); // re-enable ISRs
-  #endif
 }
 
 #if ENABLED(LIN_ADVANCE)
@@ -897,11 +909,6 @@ void Stepper::isr() {
   }
 
   void Stepper::advance_isr_scheduler() {
-    // Disable Timer0 ISRs and enable global ISR again to capture UART events (incoming chars)
-    CBI(TIMSK0, OCIE0B); // Temperature ISR
-    DISABLE_STEPPER_DRIVER_INTERRUPT();
-    sei();
-
     // Run main stepping ISR if flagged
     if (!nextMainISR) isr();
 
@@ -929,9 +936,6 @@ void Stepper::isr() {
 
     // Don't run the ISR faster than possible
     NOLESS(OCR1A, TCNT1 + 16);
-
-    // Restore original ISR settings
-    _ENABLE_ISRs();
   }
 
 #endif // LIN_ADVANCE

Marlin/stepper.h

@@ -54,6 +54,7 @@ extern Stepper stepper;
 
 #define ENABLE_STEPPER_DRIVER_INTERRUPT()  SBI(TIMSK1, OCIE1A)
 #define DISABLE_STEPPER_DRIVER_INTERRUPT() CBI(TIMSK1, OCIE1A)
+#define STEPPER_ISR_ENABLED()             TEST(TIMSK1, OCIE1A)
 
 // intRes = intIn1 * intIn2 >> 16
 // uses:

Marlin/temperature.cpp

@@ -1263,7 +1263,7 @@ void Temperature::init() {
   // Use timer0 for temperature measurement
   // Interleave temperature interrupt with millies interrupt
   OCR0B = 128;
-  SBI(TIMSK0, OCIE0B);
+  ENABLE_TEMPERATURE_INTERRUPT();
 
   // Wait for temperature measurement to settle
   delay(250);
@@ -1778,20 +1778,26 @@ void Temperature::set_current_temp_raw() {
  *  - For PINS_DEBUGGING, monitor and report endstop pins
  *  - For ENDSTOP_INTERRUPTS_FEATURE check endstops if flagged
  */
-ISR(TIMER0_COMPB_vect) { Temperature::isr(); }
+ISR(TIMER0_COMPB_vect) {
-
+  /**
-volatile bool Temperature::in_temp_isr = false;
+   * AVR has no hardware interrupt preemption, so emulate priorization
-
+   * and preemption of this ISR by all others by disabling the timer
-void Temperature::isr() {
+   * interrupt generation capability and reenabling global interrupts.
-  // The stepper ISR can interrupt this ISR. When it does it re-enables this ISR
+   * Any interrupt can then interrupt this handler and preempt it.
-  // at the end of its run, potentially causing re-entry. This flag prevents it.
+   * This ISR becomes the lowest priority one so the UART, Endstops
-  if (in_temp_isr) return;
+   * and Stepper ISRs can all preempt it.
-  in_temp_isr = true;
+   */
-
+  DISABLE_TEMPERATURE_INTERRUPT();
-  // Allow UART and stepper ISRs
-  CBI(TIMSK0, OCIE0B); //Disable Temperature ISR
   sei();
 
+  Temperature::isr();
+
+  // Disable global interrupts and reenable this ISR
+  cli();
+  ENABLE_TEMPERATURE_INTERRUPT();
+}
+
+void Temperature::isr() {
   static int8_t temp_count = -1;
   static ADCSensorState adc_sensor_state = StartupDelay;
   static uint8_t pwm_count = _BV(SOFT_PWM_SCALE);
@@ -2328,10 +2334,6 @@ void Temperature::isr() {
       e_hit--;
     }
   #endif
-
-  cli();
-  in_temp_isr = false;
-  SBI(TIMSK0, OCIE0B); //re-enable Temperature ISR
 }
 
 #if HAS_TEMP_SENSOR

Marlin/temperature.h

@@ -43,6 +43,10 @@
   #define SOFT_PWM_SCALE 0
 #endif
 
+#define ENABLE_TEMPERATURE_INTERRUPT()  SBI(TIMSK0, OCIE0B)
+#define DISABLE_TEMPERATURE_INTERRUPT() CBI(TIMSK0, OCIE0B)
+#define TEMPERATURE_ISR_ENABLED()      TEST(TIMSK0, OCIE0B)
+
 #define HOTEND_LOOP() for (int8_t e = 0; e < HOTENDS; e++)
 
 #if HOTENDS == 1
@@ -119,8 +123,6 @@ class Temperature {
 
   public:
 
-    static volatile bool in_temp_isr;
-
     static float current_temperature[HOTENDS];
     static int16_t current_temperature_raw[HOTENDS],
                    target_temperature[HOTENDS];