Skip to content
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

esp32: Fix PWM not allowing frequencies < 611 Hz. #8200

Closed
wants to merge 1 commit into from

Conversation

robert-hh
Copy link
Contributor

@robert-hh robert-hh commented Jan 22, 2022

Tested with duty_u16(), duty() and duty_ns() for frequencies down to
1 Hz.
Fixes issue #8189

ports/esp32/machine_pwm.c Outdated Show resolved Hide resolved
@robert-hh
Copy link
Contributor Author

Changed, rebased & squashed. The names of #defines chosen by the initial implementation could be more distinctive, but left them as they were.

@IhorNehrutsa
Copy link
Contributor

Bug is here:

// Possible highest resolution in device
#if (LEDC_TIMER_BIT_MAX - 1) < LEDC_TIMER_16_BIT
#define HIGHEST_PWM_RES (LEDC_TIMER_BIT_MAX - 1)
#else
#define HIGHEST_PWM_RES (LEDC_TIMER_16_BIT) // 20 bit for ESP32, but 16 bit is used
#endif

Algorithm is developed in assumption that HIGHEST_PWM_RES must be 16 or 14
but LEDC_TIMER_BIT_MAX and LEDC_TIMER_16_BIT is enum (not a macro) and we got
HIGHEST_PWM_RES == 20

I tried to fix with SOC_LEDC_TIMER_BIT_WIDE_NUM, but SOC_LEDC_TIMER_BIT_WIDE_NUM also is not difined in Micropython compile time

This is right version:

// Possible highest resolution in device
#if CONFIG_IDF_TARGET_ESP32
#define HIGHEST_PWM_RES (LEDC_TIMER_16_BIT) // 20 bit for ESP32, but 16 bit is used
#else
#define HIGHEST_PWM_RES (LEDC_TIMER_BIT_MAX - 1)
#endif

and then lines

return ledc_get_duty(self->mode, self->channel) << (HIGHEST_PWM_RES + UI_RES_SHIFT - timers[TIMER_IDX(self->mode, self->timer)].duty_resolution);

int channel_duty = duty >> (HIGHEST_PWM_RES + UI_RES_SHIFT - timer.duty_resolution);

are right too.

@robert-hh
Copy link
Contributor Author

robert-hh commented Jan 25, 2022

With:
#define UI_RES_SHIFT (UI_RES_16_BIT - HIGHEST_PWM_RES)
the expression:
HIGHEST_PWM_RES + UI_RES_SHIFT - timer.duty_resolution
results in:
UI_RES_16_BIT - timer.duty_resolution
So care must be taken about duty_resolution being larger or smaller than 16.
By limiting HIGHEST_PWM_RES to 16 and consequently limiting duty_resolution to 16 that is forced with the previous code. Or the scaling is adapted like in my code.

@IhorNehrutsa
Copy link
Contributor

@robert-hh
Yes, you are right, there are two ways. May try to compare both.

@robert-hh
Copy link
Contributor Author

Tested with your version works fine as well on a generic ESP32. The Frequency deviation of 1001.6 Hz instead of 1000Hz stay. GENERIC_C3 and UM_FEATHERS2 show better frequency resolution and granularity.

The previous code worked also well on a UM_FEATHERS2 and a GENERIC_C3.
At the GENERIC_C3 down to 10Hz, at the UM_FEATHERS2 reliable down to 2 Hz, with a strange hiccup when changing from 10Hz -> 1Hz, but not when changing from 2Hz -> 1Hz.

Tested with duty_u16(), duty() and duty_ns() for frequencies down to
1 Hz.
@robert-hh
Copy link
Contributor Author

In favor of @IhorNehrutsa's PR, being the initial contributor for this PWM module.

@robert-hh robert-hh deleted the esp32_pwmfix branch March 8, 2022 10:49
tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 24, 2023
…-makerdiary-nrf52840-connect

8.2.x backport: Added Makerdiary nRF52840 Connect Kit.
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.

None yet

3 participants