Something's wrong with my code for a 4 digit 7 segment clock display

Question

I'm trying to do a clock using 4x7 segment displays but once I run the code the Display doesn't turn on

here's my Code that I'm like 90% sure that there's nothing wrong with it

bool A = 2;
bool B = 3;
bool C = 4;
bool D = 5;
bool E = 6;
bool F = 7;
bool G = 8;
int PWR1 = 10;
int PWR2 = 11;
int PWR3 = 12;
int PWR4 = 13;
int TimeSec = 0;
int TimeMin1 = 0;
int TimeMin2 = 0;
int TimeHour1 = 8;
int TimeHour2 = 0;
int Display = 0;
int Loop = 0;
int Digit = 0;
int Memory1 = 0;
int Memory2 = 0;
int Memory3 = 0;
int Memory4 = 0;
int Rewrite1 = 0;
int Rewrite2 = 0;
int Rewrite3 = 0;
int Rewrite4 = 0;

void setup() {
  pinMode(A, OUTPUT);
  pinMode(B, OUTPUT);
  pinMode(C, OUTPUT);
  pinMode(D, OUTPUT);
  pinMode(E, OUTPUT);
  pinMode(F, OUTPUT);
  pinMode(G, OUTPUT);
  pinMode(PWR1, OUTPUT);
  pinMode(PWR2, OUTPUT);
  pinMode(PWR3, OUTPUT);
  pinMode(PWR4, OUTPUT);
}
void loop() {
  loop:
  delay(250);
  Digit = Digit + 1;
  if (Digit > 4) { Digit = 1; }
  if (Digit == 1) { goto digit1; }
  if (Digit == 2) { goto digit2; }
  if (Digit == 3) { goto digit3; }
  if (Digit == 4) { goto digit4; }

  // Supposedly turns on the 7-segment display that is next in line to have its digit edited but doesn't work?
  digit1:
  digitalWrite(PWR1, HIGH);
  digitalWrite(PWR2, LOW);
  digitalWrite(PWR3, LOW);
  digitalWrite(PWR4, LOW);
  Display = Memory1;
  goto skip;
  digit2:
  digitalWrite(PWR2, HIGH);
  digitalWrite(PWR1, LOW);
  digitalWrite(PWR3, LOW);
  digitalWrite(PWR4, LOW);
  Display = Memory2;
  goto skip;
  digit3:
  digitalWrite(PWR3, HIGH);
  digitalWrite(PWR1, LOW);
  digitalWrite(PWR2, LOW);
  digitalWrite(PWR4, LOW);
  Display = Memory3;
  goto skip;
  digit4:
  digitalWrite(PWR4, HIGH);
  digitalWrite(PWR1, LOW);
  digitalWrite(PWR2, LOW);
  digitalWrite(PWR3, LOW);
  Display = Memory4;
  skip:

   // Manual Turning on of LEDs is overcomplicated and can be made much shorter yes I know :(
   if (Display == 1) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(B, LOW);
    digitalWrite(C, LOW);
  }
  if (Display == 2) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(B, LOW);
    digitalWrite(G, LOW);
    digitalWrite(E, LOW);
    digitalWrite(D, LOW);
  }
  if (Display == 3) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(B, LOW);
    digitalWrite(C, LOW);
    digitalWrite(G, LOW);
    digitalWrite(D, LOW);
  }
  if (Display == 4) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(B, LOW);
    digitalWrite(G, LOW);
    digitalWrite(F, LOW);
    digitalWrite(C, LOW);
  }
  if (Display == 5) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(F, LOW);
    digitalWrite(G, LOW);
    digitalWrite(C, LOW);
    digitalWrite(D, LOW);
  }
  if (Display == 6) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(F, LOW);
    digitalWrite(G, LOW);
    digitalWrite(C, LOW);
    digitalWrite(D, LOW);
    digitalWrite(E, LOW);
  }
  if (Display == 7) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(B, LOW);
    digitalWrite(C, LOW);
  }
  if (Display == 8) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(B, LOW);
    digitalWrite(C, LOW);
    digitalWrite(D, LOW);
    digitalWrite(F, LOW);
    digitalWrite(G, LOW);
    digitalWrite(E, LOW);
  }
  if (Display == 9) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(B, LOW);
    digitalWrite(C, LOW);
    digitalWrite(D, LOW);
    digitalWrite(F, LOW);
    digitalWrite(G, LOW);
  }
  if (Display == 0) {
    digitalWrite(A, HIGH);
    digitalWrite(B, HIGH);
    digitalWrite(C, HIGH);
    digitalWrite(D, HIGH);
    digitalWrite(E, HIGH);
    digitalWrite(F, HIGH);
    digitalWrite(G, HIGH);
    digitalWrite(A, LOW);
    digitalWrite(B, LOW);
    digitalWrite(C, LOW);
    digitalWrite(D, LOW);
    digitalWrite(E, LOW);
    digitalWrite(F, LOW);
  }

  // Checks how many times it has looped over
  Loop = Loop + 1;
  if (Loop < 4) {goto loop;}
  else
  {
    // Resets Loop count and counts 1 second (normally it has a delay(1000) but the delay(250) on the loop to account for LED's turning on is counting for me )
    Loop = 0;
    TimeSec = (TimeSec + 1);
    if (TimeSec == 60) {
      TimeMin1 = (TimeMin1 + 1);
      Memory1 = TimeMin1;
      TimeSec = 0;
      if (TimeMin1 == 10) {
        TimeMin2 = (TimeMin2 + 1);
        Memory2 = TimeMin2;
        TimeMin1 = 0;
        Memory1 = TimeMin1;
        if (TimeMin2 == 7) {
          TimeHour1 = (TimeHour1 + 1);
          Memory3 = TimeHour1;
          TimeMin2 = 0;
          Memory2 = TimeMin2;
          if (TimeHour1 == 10) {
            TimeHour2 = 1;
            Memory4 = TimeHour2;
            TimeHour1 = 0;
            Memory3 = TimeHour1;
            if (TimeHour1 == 3 && TimeHour2 == 1) {
              TimeHour2 = 0;
              Memory4 = TimeHour2;
              TimeHour1 = 1;
              Memory3 = TimeHour1;
            }
          }
        }
      }
    }
  }
}

