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

Support PWM on multiple pins #10

Merged
merged 3 commits into from Jun 14, 2014

Conversation

Projects
None yet
4 participants
@kevinmehall
Copy link
Member

commented Jun 12, 2014

API suggestions encouraged.

The commit linking Pins back to their Port makes the printing of pins way uglier. The PWM frequency could be a global instead of a Port property if it had to be, because there's only one port with PWM.

@jiahuang

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

Can we add pwm to the pin mappings and make tessel.gpio('pwm') spit out the list of pwm pins like .analog and .digital currently do?

@jiahuang

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

I agree with you about the freq being a global instead of attached to a port. If it's a port setting I would assume that each port can have a different pwm frequency, which means I would expect the PWM freq on port A to not change if I change around the freq on port B.

@kevinmehall

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2014

No, I think design-wise it's correct to attach it to a port. Right now only the GPIO bank has PWM pins, but if we were to design it to have PWM on the module ports, we'd want a separate timer per port, so each could have a different frequency.

What I'm wondering is just an implementation detail -- I added Pin.port so the pin could do this.port.pwmPeriod when converting its duty cycle to a timer value, but if we don't want Pins to have a port property, it could go in a global instead.

@jiahuang

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

I think it would be confusing to have port.pwmFrequency and pin.pwmDutyCycle. If I didn't know the underlying mechanics of how pwm is set up and configured it's not obvious why a port would have a pwm setting and why I can't just set it through the pin.

@kevinmehall

This comment has been minimized.

Copy link
Member Author

commented Jun 12, 2014

But that's how it works. Frequency can't be set on the pin because it affects all three PWM pins.

@jiahuang

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

Yeah you're right, at least this way it forces the user to be conscious of their decision about setting a freq.

@johnnyman727

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

I think it's a good idea to keep the frequency setting on the port and the dutyCycle setting on the pin. It will make for a less aggrivating time for folks who know enough about what they're doing to change the frequency (Arduino just sets it at 980Hz).

In terms of the semantics of where the frequency is stored, I'm in favor of a global variable. I hate when I have to parse each chunks of text when I'm debugging with console log.

@johnnyman727

This comment has been minimized.

Copy link
Contributor

commented Jun 12, 2014

And @jiahuang, I already implemented the .pwm pin mappings in this PR.

int hw_pwm_pin_pulsewidth (int pin, uint32_t pulsewidth)
{
if (g_APinDescription[pin].alternate != PWM_MODE) {
return -1; // Not a PWM pin

This comment has been minimized.

Copy link
@tcr

tcr Jun 12, 2014

Member

Some of our tm_ and hw_ functions return positive value errors instead of negative (since it's not an overloaded value like ssize_t). Which should we standardize on?

This comment has been minimized.

Copy link
@kevinmehall

kevinmehall Jun 13, 2014

Author Member

I think negative is better so the same convention and error codes can be used with functions that return sizes or other positive values.

@@ -1303,6 +1302,15 @@ Port.prototype.digitalWrite = function (n, val) {
hw.digital_write(n, val ? hw.HIGH : hw.LOW);
};

Port.prototype.pwmFrequency = function (frequency) {
if (this.id.toUpperCase() == 'GPIO') {

This comment has been minimized.

Copy link
@tcr

tcr Jun 12, 2014

Member

Preferrably have this as a boolean on the port constructor (also so it's not based on a string comparison)

@@ -1303,6 +1302,15 @@ Port.prototype.digitalWrite = function (n, val) {
hw.digital_write(n, val ? hw.HIGH : hw.LOW);
};

Port.prototype.pwmFrequency = function (frequency) {
if (this.id.toUpperCase() == 'GPIO') {
this.pwmPeriod = 1/(frequency/180000000);

This comment has been minimized.

Copy link
@tcr

tcr Jun 12, 2014

Member

Math.floor(1/(frequency/180000000)) or (1/(frequency/180000000))|0 to emphasize only integer values are passed in?

This comment has been minimized.

Copy link
@kevinmehall

kevinmehall Jun 12, 2014

Author Member

You can pass floating point frequencies to the pwmFrequency function, but yes, I'll make that Math.round.

kevinmehall and others added some commits Jun 10, 2014

@kevinmehall kevinmehall referenced this pull request Jun 13, 2014

Closed

Adds pwm pin type #7

@kevinmehall

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2014

Updated addressing comments and integrating Jon's PR.

LPC_SCT->CONFIG = SCT_CONFIG_UNIFY; // 32bit, Clocked from peripheral clock
LPC_SCT->REGMODE_L = 0; // All registers are match registers
LPC_SCT->REGMODE_L = 0; // All registers are match registers

This comment has been minimized.

Copy link
@johnnyman727

johnnyman727 Jun 13, 2014

Contributor

This extra tab makes me a little sad but I'm not sure it's worth rebasing over.

@johnnyman727

This comment has been minimized.

Copy link
Contributor

commented Jun 13, 2014

It's working to spec on my Tessel. r+

kevinmehall added a commit that referenced this pull request Jun 14, 2014

Merge pull request #10 from tessel/km-pwm
Support PWM on multiple pins

@kevinmehall kevinmehall merged commit 508215f into master Jun 14, 2014

1 check passed

continuous-integration/ciplexer All targets passed CI.
Details

@kevinmehall kevinmehall deleted the km-pwm branch Sep 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.