IDE 1.8.6 / 1.8.7 Compilation Issues

filmote · 2018-11-07 09:00:19 UTC

I have a new Windows 10 Laptop and I hate it. One thing I have noticed is that things compiled on it are a little off … let me explain.

I copied the entire Arduino/libraries directory from my old machine to the new one.
I copied my ProjectABE installation from my old machine to the new.
I installed a new version of VSCode and the Arduino plugin.
I have fetched the latest code of my Blackjack game from GitHub onto both machines.

If I compile a HEX on the old machine and execute it within ProjectABE on either machine it works fines.
If I compile the same HEX on the new machine and execute it within ProjectABE it works on both machines however there are graphics glitches that I cannot explain as shown below:

ArduboyRecording

Note the ellipses that are on the right hand side of the graphics. They appear as vertical lines when in fact the code is rendering a simple pixel using the following code:


    uint8_t i = (this->counter / 15) % 4;

    for (uint8_t j = 0; j < i; j++) {
      
        arduboy.drawPixel(79 + (j * 2), 49);

    }

As I mentioned, I am using the same version of the Arduboy2 library so I am at a loss to explain what is going on.

Mr.Blinky · 2018-11-07 19:57:28 UTC

Do you also use the same Arduino IDE version (or copied a portable version from old to new)?

filmote · 2018-11-07 23:16:19 UTC

Ah … my old machine is running 1.8.5 and the new 1.8.6

I have tried compiling the same code on 1.8.6 and 1.8.7 and both exhibit the problem on the new machine.

I then downgraded the machine to 1.8.5 and everything works properly.

I was aware that there were some issue with 1.8.6 but I thought people were unable to compile some programs. I never expected the issue to manifest itself in a subtle graphics error.

The drawPixel() code is assembly - I wonder if this is contributing to the issue? I also noticed that a few fillRect() calls were giving trouble elsewhere in the program and they use drawFastVLine() which in turn calls drawPixel().

filmote · 2018-11-08 12:00:12 UTC

I might flag @MLXXXp into this thread as he may know why the drawPixel() function might be programmatic. (I am assuming that the assembler is his code, maybe not).

MLXXXp · 2018-11-08 15:08:40 UTC

No, it’s not. The assembly code added to optimise drawPixel() was written by @Dreamer3 (yyyc514 on GitHub). @Pharap fixed a bug and I also made a later fix.

My time is limited right now, so I may not get a chance to look at this problem for a while. If anyone else can look into it, I would appreciate it.

Mr.Blinky · 2018-11-08 19:58:33 UTC

actually it’s assembly with somemixed C.

A couple of weeks ago I looked into the drawPixel (and optimized the C bit a bit) as I noticed the compiler didn’t optimize out an unused pointer (didn’t solve that though)

@filmote if you zip up your project I’ll have a go and compare at it.

In case anyones wondering what’s optimized. I got rid of the else case and used a different masking technique (It will be featured in the next homemade package update)

if (color) {
    sBuffer[row_offset] |=   bit;
  } else {
    sBuffer[row_offset] &= ~ bit;
}

To:

  uint8_t data = sBuffer[row_offset] |= bit;
  if (!color) data ^= bit;
  sBuffer[row_offset] = data;

When you’ve got time again and picking up your Arduboy2 library, give me a ping and I’ll sort out all the optimisations I’ve added to simplify things.

Pharap · 2018-11-08 20:01:40 UTC

Does that need to be sBuffer[row_offset] |= bit?
You’re writing it back anyway so it could just be sBuffer[row_offset] | bit, couldn’t it?

uint8_t data = sBuffer[row_offset] | bit;
if (!color) data ^= bit;
sBuffer[row_offset] = data;

(The compiler will probably optimise the write out at the SSA stage anyway, but better safe than sorry.)

Mr.Blinky · 2018-11-08 20:11:50 UTC

Ha! right missed that. Thanks for pointing that out.

filmote · 2018-11-08 22:12:14 UTC

Its the Blackjack project from here > https://github.com/Press-Play-On-Tape/Blackjack

