Matrix led 7x10 with arduino

Question

Good afternoon, a few days ago I finished a personal project of a 7x10 led matrix programmed with the ATMEGA328p microcontroller. To control the matrix I use 2 74HC595 shift registers in cascade in the 10 columns and a CD4017 decade counter in the 7 rows. The 8 rows of the LED matrix are independently connected to a 2N3904 NPN transistor that provides a ground path to sink the combined current of all the LEDs in a row. I've been programming microcontrollers for a few months and I would like some advice to improve my code, both in terms of good practices and program optimization.

Code:

#define CD4017_CLK PORTB1
#define CD4017_RST PORTB2
#define SERIAL_DATA PORTB3
#define ST_CLK PORTB4
#define SH_CLK PORTB5
#define MAX_CHARS 100
#define DELAY 30
#define SHIFT_STEP 1

/*
 char_data is a two dimensional constant array that holds the 5-bit column values
 of individual rows for ASCII characters that are to be displayed on a 7x10 matrix. 
*/
const byte char_data[95][7]={
    {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0}, // space
    {0x4, 0x4, 0x4, 0x4, 0x4, 0x0, 0x4}, // !
    {0xA, 0xA, 0xA, 0x0, 0x0, 0x0, 0x0}, // "
    {0xA, 0xA, 0x1F, 0xA, 0x1F, 0xA, 0xA}, // #
    {0x4, 0xF, 0x14, 0xE, 0x5, 0x1E, 0x4}, // $
    {0x18, 0x19, 0x2, 0x4, 0x8, 0x13, 0x3}, // %
    {0x8, 0x14, 0x14, 0x8, 0x15, 0x12, 0xD}, // &
    {0x4, 0x4, 0x4, 0x0, 0x0, 0x0, 0x0}, // '
    {0x4, 0x8, 0x10, 0x10, 0x10, 0x8, 0x4}, // (
    {0x4, 0x2, 0x1, 0x1, 0x1, 0x2, 0x4}, // )
    {0x4, 0x15, 0xE, 0x4, 0xE, 0x15, 0x4}, // *
    {0x0, 0x4, 0x4, 0x1F, 0x4, 0x4, 0x0}, // +
    {0x0, 0x0, 0x0, 0x0, 0x4, 0x4, 0x8}, // ,
    {0x0, 0x0, 0x0, 0x1F, 0x0, 0x0, 0x0}, // -
    {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x4}, // .
    {0x0, 0x1, 0x2, 0x4, 0x8, 0x10, 0x0}, // /
    {0xE, 0x11, 0x13, 0x15, 0x19, 0x11, 0xE}, // 0
    {0x6, 0xC, 0x4, 0x4, 0x4, 0x4, 0xE}, // 1
    {0xE, 0x11, 0x1, 0x6, 0x8, 0x10, 0x1F}, // 2
    {0x1F, 0x1, 0x2, 0x6, 0x1, 0x11, 0xF}, // 3
    {0x2, 0x6, 0xA, 0x12, 0x1F, 0x2, 0x2}, // 4
    {0x1F, 0x10, 0x1E, 0x1, 0x1, 0x11, 0xE}, // 5
    {0xE, 0x11, 0x10, 0x1E, 0x11, 0x11, 0xE}, // 6
    {0x1F, 0x1, 0x2, 0x4, 0x8, 0x8, 0x8}, // 7
    {0xE, 0x11, 0x11, 0xE, 0x11, 0x11, 0xE}, // 8
    {0xE, 0x11, 0x11, 0xF, 0x1, 0x11, 0xE}, // 9
    {0x0, 0x0, 0x4, 0x0, 0x4, 0x0, 0x0}, // :
    {0x0, 0x0, 0x4, 0x0, 0x4, 0x4, 0x8}, // ;
    {0x2, 0x4, 0x8, 0x10, 0x8, 0x4, 0x2}, // <
    {0x0, 0x0, 0x1F, 0x0, 0x1F, 0x0, 0x0}, // =
    {0x8, 0x4, 0x2, 0x1, 0x2, 0x4, 0x8}, // >
    {0xE, 0x11, 0x1, 0x2, 0x4, 0x0, 0x4}, // ?
    {0xE, 0x11, 0x17, 0x15, 0x16, 0x10, 0xF}, // @
    {0xE, 0x11, 0x11, 0x1F, 0x11, 0x11, 0x11}, // A
    {0x1E, 0x11, 0x11, 0x1E, 0x11, 0x11, 0x1E}, // B
    {0xE, 0x11, 0x10, 0x10, 0x10, 0x11, 0xE}, // C
    {0x1E, 0x11, 0x11, 0x11, 0x11, 0x11, 0x1E}, // D
    {0x1F, 0x10, 0x10, 0x1E, 0x10, 0x10, 0x1F}, // E
    {0x1F, 0x10, 0x10, 0x1E, 0x10, 0x10, 0x10}, // F
    {0xE, 0x11, 0x10, 0x17, 0x15, 0x11, 0xE}, // G
    {0x11, 0x11, 0x11, 0x1F, 0x11, 0x11, 0x11}, // H
    {0x1F, 0x4, 0x4, 0x4, 0x4, 0x4, 0x1F}, // I
    {0x1, 0x1, 0x1, 0x1, 0x1, 0x11, 0xE}, // J
    {0x11, 0x11, 0x12, 0x1C, 0x12, 0x11, 0x11}, // K
    {0x10, 0x10, 0x10, 0x10, 0x10, 0x10, 0x1F}, // L
    {0x11, 0x1B, 0x15, 0x15, 0x11, 0x11, 0x11}, // M
    {0x11, 0x11, 0x19, 0x15, 0x13, 0x11, 0x11}, // N
    {0xE, 0x11, 0x11, 0x11, 0x11, 0x11, 0xE}, // O
    {0x1E, 0x11, 0x11, 0x1E, 0x10, 0x10, 0x10}, // P
    {0xE, 0x11, 0x11, 0x11, 0x15, 0x13, 0xF}, // Q
    {0x1E, 0x11, 0x11, 0x1E, 0x11, 0x11, 0x11}, // R
    {0xF, 0x10, 0x10, 0xE, 0x1, 0x1, 0x1E}, // S
    {0x1F, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4}, // T
    {0x11, 0x11, 0x11, 0x11, 0x11, 0x11, 0xE}, // U
    {0x11, 0x11, 0x11, 0x11, 0x11, 0xA, 0x4}, // V
    {0x11, 0x11, 0x11, 0x15, 0x15, 0x1B, 0x11}, // W
    {0x11, 0x11, 0xA, 0x4, 0xA, 0x11, 0x11}, // X
    {0x11, 0x11, 0xA, 0x4, 0x4, 0x4, 0x4}, // Y
    {0x1F, 0x1, 0x2, 0x4, 0x8, 0x10, 0x1F}, // Z
    {0xE, 0x8, 0x8, 0x8, 0x8, 0x8, 0xE}, // [
    {0x0, 0x10, 0x8, 0x4, 0x2, 0x1, 0x0}, // backward slash
    {0xE, 0x2, 0x2, 0x2, 0x2, 0x2, 0xE}, // ]
    {0x0, 0x0, 0x4, 0xA, 0x11, 0x0, 0x0}, // ^
    {0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x1F}, // _
    {0x8, 0x4, 0x2, 0x0, 0x0, 0x0, 0x0}, // `
    {0x0, 0x0, 0xE, 0x1, 0xF, 0x11, 0xF}, // a
    {0x10, 0x10, 0x10, 0x1E, 0x11, 0x11, 0x1E}, // b
    {0x0, 0x0, 0xE, 0x11, 0x10, 0x11, 0xE}, // c
    {0x1, 0x1, 0x1, 0xF, 0x11, 0x11, 0xF}, // d
    {0x0, 0x0, 0xE, 0x11, 0x1F, 0x10, 0xF}, // e
    {0xE, 0x9, 0x1C, 0x8, 0x8, 0x8, 0x8}, // f
    {0x0, 0x0, 0xE, 0x11, 0xF, 0x1, 0xE}, // g
    {0x10, 0x10, 0x10, 0x1E, 0x11, 0x11, 0x11}, // h
    {0x4, 0x0, 0x4, 0x4, 0x4, 0x4, 0xE}, // i
    {0x1, 0x0, 0x3, 0x1, 0x1, 0x11, 0xE}, // j
    {0x10, 0x10, 0x11, 0x12, 0x1C, 0x12, 0x11}, // k
    {0xC, 0x4, 0x4, 0x4, 0x4, 0x4, 0xE}, // l
    {0x0, 0x0, 0x1E, 0x15, 0x15, 0x15, 0x15}, // m
    {0x0, 0x0, 0x1E, 0x11, 0x11, 0x11, 0x11}, // n
    {0x0, 0x0, 0xE, 0x11, 0x11, 0x11, 0xE}, // o
    {0x0, 0x0, 0xF, 0x9, 0xE, 0x8, 0x8}, // p
    {0x0, 0x0, 0xF, 0x11, 0xF, 0x1, 0x1}, // q
    {0x0, 0x0, 0x17, 0x18, 0x10, 0x10, 0x10}, // r
    {0x0, 0x0, 0xF, 0x10, 0xE, 0x1, 0x1E}, // s
    {0x4, 0x4, 0xE, 0x4, 0x4, 0x4, 0x3}, // t
    {0x0, 0x0, 0x11, 0x11, 0x11, 0x13, 0x13}, // u
    {0x0, 0x0, 0x11, 0x11, 0x11, 0xA, 0x4}, // v
    {0x0, 0x0, 0x11, 0x11, 0x15, 0x1F, 0x15}, // w
    {0x0, 0x0, 0x11, 0xA, 0x4, 0xA, 0x11}, // x
    {0x0, 0x0, 0x11, 0x11, 0xF, 0x1, 0x1E}, // y
    {0x0, 0x0, 0x1F, 0x2, 0x4, 0x8, 0x1F}, // z
    {0x2, 0x4, 0x4, 0x8, 0x4, 0x4, 0x2}, // {
    {0x4, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4}, // |
    {0x8, 0x4, 0x4, 0x2, 0x4, 0x4, 0x8}, // }
    {0x0, 0x0, 0x0, 0xA, 0x15, 0x0, 0x0}, // ~
};
uint16_t frame_buffer[7];
char message[MAX_CHARS+2] = "MATRIZ LED 7X10 ";
int string_length = strlen(message);

void send_data(uint16_t data){
    uint16_t mask = 1, flag = 0;
    for (byte i=0; i<10; i++){
        flag = data & mask;
        if (flag) PORTB |= (1 << SERIAL_DATA);
        else PORTB &= ~(1 << SERIAL_DATA);
        PORTB |= (1 << SH_CLK);
        PORTB &= ~(1 << SH_CLK);
        mask <<= 1;
    }
    PORTB |= (1 << ST_CLK);
    PORTB &= ~(1 << ST_CLK);
}

void send_frame_buffer(){
    for (byte t=0; t<DELAY; t++){ // The delay we get with loops.
        for (byte row=0; row<7; row++){ // For each row.
            // Send 10 bits to shift registers.
            send_data(frame_buffer[row]);
            // This delay defines the time to play each pattern.
            delayMicroseconds(800);
            // Clear the row so we can go on to the next row without smearing.
            send_data(0);
            // On to the next row.
            PORTB |= (1 << CD4017_CLK);
            PORTB &= ~(1 << CD4017_CLK);
        }
        // Select the first row.
        PORTB |= (1 << CD4017_RST);
        PORTB &= ~(1 << CD4017_RST);
    }
}

void display_message(){
    for (byte c=0; c<string_length; c++){ // For each character.
        byte mask = 0x10;
        for (byte column=0; column<5; column++){ // For each column.
            for (byte row=0; row<7; row++){ // For each row.
                // To obtain the position of the current character in the char_data array, subtract 32 from the ASCII value of the character itself.     
                byte index = message[c];
                byte temp = char_data[index-32][row];
                frame_buffer[row] = (frame_buffer[row]<<1) | ((temp&mask)>>4-column);
            }
            send_frame_buffer();
            mask >>= 1;
        }
        // One column of separation between characters.
        for (byte row=0; row<7; row++){
            frame_buffer[row] <<= SHIFT_STEP;
        }
        send_frame_buffer();
    }
}

int main(){
    // PORTB as output.
    DDRB = 0b111111;
    // Makes sure the 4017 value is 0.
    PORTB |= (1 << CD4017_RST);
    PORTB &= ~(1 << CD4017_RST);
    
    while (true){
        display_message();
    }
}

Video: https://www.youtube.com/shorts/neCM424JKCY

Answer 1

The code is not bad at all, so mostly these are going to be some notes about style that won't make a significant difference to the underlying machine code as we will see.

Try not to confuse readers

This line is a little strange:

char message[MAX_CHARS+2] = "MATRIZ LED 7X10 ";

Apparently MAX_CHARS doesn't really mean MAX_CHARS? I'd suggest simply increasing the value of MAX_CHARS by two. It would be a lot less strange.

Add functions for readability

The current code includes this function.

void send_data(uint16_t data){
    uint16_t mask = 1, flag = 0;
    for (byte i=0; i<10; i++){
        flag = data & mask;
        if (flag) PORTB |= (1 << SERIAL_DATA);
        else PORTB &= ~(1 << SERIAL_DATA);
        PORTB |= (1 << SH_CLK);
        PORTB &= ~(1 << SH_CLK);
        mask <<= 1;
    }
    PORTB |= (1 << ST_CLK);
    PORTB &= ~(1 << ST_CLK);
}

