Page 1 of 1
fix pwm driver enable notification function
Posted: Thu Sep 03, 2015 9:27 pm
by antoine_cvra
Hello guys,
I have a few bugfixes in our ChibiOS (git) tree that I would like to mainline. Here is the first one, a bug in the PWM driver.
Code: Select all
From: Patrick Spieler <[email protected]>
Date: Fri, 1 May 2015 17:03:29 +0200
Subject: [PATCH] fix pwm driver enable notification function
- bit mask was not correct (not inverted)
- the flag must be cleared before enabling interrupts to have the
desired effect
---
os/hal/ports/STM32/LLD/TIMv1/pwm_lld.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/os/hal/ports/STM32/LLD/TIMv1/pwm_lld.c b/os/hal/ports/STM32/LLD/TIMv1/pwm_lld.c
index c033035..f110802 100644
--- a/os/hal/ports/STM32/LLD/TIMv1/pwm_lld.c
+++ b/os/hal/ports/STM32/LLD/TIMv1/pwm_lld.c
@@ -755,8 +755,8 @@ void pwm_lld_enable_periodic_notification(PWMDriver *pwmp) {
/* If the IRQ is not already enabled care must be taken to clear it,
it is probably already pending because the timer is running.*/
if ((dier & STM32_TIM_DIER_UIE) == 0) {
+ pwmp->tim->SR &= ~STM32_TIM_SR_UIF;
pwmp->tim->DIER = dier | STM32_TIM_DIER_UIE;
- pwmp->tim->SR &= STM32_TIM_SR_UIF;
}
}
Antoine
Re: fix pwm driver enable notification function
Posted: Thu Sep 03, 2015 9:33 pm
by Giovanni
Hi,
Thanks for the patch, what is the effect of the bug?
Giovanni
Re: fix pwm driver enable notification function
Posted: Thu Sep 03, 2015 9:45 pm
by antoine_cvra
I am not sure as I am not the one who was working on this part of the project but we are doing some current measurement in an H-bridge where we need to synchronize the PWM cycle, the ADC (used for current measurement) and our control loop. I will ask Patrick what were the effect next time I see him.
I suppose this is not a very common scenario hence the bug slipping through
Re: fix pwm driver enable notification function
Posted: Tue Apr 10, 2018 6:54 pm
by kapacuk
I've just checked the git repository - the bug is still there. Can we get it fixed, please?
Re: fix pwm driver enable notification function
Posted: Tue Apr 10, 2018 7:16 pm
by Giovanni
Hi,
Moving this topic to "bug reports", it totally slipped out my mind...
Giovanni
Re: fix pwm driver enable notification function
Posted: Wed Apr 11, 2018 9:22 am
by Giovanni
Fixed as bug #934.
Giovanni
Re: fix pwm driver enable notification function
Posted: Sun Jun 10, 2018 2:01 pm
by kapacuk
Giovanni wrote:Fixed as bug #934.
Giovanni
Hi Giovanni,
Thanks for the fix, but I think you made a small mistake. The affected line in os/hal/ports/STM32/LLD/TIMv1/hal_pwm_lld.c currently looks like this:
Code: Select all
pwmp->tim->SR = ~STM32_TIM_SR_UIF;
which tries to set all bits in SR and clear the UIF bit. I understand that most bits in that register are not settable by software, but I still think it would be better to make the intention clear and change only the UIF bit:
Code: Select all
pwmp->tim->SR &= ~STM32_TIM_SR_UIF;
Re: fix pwm driver enable notification function
Posted: Sun Jun 10, 2018 5:35 pm
by Giovanni
Hi,
Those registers are of the RC_W0 kind, it is meant to be cleared that way, that change would make just code slower.
Giovanni
Re: fix pwm driver enable notification function
Posted: Mon Jun 11, 2018 10:12 pm
by kapacuk
Ah, did not know that, I still have a lot to learn. Thank you.