8000 PWM: Reduce inconsitencies between ports. · Issue #10817 · micropython/micropython · GitHub
[go: up one dir, main page]

Skip to content

PWM: Reduce inconsitencies between ports. #10817

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

Closed
robert-hh opened this issue Feb 22, 2023 · 76 comments
Closed

PWM: Reduce inconsitencies between ports. #10817

robert-hh opened this issue Feb 22, 2023 · 76 comments
Labels
enhancement Feature requests, new feature implementations extmod Relates to extmod/ directory in source

Comments

@robert-hh
Copy link
Contributor
robert-hh commented Feb 22, 2023

Since we talked about PWM:
It seems that the ports behave different with respect to PWM. The esp32 and mimxrt ports start as soon as the PWM object is instantiated, albeit with different frequencies. The rp2, SAMD and ESP8266 ports for instance require frequency an duty cycle to be set for providing output. The nrf implementation looks kind of broken, being not able to change frequency or period and requiring period to be set even if freq was provided. No machine.PWM at the STM32 port. Not to forget the little things like no keyword arguments at the RP2 PWM constructor. Seems it's time to tidy up things. I can work on that topic, but first of all we should agree on a behavior. So the kind of common behavior would be:

  • No output until freq (or period) and duty cycle are set.
  • The constructor accepts keyword arguments.
  • deinit() stops the PWM but does not release the PWM object, and init() or setting a new freq/duty cycle restarts it again.
  • Fix the nrf port. Looks too strange.
  • Adapt the quickref doc examples to set freq and duty_u16.
@robert-hh robert-hh added the enhancement Feature requests, new feature implementations label Feb 22, 2023
@dpgeorge
Copy link
Member

No output until freq (or period) and duty cycle are set

That makes sense. The alternative would be to have a default freq/duty which it uses on construction, but that's likely to lead to more confusion than actually be useful.

We could also add a .start() method... but I don't think it's necessary. As you say, it can start once the freq and duty have been configured and if the user wants to delay the start (and potentially stop and restart) they can just use the .duty() method to set the duty to 0% (or 100%) to "stop" the output.

But still, we need to define what it actually means for a PWM object to be created and not yet started, because then it has two (possibly three) states: created but not yet running, running, and deinit'd. Eg, when the PWM is created on a pin but not yet running (no freq and/ 8000 or duty) does it make that pin an output? High or low? And what exactly does .deinit() do, what state is the pin in?

The constructor accepts keyword arguments

Yes, that would be consistent with all the other machine classes.

@dpgeorge dpgeorge added the extmod Relates to extmod/ directory in source label Feb 22, 2023
@robert-hh
Copy link
Contributor Author

We could also add a .start() method...

init() without arguments can (and does on some ports) restart PWM again, after it has been stopped by deinit().
And you are right that the state of the Pin has to be defined when the PWM is not running - if it can be set. I cannot imagine a per se useful state for any possible application. That depends on the circuitry attached to it. So we either have to grab a value (like OUT, level low) or make it configurable with a keyword argument of the constructor/init.

@th3w
Copy link
th3w commented Feb 23, 2023

A PWM.init() method to start the PWM output and PWM.deinit() to stop it would make this API almost equivalent to the machine.Timer() API, where the timer can also be restarted/reconfigured with Timer.init() and stopped with Timer.denit(). (at least for the rp2 port)

@rkompass
Copy link

From my standpoint it would make sense to have deinit() completely reverse the effects of init(). This would mean the Pin goes back to the state it had before init(), i.e. if it was OUT, level low, then it again would be OUT, low.
If you wanted to go it back to IN automatically, you could set it to IN before init().

Another problem coming into mind:
PWM channels share common timers (slices on RP2). So if you set frequency on one channel you already have it set on another channel. I can imagine 2 approaches to this complication:

  1. Be completely agnostic to it. The programmer has to know it's hardware. This would suggest that the setting of duty in init() is sufficient to start the PWM. Frequency should have a documented default value which is common and compatible with LED and motor control, say 1 kHz (something between 100 Hz and 10 kHz). If a freq value is set, the programmer is aware of the channels that he has set also.
  2. Let MP control the resources somewhat. Once the frequency is set for a channel it is registered for this timer/slice and another attempt to set it to a different value for another channel on this timer raises an exception. This may be avoided with an optional force argument which the programmer uses in awareness of the hardware dependencies.
    This approach would allow the programmer to probe which channels depend on the same timer/slice without studying the hardware datasheets: He could set one frequency for a channel and then another frequency for other channels he is interested in and catch the exceptions. When he had not set the duty this would mean not disturbance on the outputs.

@robert-hh
Copy link
Contributor Author

From my standpoint it would make sense to have deinit() completely reverse the effects of init(). This would mean the Pin goes back to the state it had before init(), i.e. if it was OUT, level low, then it again would be OUT, low.
If you wanted to go it back to IN automatically, you could set it to IN before init().

That can and should be done by the programmer's code. The pin could have been for all possible uses, not only as machine.Pin object.

PWM channels share common timers (slices on RP2).

Yes, and I see it as a feature that all channels on this timer change the frequency simultaneously if it is changed for one channel. So I went & go for option 1.

@dpgeorge
Copy link
Member

When thinking about the uninitialised state it's worth also considering how other peripherals work (or should be made to work). Eg I2C and SPi will configure their pins when constructed and the bus will be idle. ADC will configure it's given pin. DAC is more similar to PWM and I'm not sure exactly how that works.

@robert-hh
Copy link
Contributor Author
robert-hh commented Feb 24, 2023

As far as I recall, all of the modules you mention configure the pins according to the required mode. For DAC the pin is connected to the driver with an initial value, which might be set or not.
For PWM it might be the same, but I have to check all ports. When PWM is not started, I expect output mode and low for normal PWM pins and high for inverted pins. To avoid any confusion, having a start and stop method (or on and off) seems to be more clear. Then init or the constructor would configure and optionally start the PWM, and deinit() releases the resources. The latter may detach the output as well, if the port lib supports it. But setting the pin to a different mode can also be left to the Python script. Having a start method is also helpful to start several outputs of pins connected to the same PWM device with maybe different polarity and duty rate at the same time, like the two channels of a slice of rp2.

P.S.: As the first step I modified already the MIMXRT implementation to not start before freq and duty rate are deliberately set. Maybe @IhorNehrutsa could look at that for the ESP32 port. Next steps would be adding keyword options to the rp2 constructor, adding init() to SAMD, fixing the nrf PWM, and check the behavior of the ESP8266 port. Then the major ports should be comparable. I do not know if init() is required, once a start/stop mechanism exists. Having the constructor with options and init() was always a source of confusion, especially if init() sets default value. But removing init() would be a breaking change. So better let it behave reasonable for now.

@IhorNehrutsa
Copy link
Contributor

I can review the PWM of the ESP32 port.

I am against to add start()/on(), stop()/off() methods.

pwm = PWM(pin, duty_u16=0)    #   If the duty is 0 or 100%, the output has stable level
pwm.start()    #   start() does not change the output level
pwm.stop()     #   and stop() does not change the output level
pwm.start()    #   the output level stay same
pwm.stop()     #   the output level stay same

@IhorNehrutsa
Copy link
Contributor

When a novice user starts PWM,

pwm = PWM(pin)

they expect something to happen.
The ESP32 use 5kHz and 50% duty as default.

@robert-hh
Copy link
Contributor Author

Hi @IhorNehrutsa Thank you for the answer. The first step would be to make the implementations consistent in a way, that PWM does not start until freq and duty are deliberately set. Everything else it t.b.d..
The behavior for deinit()/init() may be next.
About start()/stop() resp. init()/deinit(): Something which is implemented already by chance in the SAMD port and could easily be added to the rp2 port: If deinit() is called, the pin is switched back to GPIO mode, and init() sets it to PWM mode. That way, the programmer can define the inactivity mode & level. The state with duty=0 or 100% is something different.

@IhorNehrutsa
Copy link
Contributor

