7-Segment, 3-digit display not displaying correctly

Question

I have written the code that should work, but for some reason the numbers do not display correctly. When I choose to display 3 times the same number it works correctly (so pin connections are correct) but when trying to display different numbers on each digit things get messy. Can anyone see what is wrong with the code? I don't seem to find it. thanks!

Working when 3 digits are the same:

a busy cat

Not working with different numbers (should be 3, 2, 1:

a busy cat

And the code:

int a = 2; //Defining segments corresponding to the pins
int b = 3;
int c = 4;
int d = 5;
int e = 6;
int f = 7;
int g = 8;
void setup(void) { //Setting Pins as output
 pinMode(A0, OUTPUT);
 pinMode(A1, OUTPUT);
 pinMode(A2, OUTPUT);
 pinMode(a, OUTPUT);
 pinMode(b, OUTPUT);
 pinMode(c, OUTPUT);
 pinMode(d, OUTPUT);
 pinMode(e, OUTPUT);
 pinMode(f, OUTPUT);
 pinMode(g, OUTPUT);
}
void WriteNum(int PositionN, int Number) {
 int Position;
 int NumberSegments[10][7] = {{a,b,c,d,e,g}, //Defining segments for forming numbers
 {b,c},
 {a,b,d,e,g},
 {a,b,c,d,g},
 {b,c,f,d},
 {a,c,d,f,g},
 {a,c,d,e,f,g},
 {a,b,c},
 {a,b,c,d,e,f,g},
 {a,b,c,d,f,g}};
 digitalWrite(A0, HIGH); //Setting begin conditions (All segments low and no digits turned on)
 digitalWrite(A1, HIGH);
 digitalWrite(A2, HIGH);
 digitalWrite(2, LOW);
 digitalWrite(3, LOW);
 digitalWrite(4, LOW);
 digitalWrite(5, LOW);
 digitalWrite(6, LOW);
 digitalWrite(7, LOW);
 digitalWrite(8, LOW);
 digitalWrite(9, LOW);
 switch (PositionN) { //Deciding which digit to turn on
 case 1:
 Position = A0;
 break;
 case 2:
 Position = A1;
 break;
 case 3:
 Position = A2;
 break;
 }
 digitalWrite(Position, LOW);
 for (int i = 0; i < sizeof(NumberSegments[Number]); i++){ //For each segment in number to display
 digitalWrite(NumberSegments[Number][i], HIGH); //Light that segment
 }
 delay(5);
}
void loop(void) {
 WriteNum(1,2);
 WriteNum(2,2);
 WriteNum(3,2);
}

• Gerben: sizeof(NumberSegments[Number] is 14 (as an int is two bytes; 2*7=14). The least you should do is use sizeof(NumberSegments[0]/sizeof(NumberSegments[0][0]. Since you declare the sub-arrays to be of 7 integers, {b,c}, will actually be interpreted as {b,c,0,0,0,0,0},. So your code is also setting pin 0 to HIGH. Not sure if this have anything to do with your problem.
• Sam: That was indeed the problem thanks! Because the sizeof() = 14, it will continue reading the next item in the list and light those segments as well. Setting pin 0 high did not appear to cause any problems but I filled the remaining spaces in the list with copies of an already lit segment anyway. I still wonder why this did not cause problems when the three digits were the same?

Answer 1

I have three recommendations for you. @Gerben was correct as to your problem, but doing the following three steps will improve the speed of your code:

1. Change your definition for NumberSegments to be [10][8], and put the number of active segments as the first number of each array:

    ...[10][8] = {{6,a,b,c,d,e,g}, // ...
    {2,b,c},
    {5,a,b,d,e,g}, // etcetera
   

2. Change your for loop as follows:

   for (int i = 1; i <= NumberSegments[Number][0]; i++) { // ...
    digitalWrite(NumberSegments[Number][i], HIGH); // ...
   }
   

   This will only turn on the minimum number of segments required. Note the loop now starts at 1, and uses <= instead of <.

3. Put the keyword static in front of NumberSegments:

   static int NumberSegments[10][8] = {{ // ...
   

This last one is a language nicety. Every time the function WriteNum is called, the poor thing has to re-create the (large) NumberSegments array - and it’s the same every time! The keyword static means "only create this variable once". Next time into the function, it’ll already be available: no processing required!

You might also consider moving int position; below NumberSegments, since it’s convention to put static variables first in a function.

You can also use the keyword const on NumberSegments, since its values never change:

static const int NumberSegments[10][8] = {{ // ...

• Sam: Thanks John! Much appreciated. Looks way more efficient indeed. I believe I'm getting the hang of it