my first binary clock not working

I'm having trouble getting my binary clock working. I am new to arduino programming, but have done some basic led programs to get my feet wet.

I have just followed some simple examples from websites like instructables, but for some reason my led's are cycling every second. And the time seems to be reading 101, 202, 303, 404, ..., 2222, 2323

I'm really confused if its my equipment, wiring, or code. Any places in the code I should take a better look at?
Thank you for taking a look.

int second=0, minute=0, hour=0; //start the time on 00:00:00
int munit=0;
int hunit=0;
int valm=0;
int valh=0;
int ledstats=0;
int i=0;
boolean light = 1;

void setup() 
{ //set outputs and inputs

  pinMode(1, OUTPUT);
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
  pinMode(6, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT);
  pinMode(10, OUTPUT);
  pinMode(11, OUTPUT);
  pinMode(12, OUTPUT);
  pinMode(13, OUTPUT);
  pinMode(19, INPUT);  // pushbutton for setting minutes
  pinMode(14, INPUT);  // pushbutton for setting hours
}

void loop()
{
  // set up a local variable to hold the last time we moved forward one second
  static unsigned long lastTick = 0;  
  // static variables are initialized once and keep their values between function calls
  
  // move forward one second every 1000 milliseconds
  if (millis() - lastTick >= 1000) {
    lastTick = millis();
    second++;
  }
  
  // move forward one minute every 60 seconds
  if (second >= 60) {
    minute++;
    second = 0; // reset seconds to zero
  }
 
  // move forward one hour every 60 minutes
  if (minute >=60) {
    hour++;
    minute = 0; // reset minutes to zero
  }
  
  if (hour >=24) {
    hour=0;
    minute = 0; // reset minutes to zero
  }
  
  munit = minute%10; //sets the variable munit and hunit for the unit digits
  hunit = hour%10;
  
  ledstats = digitalRead(0);  // read input value, for setting leds off, but keeping count
  if (ledstats == LOW) {
    for(i=1;i<=13;i++){
    digitalWrite(i, LOW);}
  }
  
  else {
  
  //minutes units
  if(munit == 1 || munit == 3 || munit == 5 || munit == 7 || munit == 9) {  digitalWrite(1, HIGH);} else {  digitalWrite(1,LOW);}
  if(munit == 2 || munit == 3 || munit == 6 || munit == 7) {digitalWrite(2, HIGH);} else {digitalWrite(2,LOW);}
  if(munit == 4 || munit == 5 || munit == 6 || munit == 7) {digitalWrite(3, HIGH);} else {digitalWrite(3,LOW);}
  if(munit == 8 || munit == 9) {digitalWrite(4, HIGH);} else {digitalWrite(4,LOW);}
  //minutes 
  if((minute >= 10 && minute < 20) || (minute >= 30 && minute < 40) || (minute >= 50 && minute < 60))  {digitalWrite(5, HIGH);} else {digitalWrite(5,LOW);}
  if(minute >= 20 && minute < 40)  {digitalWrite(6, HIGH);} else {digitalWrite(6,LOW);}
  if(minute >= 40 && minute < 60) {digitalWrite(7, HIGH);} else {digitalWrite(7,LOW);}
  //hour units
  if(hunit == 1 || hunit == 3 || hunit == 5 || hunit == 7 || hunit == 9) {digitalWrite(8, HIGH);} else {digitalWrite(8,LOW);}
  if(hunit == 2 || hunit == 3 || hunit == 6 || hunit == 7) {digitalWrite(9, HIGH);} else {digitalWrite(9,LOW);}
  if(hunit == 4 || hunit == 5 || hunit == 6 || hunit == 7) {digitalWrite(10, HIGH);} else {digitalWrite(10,LOW);}
  if(hunit == 8 || hunit == 9) {digitalWrite(11, HIGH);} else {digitalWrite(11,LOW);}
  //hour
  if(hour >= 10 && hour < 20)  {digitalWrite(12, HIGH);} else {digitalWrite(12,LOW);}
  if(hour >= 20 && hour < 24)  {digitalWrite(13, HIGH);} else {digitalWrite(13,LOW);}
  }
  
  valm = digitalRead(19);    // add one minute when pressed
  if(valm== LOW) {
    minute++;
    second=0;
    delay(250);
  }
  valh = digitalRead(14);    // add one hour when pressed
  if(valh== LOW) {
    hour++;
    second=0;
    delay(250);
  }
}

