Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cpu/stm32/periph/pwm: some bugfixes... #14482

Merged
merged 5 commits into from
Jul 24, 2020

Conversation

hugueslarrive
Copy link
Contributor

Contribution description

PWM_RIGHT and PWM_CENTER wasn't handled correctly for stm32 cpus.

In PWM_RIGHT mode output polarity was reversed which indeed right align
rising edge but also reverses the duty cycle which is not desirable.

In PWM_CENTER mode capture/compare mode registers was set to 0 so there
was no output at all.

Moreover it is not possible to set the duty-cycle to 100% because value
is normed to dev(pwm)->ARR which is equal to (res - 1).

I also renamed CCMR_* constants because I have used CCMR_LEFT for both
PWM_LEFT and PWM_CENTER modes.

Finally in PWM_CENTER mode the resolution have to be divided by two in
pwm_init() call so it must be multiplied by 2 for the parameters
verification, prescaler setting and frequency computation, except for
the auto-reload register.

Regarding the PWM_RIGHT mode, I first tried to reverse the timer
direction in CR1 register but it wasn't possible to get 0% duty cycle
this way because there was allways one clock cycle where the output was
activated (when TIMx_CNT=TIMx_CCR1).

Testing procedure

Using tests/periph_pwm and a bluepill or another stm32 board and
checking 2 outputs (A8 and A9 for the bluepill) with a scope.

PWM_LEFT

Initialize a left aligned pwm at 1KHz with init 0 0 1000 4 then set
the first channel to 25% duty-cycle and the second to 50% with
set 0 0 1 and set 0 1 2, we get the following oscillograms as
expected:

    _       _ 
1 _| |_____| |_____
    ___     ___
2 _|   |___|   |___

But if I try to set a 100% duty-cycle with set 0 0 4, I only get 75%
because of value is normed to dev(pwm)->ARR which is equal to (res - 1).

PWM_RIGHT

Initialize a right aligned pwm at 1KHz with init 0 1 1000 4 then set
the first channel to 25% duty-cycle and the second to 50% with
set 0 0 1 and set 0 1 2, we get the following oscillograms.

Expected :

      _       _ 
1 ___| |_____| |___
    ___     ___
2 _|   |___|   |___

But we get :

  _   _____   _____
1  |_|     |_|     
  _     ___     ___
2  |___|   |___|

PWM_CENTER

Initialize a center aligned pwm at 1KHz with init 0 2 1000 4 then set
the first channel to 25% duty-cycle and the second to 50% with
set 0 0 1 and set 0 1 2 not working at all because of the CCMR
registers are set to 0.

Expected :

     _       _
1 __| |_____| |____
    ___     ___
2 _|   |___|   |___

But we get a kind of brain dead.

Issues/PRs references

@hugueslarrive
Copy link
Contributor Author

Damn! Now I see a new bug in case we use several pwm devices with different modes and one of them uses PWM_RIGHT mode

@miri64 miri64 added Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Jul 10, 2020
@miri64 miri64 added this to the Release 2020.07 milestone Jul 10, 2020
@hugueslarrive hugueslarrive force-pushed the cpu/stm32/periph/pwm branch from 1c143c9 to 304d3f9 Compare July 10, 2020 11:48
TIM_CCMR1_OC1M_2 | TIM_CCMR1_OC2M_0 | \
TIM_CCMR1_OC2M_1 | TIM_CCMR1_OC2M_2);

static uint32_t dc_reverse = 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used to store PWM devices which are initialized in PWM_RIGHT mode in the pwm_init function and whose duty cycle must be reversed by the pwm_set function.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now the pwm_set function looks into the CCMR hardware register instead.

@hugueslarrive
Copy link
Contributor Author

No activity for more than a week, do I need to do something else? I'm going on vacation for one or two weeks tomorrow.

@fjmolinas
Copy link
Contributor

@MrKevinWeiss can you test this, maybe with the HIL setup?

@aabadie aabadie added Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines labels Jul 23, 2020
@fjmolinas
Copy link
Contributor

@hugueslarrive I currently don't have access to a oscilloscope to test your changes, if @MrKevinWeiss can't test maybe you can provide a screenshot of the issue? Otherwise I can test next monday.

@hugueslarrive
Copy link
Contributor Author

@fjmolinas I left the DSO at the lab for interns but I found an old probes pair so here are some pictures of the tests.

PWM_LEFT

init 0 0 1000 4
set 0 0 1
set 0 1 2

OK:
DSC08776

Now trying set 0 1 4

with master:
DSC08777
as set 0 1 3

OK with the patch:
DSC08780

PWM_RIGHT

init 0 1 1000 4
set 0 0 1
set 0 1 2

with master:
DSC08778
as if I had done set 0 0 3

OK with the patch:
DSC08781

PWM_CENTER

init 0 2 1000 4
set 0 0 1
set 0 1 2

with master (brain dead):
DSC08779

OK with the patch:
DSC08782

Also checked in all modes

set 0 1 0
set 0 1 1
set 0 1 3
set 0 1 4

all work as expected

Copy link
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for posting the results which clearly looks good.

ACK!

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jul 24, 2020
@aabadie aabadie merged commit 42eb044 into RIOT-OS:master Jul 24, 2020
@hugueslarrive hugueslarrive deleted the cpu/stm32/periph/pwm branch August 19, 2020 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: cpu Area: CPU/MCU ports Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants