From 796cbc28c5c1e2e7aea2a2abfd970903270c75c0 Mon Sep 17 00:00:00 2001 From: Frederic Pillon Date: Wed, 20 Nov 2024 11:42:31 +0100 Subject: [PATCH 1/4] chore(hardwaretimer): harden get API using uint32_t As all get API's use error handler if an error occur, then no need to use int as return type. Moreover, all HAL API uses uint32_t for those value. This avoid cast issue. Signed-off-by: Frederic Pillon --- libraries/SrcWrapper/inc/HardwareTimer.h | 8 +- libraries/SrcWrapper/src/HardwareTimer.cpp | 99 +++++++++++----------- 2 files changed, 54 insertions(+), 53 deletions(-) diff --git a/libraries/SrcWrapper/inc/HardwareTimer.h b/libraries/SrcWrapper/inc/HardwareTimer.h index 158e8dbd38..ad24f2d4e3 100644 --- a/libraries/SrcWrapper/inc/HardwareTimer.h +++ b/libraries/SrcWrapper/inc/HardwareTimer.h @@ -177,10 +177,10 @@ class HardwareTimer { // The following function(s) are available for more advanced timer options TIM_HandleTypeDef *getHandle(); // return the handle address for HAL related configuration - int getChannel(uint32_t channel); - int getLLChannel(uint32_t channel); - int getIT(uint32_t channel); - int getAssociatedChannel(uint32_t channel); + uint32_t getChannel(uint32_t channel); + uint32_t getLLChannel(uint32_t channel); + uint32_t getIT(uint32_t channel); + uint32_t getAssociatedChannel(uint32_t channel); private: // Store for each channel if regular, complementary or both are used diff --git a/libraries/SrcWrapper/src/HardwareTimer.cpp b/libraries/SrcWrapper/src/HardwareTimer.cpp index 7bb1da5dbf..1fedba5d0d 100644 --- a/libraries/SrcWrapper/src/HardwareTimer.cpp +++ b/libraries/SrcWrapper/src/HardwareTimer.cpp @@ -181,9 +181,9 @@ void HardwareTimer::pauseChannel(uint32_t channel) return; } - int timAssociatedInputChannel; - int LLChannel = getLLChannel(channel); - int interrupt = getIT(channel); + uint32_t timAssociatedInputChannel; + uint32_t LLChannel = getLLChannel(channel); + uint32_t interrupt = getIT(channel); // Disable channel and corresponding interrupt __HAL_TIM_DISABLE_IT(&(_timerObj.handle), interrupt); @@ -238,27 +238,27 @@ void HardwareTimer::resume(void) * @param Arduino channel [1..4] * @retval HAL channel. Error handler called if arduino channel is invalid */ -int HardwareTimer::getChannel(uint32_t channel) +uint32_t HardwareTimer::getChannel(uint32_t channel) { - int return_value = -1; + uint32_t timChannel = -1; switch (channel) { case 1: - return_value = TIM_CHANNEL_1; + timChannel = TIM_CHANNEL_1; break; case 2: - return_value = TIM_CHANNEL_2; + timChannel = TIM_CHANNEL_2; break; case 3: - return_value = TIM_CHANNEL_3; + timChannel = TIM_CHANNEL_3; break; case 4: - return_value = TIM_CHANNEL_4; + timChannel = TIM_CHANNEL_4; break; default: Error_Handler(); } - return return_value; + return timChannel; } /** @@ -266,56 +266,57 @@ int HardwareTimer::getChannel(uint32_t channel) * @param Arduino channel [1..4] * @retval LL channel. Error handler called if arduino channel is invalid */ -int HardwareTimer::getLLChannel(uint32_t channel) +uint32_t HardwareTimer::getLLChannel(uint32_t channel) { - int return_value = 0; + bool error = false; + uint32_t ll_channel = 0; #if defined(TIM_CCER_CC1NE) if (__ChannelsUsed[channel - 1] & COMPLEMENTARY_CHAN_MASK) { // Complementary channel switch (channel) { case 1: - return_value = LL_TIM_CHANNEL_CH1N; + ll_channel = LL_TIM_CHANNEL_CH1N; break; case 2: - return_value = LL_TIM_CHANNEL_CH2N; + ll_channel = LL_TIM_CHANNEL_CH2N; break; case 3: - return_value = LL_TIM_CHANNEL_CH3N; + ll_channel = LL_TIM_CHANNEL_CH3N; break; #if defined(LL_TIM_CHANNEL_CH4N) case 4: - return_value = LL_TIM_CHANNEL_CH4N; + ll_channel = LL_TIM_CHANNEL_CH4N; break; #endif default: - return_value = -1; + error = true; } } #endif - if ((return_value != -1) && (__ChannelsUsed[channel - 1] & REGULAR_CHAN_MASK)) { + if ((!error) && (__ChannelsUsed[channel - 1] & REGULAR_CHAN_MASK)) { // Regular channel not complementary switch (channel) { case 1: - return_value |= LL_TIM_CHANNEL_CH1; + ll_channel |= LL_TIM_CHANNEL_CH1; break; case 2: - return_value |= LL_TIM_CHANNEL_CH2; + ll_channel |= LL_TIM_CHANNEL_CH2; break; case 3: - return_value |= LL_TIM_CHANNEL_CH3; + ll_channel |= LL_TIM_CHANNEL_CH3; break; case 4: - return_value |= LL_TIM_CHANNEL_CH4; + ll_channel |= LL_TIM_CHANNEL_CH4; break; default: - return_value = -1; + error = true; } } - if (return_value == -1) { + if (error) { Error_Handler(); } - return return_value; + return ll_channel; } /** @@ -323,38 +324,37 @@ int HardwareTimer::getLLChannel(uint32_t channel) * @param Arduino channel [1..4] * @retval HAL channel. Error handler called if arduino channel is invalid */ -int HardwareTimer::getIT(uint32_t channel) +uint32_t HardwareTimer::getIT(uint32_t channel) { - int return_value = -1; - + uint32_t interrupt = 0; switch (channel) { case 1: - return_value = TIM_IT_CC1; + interrupt = TIM_IT_CC1; break; case 2: - return_value = TIM_IT_CC2; + interrupt = TIM_IT_CC2; break; case 3: - return_value = TIM_IT_CC3; + interrupt = TIM_IT_CC3; break; case 4: - return_value = TIM_IT_CC4; + interrupt = TIM_IT_CC4; break; default: Error_Handler(); } - return return_value; + return interrupt; } /** * @brief Get input associated channel * Channel 1 and 2 are associated; channel 3 and 4 are associated * @param Arduino channel [1..4] - * @retval HAL channel. return -1 if arduino channel is invalid + * @retval HAL channel. Error handler called if arduino channel is invalid */ -int HardwareTimer::getAssociatedChannel(uint32_t channel) +uint32_t HardwareTimer::getAssociatedChannel(uint32_t channel) { - int timAssociatedInputChannel = -1; + uint32_t timAssociatedInputChannel = 0; switch (channel) { case 1: timAssociatedInputChannel = 2; @@ -369,6 +369,7 @@ int HardwareTimer::getAssociatedChannel(uint32_t channel) timAssociatedInputChannel = 3; break; default: + Error_Handler(); break; } return timAssociatedInputChannel; @@ -381,9 +382,9 @@ int HardwareTimer::getAssociatedChannel(uint32_t channel) */ void HardwareTimer::resumeChannel(uint32_t channel) { - int timChannel = getChannel(channel); - int timAssociatedInputChannel; - int interrupt = getIT(channel); + uint32_t timChannel = getChannel(channel); + uint32_t timAssociatedInputChannel; + uint32_t interrupt = getIT(channel); // Clear flag and enable IT if (callbacks[channel]) { @@ -631,8 +632,8 @@ void HardwareTimer::setMode(uint32_t channel, TimerModes_t mode, uint32_t pin, C */ void HardwareTimer::setMode(uint32_t channel, TimerModes_t mode, PinName pin, ChannelInputFilter_t filter) { - int timChannel = getChannel(channel); - int timAssociatedInputChannel; + uint32_t timChannel = getChannel(channel); + uint32_t timAssociatedInputChannel; TIM_OC_InitTypeDef channelOC; TIM_IC_InitTypeDef channelIC; @@ -739,7 +740,7 @@ void HardwareTimer::setMode(uint32_t channel, TimerModes_t mode, PinName pin, Ch _ChannelMode[channel - 1] = mode; if (pin != NC) { - if ((int)getTimerChannel(pin) == timChannel) { + if (getTimerChannel(pin) == timChannel) { /* Configure PWM GPIO pins */ pinmap_pinout(pin, PinMap_TIM); #if defined(STM32F1xx) @@ -807,7 +808,7 @@ void HardwareTimer::setPreloadEnable(bool value) */ void HardwareTimer::setCaptureCompare(uint32_t channel, uint32_t compare, TimerCompareFormat_t format) { - int timChannel = getChannel(channel); + uint32_t timChannel = getChannel(channel); uint32_t Prescalerfactor = LL_TIM_GetPrescaler(_timerObj.handle.Instance) + 1; uint32_t CCR_RegisterValue; @@ -869,7 +870,7 @@ void HardwareTimer::setCaptureCompare(uint32_t channel, uint32_t compare, TimerC */ uint32_t HardwareTimer::getCaptureCompare(uint32_t channel, TimerCompareFormat_t format) { - int timChannel = getChannel(channel); + uint32_t timChannel = getChannel(channel); uint32_t CCR_RegisterValue = __HAL_TIM_GET_COMPARE(&(_timerObj.handle), timChannel); uint32_t Prescalerfactor = LL_TIM_GetPrescaler(_timerObj.handle.Instance) + 1; uint32_t return_value; @@ -1010,7 +1011,7 @@ void HardwareTimer::detachInterrupt() */ void HardwareTimer::attachInterrupt(uint32_t channel, callback_function_t callback) { - int interrupt = getIT(channel); + uint32_t interrupt = getIT(channel); if ((channel == 0) || (channel > (TIMER_CHANNELS + 1))) { Error_Handler(); // only channel 1..4 have an interrupt @@ -1036,7 +1037,7 @@ void HardwareTimer::attachInterrupt(uint32_t channel, callback_function_t callba */ void HardwareTimer::detachInterrupt(uint32_t channel) { - int interrupt = getIT(channel); + uint32_t interrupt = getIT(channel); if ((channel == 0) || (channel > (TIMER_CHANNELS + 1))) { Error_Handler(); // only channel 1..4 have an interrupt @@ -1169,14 +1170,14 @@ bool HardwareTimer::isRunning() */ bool HardwareTimer::isRunningChannel(uint32_t channel) { - int LLChannel = getLLChannel(channel); - int interrupt = getIT(channel); + uint32_t LLChannel = getLLChannel(channel); + uint32_t interrupt = getIT(channel); bool ret; // channel is running if: timer is running, and either output channel is // enabled or interrupt is set ret = LL_TIM_CC_IsEnabledChannel(_timerObj.handle.Instance, LLChannel) - || (__HAL_TIM_GET_IT_SOURCE(&(_timerObj.handle), (uint32_t)interrupt) == SET); + || (__HAL_TIM_GET_IT_SOURCE(&(_timerObj.handle), interrupt) == SET); return (isRunning() && ret); } From 29f0f6840bb1bcbfd5f65f4de4d57a31d3ae3534 Mon Sep 17 00:00:00 2001 From: Frederic Pillon Date: Wed, 20 Nov 2024 17:27:59 +0100 Subject: [PATCH 2/4] chore(hardwaretimer): rename TIMER_DISABLED to TIMER_OUTPUT_DISABLED to avoid confusion. Signed-off-by: Frederic Pillon --- keywords.txt | 3 +- libraries/SrcWrapper/inc/HardwareTimer.h | 7 ++- libraries/SrcWrapper/src/HardwareTimer.cpp | 55 ++++++++-------------- 3 files changed, 26 insertions(+), 39 deletions(-) diff --git a/keywords.txt b/keywords.txt index da0171d435..a4a284e59a 100644 --- a/keywords.txt +++ b/keywords.txt @@ -862,8 +862,7 @@ GPIO_PIN_14 LITERAL1 GPIO_PIN_15 LITERAL1 # HardwareTimer -TIMER_DISABLED LITERAL1 -TIMER_OUTPUT_COMPARE LITERAL1 +TIMER_OUTPUT_DISABLED LITERAL1 TIMER_OUTPUT_COMPARE_ACTIVE LITERAL1 TIMER_OUTPUT_COMPARE_INACTIVE LITERAL1 TIMER_OUTPUT_COMPARE_TOGGLE LITERAL1 diff --git a/libraries/SrcWrapper/inc/HardwareTimer.h b/libraries/SrcWrapper/inc/HardwareTimer.h index ad24f2d4e3..2a5daa3953 100644 --- a/libraries/SrcWrapper/inc/HardwareTimer.h +++ b/libraries/SrcWrapper/inc/HardwareTimer.h @@ -36,9 +36,8 @@ #define TIMER_CHANNELS 4 // channel5 and channel 6 are not considered here has they don't have gpio output and they don't have interrupt typedef enum { - TIMER_DISABLED, // == TIM_OCMODE_TIMING no output, useful for only-interrupt + TIMER_OUTPUT_DISABLED, // == TIM_OCMODE_TIMING no output, useful for only-interrupt // Output Compare - TIMER_OUTPUT_COMPARE, // == Obsolete, use TIMER_DISABLED instead. Kept for compatibility reason TIMER_OUTPUT_COMPARE_ACTIVE, // == TIM_OCMODE_ACTIVE pin is set high when counter == channel compare TIMER_OUTPUT_COMPARE_INACTIVE, // == TIM_OCMODE_INACTIVE pin is set low when counter == channel compare TIMER_OUTPUT_COMPARE_TOGGLE, // == TIM_OCMODE_TOGGLE pin toggles when counter == channel compare @@ -60,6 +59,10 @@ typedef enum { TIMER_NOT_USED = 0xFFFF // This must be the last item of this enum } TimerModes_t; +// Backward compatibility +#define TIMER_DISABLED TIMER_OUTPUT_DISABLED +#define TIMER_OUTPUT_COMPARE TIMER_OUTPUT_DISABLED + typedef enum { TICK_FORMAT, // default MICROSEC_FORMAT, diff --git a/libraries/SrcWrapper/src/HardwareTimer.cpp b/libraries/SrcWrapper/src/HardwareTimer.cpp index 1fedba5d0d..a0b5c72431 100644 --- a/libraries/SrcWrapper/src/HardwareTimer.cpp +++ b/libraries/SrcWrapper/src/HardwareTimer.cpp @@ -123,7 +123,7 @@ void HardwareTimer::setup(TIM_TypeDef *instance) // Initialize channel mode and complementary for (int i = 0; i < TIMER_CHANNELS; i++) { __ChannelsUsed[i] = 0x00; - _ChannelMode[i] = TIMER_DISABLED; + _ChannelMode[i] = TIMER_OUTPUT_DISABLED; } /* Configure timer with some default values */ @@ -438,8 +438,7 @@ void HardwareTimer::resumeChannel(uint32_t channel) HAL_TIM_IC_Start(&(_timerObj.handle), timChannel); } break; - case TIMER_OUTPUT_COMPARE: - case TIMER_DISABLED: + case TIMER_OUTPUT_DISABLED: if (!LL_TIM_IsEnabledCounter(_timerObj.handle.Instance)) { HAL_TIM_Base_Start(&(_timerObj.handle)); } @@ -657,21 +656,10 @@ void HardwareTimer::setMode(uint32_t channel, TimerModes_t mode, PinName pin, Ch channelIC.ICFilter = filter; switch (mode) { - case TIMER_DISABLED: + case TIMER_OUTPUT_DISABLED: channelOC.OCMode = TIM_OCMODE_TIMING; HAL_TIM_OC_ConfigChannel(&(_timerObj.handle), &channelOC, timChannel); break; - case TIMER_OUTPUT_COMPARE: - /* In case of TIMER_OUTPUT_COMPARE, there is no output and thus no pin to - * configure, and no channel. So nothing to do. For compatibility reason - * restore TIMER_DISABLED if necessary. - */ - if (_ChannelMode[channel - 1] != TIMER_DISABLED) { - _ChannelMode[channel - 1] = TIMER_DISABLED; - channelOC.OCMode = TIM_OCMODE_TIMING; - HAL_TIM_OC_ConfigChannel(&(_timerObj.handle), &channelOC, timChannel); - } - return; case TIMER_OUTPUT_COMPARE_ACTIVE: channelOC.OCMode = TIM_OCMODE_ACTIVE; HAL_TIM_OC_ConfigChannel(&(_timerObj.handle), &channelOC, timChannel); @@ -738,24 +726,25 @@ void HardwareTimer::setMode(uint32_t channel, TimerModes_t mode, PinName pin, Ch // Save channel selected mode to object attribute _ChannelMode[channel - 1] = mode; - - if (pin != NC) { - if (getTimerChannel(pin) == timChannel) { - /* Configure PWM GPIO pins */ - pinmap_pinout(pin, PinMap_TIM); + if (mode != TIMER_OUTPUT_DISABLED) { + if (pin != NC) { + if (getTimerChannel(pin) == timChannel) { + /* Configure PWM GPIO pins */ + pinmap_pinout(pin, PinMap_TIM); #if defined(STM32F1xx) - if ((mode == TIMER_INPUT_CAPTURE_RISING) || (mode == TIMER_INPUT_CAPTURE_FALLING) \ - || (mode == TIMER_INPUT_CAPTURE_BOTHEDGE) || (mode == TIMER_INPUT_FREQ_DUTY_MEASUREMENT)) { - // on F1 family, input alternate function must configure GPIO in input mode - pinMode(pinNametoDigitalPin(pin), INPUT); - } + if ((mode == TIMER_INPUT_CAPTURE_RISING) || (mode == TIMER_INPUT_CAPTURE_FALLING) \ + || (mode == TIMER_INPUT_CAPTURE_BOTHEDGE) || (mode == TIMER_INPUT_FREQ_DUTY_MEASUREMENT)) { + // on F1 family, input alternate function must configure GPIO in input mode + pinMode(pinNametoDigitalPin(pin), INPUT); + } #endif - } else { - // Pin doesn't match with timer output channels - Error_Handler(); - } + } else { + // Pin doesn't match with timer output channels + Error_Handler(); + } - __ChannelsUsed[channel - 1] |= (STM_PIN_INVERTED(pinmap_function(pin, PinMap_TIM))) ? COMPLEMENTARY_CHAN_MASK : REGULAR_CHAN_MASK; + __ChannelsUsed[channel - 1] |= (STM_PIN_INVERTED(pinmap_function(pin, PinMap_TIM))) ? COMPLEMENTARY_CHAN_MASK : REGULAR_CHAN_MASK; + } } } @@ -766,11 +755,7 @@ void HardwareTimer::setMode(uint32_t channel, TimerModes_t mode, PinName pin, Ch */ TimerModes_t HardwareTimer::getMode(uint32_t channel) { - if ((1 <= channel) && (channel <= TIMER_CHANNELS)) { - return _ChannelMode[channel - 1]; - } else { - return TIMER_DISABLED; - } + return ((1 <= channel) && (channel <= TIMER_CHANNELS)) ? _ChannelMode[channel - 1] : TIMER_OUTPUT_DISABLED; } /** From 1df53a4e2fceda42c9adde12fd8d5a03a89153d4 Mon Sep 17 00:00:00 2001 From: Frederic Pillon Date: Wed, 20 Nov 2024 18:03:21 +0100 Subject: [PATCH 3/4] fix(hardwaretimer): resume depending of channels mode Allow to properly set handle state. Start TIM base only if required. Signed-off-by: Frederic Pillon --- libraries/SrcWrapper/src/HardwareTimer.cpp | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/libraries/SrcWrapper/src/HardwareTimer.cpp b/libraries/SrcWrapper/src/HardwareTimer.cpp index a0b5c72431..24377a7537 100644 --- a/libraries/SrcWrapper/src/HardwareTimer.cpp +++ b/libraries/SrcWrapper/src/HardwareTimer.cpp @@ -217,20 +217,23 @@ void HardwareTimer::pauseChannel(uint32_t channel) */ void HardwareTimer::resume(void) { + bool baseStart = true; + for (uint8_t i = 1; i <= TIMER_CHANNELS; i++) { + if (_ChannelMode[i - 1] != TIMER_OUTPUT_DISABLED) { + resumeChannel(i); + baseStart = false; + } + } // Clear flag and enable IT if (callbacks[0]) { __HAL_TIM_CLEAR_FLAG(&(_timerObj.handle), TIM_FLAG_UPDATE); __HAL_TIM_ENABLE_IT(&(_timerObj.handle), TIM_IT_UPDATE); + } - // Start timer in Time base mode. Required when there is no channel used but only update interrupt. + // Start timer in Time base mode. Required when there is no channel used but only update interrupt. + if (baseStart && (!LL_TIM_IsEnabledCounter(_timerObj.handle.Instance))) { HAL_TIM_Base_Start(&(_timerObj.handle)); } - - // Resume all channels - resumeChannel(1); - resumeChannel(2); - resumeChannel(3); - resumeChannel(4); } /** From 6c30d95cd4fa414de39cb3d68f801637d0745650 Mon Sep 17 00:00:00 2001 From: Frederic Pillon Date: Thu, 21 Nov 2024 14:55:11 +0100 Subject: [PATCH 4/4] fix(hardwaretimer): avoid glitch when PWM configuration changed Fixes #2575. Note that the issue is that TIM_OCx_SetConfig() disable in unconditionally the N state output (TIM_CCER_CCxNE). Signed-off-by: Frederic Pillon --- libraries/SrcWrapper/src/HardwareTimer.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/libraries/SrcWrapper/src/HardwareTimer.cpp b/libraries/SrcWrapper/src/HardwareTimer.cpp index 24377a7537..692397a196 100644 --- a/libraries/SrcWrapper/src/HardwareTimer.cpp +++ b/libraries/SrcWrapper/src/HardwareTimer.cpp @@ -925,7 +925,10 @@ void HardwareTimer::setPWM(uint32_t channel, uint32_t pin, uint32_t frequency, u */ void HardwareTimer::setPWM(uint32_t channel, PinName pin, uint32_t frequency, uint32_t dutycycle, callback_function_t PeriodCallback, callback_function_t CompareCallback) { - setMode(channel, TIMER_OUTPUT_COMPARE_PWM1, pin); + TimerModes_t previousMode = getMode(channel); + if (previousMode != TIMER_OUTPUT_COMPARE_PWM1) { + setMode(channel, TIMER_OUTPUT_COMPARE_PWM1, pin); + } setOverflow(frequency, HERTZ_FORMAT); setCaptureCompare(channel, dutycycle, PERCENT_COMPARE_FORMAT); if (PeriodCallback) { @@ -934,7 +937,9 @@ void HardwareTimer::setPWM(uint32_t channel, PinName pin, uint32_t frequency, ui if (CompareCallback) { attachInterrupt(channel, CompareCallback); } - resume(); + if (previousMode != TIMER_OUTPUT_COMPARE_PWM1) { + resume(); + } } /**