rp2040: implement PWMGroup func to get peripheral from pin

[charlie-haley]

This PR implements a PWMGroup function which allows for easier fetching of the pwm group via the pin.

Related to this issue: #2304

[charlie-haley]

Hey @deadprogram could you trigger the tests again/review this PR? I believe the failure was a transient issue

[soypat]

This actually solves issue #2583. I think it should be properly discussed there as the API result of solving this issue should propagate to other targets.

What should the name be for this function? I prefer PWMPeripheral over PWMGroup.

[charlie-haley]

This actually solves issue #2583. I think it should be properly discussed there as the API result of solving this issue should propagate to other targets.

What should the name be for this function? I prefer PWMPeripheral over PWMGroup.

I opted for PWMGroup as PWMPeripheral was already being used in this package. We could always change PWMPeripheral to a local function if we don't think it's providing much benefit

[deadprogram]

Sorry that I am only now finally looking at this more closely.

Maybe it should be called PWMforPin() that way it can be implemented for all PWM interfaces, including ones that are organized a bit differently, but have the same issue of trying to answer that question of which PWM for a given pin.

For example, the implementation for SAMD21 would be returning a machine.TCC0.

Other than proposing the name change, agree with @charlie-haley and @soypat we need this, or something like it.

[soypat]

Thinking a bit I can imagine a couple solutions and drawbacks

---

func PWMforPin(Pin) (pwmGroup,error)

• This function signature cannot be shared between targets- non portability for cases where
  • Other non-PWMer methods of pwmGroup are used
  • Implementing this same signature for MCUs with an exported PWM type would let users then use type in other functions and structs. This is not an issue resulting from this suggested change but can be aggravated by standardizing this function signature

---

func PWMforPin(Pin) (PWMer,error)

• Needs an exported PWMer interface definition in machine package, which is not quite idiomatic. We use drivers package for this.

---

func PWMforPin(Pin) (pwmer,error)

I actually like this one the best, strangely. pwmer is an unexported interface definition.

• Interface serves as a guarantee of the functionality one may expect while preventing users from depending on method sets that may change.
• It also doubles as a railguard, preventing users from using functionality that may otherwise not be present in other MCUs

Edit:

• If a user wants the concrete type for an MCU they may always cast it.

[deadprogram]

func PWMforPin(Pin) (pwmer,error) as long as the interface is the same seems like a good approach to me.

[charlie-haley]

Sorry for the slow response to this PR, other things have been getting in the way! I've updated the PR with the feedback, I was unsure on where to put the pwmer interface so it's currently sat in the pwm.go file

Should I be retroactively updating the PWM for other boards as part of the this PR? AFAIK it's only the atmega328p, atmega1280 and nrf528xx that have support? @soypat @deadprogram

[soypat]

This feels like a lot of API surface at a glance. For starters, Enable and IsEnabled might not be present in all targets since usually a call to Configure enables the PWM on most if not all targets. Counter and SetCounter could be removed too if it is not implemented by other targets, have not seen it used much. Is Get a common method in other target implementations?

Once this interface is added, methods can't be removed without breaking backward compatibility. It would be best to be conservative.

That said...

@deadprogram @aykevl I have just had the sudden realization that users will not be able to cast pwmer to *pwmGroup since *pwmGroup is not exported! This would mean users would be stuck with the pwmer type once they call PWMforPin (unless they resort to unsafe trickery).

Possible solution

PWM objects are pwmers but we allow users to access the underlying hardware via a call to Hardware. PWMGroup contains the target-specific methods of PWM control.

// Add a Hardware method
type pwmer interface {
    Configure()
    // ...
    Hardware() *pwmGroup // Option 0
    Hardware() *PWMGroup // Option 1
}

/
var (
    // Expose PWM0-PWM15 as interfaces
    PWM0 pwmer = (*PWMGroup)(unsafe.Pointer(rp.PWMBLOCK))
    // ...
)

func PWMforPin(pin Pin) (pwmer pwmer, err error) {
    // ...
}

In this case we must decide if we want option 0 (unexported underlying hardware type) or option 1 (exported). I'm really not sure which is the best one. What I am sure is that if we decide to change it again in the future it would be a breaking change.

Final note: The pwmer interface encourages users to write more portable tinygo programs. However, today we have exposed PWM0-PWM15, which are *pwmGroup. Looking back it would have been in line with the aims of this PR if these were pwmers instead. Maybe for tinygo V2?

[soypat]

Feel like we need a Rob Pike-esque API wizard for help resolving this one.

[aykevl]

