ws2812: Fix number of nops for AVR at 8 MHz #9559
Conversation
When trying to calculate the number of nops for AVR running at 8 MHz, the value of `w3` is expected to be negative; however, because `F_CPU` is defined in tmk_core/avr.mk with the `UL` suffix, the preprocessor performs its calculations using `unsigned long`, getting a very large positive number instead of the expected negative number; this then results in generating code with a huge number of nops. Fix the broken calculations by performing a comparison before subtraction, so that the unsigned number wraparound does not occur. The keyboard which triggers the problem is `handwired/promethium`; the buggy code silently compiles, but the resulting timings would be completely wrong.
|
Looks good to me. But...
I recommend this. After merging this PR, you can fix #9558 . |
|
Thank you for your contribution! |
Remove old code which was unsuccessfully trying to clamp negative w1, w2 and w3 values to 0, and set w1_nops, w2_nops and w3_nops directly.
|
Now I actually tested the code on a 3.3V Pro Micro board, and even tried to make some actual measurements of the generated PWM signal (using Input Capture on STM32F411 @ 96 MHz). Before the fix: T0H = 250 ns, T1H = 875 ns, total period = 5250 us (within a byte) or 6250 us (between bytes). I also verified timings on a 5V Pro Micro: |
* ws2812: Fix number of nops for AVR at 8 MHz When trying to calculate the number of nops for AVR running at 8 MHz, the value of `w3` is expected to be negative; however, because `F_CPU` is defined in tmk_core/avr.mk with the `UL` suffix, the preprocessor performs its calculations using `unsigned long`, getting a very large positive number instead of the expected negative number; this then results in generating code with a huge number of nops. Fix the broken calculations by performing a comparison before subtraction, so that the unsigned number wraparound does not occur. The keyboard which triggers the problem is `handwired/promethium`; the buggy code silently compiles, but the resulting timings would be completely wrong. * ws2812: Clean up the code after the 8 MHz fix Remove old code which was unsuccessfully trying to clamp negative w1, w2 and w3 values to 0, and set w1_nops, w2_nops and w3_nops directly.
* ws2812: Fix number of nops for AVR at 8 MHz When trying to calculate the number of nops for AVR running at 8 MHz, the value of `w3` is expected to be negative; however, because `F_CPU` is defined in tmk_core/avr.mk with the `UL` suffix, the preprocessor performs its calculations using `unsigned long`, getting a very large positive number instead of the expected negative number; this then results in generating code with a huge number of nops. Fix the broken calculations by performing a comparison before subtraction, so that the unsigned number wraparound does not occur. The keyboard which triggers the problem is `handwired/promethium`; the buggy code silently compiles, but the resulting timings would be completely wrong. * ws2812: Clean up the code after the 8 MHz fix Remove old code which was unsuccessfully trying to clamp negative w1, w2 and w3 values to 0, and set w1_nops, w2_nops and w3_nops directly.
* ws2812: Fix number of nops for AVR at 8 MHz When trying to calculate the number of nops for AVR running at 8 MHz, the value of `w3` is expected to be negative; however, because `F_CPU` is defined in tmk_core/avr.mk with the `UL` suffix, the preprocessor performs its calculations using `unsigned long`, getting a very large positive number instead of the expected negative number; this then results in generating code with a huge number of nops. Fix the broken calculations by performing a comparison before subtraction, so that the unsigned number wraparound does not occur. The keyboard which triggers the problem is `handwired/promethium`; the buggy code silently compiles, but the resulting timings would be completely wrong. * ws2812: Clean up the code after the 8 MHz fix Remove old code which was unsuccessfully trying to clamp negative w1, w2 and w3 values to 0, and set w1_nops, w2_nops and w3_nops directly.
Description
When trying to calculate the number of nops for AVR running at 8 MHz, the value of
w3is expected to be negative; however, becauseF_CPUis defined in tmk_core/avr.mk with theULsuffix, the preprocessor performs its calculations usingunsigned long, getting a very large positive number instead of the expected negative number; this then results in generating code with a huge number of nops. Fix the broken calculations by performing a comparison before subtraction, so that the unsigned number wraparound does not occur.The keyboard which triggers the problem is
handwired/promethium; the buggy code silently compiles, but the resulting timings would be wrong (but apparently not wrong enough to be noticed, because for most WS8212-like LEDs a low level pulse of 6 us is not enough to trigger a reset).I cannot actually test the change on a real hardware (I tested that it does not break the generated code for XD87 HS, which has ATmega32u4 running at 16 MHz, but I don't have any hardware which uses 8 Mhz).Update: Got a 3.3V Pro Micro board for testing and verified that the code actually works (at least with those LEDs that I have).I looked at the Travis build report for #9558 and found a single keyboard which actually has ATmega32u4 running at 8 MHz with some WS2812-style LEDs connected, then looked at the generated assembly code and noticed that it is obviously wrong (has lots of NOPs), and the fix makes it look right (no NOPs at the end of the loop).
It should be possible to simplify the code by removing the subsequent checks likeUpdate: Removed useless code.w1 > 0, but I did not touch them for now to avoid conflicts with #9558.Types of Changes
Checklist