fix pwm driver enable notification function Topic is solved

Report here problems in any of ChibiOS components. This forum is NOT for support.
antoine_cvra
Posts: 2
Joined: Thu Sep 03, 2015 9:11 pm

fix pwm driver enable notification function  Topic is solved

Postby antoine_cvra » Thu Sep 03, 2015 9:27 pm

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 <stapelzeiger@gmail.com>
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

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: fix pwm driver enable notification function

Postby Giovanni » Thu Sep 03, 2015 9:33 pm

Hi,

Thanks for the patch, what is the effect of the bug?

Giovanni

antoine_cvra
Posts: 2
Joined: Thu Sep 03, 2015 9:11 pm

Re: fix pwm driver enable notification function

Postby antoine_cvra » Thu Sep 03, 2015 9:45 pm

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 ;)

kapacuk
Posts: 5
Joined: Fri Apr 06, 2018 2:30 pm

Re: fix pwm driver enable notification function

Postby kapacuk » Tue Apr 10, 2018 6:54 pm

I've just checked the git repository - the bug is still there. Can we get it fixed, please?

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: fix pwm driver enable notification function

Postby Giovanni » Tue Apr 10, 2018 7:16 pm

Hi,

Moving this topic to "bug reports", it totally slipped out my mind...

Giovanni

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: fix pwm driver enable notification function

Postby Giovanni » Wed Apr 11, 2018 9:22 am

Fixed as bug #934.

Giovanni

kapacuk
Posts: 5
Joined: Fri Apr 06, 2018 2:30 pm

Re: fix pwm driver enable notification function

Postby kapacuk » Sun Jun 10, 2018 2:01 pm

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;

User avatar
Giovanni
Site Admin
Posts: 14444
Joined: Wed May 27, 2009 8:48 am
Location: Salerno, Italy
Has thanked: 1074 times
Been thanked: 921 times
Contact:

Re: fix pwm driver enable notification function

Postby Giovanni » Sun Jun 10, 2018 5:35 pm

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

kapacuk
Posts: 5
Joined: Fri Apr 06, 2018 2:30 pm

Re: fix pwm driver enable notification function

Postby kapacuk » Mon Jun 11, 2018 10:12 pm

Ah, did not know that, I still have a lot to learn. Thank you.


Return to “Bug Reports”

Who is online

Users browsing this forum: No registered users and 16 guests