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

MCPWM 5.0 #132

Open
wants to merge 62 commits into
base: master
Choose a base branch
from
Open

MCPWM 5.0 #132

wants to merge 62 commits into from

Conversation

usbalbin
Copy link
Contributor

@usbalbin usbalbin commented Sep 16, 2022

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
Copy link
Collaborator

@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?
  • 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?

@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
Copy link
Contributor Author

@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:

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 :)

@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 :)

examples/mcpwm-simple.rs Outdated Show resolved Hide resolved
examples/mcpwm-deadtime.rs Outdated Show resolved Hide resolved
.cargo/config.toml Outdated Show resolved Hide resolved
src/mcpwm/comparator.rs Outdated Show resolved Hide resolved
@usbalbin
Copy link
Contributor Author

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
Copy link
Contributor Author

I just noticed there is esp-rs/esp-hal#255 , perhaps there could be some inspiration to take from there?

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API looks sensible and easy to use. We need to figure out how to support v4.4 as well (a limitation esp-rs/esp-hal#255 isn't bound by), I've dropped a quick comment about that but I'm sure you'll figure out a clean way of managing it :).

Overall I like the direction of this PR, so I think you're safe to move forward.

.cargo/config.toml Outdated Show resolved Hide resolved
.cargo/config.toml Outdated Show resolved Hide resolved
src/mcpwm/comparator.rs Outdated Show resolved Hide resolved
O1: OptionalOperator<1, G>,
O2: OptionalOperator<2, G>,
{
pub fn attatch_operator0<CMPX, CMPY, GENA, GENB>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So for v4.4, these bounds don't currently stop you from using timer0 & operator2, right? Perhaps we can somehow use const generics here such that TimerConnection must always match Operator for v4.4 🤔.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something roughly like(semi made up code with bad names)?

struct CanAttach<const TIMER: u8, const OPERATOR: u8>;
trait Yes;

#[cfg(v4_4)]
impl Yes for CanAttach<0, 0>{}

#[cfg(v4_4)]
impl Yes for CanAttach<1, 1>{}

#[cfg(v4_4)]
impl Yes for CanAttach<2, 2>{}

#[cfg(not(v4_4)]
impl<const TIMER: u8, const OPERATOR: u8> Yes for CanAttach<TIMER, OPERATOR>{}
 impl<const N: u8, G, O1, O2> TimerConnection<N, G, NoOperator, O1, O2>
 where
     G: Group,
     O1: OptionalOperator<1, G>,
     O2: OptionalOperator<2, G>,
+    CanAttach<N, 0>: Yes,
 {
     pub fn attatch_operator0<CMPX, CMPY, GENA, GENB>(

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! This is basically what I had in mind - maybe one day when const expressions are stable we can simply further.

src/mcpwm/operator_config.rs Outdated Show resolved Hide resolved
@usbalbin usbalbin mentioned this pull request Dec 8, 2022
@usbalbin
Copy link
Contributor Author

usbalbin commented Dec 10, 2022

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)

@usbalbin usbalbin marked this pull request as ready for review December 10, 2022 18:07
@ivmarkov
Copy link
Collaborator

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

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've tested the example on my s3 board with a logic analyser and the output looks good to me.

I think the number of TODOs is okay and can be resolved later. I think that adding v4.4 support can also be resolved in a follow-up PR, but I'll let @ivmarkov decide whether that should be done here or not.

Just one comment about using a release esp-idf-sys to resolve on my end.

Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +90 to +91
flags.set_update_cmp_on_tep(0);
flags.set_update_cmp_on_tez(1);
Copy link
Contributor Author

@usbalbin usbalbin Dec 22, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just remembered espressif/esp-idf#9904 . Do you think tep should also be set to 1 to avoid that? I have not tested this PR for that problem yet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
flags.set_update_cmp_on_tep(0);
flags.set_update_cmp_on_tez(1);
flags.set_update_cmp_on_tep(1);
flags.set_update_cmp_on_tez(1);

Copy link
Collaborator

@ivmarkov ivmarkov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@usbalbin @MabezDev I think we might be using way too many traits and generics where simpler types would suffice. As a result, the driver types are very very generics heavy, whereas the HAL crate has been moving in precisely the opposite direction during the last several months and with the latest release of the HAL. I'm also struggling to see the need for most of these traits?

Am I missing something?

src/peripherals.rs Outdated Show resolved Hide resolved
src/peripherals.rs Outdated Show resolved Hide resolved
src/mcpwm/timer_connection.rs Outdated Show resolved Hide resolved
src/mcpwm/timer_connection.rs Outdated Show resolved Hide resolved
src/mcpwm/timer_connection.rs Outdated Show resolved Hide resolved
}
}

pub struct Timer<const N: u8, G: Group> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having _group and _timer undescored means you don't actually use them after the new constructor. Can we get rid of those? That would mean getting rid of the N and G generics on the Timer(Driver) which simplifies the type.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See discussion. For(not added in this PR) to be able to support some limited version of this for IDF 4.4 later on, we need to be able to limit what Timer can be attached to what Operator.

In general a timer can only be connected to operators in the same Group, this is a hardware limitation. With IDF 4.4 there is also the constraint that Timer<X, G> can only be connect to Operator<Y, G> iff X=Y. This is done using (misspelled) attatch_operator{A} (soon to be attach_operator{A}).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, I could not come up with a less generics heavy way to express something like that. I am however very open to suggestions :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the G generic is remaining, that's still not the end of the world, as long as we can still simplify / remove most of the other generics induced by traits in the code.

}
}

