From 51004e003db21cab6c9d3c7dc943804c3cc6309d Mon Sep 17 00:00:00 2001 From: Scott Lahteine Date: Mon, 23 Apr 2018 20:47:31 -0500 Subject: [PATCH] Proper AVR preemptive interrupt handling (#10501) Co-Authored-By: ejtagle --- Marlin/planner.cpp | 2 +- Marlin/stepper.cpp | 40 ++++++++++++++++++++++------------------ Marlin/stepper.h | 1 + Marlin/temperature.cpp | 36 +++++++++++++++++++----------------- Marlin/temperature.h | 6 ++++-- 5 files changed, 47 insertions(+), 38 deletions(-) diff --git a/Marlin/planner.cpp b/Marlin/planner.cpp index 93dbb18f92..6e2ecb04e5 100644 --- a/Marlin/planner.cpp +++ b/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. diff --git a/Marlin/stepper.cpp b/Marlin/stepper.cpp index 809cc54a9d..4bd4ade692 100644 --- a/Marlin/stepper.cpp +++ b/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 diff --git a/Marlin/stepper.h b/Marlin/stepper.h index a0bb030234..f0b95ac06c 100644 --- a/Marlin/stepper.h +++ b/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: diff --git a/Marlin/temperature.cpp b/Marlin/temperature.cpp index f5132ce967..c22811048b 100644 --- a/Marlin/temperature.cpp +++ b/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(); } - -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 - CBI(TIMSK0, OCIE0B); //Disable Temperature ISR +ISR(TIMER0_COMPB_vect) { + /** + * AVR has no hardware interrupt preemption, so emulate priorization + * and preemption of this ISR by all others by disabling the timer + * interrupt generation capability and reenabling global interrupts. + * Any interrupt can then interrupt this handler and preempt it. + * This ISR becomes the lowest priority one so the UART, Endstops + * and Stepper ISRs can all preempt it. + */ + DISABLE_TEMPERATURE_INTERRUPT(); 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 diff --git a/Marlin/temperature.h b/Marlin/temperature.h index b71bd1ec6b..b01cddc0fd 100644 --- a/Marlin/temperature.h +++ b/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];