Skip to content

Commit

Permalink
ws2812: Fix number of nops for AVR at 8 MHz (qmk#9559)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
sigprof authored and sowbug committed Feb 10, 2021
1 parent 5de7782 commit cd664c0
Showing 1 changed file with 17 additions and 21 deletions.
38 changes: 17 additions & 21 deletions drivers/avr/ws2812.c
Original file line number Diff line number Diff line change
Expand Up @@ -67,19 +67,27 @@ void ws2812_setleds(LED_TYPE *ledarray, uint16_t number_of_leds) {
#define w_onecycles (((F_CPU / 1000) * w_onepulse + 500000) / 1000000)
#define w_totalcycles (((F_CPU / 1000) * w_totalperiod + 500000) / 1000000)

// w1 - nops between rising edge and falling edge - low
#define w1 (w_zerocycles - w_fixedlow)
// w2 nops between fe low and fe high
#define w2 (w_onecycles - w_fixedhigh - w1)
// w3 nops to complete loop
#define w3 (w_totalcycles - w_fixedtotal - w1 - w2)

#if w1 > 0
# define w1_nops w1
// w1_nops - nops between rising edge and falling edge - low
#if w_zerocycles >= w_fixedlow
# define w1_nops (w_zerocycles - w_fixedlow)
#else
# define w1_nops 0
#endif

// w2_nops - nops between fe low and fe high
#if w_onecycles >= (w_fixedhigh + w1_nops)
# define w2_nops (w_onecycles - w_fixedhigh - w1_nops)
#else
# define w2_nops 0
#endif

// w3_nops - nops to complete loop
#if w_totalcycles >= (w_fixedtotal + w1_nops + w2_nops)
# define w3_nops (w_totalcycles - w_fixedtotal - w1_nops - w2_nops)
#else
# define w3_nops 0
#endif

// The only critical timing parameter is the minimum pulse length of the "0"
// Warn or throw error if this timing can not be met with current F_CPU settings.
#define w_lowtime ((w1_nops + w_fixedlow) * 1000000) / (F_CPU / 1000)
Expand All @@ -90,18 +98,6 @@ void ws2812_setleds(LED_TYPE *ledarray, uint16_t number_of_leds) {
# warning "Please consider a higher clockspeed, if possible"
#endif

#if w2 > 0
# define w2_nops w2
#else
# define w2_nops 0
#endif

#if w3 > 0
# define w3_nops w3
#else
# define w3_nops 0
#endif

#define w_nop1 "nop \n\t"
#define w_nop2 "rjmp .+0 \n\t"
#define w_nop4 w_nop2 w_nop2
Expand Down

0 comments on commit cd664c0

Please sign in to comment.