Class that is used to run a 5x5x5 RGB cube

I had a hardware project for a 5x5x5 RGB cube which is driven by a 22118400Hz 8-bit AVR (1284p for those who like hardware).

Basically, the class sets the bits in a huge array of 8-bit values (exactly 5x30x10 uint8_ts).

To understand why a short excursion:

The array contains 5 for the level, 30 for the color multiplexing and 10 for the real hardware register byte.

After every 12KHz, an interrupt shifts out one of the 10 hardware bytes and increases the color counter (the 30 value). If the color counter hits 30, it gets set to 0 and the level counter increases by one (this resets on 5 to 0). Every 1 inside of the 80-bit which get shifted out is an "on" in real life and a 0 off. For example, 30x 1 in the first bit would mean full brightness of the cube.

I wrote a Cube class which handles this:

```
class Cube
{
protected:

uint8_t m_colors[5][MAX_COLOR][10];
RGB m_cur_colors[5][5][5];
//for SPI!
uint8_t level;
uint8_t cur_color_counter;
//functions
public:

static Cube &getInstance()
{
return m_instance;
}

RGB setRGB(const uint8_t &x, const uint8_t &y, const uint8_t &z,
const uint8_t &r, const uint8_t &g, const uint8_t &b);
RGB setRGB(const uint8_t &x, const uint8_t &y, const uint8_t &z,
const RGB &color);
RGB setRGB(const Vector &v, const RGB &rgb);
uint8_t getR(const uint8_t &x, const uint8_t &y, const uint8_t &z);
uint8_t getG(const uint8_t &x, const uint8_t &y, const uint8_t &z);
uint8_t getB(const uint8_t &x, const uint8_t &y, const uint8_t &z);
RGB getRGB(const uint8_t &x, const uint8_t &y, const uint8_t &z);
void off();
inline void render();

private:
Cube();
~Cube();

//no copy!
Cube( const Cube &c ) = delete;
Cube &operator=( const Cube &c ) = delete;

static Cube m_instance;

//static definitions for the cube object
static volatile uint8_t *m_to_storage_port;
static volatile uint8_t *m_to_storage_ddr;
sta

RGB

There's a few things worth saying about this class, even if it's pretty straightforward. Some of these things apply to Cube as well.

• Taking variables by const uint8_t&. There's simply no reason for it. Take them by value. Taking by const& is good practice to avoid unnecessary copies of large objects, but uint8_t is not a large object. In fact, it's smaller than the size of the pointer you're having to pass.

• Copying. Let the compiler write the copy/move constructor/assignment for you. You're just doing what it would do anyway. Save the code. Not to mention that the self-assignment check is an unnecessary branch. Self-assignment isn't unsafe here.

-
The boolean anti-pattern. You have this code in your equality comparison:

if (expr) {
    return true;
}
return false;

You can just write return expr;

-
Unnecessary cast. In your subtraction operator you have:

if((int8_t) r - i >= 0)
    r = (r - i) % MAX_COLOR_RGB;
else
    r = 0;

The cast isn't necessary. You can simply compare r >= i. Secondly, this is different behavior than your other operations which allow for overflow. This should simply be:

r -= i;

-
Destruction. Similarly with the copying comment, you don't need to write a destructor.

Cube

Several of the above comments apply here too, so I will omit them.

-
Singleton. It's more common to define the singleton within the instance function:

static Cube& getInstance() {
    static Cube instance;
    return instance;
}

-
Access. You have a lot of repeated accesses to arr[x][y][z]. I'd suggest simplifying things to make them easier to use. First of all, you do not need getR(), getG(), and getB() at all. If somebody wants to write:

Cube::getInstance().getR(x, y, z)

they can just as easily write:

Cube::getInstance().getRGB(x, y, z).r;

There's just no need for extra methods. But with that, setting is really a mouthful:

Cube::getInstance().setRGB(x, y, z, r, g, b);

First, you should take an RGB:

Cube::getInstance().setRGB(x, y, z, RGB{r, g, b});

And secondly, consider using a proxy object. Add an operator() that takes your three dimensions and returns a proxy that is readable and writeable. What I mean is, support the following syntax:

Cube::getInstance()(x, y, z) = RGB{r, g, b};

That is much more natural. You can also drop the Vector overload. If you don't want to go the proxy route, you can still imply this:

RGB ret = m_cur_colors[x][y][z];//save old value to return
//check if not already set if so return.
if(ret == RGB(r,g,b)) 
    return ret;

//change value to new one
m_cur_colors[x][y][z].r = r;
m_cur_colors[x][y][z].g = g;
m_cur_colors[x][y][z].b = b;

into something with far fewer lookups:

RGB& ret = m_cur_colors[x][y][z];
if (ret == new_rgb) return ret;

// since you have an assignment operator 
ret = new_rgb;

StackExchange Code Review Q#109152, answer score: 3