From 12bc63913cb5c05adffd5ea22fcb8837d2cd34fb Mon Sep 17 00:00:00 2001 From: Jason Smith Date: Sun, 19 Jul 2020 15:41:36 -0700 Subject: [PATCH] Fix BLTouch PWM reliability in HAL/STM32 (#18702) --- Marlin/src/HAL/STM32/HAL.cpp | 2 +- Marlin/src/HAL/STM32/Servo.cpp | 24 ++++++++++++++++-- Marlin/src/HAL/STM32/Servo.h | 1 + Marlin/src/HAL/STM32/timers.cpp | 44 ++++++++++++++++++++++----------- Marlin/src/HAL/STM32/timers.h | 3 ++- 5 files changed, 56 insertions(+), 18 deletions(-) diff --git a/Marlin/src/HAL/STM32/HAL.cpp b/Marlin/src/HAL/STM32/HAL.cpp index c09592a564..1430182fc3 100644 --- a/Marlin/src/HAL/STM32/HAL.cpp +++ b/Marlin/src/HAL/STM32/HAL.cpp @@ -79,7 +79,7 @@ void HAL_init() { while (!LL_PWR_IsActiveFlag_BRR()); // Wait until backup regulator is initialized #endif - SetSoftwareSerialTimerInterruptPriority(); + SetTimerInterruptPriorities(); TERN_(EMERGENCY_PARSER, USB_Hook_init()); } diff --git a/Marlin/src/HAL/STM32/Servo.cpp b/Marlin/src/HAL/STM32/Servo.cpp index 5fb8e3cd6a..0a79d474d4 100644 --- a/Marlin/src/HAL/STM32/Servo.cpp +++ b/Marlin/src/HAL/STM32/Servo.cpp @@ -33,6 +33,18 @@ static libServo *servos[NUM_SERVOS] = {0}; constexpr millis_t servoDelay[] = SERVO_DELAY; static_assert(COUNT(servoDelay) == NUM_SERVOS, "SERVO_DELAY must be an array NUM_SERVOS long."); +// Initialize to the default timer priority. This will be overridden by a call from timers.cpp. +// This allows all timer interrupt priorities to be managed from a single location in the HAL. +static uint32_t servo_interrupt_priority = NVIC_EncodePriority(NVIC_GetPriorityGrouping(), TIM_IRQ_PRIO, TIM_IRQ_SUBPRIO); + +// This must be called after the STM32 Servo class has intialized the timer. +// It may only be needed after the first call to attach(), but it is possible +// that is is necessary after every detach() call. To be safe this is currently +// called after every call to attach(). +static void fixServoTimerInterruptPriority() { + NVIC_SetPriority(getTimerUpIrq(TIMER_SERVO), servo_interrupt_priority); +} + libServo::libServo() : delay(servoDelay[servoCount]), was_attached_before_pause(false), @@ -44,13 +56,17 @@ libServo::libServo() int8_t libServo::attach(const int pin) { if (servoCount >= MAX_SERVOS) return -1; if (pin > 0) servo_pin = pin; - return stm32_servo.attach(servo_pin); + auto result = stm32_servo.attach(servo_pin); + fixServoTimerInterruptPriority(); + return result; } int8_t libServo::attach(const int pin, const int min, const int max) { if (servoCount >= MAX_SERVOS) return -1; if (pin > 0) servo_pin = pin; - return stm32_servo.attach(servo_pin, min, max); + auto result = stm32_servo.attach(servo_pin, min, max); + fixServoTimerInterruptPriority(); + return result; } void libServo::move(const int value) { @@ -86,5 +102,9 @@ void libServo::resume_all_servos() { if (servo) servo->resume(); } +void libServo::setInterruptPriority(uint32_t preemptPriority, uint32_t subPriority) { + servo_interrupt_priority = NVIC_EncodePriority(NVIC_GetPriorityGrouping(), preemptPriority, subPriority); +} + #endif // HAS_SERVOS #endif // ARDUINO_ARCH_STM32 && !STM32GENERIC diff --git a/Marlin/src/HAL/STM32/Servo.h b/Marlin/src/HAL/STM32/Servo.h index 50ae1a9b94..534cb97e39 100644 --- a/Marlin/src/HAL/STM32/Servo.h +++ b/Marlin/src/HAL/STM32/Servo.h @@ -41,6 +41,7 @@ class libServo { static void pause_all_servos(); static void resume_all_servos(); + static void setInterruptPriority(uint32_t preemptPriority, uint32_t subPriority); private: Servo stm32_servo; diff --git a/Marlin/src/HAL/STM32/timers.cpp b/Marlin/src/HAL/STM32/timers.cpp index 5383c82212..e98a6a5560 100644 --- a/Marlin/src/HAL/STM32/timers.cpp +++ b/Marlin/src/HAL/STM32/timers.cpp @@ -29,26 +29,41 @@ #define NUM_HARDWARE_TIMERS 2 +// Default timer priorities. Override by specifying alternate priorities in the board pins file. +// The TONE timer is not present here, as it currently cannot be set programmatically. It is set +// by defining TIM_IRQ_PRIO in the variant.h or platformio.ini file, which adjusts the default +// priority for STM32 HardwareTimer objects. +#define SWSERIAL_TIMER_IRQ_PRIO_DEFAULT 1 // Requires tight bit timing to communicate reliably with TMC drivers +#define SERVO_TIMER_IRQ_PRIO_DEFAULT 1 // Requires tight PWM timing to control a BLTouch reliably +#define STEP_TIMER_IRQ_PRIO_DEFAULT 2 +#define TEMP_TIMER_IRQ_PRIO_DEFAULT 14 // Low priority avoids interference with other hardware and timers + #ifndef STEP_TIMER_IRQ_PRIO - #define STEP_TIMER_IRQ_PRIO 2 + #define STEP_TIMER_IRQ_PRIO STEP_TIMER_IRQ_PRIO_DEFAULT #endif #ifndef TEMP_TIMER_IRQ_PRIO - #define TEMP_TIMER_IRQ_PRIO 14 // 14 = after hardware ISRs + #define TEMP_TIMER_IRQ_PRIO TEMP_TIMER_IRQ_PRIO_DEFAULT #endif - -// Ensure the default timer priority is somewhere between the STEP and TEMP priorities. -// The STM32 framework defaults to interrupt 14 for all timers. This should be increased so that -// timing-sensitive operations such as speaker output are note impacted by the long-running -// temperature ISR. This must be defined in the platformio.ini file or the board's variant.h, -// so that it will be consumed by framework code. -#if !(TIM_IRQ_PRIO > STEP_TIMER_IRQ_PRIO && TIM_IRQ_PRIO < TEMP_TIMER_IRQ_PRIO) - #error "Default timer interrupt priority is unspecified or set to a value which may degrade performance." -#endif - #if HAS_TMC_SW_SERIAL #include #ifndef SWSERIAL_TIMER_IRQ_PRIO - #define SWSERIAL_TIMER_IRQ_PRIO 1 + #define SWSERIAL_TIMER_IRQ_PRIO SWSERIAL_TIMER_IRQ_PRIO_DEFAULT + #endif +#endif +#if HAS_SERVOS + #include "Servo.h" + #ifndef SERVO_TIMER_IRQ_PRIO + #define SERVO_TIMER_IRQ_PRIO SERVO_TIMER_IRQ_PRIO_DEFAULT + #endif +#endif +#if ENABLED(SPEAKER) + // Ensure the default timer priority is somewhere between the STEP and TEMP priorities. + // The STM32 framework defaults to interrupt 14 for all timers. This should be increased so that + // timing-sensitive operations such as speaker output are not impacted by the long-running + // temperature ISR. This must be defined in the platformio.ini file or the board's variant.h, + // so that it will be consumed by framework code. + #if !(TIM_IRQ_PRIO > STEP_TIMER_IRQ_PRIO && TIM_IRQ_PRIO < TEMP_TIMER_IRQ_PRIO) + #error "Default timer interrupt priority is unspecified or set to a value which may degrade performance." #endif #endif @@ -189,8 +204,9 @@ TIM_TypeDef * HAL_timer_device(const uint8_t timer_num) { return nullptr; } -void SetSoftwareSerialTimerInterruptPriority() { +void SetTimerInterruptPriorities() { TERN_(HAS_TMC_SW_SERIAL, SoftwareSerial::setInterruptPriority(SWSERIAL_TIMER_IRQ_PRIO, 0)); + TERN_(HAS_SERVOS, libServo::setInterruptPriority(SERVO_TIMER_IRQ_PRIO, 0)); } #endif // ARDUINO_ARCH_STM32 && !STM32GENERIC diff --git a/Marlin/src/HAL/STM32/timers.h b/Marlin/src/HAL/STM32/timers.h index d6b333ef9c..23a379973c 100644 --- a/Marlin/src/HAL/STM32/timers.h +++ b/Marlin/src/HAL/STM32/timers.h @@ -86,8 +86,9 @@ 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); +// Configure timer priorities for peripherals such as Software Serial or Servos. // Exposed here to allow all timer priority information to reside in timers.cpp -void SetSoftwareSerialTimerInterruptPriority(); +void SetTimerInterruptPriorities(); //TIM_TypeDef* HAL_timer_device(const uint8_t timer_num); no need to be public for now. not public = not used externally