From 0c428a66d92ab3db49c76cfedde5b239db16195d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Eduardo=20Jos=C3=A9=20Tagle?= Date: Tue, 24 Apr 2018 00:05:07 -0300 Subject: [PATCH] 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 | 31 +++++++++++++++---- Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp | 6 ++-- Marlin/src/HAL/HAL_DUE/HAL_timers_Due.h | 4 +-- Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp | 5 +++ Marlin/src/HAL/HAL_LPC1768/HAL_timers.cpp | 1 - Marlin/src/HAL/HAL_LPC1768/HAL_timers.h | 3 +- .../src/HAL/HAL_STM32F1/HAL_timers_Stm32f1.h | 3 +- .../src/HAL/HAL_STM32F4/HAL_timers_STM32F4.h | 2 +- .../src/HAL/HAL_STM32F7/HAL_timers_STM32F7.h | 2 +- .../HAL/HAL_TEENSY35_36/HAL_timers_Teensy.h | 3 +- Marlin/src/module/stepper.cpp | 26 ++-------------- Marlin/src/module/temperature.cpp | 24 +++----------- 12 files changed, 48 insertions(+), 62 deletions(-) diff --git a/Marlin/src/HAL/HAL_AVR/HAL.h b/Marlin/src/HAL/HAL_AVR/HAL.h index 767bc1be39..9905265618 100644 --- a/Marlin/src/HAL/HAL_AVR/HAL.h +++ b/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 DISABLE_TEMPERATURE_INTERRUPT() CBI(TIMSK0, OCIE0B) +#define ENABLE_TEMPERATURE_INTERRUPT() SBI(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) diff --git a/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp b/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp index c5121f62e8..b522fbeb65 100644 --- a/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp +++ b/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] - { TC2, 0, TC6_IRQn, 15}, // 6 - tone + { TC1, 2, TC5_IRQn, 3}, // 5 - [servo timer3] + { TC2, 0, TC6_IRQn, 14}, // 6 - tone { TC2, 1, TC7_IRQn, 0}, // 7 { TC2, 2, TC8_IRQn, 0}, // 8 }; diff --git a/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.h b/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.h index 2fbe4480fe..3698cad68c 100644 --- a/Marlin/src/HAL/HAL_DUE/HAL_timers_Due.h +++ b/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 diff --git a/Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp b/Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp index 4d7aa945ef..0790c43464 100644 --- a/Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp +++ b/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); diff --git a/Marlin/src/HAL/HAL_LPC1768/HAL_timers.cpp b/Marlin/src/HAL/HAL_LPC1768/HAL_timers.cpp index 3f7507aa5b..25f1381f5c 100644 --- a/Marlin/src/HAL/HAL_LPC1768/HAL_timers.cpp +++ b/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 diff --git a/Marlin/src/HAL/HAL_LPC1768/HAL_timers.h b/Marlin/src/HAL/HAL_LPC1768/HAL_timers.h index cfbf593f51..66a4350ad4 100644 --- a/Marlin/src/HAL/HAL_LPC1768/HAL_timers.h +++ b/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 diff --git a/Marlin/src/HAL/HAL_STM32F1/HAL_timers_Stm32f1.h b/Marlin/src/HAL/HAL_STM32F1/HAL_timers_Stm32f1.h index effd528695..b97c7667e1 100644 --- a/Marlin/src/HAL/HAL_STM32F1/HAL_timers_Stm32f1.h +++ b/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 diff --git a/Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.h b/Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.h index 7c748cc33e..5fe24bbd5e 100644 --- a/Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.h +++ b/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 diff --git a/Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.h b/Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.h index 7990614b02..892ec08666 100644 --- a/Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.h +++ b/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 diff --git a/Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.h b/Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.h index 102e840b17..644ddd8f02 100644 --- a/Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.h +++ b/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 diff --git a/Marlin/src/module/stepper.cpp b/Marlin/src/module/stepper.cpp index f6f28fe4bd..04c6c270f5 100644 --- a/Marlin/src/module/stepper.cpp +++ b/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 diff --git a/Marlin/src/module/temperature.cpp b/Marlin/src/module/temperature.cpp index f383c3406e..1b7ee86a57 100644 --- a/Marlin/src/module/temperature.cpp +++ b/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(); + + HAL_timer_isr_epilogue(TEMP_TIMER_NUM); } -volatile bool Temperature::in_temp_isr = false; - 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