Skip to content

[2.0.x] AVR preemptive interrupts, ARM interrupt priorities #10496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
thinkyhead merged 1 commit into from
Apr 24, 2018
Merged

[2.0.x] AVR preemptive interrupts, ARM interrupt priorities #10496

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 25 additions & 6 deletions Marlin/src/HAL/HAL_AVR/HAL.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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(); \
Copy link
Copy Markdown
Member

@thinkyhead thinkyhead Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@p3p, @ejtagle — I have a hard time imagining any case where this would be false right at the start of processing the ISR. Could it be that this flag is unnecessary?

Copy link
Copy Markdown
Contributor Author

@ejtagle ejtagle Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a reason for that flag:
Imagine the following scenario:
The Temperature ISR was executing and the Stepper interrupts it: At that point, the temperature ISR is disabled (that is the way we prevent inmediate reentry as soon as we reenable global interrupts). When the Stepper ISR ends execution, the Temperature ISR must be kept disabled. Otherwise, as soon as the Stepper ISR reenables it, the Temperature ISR will be recalled recursively, without waiting for the previous Temperature ISR (that was interrupted by the Stepper ISR) to end.
We end with a recursive call, a stack overflow and a system crash.
So, yes, the exact sequence is extremely important here

Copy link
Copy Markdown
Contributor Author

@ejtagle ejtagle Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the flag. Admittedly, if using C++, we could use a simple object to solve this problem. In the constructor we do the prologue, and in the destructor we do the epilogue
Also, the prologue and epilogue are not exactly the same for the Stepper and for the Temperature timers.
On the Stepper we disable both, temperature and Stepper ISRs on entry (to emulate priorization) and then reenable them (but temperature ISR is only enabled if it was enabled previously)
On the Temperature isr, we just disable temperature ISR, as said before, to prevent inmediate reentry as soon as we enable the global interrupt flags.
That avoids recursive calls of the ISRs.

Copy link
Copy Markdown
Contributor Author

@ejtagle ejtagle Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to hide it inside those macros, you don´t even need to use epilogue_0/epilogue_1 ... You can use several ifs or even a switch statement. GCC constant propagation will handle and inline only the appropiate to the timer being used

Copy link
Copy Markdown
Member

@thinkyhead thinkyhead Apr 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to prefer the macros because it ensures the bool flag only exists where it's needed. Other approaches either create an extra copy where it's not needed or force it to be defined elsewhere as a static global (e.g., Temperature::temp_isr_flag).

Copy link
Copy Markdown
Contributor Author

@ejtagle ejtagle Apr 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no problems with the macros at all ;)
It would be nice to do a custom ISR wrapper in assembly for AVR, so we can reduce interrupt processing latency as much as possible...

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)
Expand Down
6 changes: 3 additions & 3 deletions Marlin/src/HAL/HAL_DUE/HAL_timers_Due.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
};
Expand Down
4 changes: 2 additions & 2 deletions Marlin/src/HAL/HAL_DUE/HAL_timers_Due.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down Expand Up @@ -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
5 changes: 5 additions & 0 deletions Marlin/src/HAL/HAL_DUE/MarlinSerial_Due.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
1 change: 0 additions & 1 deletion Marlin/src/HAL/HAL_LPC1768/HAL_timers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,5 +90,4 @@ void HAL_timer_isr_prologue(const uint8_t timer_num) {
}
}


#endif // TARGET_LPC1768
3 changes: 1 addition & 2 deletions Marlin/src/HAL/HAL_LPC1768/HAL_timers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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
3 changes: 2 additions & 1 deletion Marlin/src/HAL/HAL_STM32F1/HAL_timers_Stm32f1.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion Marlin/src/HAL/HAL_STM32F4/HAL_timers_STM32F4.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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
2 changes: 1 addition & 1 deletion Marlin/src/HAL/HAL_STM32F7/HAL_timers_STM32F7.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand Down Expand Up @@ -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
3 changes: 1 addition & 2 deletions Marlin/src/HAL/HAL_TEENSY35_36/HAL_timers_Teensy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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
26 changes: 3 additions & 23 deletions Marlin/src/module/stepper.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1144,27 +1144,21 @@ 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() {

#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.
Expand All @@ -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;
Expand All @@ -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;
}

Expand Down Expand Up @@ -1291,15 +1283,13 @@ 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
}
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;
}
}
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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
Expand Down
24 changes: 4 additions & 20 deletions Marlin/src/module/temperature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down