Skip to content

ledc_write max_duty bug #11049

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Open
scrtrees opened this issue Mar 7, 2025 · 2 comments · May be fixed by #11226
Open

ledc_write max_duty bug #11049

scrtrees opened this issue Mar 7, 2025 · 2 comments · May be fixed by #11226
Assignees
Labels
Status: In Progress ⚠️ Issue is in progress

Comments

@scrtrees
Copy link

scrtrees commented Mar 7, 2025

https://github.com/espressif/arduino-esp32/blame/9e2f75564127a9853f80f3eb637b264406cb2f30/cores/esp32/esp32-hal-ledc.c#L181

//Fixing if all bits in resolution is set = LEDC FULL ON
    uint32_t max_duty = (1 << bus->channel_resolution) - 1;

    if ((duty == max_duty) && (max_duty != 1)) {
      duty = max_duty + 1;

If a user want to set resolution as 5 and set duty 31/32, it should be failed.
Duty ratio should be like as below.

channel_resolution = 5, duty = 0 → duty ratio = 0/32
channel_resolution = 5, duty = 1 → duty ratio = 1/32
channel_resolution = 5, duty = 2 → duty ratio = 2/32
...
channel_resolution = 5, duty = 30 → duty ratio = 30/32
channel_resolution = 5, duty = 31 → duty ratio = 31/32
channel_resolution = 5, duty = 32 → duty ratio = 32/32

However, current code makes it like below.

channel_resolution = 5, duty = 31 → duty ratio = 32/32 -> WRONG RATIO
channel_resolution = 5, duty = 32 → duty ratio = 32/32

The code on top just need to be deleted.

@SuGlider SuGlider self-assigned this Mar 7, 2025
@SuGlider
Copy link
Collaborator

SuGlider commented Apr 7, 2025

Looking into the LEDC documentation, we can verify that the Duty goes from 0 to 2^resolution.
Using 5 buts resolution, duty goes from 0 to 32 (33 levels).
Duty 0 = 0% ... Duty = 32 = 100%.

But... Arduino API says that Duty shoud go from 0 to (2^resolution) - 1.
Therefore, for Arduino API compatibility, the resolution is 5 bits is like this:
Duty 0 = 0% ... Duty 31 = 100%.

Thus, we have a different metric for resolution.
In such case, ESP32 Arduino Core has decided to implement the Arduino API metric system.

channel_resolution = 5, duty = 31 → duty ratio = 32/32 -> Arduino API compatible
channel_resolution = 5, duty = 32 → duty ratio = 32/32 -> IDF comaptible

@SuGlider SuGlider added the Type: Question Only question label Apr 7, 2025
@SuGlider
Copy link
Collaborator

SuGlider commented Apr 7, 2025

@scrtrees - What we could change here is that ledcWrite() could act as IDF defines (0..32) and analogWrite() act as Arduino defines (0..31). In that case, duty = 31 would behave as 100%, mapping it to a ledcWrite(pin, 32).

In other words, the code you have pointed out shall be moved to analogWrite() and removed from ledcWrite().

@SuGlider SuGlider linked a pull request Apr 7, 2025 that will close this issue
@SuGlider SuGlider added Status: In Progress ⚠️ Issue is in progress and removed Type: Question Only question labels Apr 7, 2025
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Status: In Progress ⚠️ Issue is in progress
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants