search

Aircoookie / WLED

Control WS2812B and many more types of digital RGB LEDs with an ESP8266 or ESP32 over WiFi!
https://kno.wled.ge
MIT License
14.66k stars 3.15k forks source link

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

Closed PaoloTK closed 1 month ago

PaoloTK commented 1 month ago

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.
PaoloTK commented 1 month ago

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.

PaoloTK commented 1 month ago

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!