Mr.Blinky · 2018-11-09 00:52:22 UTC

Thanks! and… I found the glitch.

The compiler somehow decides to store bitshift_left array in ram rather then in progmem. But since the drawPixel assembly code reads from progmem, ‘random’ bitmask data is used instead.

github.com

MLXXXp/Arduboy2/blob/0eae58c760376f84e786c4927c7b33f7acdd3101/src/Arduboy2.cpp#L311




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
if (x < 0 || x > (WIDTH-1) || y < 0 || y > (HEIGHT-1))
{
  return;
}

I think the problem arises here

github.com

MLXXXp/Arduboy2/blob/0eae58c760376f84e786c4927c7b33f7acdd3101/src/Arduboy2.cpp#L337


// 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"

I don’t know if it’s an actual compiler glitch or incorrect use of C(++) @Pharap any ideas?

filmote · 2018-11-09 00:58:58 UTC

Nice pickup … I am glad I could isolate for you!

Pharap · 2018-11-09 01:16:13 UTC

Well, I can start with some information at least.
PROGMEM is actually implemented as __attribute__((__progmem__)).

It’s plausible that a newer GCC version has affected where the attribute can be placed, but I would hope not because that contradicts the docs.

It certainly sounds like a bug of some description, possibly in GCC rather than the Arduino IDE.

Mr.Blinky · 2018-11-09 01:21:04 UTC

I think I may have a nice alternative that may save a few bytes too at the cost of 1 more cycle. Will test it first though

Pharap · 2018-11-09 01:40:18 UTC

Now I think about it, I’m not sure the bsl variable is actually needed anyway.
Registers are values, not references, so copying the value of bitshift_left straight to z should have had the same effect without needing the intermediary variable.

Mr.Blinky · 2018-11-09 02:04:03 UTC

The compiler will start to complain about it being read only and ‘+z’ is used.

I found a little more about the glitch. It looks like the compiler optimized out bitshift_left and uses BIT_SHIFT instead which used in Ardbitmap and stored in ram.

Edit:

Okay my idea works the bit conversion code takes the same number of cycles, 1 instruction more but doesn’t need the byte array, saving 6 bytes.

   //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"

filmote · 2018-11-09 02:33:23 UTC

I am not sure what you mean here but are you saying Ardbutmap is the issue? Or that using Ardbitmao contributes to the issue?

Mr.Blinky · 2018-11-09 02:41:07 UTC

bitshift_left (arduboy2) and BIT_SHIFT (Ardbitmap) both contain the same data. only the 1st is in progmem and the 2nd in ram.

When reversing the code there is only bitshift_left in ram and no BIT_SHIFT at all.

When removing bitshift_left (because of using my alternative code) then after reversing the code there is only BIT_SHIFT in ram

Mr.Blinky · 2018-11-09 03:45:59 UTC

Ok here’s the optimized drawPixel that works fine with 1.8.7 :

void Arduboy2Base::drawPixel(int16_t x, int16_t y, uint8_t color)
{
  #ifdef PIXEL_SAFE_MODE
  if (x < 0 || x > (WIDTH-1) || y < 0 || y > (HEIGHT-1))
  {
    return;
  }
  #endif

  uint16_t row_offset;
  uint8_t bit;

  asm volatile
  (
    // 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" // 
    "mul  %[width_offset], %A[y]       \n"
    "movw %[row_offset], r0            \n"
    "clr  __zero_reg__                 \n"
    "add  %A[row_offset], %[x]         \n"
#if WIDTH != 128
    "adc  %B[row_offset], __zero_reg__ \n" // only none 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)
    :
  );
  uint8_t data = sBuffer[row_offset] | bit;
  if (!color) data ^= bit;
  sBuffer[row_offset] = data;
}

MLXXXp · 2018-11-09 13:30:36 UTC

Can you create a GitHub PR for this? I’ll then apply it and make a new release.

If you don’t want to do a PR you can raise an issue with the code included. Or, I can just pull it from here. I can then apply your code and credit you.

Mr.Blinky · 2018-11-09 14:05:33 UTC

I’ll create a PR

Edit:

created