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

Fix incorrect PWM bit depth on Esp32 with XTAL clock #4082

Merged
merged 4 commits into from
Aug 4, 2024

Conversation

PaoloTK
Copy link

@PaoloTK PaoloTK commented Aug 1, 2024

Fixes incorrect bit depth for ESP32 with XTAL support (new "wave" of ESP32 models like C3).

Currently 0.15 has hardcoded bit depths for ESP8266 and ESP32. Since Arduino 2.0.2, ESP32 models that support external XTAL clock (C3 for example) are set to use it instead of the built in APB clock. XTAL runs at half the frequency (40 MHz rather than 80 MHz) and so the pin fails to assign correctly due to incorrect bit depth.
My approach was to calculate the correct bit depth on the fly based on what clock the microprocessor supports (including ESP8266). I have tested the code on a regular ESP32, a C3 and an ESP8266 on all currently possible clock settings and everything looks in line with the previous values.

A few notes:

  1. I tested performance and it's less than 0.5 ms extra in the worst case I've seen.
  2. There's no frequency validation. This can technically cause issues (for example the new ESP32 boards have a smaller range of bit depths compared to the original) but since currently the frequencies are hardcoded and not user configurable I don't think it's a problem.
  3. Even before this PR, ESP8266 was configurable with frequencies above 1000 Hz. In my testing (and it's commonly agreed) 8266 cannot reliably produce frequencies above 1000 Hz, causing flickering and poor dimming performance. I think it's worth considering removing these options.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not like floating point and the dreaded log().
You can get away without.

wled00/bus_manager.cpp Outdated Show resolved Hide resolved
@PaoloTK
Copy link
Author

PaoloTK commented Aug 3, 2024

I do not like floating point and the dreaded log(). You can get away without.

Thank you, fixed. I also implemented a platform specific max bit width value but kept the lower limit at 8 bit.

I've been wondering about the best strategy in regard to esp8266. The issue stems from the fact that WLED_PWM_FREQ is set to a middle ground value on ESP32 that strikes a good balance between frequency and bit depth, but on 8266 it is set to the highest possible frequency before dimming stutters and similar issues occur.
An option could be to set WLED_PWM_FREQ to a value such that the fastest preset is = 880 Hz, but that doesn't seem great because in the future presets might change (or go away).
We could disable the options above Normal for ESP8266, which would limit the highest frequency to WLED_PWM_FREQ.

Copy link
Collaborator

@blazoncek blazoncek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Haha! I came to almost exactly the same code, except I didn't move constants into const.h and didn't use floats.
Since the constants are not needed anywhere else perhaps it makes no sense to put them into const.h.

wled00/bus_manager.cpp Outdated Show resolved Hide resolved
wled00/const.h Outdated Show resolved Hide resolved
wled00/const.h Outdated
@@ -521,6 +521,35 @@
#endif
#endif

#ifndef CLOCK_FREQUENCY
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps change to MAX_PWM_CLOCK_FREQUENCY or something similar to avoid possible future clash.

wled00/const.h Outdated
// C6/H2/P4: 20 bit, S2/S3/C2/C3: 14 bit
#define MAX_BIT_WIDTH SOC_LEDC_TIMER_BIT_WIDE_NUM
#else
// ESP32: 32 bit
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wled00/bus_manager.cpp Outdated Show resolved Hide resolved
Move contants into bus manager
@blazoncek blazoncek merged commit d2401a2 into Aircoookie:0_15 Aug 4, 2024
18 checks passed
@PaoloTK
Copy link
Author

PaoloTK commented Aug 5, 2024

Haha! I came to almost exactly the same code, except I didn't move constants into const.h and didn't use floats. Since the constants are not needed anywhere else perhaps it makes no sense to put them into const.h.

No surprise there, I tested the solutions you suggested and found the second to work better (first had issues with rounding).

Thank you for all the unvaluable feedback and for merging with the fixes you suggested!

@PaoloTK PaoloTK deleted the fix_esp32c3_pwm_bit_depth branch August 6, 2024 21:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants