[DONE] ICU driver initial state glitch
Posted: Mon Jan 27, 2014 2:09 am
Hi, I believe I have a bug with the STM32 ICU driver.
It seems it is possible to have a WAITING->IDLE transition, e.g. when using ICU_INPUT_ACTIVE_HIGH and a falling edge occurs right after the ICU is enabled. This in itself is acceptable behavior to me, although it is not documented.
However, this also invokes the width callback, since _icu_isr_invoke_width_cb doesn't check if the previous state was WAITING.
The problem is exacerbated by icu_lld_enable not clearing the timer count, so this first spurious callback invocation often has a very wrong icu_lld_get_width value if the timer had been already running for a while.
I suggest some or all of the following fixes:
It seems it is possible to have a WAITING->IDLE transition, e.g. when using ICU_INPUT_ACTIVE_HIGH and a falling edge occurs right after the ICU is enabled. This in itself is acceptable behavior to me, although it is not documented.
However, this also invokes the width callback, since _icu_isr_invoke_width_cb doesn't check if the previous state was WAITING.
The problem is exacerbated by icu_lld_enable not clearing the timer count, so this first spurious callback invocation often has a very wrong icu_lld_get_width value if the timer had been already running for a while.
I suggest some or all of the following fixes:
- Disallow WAITING to IDLE driver state changes.
- If transitioning from WAITING to IDLE, don't invoke the width callback.
- Reset the timer when enabling the driver.
Code: Select all
diff --git a/os/hal/include/icu.h b/os/hal/include/icu.h
index cf21e59..58ebbd2 100644
--- a/os/hal/include/icu.h
+++ b/os/hal/include/icu.h
@@ -147,8 +147,10 @@ typedef void (*icucallback_t)(ICUDriver *icup);
* @notapi
*/
#define _icu_isr_invoke_width_cb(icup) { \
- (icup)->state = ICU_IDLE; \
- (icup)->config->width_cb(icup); \
+ if ((icup)->state != ICU_WAITING) { \
+ (icup)->state = ICU_IDLE; \
+ (icup)->config->width_cb(icup); \
+ } \
}
/**
diff --git a/os/hal/platforms/STM32/TIMv1/icu_lld.c b/os/hal/platforms/STM32/TIMv1/icu_lld.c
index b407d33..693c1d4 100644
--- a/os/hal/platforms/STM32/TIMv1/icu_lld.c
+++ b/os/hal/platforms/STM32/TIMv1/icu_lld.c
@@ -615,6 +615,7 @@ void icu_lld_stop(ICUDriver *icup) {
void icu_lld_enable(ICUDriver *icup) {
icup->tim->SR = 0; /* Clear pending IRQs (if any). */
+ icup->tim->EGR |= STM32_TIM_EGR_UG; /* Update config and clear counter. */
if (icup->config->channel == ICU_CHANNEL_1) {
if (icup->config->period_cb != NULL)
icup->tim->DIER |= STM32_TIM_DIER_CC1IE;