I haven't looked at your code very close and don't have an answer for you but I will make a suggestion on simplifing the decoding of the time in setting the LED's.

It appears you're displaying time in BCD format.

Using 'ternary' syntax combined with the boolean '&' (bit and) the appropriate LED's states can be decode and set as follows:

        // --- minutes ones

        digitalWrite( 1, ((munit  & 0b00000001) ? HIGH : LOW)); // 1's
        digitalWrite( 2, ((munit  & 0b00000010) ? HIGH : LOW)); // 2's
        digitalWrite( 3, ((munit  & 0b00000100) ? HIGH : LOW)); // 4's
        digitalWrite( 4, ((munit  & 0b00001000) ? HIGH : LOW)); // 8's

        // --- minutes tens

        digitalWrite( 5, ((minute & 0b00000001) ? HIGH : LOW)); // 1's
        digitalWrite( 6, ((minute & 0b00000010) ? HIGH : LOW)); // 2's
        digitalWrite( 7, ((minute & 0b00000100) ? HIGH : LOW)); // 4's


        // --- hours ones

        digitalWrite( 8, ((hunit  & 0b00000001) ? HIGH : LOW)); // 1's
        digitalWrite( 9, ((hunit  & 0b00000010) ? HIGH : LOW)); // 2's
        digitalWrite(10, ((hunit  & 0b00000100) ? HIGH : LOW)); // 4's
        digitalWrite(11, ((hunit  & 0b00001000) ? HIGH : LOW)); // 8's

        // --- hours tens

        digitalWrite(12, ((hour   & 0b00000001) ? HIGH : LOW)); // 1's
        digitalWrite(13, ((hour   & 0b00000010) ? HIGH : LOW)); // 2's

Other suggestions to the code if you don't mind them at this stage of your project.

Thanks for the suggestion. I applied your suggestion and the LED's are acting a little different now. They are still cycling the time too fast, every second, but now the time starts at 1212, 2222, 3333, etc. haha, i don't know at this point.

to me it seems like my time keeping method isn't working properly. I'm really confused as to whats causing this though.

If it makes a difference, Im using Arduino Uno R3

This is where the serial port comes in handy.

Hint: It's related to -

    munit = minute	% 10;
    hunit = hour	% 10;

and the display of 'minute' and 'hour'.

Thanks for the Hint! I will focus on the hunit and munit variables. Hope I can figure it out!

! I will focus on the hunit and munit variables

Why?

my guess would be my hunit and munit values arent being stored correctly. But maybe i misread the hint

I'd say mis-focused on the wrong part.

You can simplify the display code a little more:

        // --- minutes ones

        digitalWrite( 1, (minute%10)  & 0b00000001); // 1's
        digitalWrite( 2, (minute%10)  & 0b00000010); // 2's
        digitalWrite( 3, (minute%10)  & 0b00000100); // 4's
        digitalWrite( 4, (minute%10)  & 0b00001000); // 8's

        // --- minutes tens

        digitalWrite( 5, (minute/10) & 0b00000001); // 1's
        digitalWrite( 6, (minute/10) & 0b00000010); // 2's
        digitalWrite( 7, (minute/10) & 0b00000100); // 4's


        // --- hours ones

        digitalWrite( 8, (hour%10) & 0b00000001); // 1's
        digitalWrite( 9, (hour%10) & 0b00000010)); // 2's
        digitalWrite(10, (hour%10) & 0b00000100); // 4's
        digitalWrite(11, (hour%10) & 0b00001000); // 8's

        // --- hours tens

        digitalWrite(12, (hour/10)   & 0b00000001); // 1's
        digitalWrite(13, (hour/10)   & 0b00000010); // 2's

