2
\$\begingroup\$

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

\$\endgroup\$
6
  • \$\begingroup\$ You (need to) drive 17 lines. Two SI8PO already give 16: Drop the 4017 and use one of the port bits as the 17th. If hard pressed for port bits, use the strobe as the 17th bit. Check the compiler documentation for CBI/SBI support. \$\endgroup\$
    – greybeard
    Nov 11, 2022 at 7:14
  • \$\begingroup\$ Hi, sorry, I don't understand what you mean, we have a language barrier here, I speak Spanish but I'm using the translator. What does SI8PO and strobe mean? \$\endgroup\$ Nov 11, 2022 at 15:38
  • \$\begingroup\$ You say to use the cd 4017 to drive the 10 columns and control the 7 rows with a digital port? Or what do you mean? \$\endgroup\$ Nov 11, 2022 at 15:39
  • \$\begingroup\$ SI8PO: Serial input, 8 Parallel Outputs. (SI16PO: 74X673) (C3PO: Clear, 3 Parallel Outputs?) \$\endgroup\$
    – greybeard
    Nov 11, 2022 at 17:49
  • \$\begingroup\$ "Drop" as in leave out, do not use. You have enough lines using one port pin less if you just use the two '595s. \$\endgroup\$
    – greybeard
    Nov 11, 2022 at 17:52

1 Answer 1

2
\$\begingroup\$

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.

\$\endgroup\$
6
  • \$\begingroup\$ Hi, thanks for the advice. I have a doubt, what does the word inline mean in the declaration of a function? \$\endgroup\$ Nov 6, 2022 at 22:19
  • \$\begingroup\$ I don't quite understand what you're doing here either: for (uint16_t mask {1}; mask & 0x3ff; mask <<= 1). I understand that the mask starts at 1 and shifts one bit to the left in each cycle, but I'm questioning the mask & 0x3ff part. What I understood about the for in c++ was that the second component of the for loop indicated the final value of the loop, but here the result of doing mask & 0x3ff at the beginning is 1 so the loop should not even be executed. But I just realized that I only know that I don't know anything. \$\endgroup\$ Nov 6, 2022 at 22:58
  • \$\begingroup\$ I've added some to my answer to try to address your questions. Let me know if it's still not clear to you. \$\endgroup\$
    – Edward
    Nov 7, 2022 at 12:30
  • \$\begingroup\$ Ah, then the for loop runs as long as the second component returns a 1, when it returns 0 it breaks. It's clear now, thank you. \$\endgroup\$ Nov 7, 2022 at 22:09
  • \$\begingroup\$ I have another question. I need to add 2 more spaces to the message that I am going to show in the led display so that it finishes sweeping the last 2 characters in the message. Without putting the 2 spaces at the end of the string, in the last cycle of the sweep the 2 complete characters remain in the led display and do not finish sweeping as it should. I couldn't think of a simple way to fix this that doesn't make the code significantly more complex. Do you have any suggestions on this? \$\endgroup\$ Nov 7, 2022 at 22:15

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.