And here's how it's Wired up

Also I think that I have to make a clock of my own for the Timer or else it will become a problem later on but I don't know how to make 2 loops so if someone can tell me how to do it (preferably in a way that I can understand xD) I'd be very grateful

Notes:

The Displays are Common Anode so to turn on a LED I used digitalWrite(ledpin, LOW) for a negative output
I'm not using a 4 digit 7-segment display -- I'm using 4 individual 7-segment displays
Resistors are 220 Ohm
The Picture was made using https://circuits.io
I'm still new at this so my code may have some newb mistakes or just stuff that was made overcomplicatedly simple if you know what I mean :P

UPDATE 1:

I fixed it now the problem was I was setting the variables to bool instead of integer but I still can't figure out how to do a self-sufficient clock because the current code now runs but doesn't turn on as fast as I need to

Regarding “here's my Code that I'm like 90% sure that there's nothing wrong with it”: Because it doesn't work, you should instead be 100% sure there's something wrong with it. — James Waldby - jwpat7, May 21, 2017 at 4:06

Delta_G · Accepted Answer · 2017-05-21 03:49:53Z

There are several bugs that I spot right off the bat. First off, you should never need goto in C++. You should be able to use if, for, and while to completely eliminate those. goto makes code hard to follow and read, so it makes it nearly impossible for someone else to try to debug. Be nice to yourself and forget that goto ever existed. I would suggest hitting a C++ tutorial and learning about functions, because most of the places you've used goto, what you really needed was a function call.

Second is this:

bool A = 2;
bool B = 3;
bool C = 4;
bool D = 5;
bool E = 6;
bool F = 7;
bool G = 8;

A bool is meant to hold a true or false value, not a pin number. You should use int here, or better yet save a few bytes and use byte. But not bool. The second issue here are the single capital letter variable names. Those are always a bad idea. Many of them (B and F for sure) are already defined as macros in the core. They're not available for you to use. Single letter variable names are bad form, but single capital letters are right out.

Third is the way you have the digits connected. There should be a resistor for each segment, or you will have brightness differences depending on how many segments are in use at any one time. But that can wait until you get the code worked out.

Finally, here is some code for working with 7-segment displays. It's a lot easier to work with than the whole thing you've got written out. See if you can follow it and ask questions if you don't understand. This code would count 0 to 9 over and over on a single 7 segment display.

byte digits[] = {0b1111110 , 0b0110000, 0b1101101, 0b1111001, 
0b0110011, 0b1011011, 0b1011111, 0b1110000, 0b1111111, 0b1111011};

// pins to segments g,f,e,d,c,b,a
byte pins[] = {3,4,5,6,7,8,9};

void displayNumber(int aNumber) {  
  for (int i = 0; i < 7; i++) {
    digitalWrite(pins[i], digits[aNumber] & (1 << i));
  }
}

void setup(){
  for (int i = 0; i < 7; i++){
    pinMode(pins[i], OUTPUT);
  }
}


void loop(){
  for (int i = 0; i < 10; i++) {
    displayNumber(i);
    delay(1000);
  }
}

I thought goto was easier to follow as it's a custom label that would be easier to spot but if you say so :P --- I think I'm going to stick to my Overcomplicatedly simple code as it's easier for me to understand but I'll make a separate build to use your code so I can play around with it --- I'm still stuck at how to make a separate clock since I don't know how to make a 2nd loop that works simultaneously with another if it's even possible — Dudamesh 192, May 21, 2017 at 3:56
The same argument you make for goto applies doubly to a function. It gives you a definite place in the code to go to with a name that is easy to spot, but doesn't require all the convoluted logic to then skip sections. When functions return they go right back to the same spot in the code where from they were called. And they can bring back information with them. Learn about functions and you'll see what I mean. — Delta_G, May 21, 2017 at 4:03

Stack Exchange Network

current community

your communities

more stack exchange communities

Something's wrong with my code for a 4 digit 7 segment clock display

1 Answer 1

Your Answer

Not the answer you're looking for? Browse other questions tagged arduino-uno display 7-segment or ask your own question.

Hot Network Questions

Something's wrong with my code for a 4 digit 7 segment clock display

1 Answer 1

Your Answer

Sign up or log in

Post as a guest

Not the answer you're looking for? Browse other questions tagged arduino-uno display 7-segment or ask your own question.

Related

Hot Network Questions