8
\$\begingroup\$

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).

enter image description here

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;
    static const uint8_t m_to_storage_pin_no;

    static volatile uint8_t *m_disable_port;
    static volatile uint8_t *m_disable_ddr;
    static const uint8_t m_disable_pin_no;
}; //Cube

void Cube::render()
{
    // DISABLE_LINE_PORT |= (1 << DISABLE_LINE_PIN); //set high to disable the register
    *m_disable_port |= (1 << m_disable_pin_no);
    //reverse shift out
    for(int8_t i = 9; i >= 0; i--)
    {
        SPI::transmit(m_colors[level][cur_color_counter][i]);
    }

    cur_color_counter++;
    if(cur_color_counter >= MAX_COLOR)
    {
        cur_color_counter = 0;
        level++;
        if(level > 4)
        {
            level = 0;
        }
    }

    *m_to_storage_port |= (1 << m_to_storage_pin_no); //1 clock to storage
    *m_to_storage_port &= ~(1 << m_to_storage_pin_no); //1 clock to storage

    *m_disable_port &= ~(1 << m_disable_pin_no); //set low to enable the register
}

Cube Cube::m_instance;

// default constructor
Cube::Cube(): level(0), cur_color_counter(0)
{
    SPI::init();//init the spi
    off();
    //set all 0 with a string function
    //memset(cur_colors, 0, 5*5*5*sizeof(RGB));

    //enabel output on ss and latch pins
    *m_to_storage_ddr |= (1 << m_to_storage_pin_no);
    *m_disable_ddr |= (1 << m_disable_pin_no);
} //Cube

// default destructor
Cube::~Cube()
{
} //~Cube

void Cube::off()
{
    memset(m_colors, 0, sizeof(m_colors));
    for(uint8_t i = 0; i < MAX_COLOR; i++)
    {
        //maybe reverse
        m_colors[0][i][0] = 0x04;//
        m_colors[1][i][0] = 0x03;//
        m_colors[2][i][0] = 0x02;//
        m_colors[3][i][0] = 0x01;//
        m_colors[4][i][0] = 0x00;//
    }
}

RGB Cube::setRGB(const uint8_t& x, const uint8_t& y, const uint8_t& z,
                 const RGB& color)
{
    return setRGB(x, y, z, color.r, color.g, color.b);
}

/************************************************************************/
/* Take care it has no bounds checking!                                 */
/************************************************************************/
RGB Cube::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 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;

    //update colors
    uint8_t pos = x * 3 + z * 15 + 5;
    //+0
    uint8_t rbyte = pos / 8;
    uint8_t rbit = pos % 8;
    //+1
    uint8_t gbyte = (pos + 1) / 8;
    uint8_t gbit = (pos + 1) % 8;
    //+2
    uint8_t bbyte = (pos + 2) / 8;
    uint8_t bbit = (pos + 2) % 8;

    for(uint8_t i = 0; i < MAX_COLOR; i++)
    {
        if(i < r)
        {
            m_colors[y][i][rbyte] |= 1 << rbit; //set all to 1 till r
        }
        else
        {
            //bitwise not = ~ ! (invert with &)
            m_colors[y][i][rbyte] &= ~(1 << rbit);//set all other to 0
        }
        if(i < g)
        {
            m_colors[y][i][gbyte] |= 1 << gbit; //set all to 1 till r
        }
        else
        {
            m_colors[y][i][gbyte] &= ~(1 << gbit);//set all other to 0
        }
        if(i < b)
        {
            m_colors[y][i][bbyte] |= 1 << bbit; //set all to 1 till r
        }
        else
        {
            m_colors[y][i][bbyte] &= ~(1 << bbit);//set all other to 0
        }
    }
    return ret;
}

RGB Cube::setRGB(const Vector& v, const RGB& rgb)
{
    return setRGB(v.x, v.y, v.z, rgb);
}

uint8_t Cube::getR(const uint8_t& x, const uint8_t& y, const uint8_t& z)
{
    return m_cur_colors[x][y][z].r;
}

uint8_t Cube::getG(const uint8_t& x, const uint8_t& y, const uint8_t& z)
{
    return m_cur_colors[x][y][z].g;
}

uint8_t  Cube::getB(const uint8_t& x, const uint8_t& y, const uint8_t& z)
{
    return m_cur_colors[x][y][z].b;
}

RGB  Cube::getRGB(const uint8_t& x, const uint8_t& y, const uint8_t& z)
{
    return m_cur_colors[x][y][z];
}

I'd like to speed up the method for setting those bits inside of the array which get shifted out, since it is the major performance leak:

RGB Cube::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)

To shortly explain how the bit 80-bit looks:

The first 5 bits are the level so they need to keep untouched (00001,00010,00100...). After this, it follows rgb0 rgb1 rgb2 rgb3 and so on in each bit.

Any suggestions on how I could speed that up?


The RGB definitions:

class RGB
{
//variables
public:
// the values
    uint8_t r, g, b;

//functions
public:
    RGB();
    RGB(const uint8_t &r, const uint8_t &g, const uint8_t &b);
    //constructor for hex values like
    // 0x000000
    // first 8 bit = r secon 8bit = g last 8bit = b
    explicit RGB(const uint32_t &hex);
    //copy constructor
    RGB(const RGB &c);

    //assignment
    RGB &operator= (const RGB &other);

    //compare
    bool operator== (const RGB &other) const;


    RGB operator+ (const RGB &other) const;
    RGB &operator+= (const RGB &other);

    RGB operator- (const RGB &other) const;
    RGB &operator-= (const RGB &other);

    RGB operator* (const RGB &other) const;
    RGB &operator*= (const RGB &other);

    RGB operator/ (const RGB &other) const;
    RGB &operator/= (const RGB &other);


    /**
     * Adds the value to all!
     */
    RGB operator+ (const uint8_t &i) const;
    RGB &operator+= (const uint8_t &i);
    /**
    * subs the value to all!
    */
    RGB operator- (const uint8_t &i) const;
    RGB &operator-= (const uint8_t &i);

    /**
    * mult the value to all!
    */
    RGB operator* (const float &f) const;
    RGB &operator*= (const float &f);

    RGB operator* (const uint8_t &i) const;
    RGB &operator*= (const uint8_t &i);


    /**
    * div the value to all!
    */
    RGB operator/ (const uint8_t &i) const;
    RGB &operator/= (const uint8_t &i);

    RGB operator/ (const float &f) const;
    RGB &operator/= (const float &f);

    void random();

    ~RGB();

    friend RGB operator* (const float &, const RGB &);
    friend RGB operator/ (const float &, const RGB &);
private:
    static const uint8_t MAX_COLOR_RGB;
}; //RGB

RGB::RGB() : r(0), g(0), b(0)
{
}
// default constructor
RGB::RGB(const uint8_t &r, const uint8_t &g, const uint8_t &b)
{
    this->r = r % MAX_COLOR_RGB;
    this->g = g % MAX_COLOR_RGB;
    this->b = b % MAX_COLOR_RGB;
} //RGB

RGB::RGB(const uint32_t &hex)
{

    this->b = (hex & 0xFF);
    b %= MAX_COLOR_RGB;
    this->g = ((hex >> 8) & 0xFF);
    g %= MAX_COLOR_RGB;
    this->r = ((hex >> 8) & 0xFF);
    r %= MAX_COLOR_RGB;
} //RGB

// default destructor
RGB::~RGB()
{
} //~RGB

RGB::RGB(const RGB &c) : r(c.r), g(c.g), b(c.b)
{
}

RGB &RGB::operator= (const RGB &other)
{
    if(this != &other)    //no self assignment
    {
        r = other.r;
        g = other.g;
        b = other.b;
    }
    return *this;
}

bool RGB::operator == (const RGB &other) const
{
    if(r == other.r && g == other.g && b == other.b)
        return true;
    return false;
}

RGB RGB::operator+ (const RGB &other) const
{
    // *this += other;
    return RGB(*this) += other;   //return new
}

RGB &RGB::operator+= (const RGB &other)
{
    r = (r + other.r) % MAX_COLOR_RGB;
    g = (g + other.g) % MAX_COLOR_RGB;
    b = (b + other.b) % MAX_COLOR_RGB;
    return *this;
}

RGB RGB::operator- (const RGB &other) const
{
    return RGB(*this) -= other;   //new
}

RGB &RGB::operator-= (const RGB &other)
{
    r = (r - other.r) % MAX_COLOR_RGB;
    g = (g - other.g) % MAX_COLOR_RGB;
    b = (b - other.b) % MAX_COLOR_RGB;
    return *this;
}

RGB RGB::operator* (const RGB &other) const
{
    return RGB(*this) *= other;   //new
}

RGB &RGB::operator*= (const RGB &other)
{
    r = (r * other.r) % MAX_COLOR_RGB;
    g = (g * other.g) % MAX_COLOR_RGB;
    b = (b * other.b) % MAX_COLOR_RGB;
    //per convention return "yourself"
    return *this;
}

RGB RGB::operator/ (const RGB &other) const
{
    return RGB(*this) /= other;
}

RGB &RGB::operator/= (const RGB &other)
{
    r = (r / other.r) % MAX_COLOR_RGB;
    g = (g / other.g) % MAX_COLOR_RGB;
    b = (b / other.b) % MAX_COLOR_RGB;
    //per convention return "yourself"
    return *this;
}


RGB RGB::operator+ (const uint8_t &i) const
{
    return  RGB(*this) += i;   //ret new
}

RGB &RGB::operator+= (const uint8_t &i)
{
    r = (r + i) % MAX_COLOR_RGB;
    g = (g + i) % MAX_COLOR_RGB;
    b = (b + i) % MAX_COLOR_RGB;
    //per convention return "yourself"
    return *this;
}
RGB RGB::operator- (const uint8_t &i) const
{
    return RGB(*this) -= i;
}

