Merge pull request #7212 from dhalbert/stm-pwm-fix

STM: off-by-one TIMx reference; other code cleanup and minor fixes

Author: tannewt

ports/stm/common-hal/pulseio/PulseIn.c

@@ -106,7 +106,8 @@ void pulsein_reset(void) {
     memset(callback_obj_ref, 0, sizeof(callback_obj_ref));
 
     HAL_TIM_Base_DeInit(&tim_handle);
-    tim_clock_disable(stm_peripherals_timer_get_index(tim_handle.Instance));
+    // tim_clock_disable() takes a bitmask of timers.
+    tim_clock_disable(1 << stm_peripherals_timer_get_index(tim_handle.Instance));
     memset(&tim_handle, 0, sizeof(tim_handle));
     refcount = 0;
 }

ports/stm/common-hal/pwmio/PWMOut.c

@@ -36,23 +36,24 @@
 
 #include "timers.h"
 
-#define ALL_CLOCKS 0xFFFF
-
-STATIC uint8_t reserved_tim[TIM_BANK_ARRAY_LEN];
+// Bitmask of channels taken.
+STATIC uint8_t tim_channels_taken[TIM_BANK_ARRAY_LEN];
+// Initial frequency timer is set to.
 STATIC uint32_t tim_frequencies[TIM_BANK_ARRAY_LEN];
 STATIC bool never_reset_tim[TIM_BANK_ARRAY_LEN];
 
 STATIC uint32_t timer_get_internal_duty(uint16_t duty, uint32_t period) {
     // duty cycle is duty/0xFFFF fraction x (number of pulses per period)
-    return (duty * period) / ((1 << 16) - 1);
+    return (duty * period) / 0xffff;
 }
 
 STATIC bool timer_get_optimal_divisors(uint32_t *period, uint32_t *prescaler,
     uint32_t frequency, uint32_t source_freq) {
     // Find the largest possible period supported by this frequency
-    for (int i = 0; i < (1 << 16); i++) {
+    *prescaler = 0;
+    for (uint32_t i = 1; i <= 0xffff; i++) {
         *period = source_freq / (i * frequency);
-        if (*period < (1 << 16) && *period >= 2) {
+        if (*period <= 0xffff && *period >= 2) {
             *prescaler = i;
             break;
         }
@@ -62,13 +63,10 @@ STATIC bool timer_get_optimal_divisors(uint32_t *period, uint32_t *prescaler,
 }
 
 void pwmout_reset(void) {
-    uint16_t never_reset_mask = 0x00;
     for (int i = 0; i < TIM_BANK_ARRAY_LEN; i++) {
         if (!never_reset_tim[i]) {
-            reserved_tim[i] = 0x00;
-            tim_frequencies[i] = 0x00;
-        } else {
-            never_reset_mask |= 1 << i;
+            tim_channels_taken[i] = 0x00;
+            tim_frequencies[i] = 0;
         }
     }
 }
@@ -78,73 +76,69 @@ pwmout_result_t common_hal_pwmio_pwmout_construct(pwmio_pwmout_obj_t *self,
     uint16_t duty,
     uint32_t frequency,
     bool variable_frequency) {
-    TIM_TypeDef *TIMx;
-    uint8_t tim_num = MP_ARRAY_SIZE(mcu_tim_pin_list);
-    bool tim_taken_internal = false;
-    bool tim_chan_taken = false;
-    bool tim_taken_f_mismatch = false;
-    bool var_freq_mismatch = false;
+    // Default error is no timer at all on pin.
+    pwmout_result_t last_failure = PWMOUT_INVALID_PIN;
     bool first_time_setup = true;
 
-    for (uint i = 0; i < tim_num; i++) {
-        const mcu_tim_pin_obj_t *l_tim = &mcu_tim_pin_list[i];
-        uint8_t l_tim_index = l_tim->tim_index;
-        uint8_t l_tim_channel = l_tim->channel_index;
+    uint8_t tim_index;
+    uint8_t tim_channel_index;
+
+    self->tim = NULL;
+    for (uint i = 0; i < MP_ARRAY_SIZE(mcu_tim_pin_list); i++) {
+        const mcu_tim_pin_obj_t *tim = &mcu_tim_pin_list[i];
+        tim_index = tim->tim_index;
+        tim_channel_index = tim->channel_index;
 
         // if pin is same
-        if (l_tim->pin == pin) {
+        if (tim->pin == pin) {
             // check if the timer has a channel active, or is reserved by main timer system
-            if (l_tim_index < TIM_BANK_ARRAY_LEN && reserved_tim[l_tim_index] != 0) {
+            if (tim_index < TIM_BANK_ARRAY_LEN && tim_channels_taken[tim_index] != 0) {
                 // Timer has already been reserved by an internal module
-                if (stm_peripherals_timer_is_reserved(mcu_tim_banks[l_tim_index])) {
-                    tim_taken_internal = true;
+                if (stm_peripherals_timer_is_reserved(mcu_tim_banks[tim_index])) {
+                    last_failure = PWMOUT_ALL_TIMERS_ON_PIN_IN_USE;
                     continue; // keep looking
                 }
                 // is it the same channel? (or all channels reserved by a var-freq)
-                if (reserved_tim[l_tim_index] & 1 << (l_tim_channel)) {
-                    tim_chan_taken = true;
+                if (tim_channels_taken[tim_index] & (1 << tim_channel_index)) {
+                    last_failure = PWMOUT_ALL_TIMERS_ON_PIN_IN_USE;
                     continue; // keep looking, might be another viable option
                 }
                 // If the frequencies are the same it's ok
-                if (tim_frequencies[l_tim_index] != frequency) {
-                    tim_taken_f_mismatch = true;
+                if (tim_frequencies[tim_index] != frequency) {
+                    last_failure = PWMOUT_INVALID_FREQUENCY_ON_PIN;
                     continue; // keep looking
                 }
                 // you can't put a variable frequency on a partially reserved timer
                 if (variable_frequency) {
-                    var_freq_mismatch = true;
+                    last_failure = PWMOUT_VARIABLE_FREQUENCY_NOT_AVAILABLE;
                     continue; // keep looking
                 }
                 first_time_setup = false; // skip setting up the timer
             }
             // No problems taken, so set it up
-            self->tim = l_tim;
+            self->tim = tim;
             break;
         }
     }
 
+    TIM_TypeDef *TIMx;
+
     // handle valid/invalid timer instance
     if (self->tim != NULL) {
         // create instance
-        TIMx = mcu_tim_banks[self->tim->tim_index];
+        TIMx = mcu_tim_banks[tim_index];
         // reserve timer/channel
         if (variable_frequency) {
-            reserved_tim[self->tim->tim_index] = 0x0F;
+            // Take all the channels.
+            tim_channels_taken[tim_index] = 0x0F;
         } else {
-            reserved_tim[self->tim->tim_index] |= 1 << self->tim->channel_index;
+            tim_channels_taken[tim_index] |= 1 << tim_channel_index;
         }
-        tim_frequencies[self->tim->tim_index] = frequency;
+        tim_frequencies[tim_index] = frequency;
         stm_peripherals_timer_reserve(TIMx);
-    } else { // no match found
-        if (tim_chan_taken || tim_taken_internal) {
-            return PWMOUT_ALL_TIMERS_ON_PIN_IN_USE;
-        } else if (tim_taken_f_mismatch) {
-            return PWMOUT_INVALID_FREQUENCY_ON_PIN;
-        } else if (var_freq_mismatch) {
-            return PWMOUT_VARIABLE_FREQUENCY_NOT_AVAILABLE;
-        } else {
-            return PWMOUT_INVALID_PIN;
-        }
+    } else {
+        // no match found
+        return last_failure;
     }
 
     uint32_t prescaler = 0; // prescaler is 15 bit
@@ -163,10 +157,10 @@ pwmout_result_t common_hal_pwmio_pwmout_construct(pwmio_pwmout_obj_t *self,
     HAL_GPIO_Init(pin_port(pin->port), &GPIO_InitStruct);
     self->pin = pin;
 
-    tim_clock_enable(1 << (self->tim->tim_index));
+    tim_clock_enable(1 << tim_index);
 
-    // translate channel into handle value
-    self->channel = 4 * self->tim->channel_index;
+    // translate channel into handle value: TIM_CHANNEL_1, _2, _3, _4.
+    self->channel = 4 * tim_channel_index;
 
     // Timer init
     self->handle.Instance = TIMx;
@@ -175,6 +169,7 @@ pwmout_result_t common_hal_pwmio_pwmout_construct(pwmio_pwmout_obj_t *self,
     self->handle.Init.ClockDivision = TIM_CLOCKDIVISION_DIV1;
     self->handle.Init.CounterMode = TIM_COUNTERMODE_UP;
     self->handle.Init.RepetitionCounter = 0;
+    self->handle.Init.AutoReloadPreload = TIM_AUTORELOAD_PRELOAD_DISABLE;
 
     // only run init if this is the first instance of this timer
     if (first_time_setup) {
@@ -232,15 +227,15 @@ void common_hal_pwmio_pwmout_deinit(pwmio_pwmout_obj_t *self) {
     }
     // var freq shuts down entire timer, others just their channel
     if (self->variable_frequency) {
-        reserved_tim[self->tim->tim_index] = 0x00;
+        tim_channels_taken[self->tim->tim_index] = 0x00;
     } else {
-        reserved_tim[self->tim->tim_index] &= ~(1 << self->tim->channel_index);
+        tim_channels_taken[self->tim->tim_index] &= ~(1 << self->tim->channel_index);
         HAL_TIM_PWM_Stop(&self->handle, self->channel);
     }
     common_hal_reset_pin(self->pin);
 
     // if reserved timer has no active channels, we can disable it
-    if (reserved_tim[self->tim->tim_index] == 0) {
+    if (tim_channels_taken[self->tim->tim_index] == 0) {
         tim_frequencies[self->tim->tim_index] = 0x00;
         stm_peripherals_timer_free(self->handle.Instance);
     }

ports/stm/common-hal/pwmio/PWMOut.h

@@ -39,12 +39,12 @@ typedef struct {
     TIM_HandleTypeDef handle;
     TIM_OC_InitTypeDef chan_handle;
     const mcu_tim_pin_obj_t *tim;
-    uint8_t channel : 7;
-    bool variable_frequency : 1;
-    uint16_t duty_cycle;
     uint32_t frequency;
     uint32_t period;
     const mcu_pin_obj_t *pin;
+    uint16_t duty_cycle;
+    uint8_t channel;
+    bool variable_frequency;
 } pwmio_pwmout_obj_t;
 
 void pwmout_reset(void);

ports/stm/peripherals/timers.c

@@ -150,26 +150,65 @@ static size_t irq_map[] = {
 };
 
 // Get the frequency (in Hz) of the source clock for the given timer.
-// On STM32F405/407/415/417 there are 2 cases for how the clock freq is set.
-// If the APB prescaler is 1, then the timer clock is equal to its respective
-// APB clock.  Otherwise (APB prescaler > 1) the timer clock is twice its
-// respective APB clock.  See DM00031020 Rev 4, page 115.
+//
+// From STM ref manual: DM00031020 Rev 19, section 7.2, page 217:
+//
+// The timer clock frequencies for STM32F405xx/07xx and STM32F415xx/17xx are
+// automatically set by hardware. There are two cases:
+// 1. If the APB prescaler is 1, the timer clock frequencies are set to the same frequency as
+// that of the APB domain to which the timers are connected.
+// 2. Otherwise, they are set to twice (×2) the frequency of the APB domain to which the
+// timers are connected.
+
+// From STM ref manual: DM00031020 Rev 19, section 6.2, page 153:
+//
+// The timer clock frequencies for STM32F42xxx and STM32F43xxx are automatically set by
+// hardware. There are two cases depending on the value of TIMPRE bit in RCC_CFGR [sic - should be RCC_DKCFGR]
+// register:
+// * If TIMPRE bit in RCC_DKCFGR register is reset:
+//   If the APB prescaler is configured to a division factor of 1, the timer clock frequencies
+//   (TIMxCLK) are set to PCLKx. Otherwise, the timer clock frequencies are twice the
+//   frequency of the APB domain to which the timers are connected: TIMxCLK = 2xPCLKx.
+// * If TIMPRE bit in RCC_DKCFGR register is set:
+//   If the APB prescaler is configured to a division factor of 1, 2 or 4, the timer clock
+//   frequencies (TIMxCLK) are set to HCLK. Otherwise, the timer clock frequencies is four
+//   times the frequency of the APB domain to which the timers are connected: TIMxCLK = 4xPCLKx.
+
 uint32_t stm_peripherals_timer_get_source_freq(TIM_TypeDef *timer) {
-    size_t tim_id = stm_peripherals_timer_get_index(timer);
+    // The timer index starts at 0, but the timer numbers start at TIM1.
+    size_t tim_id = stm_peripherals_timer_get_index(timer) + 1;
     uint32_t source, clk_div;
     if (tim_id == 1 || (8 <= tim_id && tim_id <= 11)) {
         // TIM{1,8,9,10,11} are on APB2
         source = HAL_RCC_GetPCLK2Freq();
-        clk_div = RCC->CFGR & RCC_CFGR_PPRE2;
+        // 0b0xx means not divided; 0b100 is divide by 2; 0b101 by 4; 0b110 by 8; 0b111 by 16.
+        clk_div = (RCC->CFGR & RCC_CFGR_PPRE2) >> RCC_CFGR_PPRE2_Pos;
     } else {
         // TIM{2,3,4,5,6,7,12,13,14} are on APB1
         source = HAL_RCC_GetPCLK1Freq();
-        clk_div = RCC->CFGR & RCC_CFGR_PPRE1;
+        // 0b0xx means not divided; 0b100 is divide by 2; 0b101 by 4; 0b110 by 8; 0b111 by 16.
+        clk_div = (RCC->CFGR & RCC_CFGR_PPRE1) >> RCC_CFGR_PPRE1_Pos;
     }
-    if (clk_div != 0) {
-        // APB prescaler for this timer is > 1
+
+    // Only some STM32's have TIMPRE.
+    #if defined(RCC_CFGR_TIMPRE)
+    uint32_t timpre = RCC->DCKCFGR & RCC_CFGR_TIMPRE;
+    if (timpre == 0) {
+        if (clk_div >= 0b100) {
+            source *= 2;
+        }
+    } else {
+        if (clk_div > 0b101) {
+            source *= 4;
+        } else {
+            source = HAL_RCC_GetHCLKFreq();
+        }
+    }
+    #else
+    if (clk_div >= 0b100) {
         source *= 2;
     }
+    #endif
     return source;
 }
 
@@ -271,6 +310,7 @@ bool stm_peripherals_timer_is_reserved(TIM_TypeDef *instance) {
     return stm_timer_reserved[tim_idx];
 }
 
+// Note this returns a timer index starting at zero, corresponding to TIM1.
 size_t stm_peripherals_timer_get_index(TIM_TypeDef *instance) {
     for (size_t i = 0; i < MP_ARRAY_SIZE(mcu_tim_banks); i++) {
         if (instance == mcu_tim_banks[i]) {