Add ESP32 PWM support, mirroring the API of the ESP8266. #79

vandys · 2017-05-07T22:19:28Z

Hi, first time contribution to micropython, so just let me know if anything should be changed
and bounce this request. Thanks!

nickzoic · 2017-05-07T23:57:38Z

That's brilliant, thanks for your contribution. I skimmed through it and it looks pretty good, I'll have a closer look most likely tomorrow. -----Nick

dpgeorge · 2017-05-09T06:59:18Z

@vandys thanks! For licensing/copyright purposes, can you please outline which parts of the code you wrote, which parts are copied (and from where), what information you used to work out the ESP32 API and what other code you looked at during the writing. For such a large piece of new code we need to be sure it can retain the MIT license.

Just a note on code style: maximum line length in this project is roughly 90-100 characters, so no need to break lines under that.

vandys · 2017-05-09T13:26:24Z

I started by copying the esp8266 machine_pwm.c. I used information from the ESP32 Technical Reference Manual, the esp-idf documentation, and the SDK's sample ledc example code (but I did not copy that code, just studied it to understand the SDK's API for PWM). So aside from the code copied from the esp8266 PWM support, everything else you see there is just new code I wrote.

I wasn't an employee of anybody when I wrote it, and I wrote it with the understanding and intention that it's simply a derivative work of the existing micropython code. I freely and willingly contribute it to the project and intend that it not change the legal status of the micropython code base in any way, even if it is included in that base in whole or part.

laurivosandi · 2017-05-12T12:26:42Z

Hi, I tried to enable PWM for pin 13 - everything works great :)

Then for pin 34, which according to pinout should support PWM, it says:

E (169529) ledc: ledc_channel_config(258): LEDC GPIO output number error

As I understandand you used LEDC PWM backend for your code and for other PWM pins there is different implementation, which pins actually use LEDC?

Also it seems to think that enabling PWM for pin 15 is perfectly okay, pin 15 is connected to ground :D

vandys · 2017-05-12T13:31:36Z

The PWM code here is just using the SDK's API, and if there's some other PWM implementation then I haven't caught a whiff of it. In driver/gpio.h they list GPIO pins 0 through 33 as input and output. Pin 15 on my own board does not seem to be connected to ground, FWIW.

laurivosandi · 2017-05-12T13:43:41Z

Hi, where have here something similar to this https://raw.githubusercontent.com/gojimmypi/ESP32/master/images/myESP32%20DevKitC%20pinout.png

According to this PWM should work on 34

vandys · 2017-05-12T13:52:27Z

Well, you need to talk with the SDK folks at Espressif. For instance, from their headers:

Only pins that support both input & output have integrated pull-up and pull-down resistors. Input-only GPIOs 34-39 do not.

What in the image you sent makes you believe it'll do output on that pin?
You will want to get the ESP32 Technical Reference Manual. Section 4.3 applies here.

MrSurly · 2017-05-19T01:13:45Z

I was thinking of adding a wrapper around the MCPWM; PWM would be a sub-object of this module.

Mostly, I want to do accurate pulse timing. Yes, I'm aware of machine.time_pulse_us().

dpgeorge · 2017-05-19T01:21:04Z

esp32/machine_pwm.c

