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/esp8266: fix pwm_set func #10982

Merged
merged 1 commit into from
Jun 27, 2019

Conversation

gschorcht
Copy link
Contributor

@gschorcht gschorcht commented Feb 10, 2019

Contribution description

In pwm_set function, switch-on and switch-off times for PWM channels were only determined for the following phase, but not for the current phase. This could result in

  • a missing duty cycle when function pwm_set was called before switch-on time of the current phase or
  • an extended duty cycle function pwm_set was called before switch-off time of the current phase.

This PR fixes the problem.

Testing procedure

Use test/periph_pwm with following changes in main.c:

-#define OSC_STEP        (10)
+#define OSC_STEP        (1)
-#define OSC_STEPS       (1000U)
+#define OSC_STEPS       (100U)

Flash the application with and without the changes in this PR to compare the results.

make BOARD=esp8266-esp-12x -C tests/periph_pwm flash test

Execute the osci command. Without the changes of this PR, you should be able to observe one extended duty cycle every 9th cycles. That is, pwm_set function is executed in 10th cycle and does not switch off the output port.

snapshot_2019-02-10_16-04-39

With changes of this PR, the output port should be switched on and off also in 10th cycle.

@gschorcht gschorcht added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Platform: ESP Platform: This PR/issue effects ESP-based platforms Area: cpu Area: CPU/MCU ports labels Feb 10, 2019
@gschorcht gschorcht force-pushed the cpu/esp8266/periph/pwm/pr branch from 70d121f to aa3f17e Compare February 10, 2019 15:38
@gschorcht gschorcht changed the title cpu/esp8266: fix of set func in periph/pwm cpu/esp8266: fix set func in periph/pwm Feb 11, 2019
@gschorcht gschorcht changed the title cpu/esp8266: fix set func in periph/pwm cpu/esp8266: fix pwm_set func Feb 11, 2019
@gschorcht gschorcht added this to the Release 2019.04 milestone Mar 3, 2019
@MrKevinWeiss MrKevinWeiss self-requested a review May 15, 2019 07:19
@MrKevinWeiss
Copy link
Contributor

make BOARD=esp8266-esp-12x -C tests/periph_pwm flash test

For your testing procedure you should maybe clarify use term not test (it has happened in a few PRs)

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

It seems like the pwm osci with those settings have some issues on master and this PR seems to resolve them. Tested with a scope.
It seems the code adds (80315 - 80223) 92 bytes or so, but I don't know how you can reduce so I think it is good.

cpu/esp8266/periph/pwm.c Outdated Show resolved Hide resolved
@MrKevinWeiss
Copy link
Contributor

Thanks, I will have it tested by tomorrow afternoon

Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

I think the signs are messed up. It doesn't work anymore.

cpu/esp8266/periph/pwm.c Outdated Show resolved Hide resolved
cpu/esp8266/periph/pwm.c Outdated Show resolved Hide resolved
Copy link
Contributor

@MrKevinWeiss MrKevinWeiss left a comment

Choose a reason for hiding this comment

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

Tested again, all good. Please squash. ACK

@MrKevinWeiss MrKevinWeiss added CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 26, 2019
In the `pwm_set` function, the switch-on and switch-off times for PWM channels were only determined for the following phase, but not for the current phase. This could result in a missing duty cycle when calling the function `pwm_set` if the switch-on time of the current phase was not yet reached or to an extended duty cycle if the switch-off time of the current phase had not yet been reached.
@gschorcht gschorcht force-pushed the cpu/esp8266/periph/pwm/pr branch from d3b11fc to d3e0b78 Compare June 26, 2019 14:00
@MrKevinWeiss MrKevinWeiss added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Jun 27, 2019
@MrKevinWeiss MrKevinWeiss merged commit 135ad38 into RIOT-OS:master Jun 27, 2019
@gschorcht
Copy link
Contributor Author

@MrKevinWeiss Thanks for reviewing and testing.

@gschorcht gschorcht deleted the cpu/esp8266/periph/pwm/pr branch June 27, 2019 16:28
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 CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Platform: ESP Platform: This PR/issue effects ESP-based platforms 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.

3 participants