pub struct TIMER<const N: u8, G: Group> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've asked for the removal of as many traits from this impl as possible, but introducing a Timer trait here, which is a marker trait for a TIMER peripheral (don't confuse it with the existing Timer struct which should be renamed to TimerDriver - see below)), has the big benefit that it will help you to remove a lot of the generics from the driver (TimerDriver).


impl<const N: u8, G: Group> crate::peripheral::sealed::Sealed for TIMER<N, G> {}

impl<const N: u8, G: Group> crate::peripheral::Peripheral for TIMER<N, G> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to note that implementing Peripheral only makes sense if you allow - in the TimerDriver::new constructor - to pass the peripheral not just by moving, but also by &mut ref, which is not possible right now. I'll elaborate further down in the code how that can become possible.

}

impl<const N: u8, G: Group> Timer<N, G> {
pub fn new(timer: TIMER<N, G>, config: TimerConfig) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timer peripheral should be passed as (roughly) timer: impl Peripheral<P = impl Timer> + 'd where Timer (to be renamed to TimerDriver) is parameterized by the 'd lifetime. The current implementation does not allow for taking a &mut ref to the timer peripheral, which means that dropping the driver will always lose the peripheral.

_timer
}*/

pub fn into_connection(self) -> TimerConnection<N, G, NoOperator, NoOperator, NoOperator> {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some concerns around this method being consuming, but let's discuss this later, after we have covered some of the other changes I've suggested.

@usbalbin
Copy link
Contributor Author

usbalbin commented Jan 23, 2023

Thanks a lot for the review(s)!

* Rename struct Timer to TimerDriver
* Fix typo in fn attach_timerX
* Remove OptionalOutputPin
@usbalbin
Copy link
Contributor Author

usbalbin commented Jan 28, 2023

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Collaborator

ivmarkov commented Jan 28, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

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 = 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

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

Comment on lines 35 to 39
OperatorConfig::empty()
.cmp_x(Default::default())
.cmp_y(Default::default())
.gen_a(GeneratorConfig::active_high(pin_a))
.gen_b(GeneratorConfig::active_high(pin_b))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here for

.gen_a(GeneratorConfig::active_high(pin_a))

due to GeneratorConfig being generic GeneratorChannel, active_high knows that the generator A should only listen to comparator X.

Similarly for .gen_b(GeneratorConfig::active_high(pin_b)), it knows that generator A should only listen for comparator Y.

I am not quite sure how to achieve something similar using enums

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you have any thoughts?

Copy link
Collaborator

@ivmarkov ivmarkov Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If Gen A always listens to Comparator X and Gen B always listens to Comparator Y, then why don't you just make Gen A to own its comparator (Comparator X), and Gen B to own its comparator as well (Comparator Y)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default configuration is to have it work that way, yes. It is also they way it works in IDF 4.4.

However in IDF 5.x you are free to have each generator listen to events from none, any or all comparators.


In a future PR when we add support for IDF 4.4 we need to somehow prevent connecting the wrong comparators to the generator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a future PR when we add support for IDF 4.4 we need to somehow prevent connecting the wrong comparators to the generator.

This is a bit similar to the discussion about how operators and timers are related

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well I'm also not happy with pushing things to runtime, especially given that one of the major premises of Rust is compile time safety.

But this is all a fine-balance act, right? As in, I've come to realize that simplicity of use and simpler types is just as important as compile-type safety.

Let me put it another way: if we do this one sacrifice - for ESP-IDF 4 only - would that allow us to get rid of most of the generics?

Copy link
Contributor Author

@usbalbin usbalbin Feb 6, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But this is all a fine-balance act, right? As in, I've come to realize that simplicity of use and simpler types is just as important as compile-type safety.

Yes, very much so :)

Let me put it another way: if we do this one sacrifice - for ESP-IDF 4 only - would that allow us to get rid of most of the generics?

Yes, I think atleast GENA/GENB and CMX/CMPY can be removed in that case, and in addition we will have to rethink OperatorConfig a bit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will give it a try :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you so much! I know it's been a long ride...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem at all, thank you!

@usbalbin
Copy link
Contributor Author

usbalbin commented Feb 7, 2023

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
Copy link
Collaborator

ivmarkov commented Feb 9, 2023

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
Copy link
Collaborator

ivmarkov commented Feb 9, 2023

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
Copy link
Contributor Author

usbalbin commented Feb 9, 2023

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
Copy link

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

@usbalbin
Copy link
Contributor Author

usbalbin commented Apr 5, 2024

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
Copy link
Contributor Author

usbalbin commented Apr 5, 2024

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants