Raspberry Pi Pico PWM only works for 8 pins on 6.2.0-beta.2 absolute latest

[kevinjwalters]

This only works on even pins, the rest are left low judging by LEDs on a Cytron Maker Pi Pico I've been using:

Adafruit CircuitPython 6.2.0-beta.2 on 2021-02-11; Raspberry Pi Pico with rp2040
>>> import pwmio
>>> import board
>>> pins = (board.GP0, board.GP1, board.GP2, board.GP3,
... board.GP4, board.GP5, board.GP6, board.GP7,
... board.GP8, board.GP9, board.GP10, board.GP11,
... board.GP12, board.GP13, board.GP14, board.GP15)
>>> pwms = [pwmio.PWMOut(p, frequency=2000, duty_cycle=3456) for p in pins]

If the odd pins are initialised first then only those work.

>>> pwms_o = [pwmio.PWMOut(pins[p_idx], frequency=2000, duty_cycle=3456) for p_idx in range(1, len(pins), 2)]
>>> pwms_e = [pwmio.PWMOut(pins[p_idx], frequency=2000, duty_cycle=3456) for p_idx in range(0, len(pins), 2)]

Based on some discussions it sounds like the hardware is capable of 16 especially if the frequency values are all the same.

Tested on 6.2.0-beta.2 and 6.2.0-beta.2-18-g2a467f137 on 2021-02-15.

Originally mentioned in #4167 as I wasn't sure if this was a bug although having said that CP normally gives an instantiation error if it's run out of PWM things.

Curiously, #4208 shows that the consecutive pair GP18 and GP19 can both do PWM using PWMAudioOut.

[tannewt]

No errors are thrown right? What does the non-working signal look like?

[kevinjwalters]

No errors. I've not looked at signal, the relevant LEDs on the pins appear to be off so it's likely to just be held low.

[kevinjwalters]

I slowed it down so I can see what's going on, GP0 flashes as expected, GP1 doesn't flash and appears to be low for this code:

Adafruit CircuitPython 6.2.0-beta.2-18-g2a467f137 on 2021-02-15; Raspberry Pi Pico with rp2040
>>> import pwmio
>>> import board
>>> p0 = pwmio.PWMOut(board.GP0, frequency=2, duty_cycle=32768)
>>> p1 = pwmio.PWMOut(board.GP1, frequency=2, duty_cycle=32768)
>>> p0.duty_cycle
32768
>>> p1.duty_cycle
32768

[kevinjwalters]

I don't think this is particularly significant but worth checking this works on any bugfix

Adafruit CircuitPython 6.2.0-beta.2-18-g2a467f137 on 2021-02-15; Raspberry Pi Pico with rp2040
>>> import pwmio
>>> import board
>>> pwm0 = pwmio.PWMOut(board.GP0, frequency=5000, variable_frequency=False, duty_cycle=0)
>>> pwm1 = pwmio.PWMOut(board.GP1, frequency=5000, variable_frequency=False, duty_cycle=0)
>>> pwm0.frequency  ### working output
5000
>>> pwm1.frequency  ### broken output
0

[kevinjwalters]

I had a read of the code. This might be caused by a failure to initialise self->top for the second PWM in a channel's slice. The duty cycle code depends on this

circuitpython/ports/raspberrypi/common-hal/pwmio/PWMOut.c

Lines 177 to 190 in 0ec99b3

	extern void common_hal_pwmio_pwmout_set_duty_cycle(pwmio_pwmout_obj_t* self, uint16_t duty) {
	self->duty_cycle = duty;
	// Do arithmetic in 32 bits to prevent overflow.
	uint16_t compare_count;
	if (duty == 65535) {
	// Ensure that 100% duty cycle is 100% full on and not rounded down,
	// but do MIN() to keep value in range, just in case.
	compare_count = MIN(UINT16_MAX, (uint32_t) self->top + 1);
	} else {
	compare_count= ((uint32_t) duty * self->top + MAX_TOP / 2) / MAX_TOP;
	}
	// compare_count is the CC register value, which should be TOP+1 for 100% duty cycle.
	pwm_set_chan_level(self->slice, self->channel, compare_count);
	}

but it's only set in common_hal_pwmio_pwmout_set_frequency and pwmio_pwmout_set_top. The latter is not called in PWMOut.c and the former is only called once per slice. This would also explain the frequency attribute being 0 as actual_frequency is set in those two functions.

BTW, why does common_hal_pwmio_pwmout_set_frequency call common_hal_pwmio_pwmout_set_duty_cycle? If that's necessary a comment justifying it would be useful.

circuitpython/ports/raspberrypi/common-hal/pwmio/PWMOut.c

Line 237 in 0ec99b3

common_hal_pwmio_pwmout_set_duty_cycle(self, self->duty_cycle);

It also may make

circuitpython/ports/raspberrypi/common-hal/pwmio/PWMOut.c

Line 142 in 0ec99b3

common_hal_pwmio_pwmout_set_duty_cycle(self, duty);

if ordering is not important.

[kevinjwalters]

On reflection, there's a bit of an issue here with having independent values of top per PWMOut when it's a per slice value...

[tannewt]

This might be caused by a failure to initialise self->top for the second PWM in a channel's slice

This definitely seems like a bug! Mind making a PR for it?

BTW, why does common_hal_pwmio_pwmout_set_frequency call common_hal_pwmio_pwmout_set_duty_cycle?

duty_cycle is called again because the duty_cycle is scaled by the top value (which set_frequency changes.)

On reflection, there's a bit of an issue here with having independent values of top per PWMOut when it's a per slice value...

top shouldn't change if two PWMOuts are on the same slice because they are fixed frequency. (The internal set_top violates this for AudioPWMOut but only on its internal PWMOuts.)

[kevinjwalters]

There's a single execution of pwmio_pwmout_set_top lurking in the mono/stereo PWM audio code:

circuitpython/ports/raspberrypi/common-hal/audiopwmio/PWMAudioOut.c

Line 150 in 191b143

pwmio_pwmout_set_top(&self->left_pwm, 1023);