Skip to content
This repository has been archived by the owner on Aug 5, 2022. It is now read-only.

[PWM] Error thrown when set pulsewidth before period #359

Closed
cuiyanx opened this issue Nov 3, 2016 · 3 comments
Closed

[PWM] Error thrown when set pulsewidth before period #359

cuiyanx opened this issue Nov 3, 2016 · 3 comments
Labels
bug fixed P2
Milestone
0.3

Comments

@cuiyanx
Copy link
Contributor

cuiyanx commented Nov 3, 2016 

var pinA = pwm.open({ channel: pins.IO3 });
pinA.setPeriod(1000);
pinA.setPulseWidth(300);

It work well.

pinA.setPulseWidth(300);
pinA.setPeriod(1000);

error message:

zjs_pwm_set: pulseWidth was greater than period
@grgustaf grgustaf added the bug label Dec 5, 2016
@grgustaf grgustaf self-assigned this Dec 5, 2016
@grgustaf
Copy link
Contributor

grgustaf commented Dec 5, 2016

My conclusion looking at the Zephyr API was it's not good to set these things separately because you're in an invalid state in between. They agreed and changed their API based on my suggestions. So hopefully we can follow suit. @zolkis may be interested, I am afraid I'm not digging in at the moment to see what it's like in the new APIs currently.

@grgustaf grgustaf added the P2 label Dec 5, 2016
@grgustaf grgustaf added this to the 0.2 milestone Dec 5, 2016
@grgustaf grgustaf modified the milestones: 0.3, 0.2 Jan 20, 2017
grgustaf added a commit to grgustaf/zephyr.js-1 that referenced this issue Mar 28, 2017
@grgustaf
[pwm] Require PWM period/pulseWidth to be set at the same time
a4038e5 
This is a wise API choice because you can go through invalid states
by setting the wrong one first, where the pulse width is bigger than
the period. I convinced Zephyr itself to make this change, but we
hadn't gotten around to reflecting this in our own API.

Fixes intel#359.

Signed-off-by: Geoff Gustafson <geoff@linux.intel.com>
@grgustaf grgustaf mentioned this issue Mar 28, 2017
Merged
@grgustaf
Copy link
Contributor

This is addressed by #928. Now the APIs require you to set period and pulseWidth at the same time. If pulseWidth is greater than period, they throw an error immediately.

@grgustaf grgustaf added the fixed label Mar 29, 2017
@qiaojingx
Copy link

Verified with commit 1c67773 + #928. This issue is not reproducible.
Will close it after patch merged.

jimmy-huang pushed a commit that referenced this issue Mar 30, 2017
@grgustaf
[pwm] Require PWM period/pulseWidth to be set at the same time (#928)
d56bac2 
This is a wise API choice because you can go through invalid states
by setting the wrong one first, where the pulse width is bigger than
the period. I convinced Zephyr itself to make this change, but we
hadn't gotten around to reflecting this in our own API.

Fixes #359.

Signed-off-by: Geoff Gustafson <geoff@linux.intel.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
bug fixed P2
Projects
None yet
Milestone
0.3
Development

No branches or pull requests
3 participants
@grgustaf @qiaojingx @cuiyanx