From 2941ed100fe28f3cc32a0b36021902347a019141 Mon Sep 17 00:00:00 2001 From: "Mr.Blinky" <4971163+MrBlinky@users.noreply.github.com> Date: Fri, 9 Nov 2018 21:10:05 +0100 Subject: [PATCH] Fix a compiler issue and optimize drawPixel() The new compiler in Arduino IDE 1.8.6/1.8.7 has an optimization glitch where it fails to see the difference between a data structure in RAM and in PROGMEM when the data is the same and optimizes one out. drawPixel() is unable to fetch the correct pixel mask from the bitshift_left array in PROGMEM causing junk to be drawn. The bitshift_left[] array has now been optimised out and drawPixel() will function properly. The optimization uses the same number of cycles and saves 6 bytes. drawPixel() has also been made static so the Arduboy object pointer is no longer needlesly passed on to drawPixel() saving more bytes. --- src/Arduboy2.cpp | 71 ++++++++++++++++++------------------------------ src/Arduboy2.h | 2 +- 2 files changed, 27 insertions(+), 46 deletions(-) diff --git a/src/Arduboy2.cpp b/src/Arduboy2.cpp index d6cdcd4..8f103b2 100644 --- a/src/Arduboy2.cpp +++ b/src/Arduboy2.cpp @@ -322,14 +322,6 @@ void Arduboy2Base::clear() fillScreen(BLACK); } - -// Used by drawPixel to help with left bitshifting since AVR has no -// multiple bit shift instruction. We can bit shift from a lookup table -// in flash faster than we can calculate the bit shifts on the CPU. -const uint8_t bitshift_left[] PROGMEM = { - _BV(0), _BV(1), _BV(2), _BV(3), _BV(4), _BV(5), _BV(6), _BV(7) -}; - void Arduboy2Base::drawPixel(int16_t x, int16_t y, uint8_t color) { #ifdef PIXEL_SAFE_MODE @@ -342,46 +334,35 @@ void Arduboy2Base::drawPixel(int16_t x, int16_t y, uint8_t color) uint16_t row_offset; uint8_t bit; - // uint8_t row = (uint8_t)y / 8; - // row_offset = (row*WIDTH) + (uint8_t)x; - // bit = _BV((uint8_t)y % 8); - - // the above math can also be rewritten more simply as; - // row_offset = (y * WIDTH/8) & ~0b01111111 + (uint8_t)x; - // which is what the below assembler does - - // local variable for the bitshift_left array pointer, - // which can be declared a read-write operand - const uint8_t* bsl = bitshift_left; - asm volatile ( - "mul %[width_offset], %A[y]\n" - "movw %[row_offset], r0\n" - "andi %A[row_offset], 0x80\n" // row_offset &= (~0b01111111); - "clr __zero_reg__\n" - "add %A[row_offset], %[x]\n" - // mask for only 0-7 - "andi %A[y], 0x07\n" - // Z += y - "add r30, %A[y]\n" - "adc r31, __zero_reg__\n" - // load correct bitshift from program RAM - "lpm %[bit], Z\n" - : [row_offset] "=&x" (row_offset), // upper register (ANDI) - [bit] "=r" (bit), - [y] "+d" (y), // upper register (ANDI), must be writable - "+z" (bsl) // is modified to point to the proper shift array element - : [width_offset] "r" ((uint8_t)(WIDTH/8)), - [x] "r" ((uint8_t)x) + // bit = 1 << (y & 7) + "ldi %[bit], 1 \n" //bit = 1; + "sbrc %[y], 1 \n" //if (y & _BV(1)) bit = 4; + "ldi %[bit], 4 \n" + "sbrc %[y], 0 \n" //if (y & _BV(0)) bit = bit << 1; + "lsl %[bit] \n" + "sbrc %[y], 2 \n" //if (y & _BV(2)) bit = (bit << 4) | (bit >> 4); + "swap %[bit] \n" + //row_offset = y / 8 * WIDTH + x; + "andi %A[y], 0xf8 \n" //row_offset = (y & 0xF8) * WIDTH / 8 + "mul %[width_offset], %A[y] \n" + "movw %[row_offset], r0 \n" + "clr __zero_reg__ \n" + "add %A[row_offset], %[x] \n" //row_offset += x +#if WIDTH != 128 + "adc %B[row_offset], __zero_reg__ \n" // only non 128 width can overflow +#endif + : [row_offset] "=&x" (row_offset), // upper register (ANDI) + [bit] "=&d" (bit), // upper register (LDI) + [y] "+d" (y) // upper register (ANDI), must be writable + : [width_offset] "r" ((uint8_t)(WIDTH/8)), + [x] "r" ((uint8_t)x) : ); - - if (color) { - sBuffer[row_offset] |= bit; - } else { - sBuffer[row_offset] &= ~ bit; - } + uint8_t data = sBuffer[row_offset] | bit; + if (!(color & _BV(0))) data ^= bit; + sBuffer[row_offset] = data; } uint8_t Arduboy2Base::getPixel(uint8_t x, uint8_t y) @@ -922,7 +903,7 @@ struct BitStreamReader } if ((this->byteBuffer & this->bitBuffer) != 0) - result |= (1 << i); // result |= bitshift_left[i]; + result |= (1 << i); this->bitBuffer <<= 1; } diff --git a/src/Arduboy2.h b/src/Arduboy2.h index 15a269b..2c9837b 100644 --- a/src/Arduboy2.h +++ b/src/Arduboy2.h @@ -449,7 +449,7 @@ class Arduboy2Base : public Arduboy2Core * specified color. The values WHITE or BLACK can be used for the color. * If the `color` parameter isn't included, the pixel will be set to WHITE. */ - void drawPixel(int16_t x, int16_t y, uint8_t color = WHITE); + static void drawPixel(int16_t x, int16_t y, uint8_t color = WHITE); /** \brief * Returns the state of the given pixel in the screen buffer.