That works, of course, but it can be prone to error. It would be all to easy to type |= when you meant &= or to forget the ~. Also, the lines of code dealing with moving the clock from high to low must operate in pairs. For all of these reasons, I'd prefer to write it like this:

void send_data(uint16_t data){
    for (uint16_t mask{1}; mask & 0x3ff; mask <<= 1) {
        bit(SERIAL_DATA, data & mask);
        bithilo(SH_CLK);
    }
    bithilo(ST_CLK);
}

You could use the digitalWrite functions provided by the Arduino platform, or you might prefer to create more efficient ones as I've done here:

inline void setbit(int bitnum) {
    PORTB |= (1 << bitnum);
}
inline void clrbit(int bitnum) {
    PORTB &= ~(1 << bitnum);
}
inline void bit(int bitnum, bool val) {
    if (val)
        setbit(bitnum);
    else
        clrbit(bitnum);
}
inline void bithilo(int bitnum) {
    setbit(bitnum);
    clrbit(bitnum);
}

One might object that it's more code, or that it will make the program slower, but in fact, as this online sample shows, the two versions produce identical assembly language instruction sequences.

Marking these functions as inline is a way to suggest to the compiler that inline substitution of this code might be better than having the code actually called as a subroutine. In this particular case, it allows the compiler's optimizer to recognize that the operations can be accomplished with a very efficient sbi or cbi assembly language instruction.

This line: for (uint16_t mask{1}; mask & 0x3ff; mask <<= 1) may seem confusing at first, but it's a common idiom, especially for embedded systems. We want to extract one bit at a time for ten bits. The 0x3ff is a uint16_t with the low ten bits set. As we shift the bits to the left, the value of mask & 0x3ff will be a non-zero number until we get to the eleventh bit. At that point, the experession is 0, which C++ interprets as false and so the loop ends.

As you can see from the online compiler demonstration, both versions result in identical code, so I would advocate using the one that you think makes the code easiest to read and maintain.

Make it easier to spot data errors

Here's the code for the '0' character, but with an error in it:

    {0xE, 0x11, 0x13, 0x15, 0x1B, 0x11, 0xE}, // 0 but with an error

How long would it take you to find the error? I'd suggest doing one of two things. Either use an external program that runs on your computer (not the Arduino) to generate both the visual and hex form, or write the data in binary to make it easier to see what's going on:

    { 0b01110,
      0b10001,
      0b10011,
      0b10101,
      0b11011,  // <-- this line has the error
      0b10001,
      0b01110
    },

It's still not great, but I would suggest that it's a lot easier for a human to verify that 1) all lines are exactly 5 bits and 2) each character has a plausible representation. Consider if you were asked to change this character so that it didn't have a diagonal line in it. It would be a lot simpler in this form.

Consider using objects

Arduino's language is actually a C++ variant, which has some very useful object oriented features. You could for example wrap the display into an object so that if you wanted to change it to accommodate two 10x7 displays side-by-side acting as a 20x7 display, the interface to the rest of the code could stay the same.