Other suggestions to the code if you don't mind them at this stage of your project.
[/quote]

If for some reason the time setting buttons are reading LOW, the minutes and hours will increment about twice a second (total of 500 mSec of delays). Make sure you have a pull-up resistor on the input pins.

  valm = digitalRead(19);    // add one minute when pressed
  if(valm== LOW) {
    minute++;
    second=0;
    delay(250);
  }
  valh = digitalRead(14);    // add one hour when pressed
  if(valh== LOW) {
    hour++;
    second=0;
    delay(250);
  }

Well since you already gave it away -

    {
        uint8_t     digit;

        // --- minutes ones

        digit = (minute / 10);
        digitalWrite( 1, ((digit & 0b00000001) ? LED_ON : LED_OFF)); // 1's
        digitalWrite( 2, ((digit & 0b00000010) ? LED_ON : LED_OFF)); // 2's
        digitalWrite( 3, ((digit & 0b00000100) ? LED_ON : LED_OFF)); // 4's
        digitalWrite( 4, ((digit & 0b00001000) ? LED_ON : LED_OFF)); // 8's

        // --- minutes tens

        digit = (minute % 10);
        digitalWrite( 5, ((digit & 0b00000001) ? LED_ON : LED_OFF)); // 1's
        digitalWrite( 6, ((digit & 0b00000010) ? LED_ON : LED_OFF)); // 2's
        digitalWrite( 7, ((digit & 0b00000100) ? LED_ON : LED_OFF)); // 4's


        // --- hours ones

        digit = (hour / 10);
        digitalWrite( 8, ((digit & 0b00000001) ? LED_ON : LED_OFF)); // 1's
        digitalWrite( 9, ((digit & 0b00000010) ? LED_ON : LED_OFF)); // 2's
        digitalWrite(10, ((digit & 0b00000100) ? LED_ON : LED_OFF)); // 4's
        digitalWrite(11, ((digit & 0b00001000) ? LED_ON : LED_OFF)); // 8's

        // --- hours tens

        digit = (hour % 10);
        digitalWrite(12, ((digit & 0b00000001) ? LED_ON : LED_OFF)); // 1's
        digitalWrite(13, ((digit & 0b00000010) ? LED_ON : LED_OFF)); // 2's
    }

... and if I'm not mistaken your time tracking code could be simplified (and more readable in my opinion) with -

    // static variables are initialized once and keep their values between function calls
    // set up a local variable to hold the last time we moved forward one second

    static unsigned long lastTick = 0UL;

    if ( (millis() - lastTick) >= 1000UL )
    {
        lastTick = millis();
    
        if ( 0 == (second = ((++second) % 60)) )
        {
            if ( 0 == (minute = ((++minute) % 60)) )
            {
                hour = ((++hour) % 24);
            }
        }
    }

thank you so much for your help. I will look into pull up resistors (since im not familiar with them :blush: )
you guys are too kind helping me cleanup the code, I am in no means an efficient programmer at this point. this project is getting me hooked, even though it has been discouraging lol

Come back and let us know how it's going. Preferably with complete code posting.

This can be further simplified and shortened if you don't mind iterative improvement as a learning method.

Have fun!

Success!!

Thanks for pointing me in the right direction, I modified the code for the button inputs and it worked like a charm. To make sure that the LED's were displaying the time correctly I set the time in the top line of the code to a few different times to make sure they read correctly (taking out the code that incremented the seconds, minutes, and hours). That all worked great. so then I added back the code at the bottom to increment minutes and hours when the buttons were pressed, and the LED's went hyper again. So I changed the values from LOW to HIGH and this worked!

Here's the code that works, thanks to your help.

int second=0, minute=59, hour=23; //start the time on 00:00:00
int munit=0;
int hunit=0;
int valm=0;
int valh=0;
int ledstats=0;
int i=0;
boolean light = 1;

void setup() 
{ //set outputs and inputs

  pinMode(1, OUTPUT);
  pinMode(2, OUTPUT);
  pinMode(3, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(5, OUTPUT);
  pinMode(6, OUTPUT);
  pinMode(7, OUTPUT);
  pinMode(8, OUTPUT);
  pinMode(9, OUTPUT);
  pinMode(10, OUTPUT);
  pinMode(11, OUTPUT);
  pinMode(12, OUTPUT);
  pinMode(13, OUTPUT);
  pinMode(19, INPUT);  // pushbutton for setting minutes
  pinMode(14, INPUT);  // pushbutton for setting hours
  
}

void loop()
{
  
  // set up a local variable to hold the last time we moved forward one second
  static unsigned long lastTick = 0;  
  // static variables are initialized once and keep their values between function calls
  
  // move forward one second every 1000 milliseconds
  if (millis() - lastTick >= 1000) {
    lastTick = millis();
    second++;
  }
  
  // move forward one minute every 60 seconds
  if (second >= 60) {
    minute++;
    second = 0; // reset seconds to zero
  }
 
  // move forward one hour every 60 minutes
  if (minute >=60) {
    hour++;
    minute = 0; // reset minutes to zero
  }
  
  if (hour >=24) {
    hour=1;
    minute = 0; // reset minutes to zero
  }

  
  munit = minute % 10; //sets the variable munit and hunit for the unit digits
  hunit = hour % 10;
  
  ledstats = digitalRead(0);  // read input value, for setting leds off, but keeping count
  if (ledstats == LOW) {
    for(i=1;i<=13;i++){
    digitalWrite(i, LOW);}
  }
  
  else {
  
  // --- minutes ones

        digitalWrite( 1, (minute%10)  & 0b00000001); // 1's
        digitalWrite( 2, (minute%10)  & 0b00000010); // 2's
        digitalWrite( 3, (minute%10)  & 0b00000100); // 4's
        digitalWrite( 4, (minute%10)  & 0b00001000); // 8's

        // --- minutes tens

        digitalWrite( 5, (minute/10) & 0b00000001); // 1's
        digitalWrite( 6, (minute/10) & 0b00000010); // 2's
        digitalWrite( 7, (minute/10) & 0b00000100); // 4's


        // --- hours ones

        digitalWrite( 8, (hour%10) & 0b00000001); // 1's
        digitalWrite( 9, (hour%10) & 0b00000010); // 2's
        digitalWrite(10, (hour%10) & 0b00000100); // 4's
        digitalWrite(11, (hour%10) & 0b00001000); // 8's

        // --- hours tens

        digitalWrite(12, (hour/10)   & 0b00000001); // 1's
        digitalWrite(13, (hour/10)   & 0b00000010); // 2's

  }
  
  valm = digitalRead(19);    // add one minute when pressed
  if(valm== HIGH) {
    minute++;
    second=0;
    delay(250);
  }
  valh = digitalRead(14);    // add one hour when pressed
  if(valh== HIGH) {
    hour++;
    second=0;
    delay(250);
  }

 
}

I also changed this section. Before it would reset hour to 0 instead of 1 after 24 hrs.

if (hour >=24) {
    hour=1;  // after 24 hrs reset to 1am instead of 0
    minute = 0; // reset minutes to zero
  }

Excellent!

Does this also work?

#define SIZEOF_ARRAY(ARRAY)             (sizeof(ARRAY) / sizeof(ARRAY[0]))


// CONSTANTS

const uint8_t   pinBUTTON_LIGHTS        = 0;        // SYNONYM ENHANCES READABILITY

const uint8_t   pinBUTTON_MINUTES       = 19;       // SYNONYM ENHANCES READABILITY
const uint8_t   pinBUTTON_HOURS         = 14;       // SYNONYM ENHANCES READABILITY

const uint8_t   LED_OFF                 = LOW;      // SYNONYM ENHANCES READABILITY
const uint8_t   LED_ON                  = HIGH;     // SYNONYM ENHANCES READABILITY

const uint8_t   BUTTON_UP               = LOW;      // SYNONYM ENHANCES READABILITY
const uint8_t   BUTTON_DOWN             = HIGH;     // SYNONYM ENHANCES READABILITY


const uint8_t   pinsLED[]               = { 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13 };

const uint8_t   pinsMinutesOnesLEDS[]   = { 1, 2, 3, 4 };
const uint8_t   pinsMinutesTensLEDS[]   = { 5, 6, 7 };

const uint8_t   pinsHoursOnesLEDS[]     = { 8, 9, 10, 11 };
const uint8_t   pinsHoursTensLEDS[]     = { 12, 13 };


int     second;                                     // GLOBAL ZERO'D BY COMPILER <-uint8_t to int
int     minute;                                     // GLOBAL ZERO'D BY COMPILER <-uint8_t to int
int     hour;                                       // GLOBAL ZERO'D BY COMPILER <-uint8_t to int


void setDigits(uint8_t digit, const uint8_t pinsLED[], uint8_t lengthArray)
{
    for ( int i = lengthArray; i--; )
    {
        digitalWrite(pinsLED[i], ((digit & (1 << i)) ? LED_ON : LED_OFF));
    }
}


void loop()
{
    // static variables are initialized once and keep their values between function calls
    // set up a local variable to hold the last time we moved forward one second

    static unsigned long lastTick = 0UL;

    if ( (millis() - lastTick) >= 1000UL )
    {
        lastTick = millis();
    
        if ( 0 == (second = ((++second) % 60)) )
        {
            if ( 0 == (minute = ((++minute) % 60)) )
            {
                hour = ((++hour) % 24);
            }
        }
    }
    

    if ( BUTTON_DOWN == digitalRead(pinBUTTON_LIGHTS) )
    {
        for ( int i = SIZEOF_ARRAY(pinsLED); i--; )
        {
            digitalWrite(pinsLED[i], LED_OFF);
        }
    }
    else
    {
        setDigits((minute / 10), pinsMinutesOnesLEDS, SIZEOF_ARRAY(pinsMinutesOnesLEDS));   // minutes ones
        setDigits((minute % 10), pinsMinutesTensLEDS, SIZEOF_ARRAY(pinsMinutesTensLEDS));   // minutes tens

        setDigits((hour   / 10), pinsHoursOnesLEDS,   SIZEOF_ARRAY(pinsHoursOnesLEDS));     // hours ones
        setDigits((hour   % 10), pinsHoursTensLEDS,   SIZEOF_ARRAY(pinsHoursTensLEDS));     // hours tens
    }

    
    if ( BUTTON_DOWN == digitalRead(pinBUTTON_MINUTES) )
    {
        minute++;
        second  = 0;

        delay(250);
    }

    if ( BUTTON_DOWN == digitalRead(pinBUTTON_HOURS) )
    {
        hour++;
        second  = 0;

        delay(250);
    }
}


void setup() 
{
    pinMode(pinBUTTON_MINUTES, INPUT);
    pinMode(pinBUTTON_HOURS, INPUT);

    for ( int i = SIZEOF_ARRAY(pinsLED); i--; )
    {
        pinMode(pinsLED[i], OUTPUT);
    }
}

EDIT: Changed data type of 'second', 'minute' and 'hour'

no this doesn't do anything, no lights on and nothing when buttons are pressed.

jcharsh,

My mistake.
If you can spare the time try it again using the modified code changed above.

Either way thanks for the effort!