-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Conversation
There was a problem hiding this 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.
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. |
There was a problem hiding this 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/const.h
Outdated
@@ -521,6 +521,35 @@ | |||
#endif | |||
#endif | |||
|
|||
#ifndef CLOCK_FREQUENCY |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move contants into bus manager
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! |
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: