Basic MCPWM support#5818
Conversation
| from machine import Pin | ||
| from esp32 import MCPWM | ||
|
|
||
| pwm0 = PWM(0) # Create MCPWM object with timer ID (0..5) |
There was a problem hiding this comment.
Should be MCPWM not PWM.
Also the docs (and MCPWM_TIMER_MAX in mcpwm.h) says that it can be 0,1,2
There was a problem hiding this comment.
Agreed, should be MCPWM!
However, the MCPWM object abstracts both ESP32 MCPWM units, where timer ids 0..2 are on unit 1, and ids 3..5 on unit 2
| STATIC mp_obj_t mcpwm_make_new(const mp_obj_type_t *type, size_t n_args, size_t n_kw, const mp_obj_t *args) { | ||
| mp_arg_check_num(n_args, n_kw, 1, 3, true); | ||
|
|
||
| int block_id = mp_obj_get_int(args[0]); |
There was a problem hiding this comment.
What is a "block" ? I don't see this defined anywhere in the docs.
|
|
||
| // BIND | ||
| STATIC mp_obj_t _mcpwm_bind(mp_obj_t self_in, mp_obj_t pin) { | ||
| mcpwm_obj_t *self = MP_OBJ_TO_PTR(self_in); |
There was a problem hiding this comment.
MCPWM deals in pairs of pins (A & B). From reading the docs it seems like this API should be based around units (there are two), each of which has three pairs, which can each be bound to a two pins and one of the three timers.
i.e. the basics would need to
- create a MCPWM instance for a given unit
- bind each of its pairs to a (timer, pina, pinb)
- set frequency on a timer (or maybe on a pair, which implicitly sets it on the attached timer)
- set duty cycle on an output (which I think can be A or B individually)
I realise this PR is aiming to implement the simplest possible thing that works, but ideally it should be done in a way that can be extended to the full functionality.
|
|
||
|
|
||
| // BIND | ||
| STATIC mp_obj_t _mcpwm_bind(mp_obj_t self_in, mp_obj_t pin) { |
There was a problem hiding this comment.
There's a few places that do this, but generally I think the convention is to not have the leading underscore for functions like this.
jimmo
left a comment
jimmo
left a comment
There was a problem hiding this comment.
Thanks for the contribution!! I've seen this mentioned a few times so I'm sure there'll be some people keen to see this implemented.
I agree with @pidou46 that maybe this also makes sense as a loadable module (I'm not really sure the long-term plan here for ESP32).
Just some questions on the API, mostly with regard to how it will be extended to support more functionality from this peripheral.
|
Thank you very much, @pidou46 and @jimmo, for your feedback. I agree that the current way of exposing the API is probably not very sustainable. Basically, I wrapped the MCPWM methods to presents them as "just another PWM module" (which was what I needed at the time). While my project I used this MCPWM module for is mostly done, I'm happy to improve on this PR in order to make it usable for someone else. However, I'm afraid I won't find the time soon to implement a new and better wrapper API based on the four steps you mentioned. Mostly, because I no longer have the hardware with the oscilloscopes etc. available. How do you suggest I'd go on with this? Probably, such a preliminary MCPWM support would better be published as an external module? |
|
@dpgeorge Should apply the esp32 label... |
|
@bskp nice job done here, any update on this ? |
|
Hi @xky183, thank you for your interest on the MCPWM feature. Unfortunately, I no longer have access to the hardware I used to work on this MCPWM draft. However, the fork implementing this feature – https://github.com/bskp/micropython_esp32_mcpwm – remains functional. Since it is based on Micropython v1.11, it's lacking any Micropython features/bugfixes which have been added since. I hope this can get you started! |
|
@IhorNehrutsa Great to see, thank you for picking up the work! |
|
Thanks @bskp for contributing this and @IhorNehrutsa for picking it up. Will close this out in favour of the newer #12345. |
9 participants
Following up to micropython/micropython-esp32#94, I implemented a basic wrapper to support the ESP32's MCPWM module.
Would it make sense to include such a "basic" implementation to micropython?
Since this is my first pull request, I'm happy about any feedback!