+    int x;
+
+    // Initial condition: no channels assigned
+    for (x = 0; x < LEDC_CHANNEL_MAX; ++x) {


Code style: variable decls go in the for loop (if not needed after the loop).

dpgeorge · 2017-05-19T01:21:29Z

esp32/machine_pwm.c

+    timer_cfg.freq_hz = newval;
+    if (ledc_timer_config(&timer_cfg) != ESP_OK) {
+        timer_cfg.freq_hz = oval;
+        return(0);


Code style: should be return 0;

dpgeorge · 2017-05-19T01:21:46Z

esp32/machine_pwm.c

+// MicroPython bindings for PWM
+
+STATIC void esp32_pwm_print(const mp_print_t *print,
+        mp_obj_t self_in, mp_print_kind_t kind) {


Code style: single line

dpgeorge · 2017-05-19T01:22:07Z

esp32/machine_pwm.c

+    if (self->active) {
+        mp_printf(print, ", freq=%u, duty=%u",
+            timer_cfg.freq_hz,
+            ledc_get_duty(PWMODE, self->channel));


Only break the line if its larger than 90-100 chars.

dpgeorge · 2017-05-19T01:22:44Z

esp32/machine_pwm.c

+    }
+
+    // Maybe change PWM timer
+    tval = args[ARG_freq].u_int;


tval should be declared here.

dpgeorge · 2017-05-19T01:22:58Z

esp32/machine_pwm.c

+    if (channel >= LEDC_CHANNEL_MAX) {
+        if (avail == -1) {
+            nlr_raise(mp_obj_new_exception_msg_varg(&mp_type_ValueError,
+                "Out of PWM channels"));


error messages start with lower case letter

also, use the short-hand mp_raise_ValueError("...")

dpgeorge · 2017-05-19T01:23:37Z

esp32/machine_pwm.c

+    if (chan_gpio[channel] == -1) {
+        ledc_channel_config_t cfg = {
+            .channel = channel,
+            .duty = (1 << PWRES)/2,


code style: space around "/"

dpgeorge · 2017-05-19T01:23:48Z

esp32/machine_pwm.c

+    }
+
+    // Set duty cycle?
+    dval = args[ARG_duty].u_int;


dpgeorge · 2017-05-19T01:24:17Z

esp32/machine_pwm.c

+}
+
+STATIC MP_DEFINE_CONST_FUN_OBJ_VAR_BETWEEN(esp32_pwm_freq_obj,
+    1, 2, esp32_pwm_freq);


put on one line

dpgeorge · 2017-05-19T01:26:35Z

@laurivosandi that pin out looks wrong.

dpgeorge · 2017-05-19T01:29:48Z

I was thinking of adding a wrapper around the MCPWM; PWM would be a sub-object of this module.

It looks like MCPWM and LEDC are 2 different ways to do PWM on a pin. It doesn't really matter what the underlying implementation is, as long as the machine.PWM class works as expected (it's not yet properly specified in the docs so it's subject to change).

If MCPWM provides lots of extra functionality that doesn't fit a standard peripheral (is it similar enough to a Timer?) then it could go in a new "esp32" module as MCPWM class.

vandys · 2017-05-19T01:31:49Z

that pin out looks wrong.

I'm sorry, which pin out? The one from the earlier discussion? Pin 34?

dpgeorge · 2017-05-19T01:45:06Z

I'm sorry, which pin out? The one from the earlier discussion? Pin 34?

Sorry, yes, the link that @laurivosandi posted above to "myESP32 DevKitC pinout.png" looks wrong in that it says all IO pins should be able to do PWM when (as you pointed out) higher-numbered pins are input only.

vandys · 2017-05-19T01:45:47Z

Ok, thanks. I'm closing this pull request too, will change code and re-test against new SDK.

MrSurly · 2017-05-19T16:34:02Z

If MCPWM provides lots of extra functionality that doesn't fit a standard peripheral (is it similar enough to a Timer?) then it could go in a new "esp32" module as MCPWM class.

It's more focused around motor control -- you could drive BLDC motors, for example. Or generate servo signals.

But, it has a separate "capture" module that's mostly for edge capture / pulse timing. Not purely PWM.

It looks like LEDC is more about using timers, and animating led brightness?

dpgeorge · 2017-05-24T07:06:19Z

The follow-up to this was merged in #96

Add ESP32 PWM support, mirroring the API of the ESP8266.

e94cde8

mattytrentini mentioned this pull request May 10, 2017

PWM support #75

Closed

dpgeorge reviewed May 19, 2017

View reviewed changes

vandys closed this May 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ESP32 PWM support, mirroring the API of the ESP8266. #79

Add ESP32 PWM support, mirroring the API of the ESP8266. #79

vandys commented May 7, 2017

nickzoic commented May 7, 2017 via email

dpgeorge commented May 9, 2017

vandys commented May 9, 2017 •

edited

laurivosandi commented May 12, 2017 •

edited

vandys commented May 12, 2017

laurivosandi commented May 12, 2017 •

edited

vandys commented May 12, 2017 •

edited

MrSurly commented May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge May 19, 2017

dpgeorge commented May 19, 2017

dpgeorge commented May 19, 2017

vandys commented May 19, 2017

dpgeorge commented May 19, 2017

vandys commented May 19, 2017

MrSurly commented May 19, 2017

dpgeorge commented May 24, 2017

Add ESP32 PWM support, mirroring the API of the ESP8266. #79

Add ESP32 PWM support, mirroring the API of the ESP8266. #79

Conversation

vandys commented May 7, 2017

nickzoic commented May 7, 2017 via email

dpgeorge commented May 9, 2017

vandys commented May 9, 2017 • edited

laurivosandi commented May 12, 2017 • edited

vandys commented May 12, 2017

laurivosandi commented May 12, 2017 • edited

vandys commented May 12, 2017 • edited

MrSurly commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpgeorge commented May 19, 2017

dpgeorge commented May 19, 2017

vandys commented May 19, 2017

dpgeorge commented May 19, 2017

vandys commented May 19, 2017

MrSurly commented May 19, 2017

dpgeorge commented May 24, 2017

vandys commented May 9, 2017 •

edited

laurivosandi commented May 12, 2017 •

edited

laurivosandi commented May 12, 2017 •

edited

vandys commented May 12, 2017 •

edited