Returning to GPIO mode when deinit() is not advisable, because you do not know from which mode you came to PWM. It could be, for example, the DAC or something else in general (you won't guess).

@robert-hh
Copy link
Contributor Author

It's not any mode, and the code does not have to guess. It is the mode that was set for the GPIO before by the program. And for me it looks like a good method to control the mode the Pin must have, depending on what is connected.

@IhorNehrutsa
Copy link
Contributor

Setting (and restoring in deinit()) the PWM/DAC Pin(26) as GPIO Pin.IN or Pin.OUT is redundant in this scenario.

from time import sleep
from machine import Pin, PWM, DAC

pin = Pin(26)
while True:
    pwm = PWM(pin, freq=30_000, duty_u16=32768)  # 50% duty ≈ 50% of output voltage after filter
    print(pwm)
    sleep(5)
    pwm.deinit()
    
    dac = DAC(pin)
    print(dac)
    dac.write(127)  # 50% of output voltage
    sleep(5)
    dac.deinit()

Ideally, this code must hold 50% of the output voltage at all times.

P.S.Use ESP32: Add DAC.deinit() method. #10852

@robert-hh
Copy link
Contributor Author
robert-hh commented Feb 25, 2023

Of course code can set the Pin to any other mode after deinit(). Setting it (back) to GPIO would be just the common known behavior, which is supported by all chips and pins.

@IhorNehrutsa
Copy link
Contributor

Let made freq and (duty_u16 or duty_ns) REQUIRED in the PWM constructor and init()

The constructor configures all hardware and starts to work:

PWM(Pin(), freq=f, duty_u16=d)    - on/start immediately
PWM(Pin(), freq=f, duty_u16=0)    - set low level/pause
PWM(Pin(), freq=f, duty_u16=100%)  - set high level/pause

init reconfigure hardware:

init(freq=f, duty_u16=d)  -  set new freq or duty immediately/on/resume
init(freq=f, duty_u16=0)  -  off/stop/pause/set low level 
init(freq=f, duty_u16=100%)  -  off/stop/pause/set high level 

freq get/set

freq()                      - get the frequency
freq(f)                     - set frequency

duty get/set

duty_u16()   or     duty_ns()     -  get the duty
duty_u16(0)  or     duty_ns(0)    -  off/stop/pause/set low level 
duty_u16(100%)  or  duty_ns(100%) -  off/stop/pause/set high level 
duty_u16(d)     or  duty_ns(d)    -  on/start,/resume/set duty

deinit() - free the hardware, allow using Pin with other hardware. The state of Pin is indeterminate after deinit()

@robert-hh
Copy link
Contributor Author

Thank you for you work. That's a good option, however not just like the other ports work. There, you can still omit freq and duty setting in the constructor, only PWM will not start until both are set.
There was no conclusion yet on the Pin behavior before PWM starts and after PWM stopped. The same for the 0 and 100% duty cycle state. Using low and high for the 0 and 100% level seems intuitive. And by chance it's how the ESP32 works.

@IhorNehrutsa
Copy link
Contributor

Let's mandatory require the frequency and (duty_u16 or duty_ns) in the PWM constructor and init()
This excludes created but not yet started PWM's state.
To avoid omitting freq:
https://github.com/micropython/micropython/pull/10854/files#r1118044560
To avoid omitting duty:
https://github.com/micropython/micropython/pull/10854/files#r1118044766

@IhorNehrutsa
Copy link
Contributor

@jonathanhogg, @harbaum may interesting

@robert-hh
Copy link
Contributor Author

I agree that things get more complicated if freq and duty can be set independent. But that's how the other ports work, like rp2, esp8266, mimxrt, samd. It can of course be changed.

@rkompass
Copy link

I see now (and agree) that my suggestions 4 days ago were too complicated.

The first: suggesting that deinit() reverses the effect of init() came from a scenario, that a PWM-routine takes over from a GPIO (or other routine) and with ending should give control back, like in an ISR. For this scenario it would suffice that the state of the pin could be queried and saved somehow. That would make it possible to re-establish that state. But that again sounds very complicated and probably this scenario is too unrealistic to invest into that direction.

The other scenario is probably much more common: The PWM routine supersedes the former control. Then deinit() should just go back to a commonly accepted state, where overtaking control by other tasks is possible. GPIO would be fine. The state that the pin has after booting would be fine too. I assume it is GPIO input. Is that right?
I see no problem with the same state if init() has not received all required arguments yet.
That would mean that the PWM object is just storing it's parameters for further specification, like after deinit().
Perhaps I miss something? Should the existence of such an object in connection with a pin already take over control over the pin to some degree? Or prevent other control? After checking the above discussion again I think that there is consensus against it.

Let's mandatory require the frequency and (duty_u16 or duty_ns) in the PWM constructor and init()
This excludes created but not yet started PWM's state.

I interpret this as a way (and desire) to avoid such a non-controlled state.
There seems to be an alternative that has a similar effect:
What speaks against having a default frequency, of say 1000 Hz? Then the same would be achieved with the duty argument alone:
As soon as it was there the PWM would take over control. If it would not be there or perhaps was revoked with duty=None PWM would stop and give control of the pin back like with deinit().

Being able to setting freq argument alone without starting PWM had one other advantage: One could check the interdependencies of the freq's in the different channels without activating the PWM. Just setting them to different values and retrieving them afterwards (which is possible currently with pwm.freq()) would suffice.

That brings another complication into account: PWM exerts control over the PWM timers too.
Considering all this my tentative suggestion is:

  • The freq argument in init() would set the associated timer.
    deinit() would give this control back, which would not require any action, as the timer control is not locked anyway.
    pwm.freq() should inspect the hardware control registers of the timers and not just return a previously given freq argument.

  • The duty argument would start control of the pin. Revoking it or deinit() would give back control of the pin and set it back to the state like after booting. init() would start controlling the pin again.

Do the duty and freq settings interact? On RP2 with high frequency the duty settings automatically have restricted resolution. This would imho not pose a problem. But if the duty setting would have to change/correct frequency then I would argue like @IhorNehrutsa that only both together should start the PWM. Because then the idea of having a (known) default frequency which may be from the docs or from some previous setting of timers by the freq argument does not make sense anymore.

I checked and noted that the current RP2 behaviour is close to the above suggestion:

from machine import Pin, PWM
pwm = PWM(Pin(8))
pwm.duty_u16(20000)       # --> my LED goes on
pwm.freq()                           # --> 1907, which might be a default?

1907 Hz is also the frequency returned by the above without the duty_u16() setting.

@robert-hh
Copy link
Contributor Author
robert-hh commented Feb 27, 2023

Thank you for the suggestions. Meanwhile I more back to one of your ideas to set the pin back to GPIO mode on PWM deinit(), where possible. In most ports that would be done by switching the AF function of the PIN. So the Pin would be ideally be back in in the configuration id had before. But that may not work for all ports. The ESP8266 for instance has a software PWM with the PIN in GPIO mode. But at least it could be set to GPIO input mode, and user's code can configure the pin at will.

Not starting the PWM until all parameters are set has an advantage. That is useful, if you do not look at single Pin but groups of synchronized pin, which are driven by the same timer. On rp2 for instance these are the two channels at a slice. These pins can then be set up one after the other and then both be started by setting the frequency (not the duty). The MIMXRT and SAMD ports allow more pins and pin configurations to be set. At the SAMD port only the invert was added, even if the hardware allows for finer control. I plan to add at least an invert option to the RP2 port, such that the two channels of a slice can operate in a push-pull manner.

The RP2 port had a bug, which is fixed in my PR, in that it started with a frequency of 1907 Hz once the duty rate was set. That's truly not right.

I do not argue against setting frequency and duty together. I only argue against requiring it.
The first set of commits makes the ESP8266, RP2, SAMD and MIMXRT port behave the same with respect to starting and stopping and independent setting freq and duty cycle. Since the discussion about a dedicated start/stop resp. on/off is and the state of pins is still open, these ports behave different with respect to configuring and releasing pins, except that you can stop PWM with deinit() and restart it with init().

Duty and freq should be independent, besides that fact, that the duty resolution decreases with increasing frequency. PWM is created by counting cycles of a base frequency, which is not infinite. So on changing the frequency the duty cycle should stay the same. Whatever the same means. When set by duty_u16(), it is the ratio between the first section and the full period. If set by duty_ns(), then it is the duration of the high section. Note that the ESP8266 does fail in the latter case.

@jonathanhogg
Copy link
Contributor

I'm late to this party, and I'm not sure if I have much add beyond my own preferences.

If it was me making the calls on a standard API, I'd argue against being able to construct a PWM() object that is not immediately usable. The current docs even give this example of using PWM right at the top of the page:

from machine import PWM

pwm = PWM(pin)          # create a PWM object on a pin
pwm.duty_u16(32768)     # set duty to 50%

# reinitialise with a period of 200us, duty of 5us
pwm.init(freq=5000, duty_ns=5000)

pwm.duty_ns(3000)       # set pulse width to 3us

pwm.deinit()

If PWM(pin) creates an object that is not immediately usable then it raises the question of what happens if you try to? Should the above code throw an exception on the first call to .duty_u16()? If it silently does nothing then that feels insanely confusing for users. If it's going to throw an exception there, why not in the constructor?

I'd say just have freq default to some port-specified, reasonable value – it allows a beginner to quickly make an LED glow without having to worry about what frequency they should use to do this. As far as I can make out, SPI() operates in the same fashion with regard to baudrate. If I don't set it then it presumably means that I either don't know or don't care what it should be. Having duty also default to 0 means that the PWM will be doing nothing anyway.

In the case of deliberately wanting to half-initialise PWM to make use of port-specific timer behaviour, then that is surely better achieved through a more powerful API. Ideally, I'd say that a complete PWM interface would expose the timer machinery underneath and allow direct control of this. My go-to comparison here would be how ADCBlock() is visible underneath the higher-level, easy-to-use ADC() API.

In terms of deinit(), I'd have it simply unhook the pin from the PWM hardware and return it to some undefined "uninitialised" state. What this is should be port-specific and users should almost certainly be discouraged from doing anything other than immediately reinitialising the pin into a more defined state.

I would argue that what happens to the duty when you reinitialise a PWM frequency with init() should also be undefined – it's too esoteric an edge case to be worth putting the effort into making all the ports do the same thing. If a user really needs to change frequency on-the-fly for a specific application then they probably should be simultaneously re-specifying the duty cycle.

In general, my thinking is: provide users who know what they they need to do with the ability to explicitly control the hardware, while holding the hands of users who are just starting out.

@rkompass
Copy link

These pins can then be set up one after the other and then both be started by setting the frequency (not the duty).

Do you mean by this that setting the frequency in one channel of the slice counts as having set the frequency in both channels?
I would argue for it, because by hardware it would be so anyway. Then of course it should be noted and stated (in the docs) that both arguments freq and duty are required (and start PWM), where freq may be set indirectly through a timer-related channel. I had a look at the RP2040 datasheet and there is the PWM enable bit (EN) that gates the fractional clock divider. Setting one frequency would then set EN and activate both channels of the slice truly in sync.
Only both the direct or indirect freq and the duty setting would lead to switching the pin multiplexer (i.e. the AF) to PWM.

How about deinit()? It would switch the AF back. How would the PWM enable (EN) bit be set to 0 again? At the point when both (all) channels of the timer/slice are deinit'ed? Or not at all? In the second case the truly synchronous PWM start would be possible only once. In the first case one could deinit() all channels of a timer/slice and then start again in (perfect synchrony) with setting duty, inversion perhaps and then common freq.
I will have to have a look at your code.

Duty and freq should be independent, besides . . .

This would require that the duty_u16 or duty_ns setting is held in storage and is adjusted every time the freq argument changes. Whereas the freq argument could be in the PWM dividers + counters registers alone, and would be retrieved from there. .?.

@rkompass
Copy link

Having just read @jonathanhogg s arguments I would agree (and it was my idea too), that default frequencies help the naive user who just wants to set a LED to a certain brightness or a motor to 40% speed.

Trying to save the nice synchronous start idea. What about the possibility of setting freq=0 which would allow to specify the halting of the common counters which would mean the enable bit (EN) in the RP2040 case could be set to 0?
That would not hurt the naive approach.

@robert-hh
Copy link
Contributor Author

@jonathanhogg I do agree that the PWM shall work as specified. And specified means both freq and duty rate set. If the documentation is misleading, it can be changed. In fact the PR I made includes changes to the documentation showing the constructor setting both freq and duty rate.
Interfaces like SPI and UART set a default baudrate to commonly used values. These do not exist for PWM, except maybe 50Hz for servos, but then, at which duty cycle? This depends heavily on the peripheral attached to the device.

About init() without arguments to re-enable a PWM with previously set value: at the moment that is partly hijacking the init() function, lacking start()/stop() methods. But it also meets another design goal in that init() shall only parameters which are mentioned and shall not set default values or implicitly change the behavior. Setting silently default values cause(d) people a lot of trouble. So if you use init() to change just the frequency, the duty cycle stays what it was. That is and was already implemented. So pwm.init(freq=xyz) is the same as pwm.freq(xyz), sometimes literally, sometimes effectively.

@rkompass Yes, at the RP2 both channels of a slice have the same frequency, but can have different duty cycles and polarity. If you change the frequency on one channel, it changes on both channels. But that depends a lot on how the frequency is generated.

This would require that the duty_u16 or duty_ns setting is held in storage and is adjusted every time the freq argument changes. Whereas the freq argument could be in the PWM dividers + counters registers alone, and would be retrieved from there. .?.

The PWM modules of the MCUs are different. Some hold something like a frequency value, some hold something like the duty_ns value, or a mix of both. And yes, for being able to keep the duty when the frequency is changed these values must be held in storage, typically at the PWM object. And sometimes that is the most complicated part of the code, especially if several PWM outputs are affected by a common element of the PWM hardware, like the clock pre-scaler in SAMD devices.

The behavior after deinit() and before fully configuring the PWM object is to be defined and I did not make any effort yet to harmonize that. At the moment, most port leave the pin in the state it was, just the PWM signal is stopped or swítched off. And I agree @jonathanhogg that a general solution is impossible to find and that can be left to user code. If deinit is used as stop() to be restarted again: fine. If the device is going to be used otherwise, then the user code can reconfigure the port. Still, something like start()/stop() would be helpful, to make a difference between stop() and deinit(). The latter can then release any resources held by the PWM, if required. At an early state of the initial MIMXRT PR I had keyword start=True|False option to the constructor and init(), which would work as a start()/stop() mechanism. with start=True the PWM would start if fully configured, with start=False it would either stop or not start, even if fully configured. But that did not make it into the release.

If you look at PR #10850 you see, that the basic functionality of each port is unchanged. Only the external behavior is harmonized. Especially the rp2 port, which is kind of the reference for the group, always required freq() and duty to be set separately, and for consistency, init() and keyword arguments for the constructor was added. The MIMXRT port for instance used to start with default parameters (1000Hz, 50%) when a object was created without parameters. The SAMD port just had the init() method added. And at the esp8266 just the two other duty setting options were added.

@IhorNehrutsa
Copy link
Contributor
IhorNehrutsa commented Feb 27, 2023

I have questions about the invert argument in the mimxrt PWM from the link
https://docs.micropython.org/en/latest/mimxrt/quickref.html#pwm-pulse-width-modulation

classPWM(dest, freq, duty_u16, duty_ns, *, center, align, invert, sync, xor, deadtime)
....
invert=True|False channel_mask. Setting a bit in the mask inverts the respective channel. 
Bit 0 inverts the first specified channel, bit 2 the second. The default is 0.
  1. Is bit 1 invert any channels?
  2. Is the second channel complimentary from the pwm2 example?
pwm2 = PWM((2, 3), freq=2000, duty_ns=20000)
  1. Does "invert=5" invert both channels?
pwm2 = PWM((2, 3), freq=2000, duty_ns=20000, invert=5)     

Thanks.

@robert-hh
Copy link
Contributor Author

Bit 0 inverts the first specified channel, bit 2 the second. The default is 0.
There might be a typo in the documentation. It probably should read:
Bit 0 inverts the first specified channel, bit 1 the second. The default is 0.
It is a pure bit mask, and independent from the complementary channel definition.
Complementary channels are specified by providing a two-element tuple of pins.

' pwm2 = PWM((2, 3), freq=2000, duty_ns=20000, invert=3)'

Would invert both signals (bit 0 and bit 1 set). This is not related to the pin numbers. For assignment of pins to PWM modules and channels you have to look at the pin assignment documentation. In case of Teensy 4.x, Pin 2 is connected to module 4, submodules 2, channel A, and Pin 3 to channel B. That make it a candidate for a complementary channel pair.
See https://docs.micropython.org/en/latest/mimxrt/pinout.html#pwm-pin-assignment

@IhorNehrutsa
Copy link
Contributor

@rkompass
In fact, you discuss not the PWM entity, but the H-bridge entity.
Let don't push H-bridge properties and features into PWM.
Let's stay short, simple, and clear in PWM class
pwm =PWM(Pin(p), / freq=f, duty_x=d, invert=0)
pwm.init(
/ freq=f, duty_x=d)
pwm.freq(f)
pwm.freq()
pwm.duty_x(d)
pwm.duty_x()
pwm.deinit()

Let's make H_bridge class with as full functionality as possible.
h_bridge = H_bridge((Pin(p1, mode=OUT, value=1), Pin(p2, mode=OUT, value=0), Pin(....), ....), freq=f, duty_x=d, death_time, align, sync, active, .... etc ...)
h_bridge.freq()
h_bridge.duty()
h_bridge.start()
h_bridge.stop()
h_bridge.deinit()
etc..

H_bridge: Reduce inconsistencies between ports. #10983

@cve2022
Copy link
cve2022 commented Mar 8, 2023
The RP2040 also has an align feature. It is called the phase-correct mode.

I have seen it. As far as I understand it, that could be used in combination with invert to get a deadtime feature. It could be used always, with the frequency halved. That can easily be adapted.

I hope it is OK an outsider/non-developer entering the discussion or that what follows is not too OT. About synchronization of the PWM slices of the RP2040, I did not understand the functioning of the align feature of the RP2040 when I looked at this matter a while ago (I needed six slices synchronized in my project). I used a different approach, stopping/starting the counters via the global enable registers and zeroing the counters. As it solved my problem, I didn't dig deeper.

The codes below are the two ways I used to do it; they are very crude as they are proof-of-concept only.

HTH.

from micropython import const
from machine import Pin, PWM, mem32

FREQ = const(10000)
DUTY = const(2**15)
PWM_CTR = const((
    0x40050008, 0x4005001c, 0x40050030, 0x40050044,
    0x40050058, 0x4005006c, 0x40050080, 0x40050094
))
GLOBAL_EN = const(0x400500a0)
GLOBAL_EN_SET = const(0x400520a0)
GLOBAL_EN_CLR = const(0x400530a0)
PWM_SET = const(0xFF)

pins = (0, 2, 4, 6, 8, 10, 12, 14)
pwm = [PWM(Pin(i)) for i in pins]
[i.freq(FREQ) for i in pwm]
mem32[GLOBAL_EN_CLR] = PWM_SET
for i in PWM_CTR: mem32[i] = 0
mem32[GLOBAL_EN_SET] = PWM_SET
[i.duty_u16(DUTY) for i in pwm]
from micropython import const
from machine import Pin, PWM

FREQ = const(10000)
DUTY = const(2**15)
PWM_CTR = const((
    0x40050008, 0x4005001c, 0x40050030, 0x40050044,
    0x40050058, 0x4005006c, 0x40050080, 0x40050094
))
GLOBAL_EN = const(0x400500a0)
GLOBAL_EN_SET = const(0x400520a0)
GLOBAL_EN_CLR = const(0x400530a0)
PWM_SET = const(0xFF)

pins = (0, 2, 4, 6, 8, 10, 12, 14)
pwm = [PWM(Pin(i)) for i in pins]

@micropython.viper
def init (pwm):
    [i.freq(FREQ) for i in pwm]
    ptr32(GLOBAL_EN_CLR)[0] = PWM_SET
    for i in PWM_CTR: ptr32(i)[0] = 0
    ptr32(GLOBAL_EN_SET)[0] = PWM_SET
    [i.duty_u16(DUTY) for i in pwm]

init(pwm)

@rkompass
Copy link
rkompass commented Mar 8, 2023

@IhorNehrutsa

Let's stay short, simple, and clear in PWM class
pwm =PWM(Pin(p), / freq=f, duty_x=d, invert=0)
pwm.init(/ freq=f, duty_x=d)
pwm.freq(f)
pwm.freq()
pwm.duty_x(d)
pwm.duty_x()
pwm.deinit()

I'm pro simplicity from a users perspective. And I thought to have argued in that direction. All the above commands should work as expected with my last suggestion (provided it can also be implemented without too much complication, of course).
My intention was to suggest a simple API that also allows for more complex setups.
Behind the simple API there should still be an exact understanding of what happens internally. Therefore the 3 levels.
I see now that I mismatched level 2 and 3. At least on RP2 counters already are common for the slice - so their start/enable should be level 2.

In fact, you discuss not the PWM entity, but the H-bridge entity.

Both PWM and H-bridge functionality clearly overlap. If you have sign-magnitude H-bridge control then you need an ordinary PWM and a GPIO for direction, for instance.
For lock anti-phase H-bridge mode you need off-center alignment of two channels with duty restricted so that a minimal gap is guaranteed. This is H-bridge functionality but why should PWM be restricted therefore?
One also could imagine such an alignment (90° phase) that a quadratic code is provided - why should we forbid it?

Let's make H_bridge class with as full functionality as possible.

I agree. Definitely nice to have. Should it be implemented internally - I don't know. Could perhaps be easier to make it available as a class derived from PWM with the alignment features included.

But ESP32 has special separate hardware for H_bridge - Motor Control Pulse Width Modulator (MCPWM)

Would be nice to have accessible from MP, of course. Overlapping functionality offered is no bad thing imho. Perhaps someone needs Motor control and has to provide aligned PWM at the same time...

The basic question imho is whether alignment of channels should be an issue/topic for PWM at all.
Of course it would be simpler to restrict the functionality completely to the above functions - from the MP-developers perspective. Keeping alignment out. Would make it harder for a user who needs that. I do not argue here, as I do not have to implement all that. Many thanks to Robert and all who implement(ed) PWM on the different platforms.

@jonathanhogg
Copy link
Contributor

I'm a big fan of a simple, common PWM API available across all ports. However, where the hardware for a particular port can go beyond this, it makes sense to expose that additional functionality.

If H-bridge functionality is part of the PWM peripherals in RP2040 then I would guess the machine.PWM() class is the natural place to expose it. If it's a different peripheral on ESP32, then a new class in esp32 would probably make sense.

If we then want to have a cross-port H-bridge API, the obvious thing to do would be to put per-port, pure-Python wrappers around these in new machine.HBridge() (or machine.Motor()?) classes that provide a common API – which itself, of course, may not be the full functionality offered by the ports.

These feel like separable problems to me.

(And where I've been trying to go with esp32.PCNT()/machine.Counter() should that ever see the light of day…)

@robert-hh
Copy link
Contributor Author

At the moment my attempt is to make the existing machine.PWM implementations compatible at the set of methods @IhorNehrutsa mentioned. We are close to that. Only pwm.duty() differs, as it always did. But that method is available at the moment for legacy and is supposed to be dropped sometime.
Beyond the common set of methods, the implementations differ slightly in additional hardware specific options that can be enabled with keyword arguments of the constructor/init(). And the internal dependencies between PWM output, like being able to run at different frequencies, duty cycles, polarity, ... differ between ports. That will continue to exist and must be documented.
H-Bridge support is possible by quite a few of these platforms, some basic, some sophisticated. If a class is to be designed, the implementation can be port-specific, Some as C-Class, some as Python class. The latter can be made "built-in"! by freezing it into the code.

@robert-hh
Copy link
Contributor Author

@dpgeorge @IhorNehrutsa @rkompass @jonathanhogg @cve2022

Two topics to discuss for the first step.

  1. Names: Most MCU have PWM peripheral with several outputs. The peripheral determines the frequency, and the output section the duty rate and polarity. So lets call the peripheral a "device", and the output path "channel". Some devices like the ESP32 or nrf52xxx family allow free assignment of GPIO pins to channels, for others like rp2, MIMXRT or SAMD it is more or less fixed, even if there may be few alternatives. The ESP32 has the specialty that the assignment of channels to timers (=devices) can be configured as well. But that's another story, making the ESP32 PWM implementation tricky. Does anyone object against using the terms "device" and "channel" at the PWM constructor if they allow to configure their relation to a GPIO pin?
  2. pwm.deinit(): That behavior is still inconsistent. Some ports like the ESP32 completely release the PWM machinery from the GPIO and stop the PWM, some like the SADM just detach the ouput, some stop the PWM device but keep the configuration. In any case, the output of the channel and sometimes the device is stopped, and can in most cases restarted with pwm.init() without arguments. So it should behave consistent both from the "visible" (signal at the pin) and "hidden" (state of the PWM peripheral) aspect. I would go for a complete deinit(), if there were a stop() method, or however it's called. But that must be implemented in extmod/machine_pwm.c, which then calls the respecitive functions in the port implementations. For some ports like ESP8266 or nrf51xxx, stop() may be the same as deinit(). For symmetry, a start() could exist as well. Most ports have an internal function like that anyhow, which is called after having collected the configuration. So it's not a big deal.

@rkompass
Copy link

Point 1: Agree.
Point 2: I still see the need to distinguish levels of activity, at least for clarifying the discussion. In corrected order now this would be:

  1. The PWM class instance is created, duty, frequency, possibly phase and alignment may be configured.
  2. A: The device is enabled. B: The channel counters are enabled.
  3. The respective pins are switched to output-PWM. PWM fully operates.

For example, if pwm.start() and pwm.stop() were introduced should they switch between levels 2B <--> 3 or 1<--> 3 ?

@rkompass
Copy link

----- A remark on H-bridge ----

Working on H-bridge driver yesterday I see a desire/need of either:
A: Have a way to simply/swiftly (< 1ms) switch between a defined constant level of a pin and PWM-signal on that pin. or
B: Have a defined low level at pwm.duty(0)

To illustrate this:

#  ----   Circuitry and Setup for L298  -----
#
#  in2 | in1 | enA
# -----------------
#   X  |  X  |  L    Stop   (free running motor stop, i.e. high impedance)
# -----------------
#   L  |  L  |  H    Brake  (fast motor stop, motor outputs connected)
#   L  |  H  |  H    Forward
#   H  |  L  |  H    Reverse
#   H  |  H  |  H    Brake  (fast motor stop, motor outputs connected)

To make L298 driver behaviour identical to that of TB6612 driver I will rewrite the H-bridge driver so that we have pwm signals at inputs in1 and in2.
For Forward mode in1 would get an active PWM signal, in2 be low.
For Reverse mode in1 would be low and in2 get an active PWM signal.
This way in either direction the PWM would switch between driving and braking which is better (more defined, and current-preserving than switching between driving and stopping/free-running. As far as I understand it. So it is on the TB6612 by hardware anyway. It also allows to get rid of the third enable signal btw. (set it by jumper as on the L298 photos).
I first was thinking whether pwm.deinit() could just switch back to GPIO mode resuming a Pin.OUT mode previously set. Probably this is unrealistic - just to mention.
Option B (have a defined low level at pwm.duty(0)) would be fine though. I know this is the case for RP2.

@robert-hh Do you know if we have a defined 0-level at pwm.duty(0) currently on the other hardware too?

@robert-hh
Copy link
Contributor Author
robert-hh commented Mar 12, 2023

@rkompass About steps 1 to 3. step 1 includes the allocation of the resources as well. For steps 2 an 3, it's hardware dependent how much the steps are separated. And they are usually handled in a single function. At SAMD for instance there is some leakage in a stopped channel from other channels at the device still working. Therefore the output is detached from the PWM device.

About Pin state after deinit(), given that there is a stop method(). My favorite is to switch the pin back to GPIO mode, hoping it will be in the state it was left. Alternatively, it could be switched to Input mode, leaving the desired level, if it is critical, to external pull resistors.

For the pin state after stop() I would favor low level, unless it is made configurable.

I did not test whether all ports have a low level at duty(0). It may as well depend on the invert=True/False setting. Which is the case for RP2 as well (just tested) and SAMD.

Edit:
So RP2 (with new PR), MIMXRT, nrf52xx, SAMD and ESP32 (with the new PR) all have the output at 0 with duty=0 and invert=0, and at 1 for duty=0 and invert=1.

@rkompass
Copy link

So RP2 (with new PR), MIMXRT, nrf, SAMD and ESP32 (with the new PR) all have the output at 0 with duty=0 and invert=0, and at 1 for duty=0 and invert=1.

#  ----   Circuitry and Setup for DRV8871  -----
#
#   in2 | in1 
#  -----------
#    X  |  X     
#  -----------
#    L  |  L     Stop / Coast (free running motor stop, i.e. high impedance)
#    L  |  H     Forward
#    H  |  L     Reverse
#    H  |  H     Brake  (fast motor stop)
#
#  This is called IN-IN interface in the Datasheet

Nice, so also this chip / interface version can be included in the H-bridge module by using the invert mode.

@rkompass
Copy link

I assume now that the PWM may at any time be switched from inverted to normal mode, with pwm.init() like in:

# motor.go(80)          # Forward, 80% speed
# motor.go(0)           # Brake,  (both motor outputs connected to GND)
#   or motor.go(motor.Brake)
# motor.go(-100)        # Backward, full speed
# motor.go(None)        # Stop, (both motor outputs high impedance)
#   or motor.go(motor.Stop)
#
def go(self, speed):
    if speed is not None:            # Other than Stop mode: Enforce inverted PWM mode.
        self.pwm2.init(duty_u16 = min(int(speed*655.35), 65535) if speed > 0 else 0, invert=True)  # We rely on the pin being high at
        self.pwm1.init(duty_u16 = min(int(-speed*655.35), 65535) if speed < 0 else 0, invert=True) #   duty_u16(0), with invert=True.
    else:       # We set the Stop mode by setting both pins=low and have to leave the inverted PWM mode for that.
        self.pwm1.init(duty_u16 = 0, invert=False)  # We rely on the pin being low at
        self.pwm2.init(duty_u16 = 0, invert=False)  #   duty_u16(0), with invert=False.

@robert-hh
Copy link
Contributor Author

I assume now that the PWM may at any time be switched from inverted to normal mode, with pwm.init()

That's the intention, and verified for RP2, MIMXRT, SAMD and NRF. ESP32 accepts the invert keyword only at the constructor. But that is WIP. Also RP2, MIMXRT and NRF have the output at 1, if duty_u16 is set to 65536. MIMXRT shows this at 65535, since it rejects 65536, which is kind of wrong, since the PWM input clock is 150Mhz. ESP32 still has a short pulse of 1-2 clock cycles, depending on the frequency set. That seems technically correct. But you cannot set 65536.

@IhorNehrutsa
Copy link
Contributor

The ESP32 uses the same code for the constructor and init()

pwm = PWM(Pin(23), freq=10_000, duty_u16=6_000)
print(pwm)
pwm.init(invert=1)
print(pwm)

Output is:

PWM(Pin(23), freq=10000, duty_u16=6000)  # resolution=12, (duty=9.16%, resolution=0.024%), mode=0, channel=0, timer=0
PWM(Pin(23), freq=10000, duty_u16=6000, invert=1)  # resolution=12, (duty=9.16%, resolution=0.024%), mode=0, channel=0, timer=0

@robert-hh
Copy link
Contributor Author
robert-hh commented Mar 14, 2023

Yes, I know. But if you apply invert at init(), that waveform does not change. It changes only if you set invert=1 at the constructor. That's what I meant to say.

@rkompass
Copy link

Thank you.
If/because the inversion may be set forth and back at every duty setting like in the above code example there is no urgent need for exact behaviour at duty_u16(65535) or duty_u16(65536) anymore.
Of course it would be nice to have agreement here too. The above details are worth to be kept in the docs somewhere..
For H-Bridge programming it's sufficient now to be able to rely on duty_u16(0).

@robert-hh
Copy link
Contributor Author

Addendum: For MIMXRT the behavior at 65535 depends on the frequency. E.g,. at 2889 Hz it has the best duty resolution, and then at 65535 it shows a short ~7ns pulse, which is 1 clock cycle.
At 1kHz, which I used before, the PWM counter clock is 37,5 MHz, and the best duty resolution is 1/37500.

@robert-hh
Copy link
Contributor Author

I changed the MIMXRt port to accept 65536 as duty_u16 value. With that, the output is high even at a frequency with the best duty resolution (e. g. 2889 Hz).

@rkompass
Copy link

Trying to add a __repr__() function to the HBridge class I have the problem to extract Pin information from the PWM object.
Also it's not clear to me, what this function is about. Shouldn't it give a representation of how the object may be recreated?
I was thinking of:

in1, in2 = PWM(Pin(2)), PWM(Pin(3))
enA = Pin(4)
motor = HBridge(in1, in2, en=enA)
repr(motor)

returning: 'HBridge(PWM(Pin(2)), PWM(Pin3)), en=Pin(4))'

But the PWM object representation differs on the different architectures.
Two examples:
On RP2:
pwm = machine.PWM(machine.Pin(2))
repr(pwm) --> '<PWM slice=1 channel=0 invert=0 freq=0 duty=0>'

On MIMXRT:
pwm = machine.PWM(machine.Pin(2))
repr(pwm) -->'<FLEXPWM module=4 submodule=2 channel=A duty_u16=32768 freq=1000 center=32768, deadtime=0, sync=0>'

For RP2 I have the following function to extract the pin number:

def pwm_pin(o):
    slic = int(str(obj).split(' ')[1].split('=')[1])
    chan = int(str(obj).split(' ')[2].split('=')[1][0])
    return(2*slic + chan)

But on other architectures other functions would be needed. Handling this seems very expensive. Perhaps more expensive than coding the present functionality of the HBridge class.

@robert-hh: Do you have an idea, how to proceed with__repr__() ?

@robert-hh
Copy link
Contributor Author

Do you have an idea, how to proceed with__repr__()

No. this is a big mess not only for the PWM class and need to be harmonized. MIMXRT is worse since there are two types of PWM peripherals: FLEXPWM and QTMR. Even for RP2 the pin number you get may be wrong, because the same slice/channel is available at two pins. So maybe it's better to supply the Pin object instead of the PWM objects.

@rkompass
Copy link

I switched to PWM objects because then there is no need of deinit(). Deiniting the PWM objects is left to the user. The HBridge class only specifies parameters of pins and PWM.
Also I combined two H-Bridge interface modes by checking the type of argument Pin or PWM.
See the code at #10983.
I feel like being in need of a feedback whether this is o.k.. I have to test the code yet, of course.
The __repr__() will not be available.

In general I would opt for a pin_id() function for pins as well as PWM.

@robert-hh
Copy link
Contributor Author
robert-hh commented Mar 17, 2023

The output of print for any object has to be reconsidered. There is total chaos. It should be uniform. One idea I had is to make it in the form, an object would be created. So repr of a pin for RP2 would be:
2, mode=Pin.OUT, pull=Pin.PULL_NONE
For PWM (since we're at it):
Pin(2), freq=1234, duty_u16=8192, invert=0
Whether optional information is to be supplied has to be discussed. But that will be quite a bit of work across the ports. And as a breaking change that is to be done for a version 2.0.0.

@rkompass
Copy link
rkompass commented Mar 17, 2023 via email

@IhorNehrutsa
Copy link
Contributor
IhorNehrutsa commented Jul 19, 2023

Where is the updated PWM API documentation located?
Is https://docs.micropython.org/en/latest/library/machine.PWM.html out of date?

Can someone explain the pin states(input/output mode/attached/detached/levels/???) and pwm timer states(inited/deinited/???) at this sequence in new PWM API?

from machine import PWM, Pin  # PWM timer deinited,  Pin detached, Pin hold previous state(input no pull-up, no pull-down) after reset
pwm.PWM(Pin(19))  # PWM timer ???, Pin ???
pwm.PWM(Pin(19), freq=1250, duty_u16=2**16//4, invert=1)  # PWM timer inited,  Pin output work
pwm.PWM(Pin(19))  # PWM timer ???, Pin ???
pwm.deinit()   # PWM timer deinited,  Pin detached, Pin state is indeterminate(undefined)

IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this issue Feb 19, 2025
According to the: micropython#10817.

Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this issue Feb 19, 2025
According to the: micropython#10817.

Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this issue Feb 20, 2025
According to the: micropython#10817.

Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
IhorNehrutsa added a commit to IhorNehrutsa/micropython that referenced this issue Mar 13, 2025
According to the: micropython#10817.

Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
dpgeorge pushed a commit to IhorNehrutsa/micropython that referenced this issue May 16, 2025
This reduces inconsitencies between esp32 and other ports.

According to the discussion in micropython#10817.

Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests, new feature implementations extmod Relates to extmod/ directory in source
Projects
None yet
Development

No branches or pull requests

7 participants
0