MCPWM 5.0

[usbalbin]

trafficstars

The beginning of a very very rough draft of IDF 5.0 style MCPWM. (mostly reused from #93, thus a lot of code will change)

Counter proposal for #93

~~Note: This depends on esp-idf/esp-idf-sys#133~~ - Merged

Status as of 2022-12-10

• Does compile
• examples/mcpwm-simple tested and working on ESP32-S3

[ivmarkov]

@usbalbin @MabezDev Folks, I'm missing a little bit the context of what is going on here. Can somebody summarize it for me? I'm struggling with the following:

• We have #93 now. However it does not explain what it is improving upon? Is it about the fact that you can somehow support two duty signals with it?
• Then we have this one (#132). It is named "MCPWM 5.0". Also no explanations. Is this also supporting two duty signals, but only for ESP IDF 5? If so, what would be our story for ESP IDF 4.4, which would be around for quite some time?

@usbalbin I think it would be very useful if you create a comment that summarizes what these changes are about, and what is their relation to ESP IDF4 vs ESP IDF 5. I think you might be assuming that we have a bit more context than what we actually know about this stuff. :) Thanks!

[usbalbin]

@usbalbin @MabezDev Folks, I'm missing a little bit the context of what is going on here. Can somebody summarize it for me? I'm struggling with the following:

• We have Motor Control Pulse Width Modulator (MCPWM) #93 now. However it does not explain what it is improving upon? Is it about the fact that you can somehow support two duty signals with it?[...]

Sorry about the confusion. Sure thing! First of, I am no authority on the subject myself and please correct me if I am wrong, but I will do my best :)

MCPWM vs LEDC

MCPWM, as compared to the other way to generate PWM signals(LEDC) on the ESP devices, has more motor control related features. For example signals can be synced with each other or with an external source, enabling things like phase shifting with very precise timing. PWM signals are internally generated in pairs with the same clock source and can be configured to have completely independent duty or to work in complimentary mode with a dead time. So for example to control the switches of a half bridge without risk of short circuit.

There is also things like fault detection, where without the need for software intervention, an action can be performed such as "force output low"(think over current protection). Then we have the capture unit which can decode timing related things such as the duty of a PWM signal or similar.

---

• Then we have this one (MCPWM 5.0 #132). It is named "MCPWM 5.0". Also no explanations. Is this also supporting two duty signals, but only for ESP IDF 5? If so, what would be our story for ESP IDF 4.4, which would be around for quite some time?

I started #93 without any knowledge of there being a IDF 5.0 on the horizon. It then turned out that IDF 5.0 changes how MCPWM is exposed in some quite fundamental, very breaking ways. Also the "IDF <= 4.4" style MCPWM API is deprecated in 5.0. Thus I started experimenting with the 5.0 version in parallell. Currently #132 is very much just #93 copypasted with some experimental changes on top(will most likely change a lot and perhaps be rewritten completely just for 5.0 depending on what we want to do).

Regarding #93 vs #132 vs #93 + #132, I believe that is a question for you to answer :)

• Only use the (as of IDF 5.0) deprecated 4.4 MCPWM version - Merge #93, Close #132
• Only use the new MCPWM version feature gated behind IDF 5.0 - Close #93, Finish and merge #132
• Try to cfg together something that compiles with both IDF versions but only enabling the added flexibility when using IDF 5.0 - ?

@usbalbin I think it would be very useful if you create a comment that summarizes what these changes are about, and what is their relation to ESP IDF4 vs ESP IDF 5. I think you might be assuming that we have a bit more context than what we actually know about this stuff. :) Thanks!

Please let me know if this answered your questions :)

---

Again #132 is nowhere near ready for review yet :)

[usbalbin]

This is still quite the mess, and there are a lot of things I am unsure of. However before I get too far down the road, may I request a very light review of your thoughts of the overall picture of this PR?

• Does the API make sense?
• Does the usage in the example mcpwm-simple make sense?
• Any ideas as for how to reduce the perhaps a little bit too heavy use of generics :)
• Naming of things...

[usbalbin]

I just noticed there is https://github.com/esp-rs/esp-hal/pull/255 , perhaps there could be some inspiration to take from there?

[usbalbin]

Rebased over master and removed duplicate commits from #183

[usbalbin]

I have now tested examples/mcpwm-simple.rs on an ESP32-S3 and it does (after a lot of fixes) seem to work.

[usbalbin]

Note sure how many TODO's we are comfortable with before merging, there are quite a few still. However I guess the code is perhaps atleast ready to be looked at now :) (as in no longer a draft)

[ivmarkov]

Yes - will look at it over the weekend. @MabezDev if you can take a look too, that would be appreciated!

[usbalbin]

Thanks a lot for the review(s)!

[usbalbin]

I have now made the two Comparators of every Operator mandatory. That way we get rid two of Operator's type parameters.

Same thing could be done for the Generators, however there is, as far as I know, no way to create a generator without assigning a pin. Since not all users need both pins, I would prefer if this was optional. I suppose do that by Optional<Generator<.., PIN>>. However is there any good way to not force the user to specify the type of PIN when passing None?

[ivmarkov]

I have now made the two Comparators of every Operator mandatory. That way we get rid two of Operator's type parameters.

Same thing could be done for the Generators, however there is, as far as I know, no way to create a generator without assigning a pin. Since not all users need both pins, I would prefer if this was optional. I suppose do that by Optional<Generator<.., PIN>>. However is there any good way to not force the user to specify the type of PIN when passing None?

const NO_PIN: Option<AnyIOPin> = None. And then you can pass NO_PIN everywhere.

[ivmarkov]

I have now made the two Comparators of every Operator mandatory. That way we get rid two of Operator's type parameters.

Same thing could be done for the Generators, however there is, as far as I know, no way to create a generator without assigning a pin. Since not all users need both pins, I would prefer if this was optional. I suppose do that by Optional<Generator<.., PIN>>. However is there any good way to not force the user to specify the type of PIN when passing None?

And BTW you can get rid of the P generic in your generator struct. I.e. from

pub struct Generator<G, P: OutputPin>

into

pub struct Generator<G>

Why don't you look into how the other drivers do it? As in the SPI and I2C ones? The idea is that only the constructor of the struct is generified by P but the struct itself isn't. You just keep the pin number in the struct, not the actual pin peripheral.

[ivmarkov]

There are also other issues, like the fact that you need to acquaint yourself with the PeripheralRef and pin: impl Peripheral<P = impl OutputPin> + d syntax. Where I'm getting at, is that your current Generator struct in its current shape has also other issues, like the fact that it keeps the pin as P: OutputPin, while it should keep it as PeripheralRef<P> where P: OutputPin. That is, if it was necessary to keep the peripheral in the Generator struct in the first place, which it isn't.

[ivmarkov]

There are BTW a bunch of other Option-inventing traits throughout the code. I hope you plan to get rid of these as well. If not, let me know, and we can discuss why not (as in what difficulties you see.)

[usbalbin]

There are BTW a bunch of other Option-inventing traits throughout the code. I hope you plan to get rid of these as well. If not, let me know, and we can discuss why not (as in what difficulties you see.)

Yes, I know, I think I should be able to fix most/all of those now...

const NO_PIN: Option<AnyIOPin> = None. And then you can pass NO_PIN everywhere.

Oh, that is very nice.

There are also other issues, like the fact that you need to acquaint yourself with the PeripheralRef and pin: impl Peripheral<P = impl OutputPin> + d syntax. Where I'm getting at, is that your current Generator struct in its current shape has also other issues, like the fact that it keeps the pin as P: OutputPin, while it should keep it as PeripheralRef<P> where P: OutputPin. That is, if it was necessary to keep the peripheral in the Generator struct in the first place, which it isn't.

Thanks, I'll look into that

[usbalbin]

The main types the user will be passing around as of right now looks like this

struct TimerDriver<const N: u8, G: Group> {...}
struct Operator<'d, const N: u8, G: Group> {...}
struct TimerConnection<const N: u8, G: Group, O0, O1, O2> {...}

I suppose TimerDriver(and therefore also TimerConnection) will also get a lifetime parameter once i have done the impl Peripheral<T = Timer> thing for that too.

What types should get the Driver suffix? All of Operator, TimerConnection, Generator and Comparator?

[ivmarkov]

The main types the user will be passing around as of right now looks like this

struct TimerDriver<const N: u8, G: Group> {...}
struct Operator<'d, const N: u8, G: Group> {...}
struct TimerConnection<const N: u8, G: Group, O0, O1, O2> {...}

I suppose TimerDriver(and therefore also TimerConnection) will also get a lifetime parameter once i have done the impl Peripheral<T = Timer> thing for that too.

What types should get the Driver suffix? All of Operator, TimerConnection, Generator and Comparator?

For now, let's just have TimerDriver suffixed with *Driver. Once this lands, we can revisit shortly after that. Question about the remaining generics:

• Why do we need N?
• Why do we still need O0, O1 and O2?

[ivmarkov]

If that brings additional clarity: we are continuing to (ab)use traits to express patterns where I hoped a simple Option would do. As in OptionalOperator. Why do we need to express that an operator is optional with a dedicated trait?

[usbalbin]

For now, let's just have TimerDriver suffixed with *Driver. Once this lands, we can revisit shortly after that. [...]

Ok

• Why do we need N?

TimerConnection is used to ensure the operators' won't outlive the Timer. TimerConnection::split() gives us mutable references to both timer and operators. If we allowed multiple instances of the same operator type, the user could use mem::swap to move the Operator out of the TimerConnection which would be bad. Could this be checked at runtime? Maybe, have not given it enough thought..

Other than that, there is also the issue of IDF4 only supporting Timer<A> connecting to Operator<B> iff A=B.

• Why do we still need O0, O1 and O2?

Thanks to that, TimerConnection::split() can return the real types without the user having to unwrap any Options. I was also considering adding TimerConnection::timer() and TimerConnection::operator{N} methods at some point(nice to have when only needing access to one object at a time). TimerConnection::operator{N} would only be implemented if O{N} is a real Operator. In my mind this would be really nice for discoverability when using something like vs code + rust analyzer or similar.

Is it worth the three type parameters? Not sure... :)

[noahbliss]

Hey @usbalbin I'm looking to implement/leverage this as a pwm capture method, any pointers on where you left off?

[usbalbin]

Hey @usbalbin I'm looking to implement/leverage this as a pwm capture method, any pointers on where you left off?

I have not really looked at the capture thing. However most of the types so far are quite close to how the diagram are drawn here, so I would probably start there and look through the diagrams and functions/structures to figure out how things relate.

From a quick glance, the capture thing seems to be quite independent having its own internal timer etc, yet it seems it can be synced with other timers from the same mcpwm unit.

As to the state of the code here, I do not quite remember. I have not worked on this for quite a while. I believe it should work but there is likely a lot of room for improvement :)

[usbalbin]

I am not working at this right now, and I do not see myself doing so anytime soon. So feel free to use this/finish it as you wish :)