RGB &RGB::operator-= (const uint8_t &i)
{
    if((int8_t) r - i >= 0)
        r = (r - i) % MAX_COLOR_RGB;
    else
        r = 0;
    if((int8_t) g - i > 0)
        g = (g - i) % MAX_COLOR_RGB;
    else
        g = 0;
    if((int8_t) b - i > 0)
        b = (b - i) % MAX_COLOR_RGB;
    else
        b = 0;
    //per convention return "yourself"
    return *this;
}


RGB RGB::operator* (const uint8_t &i) const
{
    return RGB(*this) *= i;   //new
}

RGB &RGB::operator*= (const uint8_t &i)
{
    r = (r * i) % MAX_COLOR_RGB;
    g = (g * i) % MAX_COLOR_RGB;
    b = (b * i) % MAX_COLOR_RGB;
    //per convention return "yourself"
    return *this;
}

RGB &RGB::operator*= (const float &f)
{
    r = ((uint8_t)(r * f)) % MAX_COLOR_RGB;
    g = ((uint8_t)(g * f)) % MAX_COLOR_RGB;
    b = ((uint8_t)(b * f)) % MAX_COLOR_RGB;
    return *this;
}

RGB RGB::operator* (const float &f) const
{
    return RGB(*this) *= f;   //return a new one
}

RGB RGB::operator/ (const uint8_t &i) const
{
    //per convention return "yourself"
    return RGB(*this) /= i;
}

RGB &RGB::operator/= (const uint8_t &i)
{
    r = (r / i) % MAX_COLOR_RGB;
    g = (g / i) % MAX_COLOR_RGB;
    b = (b / i) % MAX_COLOR_RGB;
    //per convention return "yourself"
    return *this;
}

RGB RGB::operator/ (const float &f) const
{
    return RGB(*this) /= f;
}

RGB &RGB::operator/= (const float &f)
{
    r = ((uint8_t)(r / f)) % MAX_COLOR_RGB;
    g = ((uint8_t)(g / f)) % MAX_COLOR_RGB;
    b = ((uint8_t)(b / f)) % MAX_COLOR_RGB;
    //per convention return "yourself"
    return *this;
}

void RGB::random()
{
    r = rnd(MAX_COLOR_RGB);
    g = rnd(MAX_COLOR_RGB);
    b = rnd(MAX_COLOR_RGB);
}

Note: I know there is some technik called bit angle modulation which could do this in just 5 bit per colour (result in 32 color diff colors per channel) instead of 30. But this has an awful flickering even at a really high multiplexing. Tested up to 20kHz which results in less time for animations because I need more cycles to get out the colors.

\$\endgroup\$
  • \$\begingroup\$ Can you add the definition for RGB? \$\endgroup\$ – Barry Oct 29 '15 at 17:40
  • \$\begingroup\$ Of cause. I just sciped it because it has alot of operators defined. \$\endgroup\$ – BennX Oct 29 '15 at 17:41
  • 1
    \$\begingroup\$ Added The definition of the RGB. \$\endgroup\$ – BennX Oct 29 '15 at 17:43
3
\$\begingroup\$

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.

  1. 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.
  2. 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.
  3. The boolean anti-pattern. You have this code in your equality comparison:

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

    You can just write return expr;

  4. 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;
    
  5. 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.

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

    static Cube& getInstance() {
        static Cube instance;
        return instance;
    }
    
  2. 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;
    
\$\endgroup\$
  • \$\begingroup\$ Well taking by const& avoids a copy but you were right that it is a internal copy of a 16bit pointer just by avoding the 8bit copy. I like the idea of the Proxy object. I have no clue about this yet but it's a good idea. I guess its some object which overloads the = and has .getrgb. On = it calls the Cube.set Methods. This does not speed up the setting of the bits inside of the array but it reduces the operations before which is also really good and keeps the code more clean. Thanks on that. \$\endgroup\$ – BennX Oct 29 '15 at 18:18
  • \$\begingroup\$ One short question to this. You mention the r -=ii didnt do this because r need to be %MAX_COLOR. Is the assignment with r*i % color slower then a r-=i; r %=MAX_COLOR ? \$\endgroup\$ – BennX Oct 29 '15 at 18:48
  • \$\begingroup\$ @BennX What's MAX_COLOR? \$\endgroup\$ – Barry Oct 29 '15 at 18:50
  • \$\begingroup\$ It's should be an uint8_t which defines the max colorvalue (atm 30). #define MAX_COLOR_RGB 30. Sorry that I missed adding it \$\endgroup\$ – BennX Oct 29 '15 at 19:00
  • \$\begingroup\$ @BennX Oh. You should make it 32 :) Much faster to mod by powers of two (the compiler will likely replace it with & 31) \$\endgroup\$ – Barry Oct 29 '15 at 19:05

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.