Proper AVR preemptive interrupt handling (#10496)

Also simplify logic on all ARM-based interrupts. Now, it is REQUIRED to properly configure interrupt priority. USART should have highest priority, followed by Stepper, and then all others.

Marlin/src/HAL/HAL_AVR/HAL.h

@@ -142,10 +142,11 @@ extern "C" {
 
 #define ENABLE_STEPPER_DRIVER_INTERRUPT()  SBI(TIMSK1, OCIE1A)
 #define DISABLE_STEPPER_DRIVER_INTERRUPT() CBI(TIMSK1, OCIE1A)
-#define STEPPER_ISR_ENABLED() TEST(TIMSK1, OCIE1A)
+#define STEPPER_ISR_ENABLED()             TEST(TIMSK1, OCIE1A)
 
-#define ENABLE_TEMPERATURE_INTERRUPT()  SBI(TIMSK0, OCIE0B)
+#define ENABLE_TEMPERATURE_INTERRUPT()     SBI(TIMSK0, OCIE0B)
-#define DISABLE_TEMPERATURE_INTERRUPT() CBI(TIMSK0, OCIE0B)
+#define DISABLE_TEMPERATURE_INTERRUPT()    CBI(TIMSK0, OCIE0B)
+#define TEMPERATURE_ISR_ENABLED()         TEST(TIMSK0, OCIE0B)
 
 #define HAL_timer_start(timer_num, frequency)
 
@@ -156,13 +157,31 @@ extern "C" {
 #define HAL_timer_get_compare(timer) _CAT(TIMER_OCR_, timer)
 #define HAL_timer_get_count(timer) _CAT(TIMER_COUNTER_, timer)
 
-#define HAL_timer_isr_prologue(timer_num)
+/**
+ * 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.
+ */
+#define HAL_timer_isr_prologue_0 do{ DISABLE_TEMPERATURE_INTERRUPT(); sei(); }while(0)
+#define HAL_timer_isr_epilogue_0 do{ cli(); ENABLE_TEMPERATURE_INTERRUPT(); }while(0)
+
+#define HAL_timer_isr_prologue_1 \
+  const bool temp_isr_was_enabled = TEMPERATURE_ISR_ENABLED(); \
+  do{ \
+    DISABLE_TEMPERATURE_INTERRUPT(); \
+    DISABLE_STEPPER_DRIVER_INTERRUPT(); \
+    sei(); \
+  }while(0)
+
+#define HAL_timer_isr_epilogue_1 do{ cli(); ENABLE_STEPPER_DRIVER_INTERRUPT(); if (temp_isr_was_enabled) ENABLE_TEMPERATURE_INTERRUPT(); }while(0)
+
+#define HAL_timer_isr_prologue(TIMER_NUM) _CAT(HAL_timer_isr_prologue_, TIMER_NUM)
+#define HAL_timer_isr_epilogue(TIMER_NUM) _CAT(HAL_timer_isr_epilogue_, TIMER_NUM)
 
 #define HAL_STEP_TIMER_ISR ISR(TIMER1_COMPA_vect)
 #define HAL_TEMP_TIMER_ISR ISR(TIMER0_COMPB_vect)
 
-#define HAL_ENABLE_ISRs() do { cli(); if (thermalManager.in_temp_isr) DISABLE_TEMPERATURE_INTERRUPT(); else ENABLE_TEMPERATURE_INTERRUPT(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
-
 // ADC
 #ifdef DIDR2
   #define HAL_ANALOG_SELECT(pin) do{ if (pin < 8) SBI(DIDR0, pin); else SBI(DIDR2, pin & 0x07); }while(0)

Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp

@@ -61,13 +61,13 @@
 // --------------------------------------------------------------------------
 
 const tTimerConfig TimerConfig [NUM_HARDWARE_TIMERS] = {
-  { TC0, 0, TC0_IRQn,  0}, // 0 - [servo timer5]
+  { TC0, 0, TC0_IRQn,  3}, // 0 - [servo timer5]
   { TC0, 1, TC1_IRQn,  0}, // 1
   { TC0, 2, TC2_IRQn,  0}, // 2
   { TC1, 0, TC3_IRQn,  2}, // 3 - stepper
   { TC1, 1, TC4_IRQn, 15}, // 4 - temperature
-  { TC1, 2, TC5_IRQn,  0}, // 5 - [servo timer3]
+  { TC1, 2, TC5_IRQn,  3}, // 5 - [servo timer3]
-  { TC2, 0, TC6_IRQn, 15}, // 6 - tone
+  { TC2, 0, TC6_IRQn, 14}, // 6 - tone
   { TC2, 1, TC7_IRQn,  0}, // 7
   { TC2, 2, TC8_IRQn,  0}, // 8
 };

Marlin/src/HAL/HAL_DUE/HAL_timers_Due.h

@@ -62,8 +62,6 @@ typedef uint32_t hal_timer_t;
 #define ENABLE_TEMPERATURE_INTERRUPT()  HAL_timer_enable_interrupt(TEMP_TIMER_NUM)
 #define DISABLE_TEMPERATURE_INTERRUPT() HAL_timer_disable_interrupt(TEMP_TIMER_NUM)
 
-#define HAL_ENABLE_ISRs() do { if (thermalManager.in_temp_isr) DISABLE_TEMPERATURE_INTERRUPT(); else ENABLE_TEMPERATURE_INTERRUPT(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
-
 #define HAL_STEP_TIMER_ISR  void TC3_Handler()
 #define HAL_TEMP_TIMER_ISR  void TC4_Handler()
 #define HAL_TONE_TIMER_ISR  void TC6_Handler()
@@ -127,4 +125,6 @@ FORCE_INLINE static void HAL_timer_isr_prologue(const uint8_t timer_num) {
   pConfig->pTimerRegs->TC_CHANNEL[pConfig->channel].TC_SR;
 }
 
+#define HAL_timer_isr_epilogue(TIMER_NUM)
+
 #endif // _HAL_TIMERS_DUE_H

Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp

@@ -353,6 +353,11 @@
     // Install interrupt handler
     install_isr(HWUART_IRQ, UART_ISR);
 
+    // Configure priority. We need a very high priority to avoid losing characters
+    // and we need to be able to preempt the Stepper ISR and everything else!
+    // (this could probably be fixed by using DMA with the Serial port)
+    NVIC_SetPriority(HWUART_IRQ, 1);
+
     // Enable UART interrupt in NVIC
     NVIC_EnableIRQ(HWUART_IRQ);
 

Marlin/src/HAL/HAL_LPC1768/HAL_timers.cpp

@@ -90,5 +90,4 @@ void HAL_timer_isr_prologue(const uint8_t timer_num) {
   }
 }
 
-
 #endif // TARGET_LPC1768

Marlin/src/HAL/HAL_LPC1768/HAL_timers.h

@@ -65,8 +65,6 @@ typedef uint32_t hal_timer_t;
 #define ENABLE_TEMPERATURE_INTERRUPT() HAL_timer_enable_interrupt(TEMP_TIMER_NUM)
 #define DISABLE_TEMPERATURE_INTERRUPT() HAL_timer_disable_interrupt(TEMP_TIMER_NUM)
 
-#define HAL_ENABLE_ISRs() do { if (thermalManager.in_temp_isr) DISABLE_TEMPERATURE_INTERRUPT(); else ENABLE_TEMPERATURE_INTERRUPT(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
-
 #define HAL_STEP_TIMER_ISR  extern "C" void TIMER0_IRQHandler(void)
 #define HAL_TEMP_TIMER_ISR  extern "C" void TIMER1_IRQHandler(void)
 
@@ -129,5 +127,6 @@ void HAL_timer_enable_interrupt(const uint8_t timer_num);
 void HAL_timer_disable_interrupt(const uint8_t timer_num);
 bool HAL_timer_interrupt_enabled(const uint8_t timer_num);
 void HAL_timer_isr_prologue(const uint8_t timer_num);
+#define HAL_timer_isr_epilogue(TIMER_NUM)
 
 #endif // _HAL_TIMERS_DUE_H

Marlin/src/HAL/HAL_STM32F1/HAL_timers_Stm32f1.h

@@ -88,7 +88,6 @@ timer_dev* get_timer_dev(int number);
 
 #define HAL_timer_get_count(timer_num) timer_get_count(TIMER_DEV(timer_num))
 
-#define HAL_ENABLE_ISRs() do { if (thermalManager.in_temp_isr)DISABLE_TEMPERATURE_INTERRUPT(); else ENABLE_TEMPERATURE_INTERRUPT(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
 // TODO change this
 
 
@@ -175,4 +174,6 @@ FORCE_INLINE static void HAL_timer_isr_prologue(const uint8_t timer_num) {
   }
 }
 
+#define HAL_timer_isr_epilogue(TIMER_NUM)
+
 #endif // _HAL_TIMERS_STM32F1_H

Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.h

@@ -61,7 +61,6 @@
 #define ENABLE_TEMPERATURE_INTERRUPT() HAL_timer_enable_interrupt(TEMP_TIMER_NUM)
 #define DISABLE_TEMPERATURE_INTERRUPT() HAL_timer_disable_interrupt(TEMP_TIMER_NUM)
 
-#define HAL_ENABLE_ISRs() do { if (thermalManager.in_temp_isr)DISABLE_TEMPERATURE_INTERRUPT(); else ENABLE_TEMPERATURE_INTERRUPT(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
 // TODO change this
 
 
@@ -101,5 +100,6 @@ uint32_t HAL_timer_get_count(const uint8_t timer_num);
 void HAL_timer_restrain(const uint8_t timer_num, const uint16_t interval_ticks);
 
 void HAL_timer_isr_prologue(const uint8_t timer_num);
+#define HAL_timer_isr_epilogue(TIMER_NUM)
 
 #endif // _HAL_TIMERS_STM32F4_H

Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.h

@@ -60,7 +60,6 @@
 #define ENABLE_TEMPERATURE_INTERRUPT() HAL_timer_enable_interrupt(TEMP_TIMER_NUM)
 #define DISABLE_TEMPERATURE_INTERRUPT() HAL_timer_disable_interrupt(TEMP_TIMER_NUM)
 
-#define HAL_ENABLE_ISRs() do { if (thermalManager.in_temp_isr)DISABLE_TEMPERATURE_INTERRUPT(); else ENABLE_TEMPERATURE_INTERRUPT(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
 // TODO change this
 
 
@@ -99,5 +98,6 @@ uint32_t HAL_timer_get_count(const uint8_t timer_num);
 void HAL_timer_restrain(const uint8_t timer_num, const uint16_t interval_ticks);
 
 void HAL_timer_isr_prologue(const uint8_t timer_num);
+#define HAL_timer_isr_epilogue(TIMER_NUM)
 
 #endif // _HAL_TIMERS_STM32F7_H

Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.h

@@ -78,8 +78,6 @@ typedef uint32_t hal_timer_t;
 #define HAL_STEP_TIMER_ISR  extern "C" void ftm0_isr(void) //void TC3_Handler()
 #define HAL_TEMP_TIMER_ISR  extern "C" void ftm1_isr(void) //void TC4_Handler()
 
-#define HAL_ENABLE_ISRs() do { if (thermalManager.in_temp_isr) DISABLE_TEMPERATURE_INTERRUPT(); else ENABLE_TEMPERATURE_INTERRUPT(); ENABLE_STEPPER_DRIVER_INTERRUPT(); } while(0)
-
 void HAL_timer_start(const uint8_t timer_num, const uint32_t frequency);
 
 FORCE_INLINE static void HAL_timer_set_compare(const uint8_t timer_num, const hal_timer_t compare) {
@@ -115,5 +113,6 @@ void HAL_timer_disable_interrupt(const uint8_t timer_num);
 bool HAL_timer_interrupt_enabled(const uint8_t timer_num);
 
 void HAL_timer_isr_prologue(const uint8_t timer_num);
+#define HAL_timer_isr_epilogue(TIMER_NUM)
 
 #endif // _HAL_TIMERS_TEENSY_H

Marlin/src/module/stepper.cpp

@@ -1144,11 +1144,14 @@ void Stepper::set_directions() {
 
 HAL_STEP_TIMER_ISR {
   HAL_timer_isr_prologue(STEP_TIMER_NUM);
+
   #if ENABLED(LIN_ADVANCE)
     Stepper::advance_isr_scheduler();
   #else
     Stepper::isr();
   #endif
+
+  HAL_timer_isr_epilogue(STEP_TIMER_NUM);
 }
 
 void Stepper::isr() {
@@ -1156,15 +1159,6 @@ void Stepper::isr() {
   #define ENDSTOP_NOMINAL_OCR_VAL 1500 * HAL_TICKS_PER_US // Check endstops every 1.5ms to guarantee two stepper ISRs within 5ms for BLTouch
   #define OCR_VAL_TOLERANCE        500 * HAL_TICKS_PER_US // First max delay is 2.0ms, last min delay is 0.5ms, all others 1.5ms
 
-  #if DISABLED(LIN_ADVANCE)
-    // Disable Timer0 ISRs and enable global ISR again to capture UART events (incoming chars)
-    DISABLE_TEMPERATURE_INTERRUPT(); // Temperature ISR
-    DISABLE_STEPPER_DRIVER_INTERRUPT();
-    #ifndef CPU_32_BIT
-      sei();
-    #endif
-  #endif
-
   hal_timer_t ocr_val;
   static uint32_t step_remaining = 0;  // SPLIT function always runs.  This allows 16 bit timers to be
                                        // used to generate the stepper ISR.
@@ -1191,7 +1185,6 @@ void Stepper::isr() {
 
     #if DISABLED(LIN_ADVANCE)
       HAL_timer_restrain(STEP_TIMER_NUM, STEP_TIMER_MIN_INTERVAL * HAL_TICKS_PER_US);
-      HAL_ENABLE_ISRs();
     #endif
 
     return;
@@ -1215,7 +1208,6 @@ void Stepper::isr() {
     }
     current_block = NULL;                       // Prep to get a new block after cleaning
     _NEXT_ISR(HAL_STEPPER_TIMER_RATE / 10000);  // Run at max speed - 10 KHz
-    HAL_ENABLE_ISRs();
     return;
   }
 
@@ -1291,7 +1283,6 @@ void Stepper::isr() {
         if (current_block->steps[Z_AXIS] > 0) {
           enable_Z();
           _NEXT_ISR(HAL_STEPPER_TIMER_RATE / 1000); // Run at slow speed - 1 KHz
-          HAL_ENABLE_ISRs();
           return;
         }
       #endif
@@ -1299,7 +1290,6 @@ void Stepper::isr() {
     else {
       // If no more queued moves, postpone next check for 1mS
       _NEXT_ISR(HAL_STEPPER_TIMER_RATE / 1000); // Run at slow speed - 1 KHz
-      HAL_ENABLE_ISRs();
       return;
     }
   }
@@ -1631,9 +1621,6 @@ void Stepper::isr() {
     current_block = NULL;
     planner.discard_current_block();
   }
-  #if DISABLED(LIN_ADVANCE)
-    HAL_ENABLE_ISRs();
-  #endif
 }
 
 #if ENABLED(LIN_ADVANCE)
@@ -1755,10 +1742,6 @@ void Stepper::isr() {
   }
 
   void Stepper::advance_isr_scheduler() {
-    // Disable Timer0 ISRs and enable global ISR again to capture UART events (incoming chars)
-    DISABLE_TEMPERATURE_INTERRUPT(); // Temperature ISR
-    DISABLE_STEPPER_DRIVER_INTERRUPT();
-    sei();
 
     // Run main stepping ISR if flagged
     if (!nextMainISR) isr();
@@ -1787,9 +1770,6 @@ void Stepper::isr() {
 
     // Make sure stepper ISR doesn't monopolize the CPU
     HAL_timer_restrain(STEP_TIMER_NUM, STEP_TIMER_MIN_INTERVAL * HAL_TICKS_PER_US);
-
-    // Restore original ISR settings
-    HAL_ENABLE_ISRs();
   }
 
 #endif // LIN_ADVANCE

Marlin/src/module/temperature.cpp

@@ -1219,11 +1219,10 @@ void Temperature::init() {
     // Use timer0 for temperature measurement
     // Interleave temperature interrupt with millies interrupt
     OCR0B = 128;
-    SBI(TIMSK0, OCIE0B);
   #else
     HAL_timer_start(TEMP_TIMER_NUM, TEMP_TIMER_FREQUENCY);
-    HAL_timer_enable_interrupt(TEMP_TIMER_NUM);
   #endif
+  ENABLE_TEMPERATURE_INTERRUPT();
 
   #if HAS_AUTO_FAN_0
     #if E0_AUTO_FAN_PIN == FAN1_PIN
@@ -1716,22 +1715,13 @@ void Temperature::set_current_temp_raw() {
  */
 HAL_TEMP_TIMER_ISR {
   HAL_timer_isr_prologue(TEMP_TIMER_NUM);
+
   Temperature::isr();
-}
 
-volatile bool Temperature::in_temp_isr = false;
+  HAL_timer_isr_epilogue(TEMP_TIMER_NUM);
+}
 
 void Temperature::isr() {
-  // The stepper ISR can interrupt this ISR. When it does it re-enables this ISR
-  // at the end of its run, potentially causing re-entry. This flag prevents it.
-  if (in_temp_isr) return;
-  in_temp_isr = true;
-
-  // Allow UART and stepper ISRs
-  DISABLE_TEMPERATURE_INTERRUPT(); //Disable Temperature ISR
-  #ifndef CPU_32_BIT
-    sei();
-  #endif
 
   static int8_t temp_count = -1;
   static ADCSensorState adc_sensor_state = StartupDelay;
@@ -2255,12 +2245,6 @@ void Temperature::isr() {
       e_hit--;
     }
   #endif
-
-  #ifndef CPU_32_BIT
-    cli();
-  #endif
-  in_temp_isr = false;
-  ENABLE_TEMPERATURE_INTERRUPT(); //re-enable Temperature ISR
 }
 
 #if HAS_TEMP_SENSOR