Skip to content

esp32/machine_pwm.c: Add support for all PWM timers and channels. #7817

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
wants to merge 7 commits into from

Conversation

IhorNehrutsa
Copy link
Contributor

@IhorNehrutsa IhorNehrutsa commented Sep 18, 2021

This pull request allows using all the available PWM timers (up to 8) and channels(up to 16),
without affecting the PWM API.

If a new frequency is set, first it checks if another timer is using
the same freq.
If yes, then it uses this timer, otherwise, it creates a new one.

If all timers are used, the user should set an already used frequency,
or de-init a channel.

This PR is based on #6276 and #3608.
Thanks, @p1ng0o
Thanks, @dragomirecky
Thanks, @dpgeorge

Tested on ESP WROOM 32
Need to test on ESP32 GENERIC_S2 and GENERIC_C3 boards.

@IhorNehrutsa
Copy link
Contributor Author

@dpgeorge It seems that all your remarks are fixed.
I see failed checks, but I don't understand what is wrong.
I open Code formatters #7829

@robert-hh
Copy link
Contributor

If you click at the 'details' link of the fails checks, you'll see what is wrong. For code formatting, the lines preceded with a + show how it should look like, and the line starting with - show how the code looks at t he moment.
At the commit messages the period at the end of the subject line is missing. So e.g. instead of:
esp32\machine_pwm.c: Add support for all PWM timers and channels
it shall be
esp32\machine_pwm.c: Add support for all PWM timers and channels.

@IhorNehrutsa IhorNehrutsa changed the title esp32/machine_pwm.c: Add support for all PWM timers and channels esp32/machine_pwm.c: Add support for all PWM timers and channels. Sep 20, 2021
@IhorNehrutsa
Copy link
Contributor Author

@robert-hh Thank you

@IhorNehrutsa
Copy link
Contributor Author

As for me, it is an horrible code formatting style:

        if (self->mode == LEDC_LOW_SPEED_MODE) {
            gpio_matrix_out(self->pin, LEDC_LS_SIG_OUT0_IDX + self->channel, false, true);
        } else {
            #if LEDC_SPEED_MODE_MAX > 1
            #if CONFIG_IDF_TARGET_ESP32
            gpio_matrix_out(self->pin, LEDC_HS_SIG_OUT0_IDX + self->channel, false, true);
            #else
            #error Add supported CONFIG_IDF_TARGET_ESP32_xxx
            #endif
            #endif
        }

I'd prefer like this:

        if (self->mode == LEDC_LOW_SPEED_MODE) {
            gpio_matrix_out(self->pin, LEDC_LS_SIG_OUT0_IDX + self->channel, false, true);
        } else {
            #if LEDC_SPEED_MODE_MAX > 1
                #if CONFIG_IDF_TARGET_ESP32
                    gpio_matrix_out(self->pin, LEDC_HS_SIG_OUT0_IDX + self->channel, false, true);
                #else
                    #error Add supported CONFIG_IDF_TARGET_ESP32_xxx
                #endif
            #endif
        }

:(

@IhorNehrutsa
Copy link
Contributor Author

IhorNehrutsa commented Sep 20, 2021

Nevertheless. Could someone check the performance of the PR on ESP32
GENERIC_S2 and GENERIC_C3 boards?
Thanks.

@dpgeorge
Copy link
Member

Tested GENERIC on TinyPICO, GENERIC_S2 on FeatherS2, and GENERIC_C3 on ESP32-C3-DevKitC. PWM works as expected.

@dpgeorge
Copy link
Member

Squashed and merged in 52636fa and 71111cf

Thank you very much for the work on this!

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

Successfully merging this pull request may close these issues.

3 participants