Sorry I haven't looked at the full design yet! But just wanted to quickly note that I very intentionally didn't put a PWM interface type in the machine package. The problem is that PWM is pretty complex and every chip has a different subset of features.

[aykevl]

The reason that the PWM peripheral types are different between chips is that a single chip can have multiple different kinds of PWM with different features. So this won't really work:

func PWMforPin(Pin) (pwmGroup,error)

And I also don't think we should define an interface here, precisely because it varies by chip. It is perhaps possible to return a very generic pwmer type that only supports the most core features (like Configure() and Top()) that can then be type-asserted by users of the PWMForPin function. That works, but to be honest I'm not a big fan of it either.

Perhaps a better question is: why do we need this at all? I think the main reason people want a function like PWMForPin() is because it is currently difficult to know which pins can be used by which PWM peripherals. It requires knowing a fair amount of chip internals. And that's not good.
To fix this, I have been working on better documentation. It would take a lot of work (and be very error-prone) to document this for every board. However, doing this per chip is doable. What I'd like to see is documentation like this:

Pin	Hardware pin	UART	I2C	SPI	PWM
D0	PB09	UART1 (RX)	-	-	TC4 (channel 1)
D1	PB08	UART1 (TX, RX)	-	-	TC4 (channel 0)

I started work on this in tinygo-org/tinygo-site#272. But that's just the start: that will only generate some basic information about pins. I would like to extend this to cover all peripherals like in the table above.

What do you think about this? Would this be good enough to make PWM (and other peripherals) usable?

(Sidenote: TC4 is currently unimplemented, it would be implemented as a different peripheral from the TCC peripherals because it has a different and more limited feature set).

[aykevl]

Here is a PR to add PWM peripheral information to tinygo.org: tinygo-org/tinygo-site#288

[charlie-haley]

Perhaps a better question is: why do we need this at all? I think the main reason people want a function like PWMForPin() is because it is currently difficult to know which pins can be used by which PWM peripherals. It requires knowing a fair amount of chip internals. And that's not good.

The main motivation for this, at least for me, is the rp2040 is quite a unique case when it comes to microcontrollers, technically every single GPIO can be used as PWM output, this means there's some overlap between which GPIO exposes which PWM peripheral and they can clash (e.g GP18/GP19 and GP2/GP3 can both expose PWM1A/PWM1B)

Being able to fetch the PWM peripheral by pin, for the rp2040 at least, makes sense - this also aligns with functionality provided by MicroPython and the Pico SDK.

Considering this issue is more present with the rp2040 over other MCU's, should we just go back to the original implementation like below, then we only need to expose this function to rp2040 users?

PWMforPin(pin Pin) (*pwmGroup, err error) {

[aykevl]

The main motivation for this, at least for me, is the rp2040 is quite a unique case when it comes to microcontrollers, technically every single GPIO can be used as PWM output, this means there's some overlap between which GPIO exposes which PWM peripheral and they can clash (e.g GP18/GP19 and GP2/GP3 can both expose PWM1A/PWM1B)

I don't think the rp2040 is unique in this at all. The samd21/samd51 chips also have multiple pins per PWM output. For example, the Circuit Playground Express can attach TCC0 channel 0 to four different pins.

Also, I don't understand why PWMForPin would be more necessary when every PWM output can connect to multiple pins? I would argue it's the opposite: when you have to use PWM instances (aka "slices") explicitly, it's a lot more obvious that you're using the same PWM instance for two different pins. With PWMForPin, you might try to configure two pins with a particular PWM output and wonder why they both have the same frequency (because they happen to use the same PWM peripheral).

[soypat]

Ayke, A quick drive-by response to your worry about obfuscating the underlying hardware with an API: You can't use a pwmGroup right away after calling PWMforPin- you still need the channel corresponding to the pin. This idea is ubiquitous throughout the pwmGroup API:

func (p *pwmGroup) Set(channel uint8, value uint32)

I don't think the abstraction is lost by adding PWMForPin.

Opinion of mine: with every passing day I come to think representing PWM with an unexported interface ain't so bad. I'm not sure what board would be unrepresentable with a properly defined, tiny interface. This may also be a terrible idea, I have not put much thought into it.

[soypat]

Of all the RP2040 related ideas I find this one to be the most pressing. I'd really like to solve this as soon as possible as I keep running into this issue (#2583) every time I use PWMs on the RP2040.

Before solving this issue though I think tinygo-org/drivers#491 is part of the critical path. I think we could probably define the PWM interface there. That would be a godsend of a solution for both the Netdev driver model and this PWM interface dilemma since it would solve issues posted here. It is by far the most idiomatic way of solving this in my honest opinion.