-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Comments
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 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
Yes, that would be consistent with all the other |
init() without arguments can (and does on some ports) restart PWM again, after it has been stopped by deinit(). |
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) |
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. Another problem coming into mind:
|
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.
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. |
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. |
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. 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. |
I can review the PWM of the ESP32 port. I am against to add start()/on(), stop()/off() methods.
|
When a novice user starts PWM,
they expect something to happen. |
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.. |
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). |
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. |
Setting (and restoring in deinit()) the PWM/DAC Pin(26) as GPIO Pin.IN or Pin.OUT is redundant in this scenario.
Ideally, this code must hold 50% of the output voltage at all times. P.S.Use ESP32: Add DAC.deinit() method. #10852 |
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. |
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:
init reconfigure hardware:
freq get/set
duty get/set
deinit() - free the hardware, allow using Pin with other hardware. The state of Pin is indeterminate after deinit() |
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. |
Let's mandatory require the frequency and (duty_u16 or duty_ns) in the PWM constructor and init() |
@jonathanhogg, @harbaum may interesting |
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. |
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 interpret this as a way (and desire) to avoid such a non-controlled state. 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.
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:
1907 Hz is also the frequency returned by the above without the duty_u16() setting. |
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. 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. |
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
If I'd say just have 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 In terms of I would argue that what happens to the duty when you reinitialise a PWM frequency with 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. |
Do you mean by this that setting the frequency in one channel of the slice counts as having set the frequency in both channels? 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.
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. .?. |
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? |
@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. 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.
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. |
I have questions about the invert argument in the mimxrt PWM from the link
Thanks. |
' 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. |
@rkompass Let's make H_bridge class with as full functionality as possible. H_bridge: Reduce inconsistencies between ports. #10983 |
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.
|
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).
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.
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.
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. |
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 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 These feel like separable problems to me. (And where I've been trying to go with |
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. |
@dpgeorge @IhorNehrutsa @rkompass @jonathanhogg @cve2022 Two topics to discuss for the first step.
|
Point 1: Agree.
For example, if pwm.start() and pwm.stop() were introduced should they switch between levels 2B <--> 3 or 1<--> 3 ? |
----- A remark on H-bridge ---- Working on H-bridge driver yesterday I see a desire/need of either: To illustrate this:
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. @robert-hh Do you know if we have a defined 0-level at pwm.duty(0) currently on the other hardware too? |
@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: |
Nice, so also this chip / interface version can be included in the H-bridge module by using the invert mode. |
I assume now that the PWM may at any time be switched from inverted to normal mode, with
|
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. |
The ESP32 uses the same code for the constructor and init()
Output is:
|
Yes, I know. But if you apply invert at |
Thank you. |
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. |
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). |
Trying to add a
returning: But the PWM object representation differs on the different architectures. On MIMXRT: For RP2 I have the following function to extract the pin number:
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 |
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. |
I switched to PWM objects because then there is no need of In general I would opt for a pin_id() function for pins as well as PWM. |
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: |
In the HBridge class there would be no need of deinit, as all passed objects would still be accessible by the caller.
I.e. if you instantiate motor = HBridge(pwm1, pwm2)
and you don't need motor any more, you can del motor.
Afterwards you still have pwm1 and pwm2 (if you did not forget them).
So if you want to use the pins behind them, you can do pwm1.deinit()
and pwm2.deinit() and the pis are switched back to GPIO.
If the api of HBridge were motor = HBridge(pin1, pin2)
and the pwm1 = PWM(pin1) and pwm2 = PWM(pin2) would happen internally
then after deleting motor the pin objects pin1 and pin2 would not be usable, because they are
changed to PWM alternative function. It would be correct to deliver a motor.deinit() function that would
return the pins to the state they were in when they were passed to the HBridge initializer.
Gesendet: Donnerstag, 16. März 2023 um 22:55 Uhr
Von: "cve2022" ***@***.***>
An: "micropython/micropython" ***@***.***>
Cc: "rkompass" ***@***.***>, "Mention" ***@***.***>
Betreff: Re: [micropython/micropython] PWM: Reduce inconsitencies between ports. (Issue #10817)
I switched to PWM objects because then there is no need of deinit(). Deiniting the PWM objects is left to the user.
I did not follow this. Would deinit() be eliminated from the interface?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
Where is the updated PWM API documentation located? 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?
|
According to the: micropython#10817. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
According to the: micropython#10817. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
According to the: micropython#10817. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
According to the: micropython#10817. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
This reduces inconsitencies between esp32 and other ports. According to the discussion in micropython#10817. Signed-off-by: Ihor Nehrutsa <Ihor.Nehrutsa@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
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:
Fix the nrf port.Looks too strange.The text was updated successfully, but these errors were encountered: