Arduino driver for 7 segment display with bit-shift register

Question

This code counts down from 30 to -999 on a 7-segment 4-digit display; it works but I don't know if it is a good way to do it or if it follows best practices. I want to start writing libraries for components that are not specifically made for Arduino. This is my first attempt. Please tell me if there are any problems with it or if there is some way for me to improve it. I am working on also making it able to show decimals but am not done yet.

//This is code to put out any signed integer to a seven segment display using a bit shift register.
int number[12] = {80,95,50,22,29,148,144,94,16,28,191,16}; //Values 0 through 9 on a 7-segment LED display, negative symbol is 10 and subtract 11 from any number to put a decimal
int shiftPins[3] = {7,8,9}; // (clock pin, data pin, latch pin)
int digitPins[4] = {2,3,4,6}; // (ones, tens, hundreds, thousands)
int show[4]; // (thousands, hundreds, tens, ones)
void setup(){
 pinMode(digitPins[0], OUTPUT);
 pinMode(digitPins[1], OUTPUT);
 pinMode(digitPins[2], OUTPUT);
 pinMode(digitPins[3], OUTPUT);
 pinMode(shiftPins[0], OUTPUT);
 pinMode(shiftPins[1], OUTPUT);
 pinMode(shiftPins[2], OUTPUT);
}
void loop(){
 print2LEDs(30-millis()/1000);
}
void print2LEDs(int number2show){
 if(number2show >= 0){
 show[0] = ((int)number2show/1000);
 show[1] = ((int)number2show%1000)/100;
 show[3] = (((int)number2show%1000)%100)/10;
 show[2] = ((((int)number2show%1000)%100)%10);
 }
 else {
 number2show = -(number2show);
 show[0] = 10;
 show[1] = ((int)number2show%1000)/100;
 show[3] = (((int)number2show%1000)%100)/10;
 show[2] = ((((int)number2show%1000)%100)%10);
 }
 showDigit(show[0],digitPins[0]);
 showDigit(show[1],digitPins[1]);
 showDigit(show[2],digitPins[2]);
 showDigit(show[3],digitPins[3]);
}
void showDigit(int i, int f){
 digitalWrite(shiftPins[2], 0);
 digitalWrite(f, 1);
 shiftOut(shiftPins[1], shiftPins[0], LSBFIRST, number[i]);
 digitalWrite(f, 0); 
 digitalWrite(shiftPins[2], 1);
}

• 200_success: What you may and may not do after receiving answers. I've rolled back Rev 3 → 2.

Answer 1

What's really in show[2]?`

According to your comment:

int show[4]; // (thousands, hundreds, tens, ones)

the digits in show are in descending order. But according to your code:

show[0] = ((int)number2show/1000);
show[1] = ((int)number2show%1000)/100;
show[3] = (((int)number2show%1000)%100)/10;
show[2] = ((((int)number2show%1000)%100)%10);

you swapped show[2] and show[3]. Was this a copy paste error?

Simplification

Your code for setting the show array can be simplified.

1. There's no need for an int cast because everything is already an int.
2. (((n % 1000) % 100) % 10) is equivalent to n % 10
3. Three of the four lines in the if and else case are common.

I would rewrite it like this:

 if (number2show >= 0) {
 show[0] = (number2show / 1000) % 10;
 } else {
 show[0] = MINUS_SIGN;
 number2show = -number2show;
 }
 show[1] = (number2show / 100) % 10;
 show[2] = (number2show / 10) % 10;
 show[3] = number2show % 10;
 for (int i=0; i < 4; i++)
 showDigit(show[i], digitPins[i]);

Variable scopes

I'm not sure if you use show anywhere else, but if you don't then you should make it be a local variable instead of a global. Also, your other global arrays can be marked const since they are read-only.

• Magister Ludi: I didn't think to use defines for the minus and decimal signs, that makes it much clearer in the code; it doesn't rely on comments. The show[2] vs [3] thing was a mistake, I copied the code from when I had accidentally swapped the pins in the wiring.

Answer 2

Variables and identifiers

I think you can find better variable names, and organize the variables in a more natural (hence readable) way.

int number[12] = {80,95,50,22,29,148,144,94,16,28,191,16}; //Values 0 through 9 on a 7-segment LED display, negative symbol is 10 and subtract 11 from any number to put a decimal
int shiftPins[3] = {7,8,9}; // (clock pin, data pin, latch pin)
int digitPins[4] = {2,3,4,6}; // (ones, tens, hundreds, thousands)
int show[4]; // (thousands, hundreds, tens, ones)

Is number[] an array of bit patterns to send to the shift registers? In that case it should show in the name: they are bit patterns, not numbers. Also, bit patterns are much more readable in hexadecimal than decimal. If you have access to c99 (what is your compiler?), you can further add to readability with explicit designators:

int number_bit_pattern[] = {
 [0] = 0x50,
 [1] = 0x5f,
};

Likewise, shiftPins is not very descriptive. Why not this:

int pin_clock = 7;
int pin_data = 8;
int pin_latch = 9;

Same for digitPins.

It is also unclear for me what show[] does. It sounds like a flag that you set when it is time to show the digit, but from the code this is not it. Is it the bit pattern that you want to show? Then I think you need a better name.

The input parameters to showDigit are not descriptive enough either: (int i, int f). Find good names that describe what i and f actually are.

Your function loop() is not a loop. If it's not confusing for you now, it will be for someone else, or for yourself in the future.

Naming things is very hard, but you should take the time to find good identifiers, both for functions and variables. It's time well used.

Line length

Traditionally, code width is limited to 80 characters. People argue that this comes from the time when we had limited monitors, but SO is a good example of why the 80-chars limit is desirable. I had to scroll several times to read your comment. I often have 3 files open beside each other on my 1920x1200 monitor, so I appreciate 80 chars because of this too.