I’m going to ramble on a bit about the Sprite::drawExternalMask bug and the process I went through to solve it.
I’m going to put it in a details block because I don’t know how many people are actually interested in the ramblings of a madman, and frankly it’s probably a bit boring, but maybe someone will find it useful or interesting.
I guess Arduboy2 porting isn’t as shiny as 3D rendering and emulation…
(Warning: these are really long.)
The Long And Tedious Process
My first port of call was to narrow down the cause of the bug.
I don’t have a hardware debugger yet, so I had to do things the old fashioned way - inserting print statements into the code to print information onto the screen.
This can be very tedious.
First I narrowed it down to gameplay(),
then I narrowed it down to drawObjects()
then I narrowed it down to drawObject(object)
and from there I narrowed it down to Sprites::drawExternalMask.
I tried substituting Spritess:drawOverwrite and SpritesB::drawExternalMask… same result.
I then thought “ah, maybe I broke it during clean up”, so I substituted the original Arduboy2 Sprites code (minus the assembly).
Same problem, which means that for some reason code that works perfectly well on the Arduboy doesn’t work perfectly well on the Pokitto.
Next I tried commenting out drawObject to see if everything else worked.
I discovered drawBackground() had the same issue, coming from the exact same functions.
So I commented out drawBackground() and suddenly the game worked - all the logic was there and working perfectly without that rendering code.
I realised that other parts of the code were still using Sprites::drawExternalMask just fine, so I decided to log the parameters being fed into the drawing function in drawObject (since that was the first breaking point).
At first I thought it was specific sprites causing the problem so I disabled specific object types.
I’d got to the third disabled object when I suddenly spotted a pattern.
Every time it froze, the y input was negative.
So I inserted an if(y > -1) above the drawing call, and sure enough everything started working.
So at this point I’d established that:
- The code works fine on Arduboy
- There’s something causing it to behave differently on the Pokitto
- That something is related to negative
y values
So I moved into the drawing functions and started logging everywhere to find the breaking point, as standard.
I manage to pinpoint the break to one exact line:
uint8_t data = Arduboy2Base::sBuffer[ofs + WIDTH];
And I discovered that actually ofs was surprisingly high (in about the 65000 range).
I know from experience that’s usually a tell-tale sign of negative values being converted to unsigned values.
So I trace the code to ofs's defnition.
uint16_t ofs = ((sRow * WIDTH) + x + xOffset);
I find that sRow is defined:
const int8_t yOffset = (y % 8);
const int8_t tempSRow = (y / 8);
int8_t sRow = ((y < 0) && (yOffset > 0)) ? (tempSRow - 1) : tempSRow;
So evidently this is somehow related.
I start analysing the uint16_t ofs = ((sRow * WIDTH) + x + xOffset); expression and logging its components.
I find that sRow = -1, WIDTH = 128, x = 63 and xOffset = 0, which means (sRow * WIDTH) = -128 and x + xOffset = 63, so obviously -128 + 63 = -65 and of course -65 converted to a uint16_t is 65471.
So then I come back down to uint8_t data = Arduboy2Base::sBuffer[ofs + WIDTH];.
I think “ok, so 65471 + 128 = 65599, and obviously -65 + 128 = 63, which cancels out the -128 from earlier (which was from sRow * WIDTH while sRow = -1)…”
“But…” I ask myself, “this works on Arduboy, so what would cause it to not be working now?”
And then it hit me.
I remembered a basic law of C++ and suddenly everything made sense…
Note
(There was a point where I went down the wrong path and started looking into endianness, but after establishing both devices are little endian I scrapped that idea. A small 15-20 minute red herring.)
The Solution To The Problem
So eventually I had isolated the problem and had a moment of inspiration.
I realised two important bits of information and put 2 and 2 together and arrived at a wonderful 4.
The first thing I realised was the solution to the thing that was bugging me - why would it work on Pokitto and not Arduboy?
The answer has to be something that’s different between the Arduboy and Pokitto, and sure enough it is.
The answer is int.
On the Arduboy an int is 16 bits wide.
On the Pokitto an int is 32 bits` wide.
But if ofs is a uint16_t, then where is this int?
And then I remembered one of the fundamental rules of C++.
Something that happens all the time right under everyone’s noses but it often goes unnoticed.
The answer is ‘integer promotion’.
From cppreference’s section on Numeric Promotions:
unsigned char or unsigned short can be converted to int if it can hold its entire value range, and unsigned int otherwise;
arithmetic operators do not accept types smaller than int as arguments
And on the Pokitto, uint16_t is a type alias for unsigned short, hence ofs + WIDTH (i.e. uint16_t + <integer literal>) causes ofs to be promoted to int.
On the Arduboy, with or without promotion the arithmetic is 16-bit arithmetic, thus overflow occurs, the overflow bits are discarded and the addition correctly cancels out the earlier addition of a negative.
On the Pokitto, the promotion causes 32-bit arithmetic to be used, so instead of the overflow bit being discarded, the overflow bit is kept and the result is larger than the size of the display buffer, thus resulting in a buffer overrun.
It is that buffer overrun that caused the Pokitto to halt.
With this realisation I instantly form a solution to mimic the original behaviour:
uint8_t data = Arduboy2Base::sBuffer[(ofs + WIDTH) & 0xFFFF];
Mask off the higher bits that are dropped in the Arduboy’s original calculations.
To think that the addition of one single mask operation can be the difference between perfectly running code and a processor halting bug.
So what can we learn from this…
The Moral Of This Story
There are many things to take away from this.
Type safety is important. Always know the possible ranges of types.
int is at least 16 bits. It could be 16 bits, it could be 32 bits, it could be 64 bits.
If you want to write portable code, keep that in mind.
Be aware of integer promotion.
It can sneak up on you if you aren’t careful.
Sometimes people think their code means one thing,
but integer promotion changes its meaning and result.
Be careful of mixing signed and unsigned values.
Try not to mix them, but if you must mix them, make sure to double check the behaviour.
Always try to make type conversion explicit.
Hiding type conversion makes code harder to undestand.
If conversions aren’t explicit then sometimes you have to really look to understand what’s going on.
And lastly and most importantly:
Avoid integer overflow like the plague.
If your solution to a problem relies on overflow or underflow then you are probably tackling the problem incorrectly.
Don’t wait for overflow to happen - prevent it!
