Timing

Hello, this is my first post here. I am new to programming and micro controllers and look forward to learning. Until now I have always done things the hardware way. Anyway, I have a question about a digital clock sketch I am trying. The problem is that the minutes advance every 40 seconds. I am sending a clock signal of 256 Hz to pin 3. Bear with me, I know this code is nasty.

int mins_adjustPin = A0;
int hours_adjustPin = A1;
int clock_inPin = 3;
int _1Pin = 2;  
int _2Pin = 4;
int _4Pin = 7;
int _8Pin = 8;
int select_mins_onesPin = A2;
int select_mins_tensPin = A3;
int select_hours_onesPin = A4;
int select_hours_tensPin = A5;
int clk_in = 0;
int readin;
int mins_ones;
int mins_tens;
int hours_ones;
int hours_tens;
unsigned char seconds = 0;
unsigned char mins = 0;
unsigned char hours = 0;
int mins_increase = 0;
int hours_increase = 0;

void display_time  ()  {
  
   mins_ones = mins % 10;	     
   if (mins >= 10)  {
     mins_tens = mins / 10 ;
   }
   else  {
       mins_tens = 0;
   }
  
    hours_ones = hours % 10;		   
    if (hours >= 10)  {
      hours_tens = hours / 10 ;
    }
    else  {
        hours_tens = 0;
    } 
   
   digitalWrite(select_mins_onesPin, HIGH);
   digitalWrite(_8Pin, HIGH && (mins_ones & B00001000));
   digitalWrite(_4Pin, HIGH && (mins_ones & B00000100));
   digitalWrite(_2Pin, HIGH && (mins_ones & B00000010));
   digitalWrite(_1Pin, HIGH && (mins_ones & B00000001));
   delay(1);
   
   digitalWrite(select_mins_onesPin, LOW);
   digitalWrite(select_mins_tensPin, HIGH);
   digitalWrite(_8Pin, HIGH && (mins_tens & B00001000));
   digitalWrite(_4Pin, HIGH && (mins_tens & B00000100));
   digitalWrite(_2Pin, HIGH && (mins_tens & B00000010));
   digitalWrite(_1Pin, HIGH && (mins_tens & B00000001));
   delay(1);
   
   digitalWrite(select_mins_tensPin, LOW);
   digitalWrite(select_hours_onesPin, HIGH);
   digitalWrite(_8Pin, HIGH && (hours_ones & B00001000));
   digitalWrite(_4Pin, HIGH && (hours_ones & B00000100));
   digitalWrite(_2Pin, HIGH && (hours_ones & B00000010));
   digitalWrite(_1Pin, HIGH && (hours_ones & B00000001));
   delay(1);
   
   digitalWrite(select_hours_onesPin, LOW);
   digitalWrite(select_hours_tensPin, HIGH);
   digitalWrite(_8Pin, HIGH && (hours_tens & B00001000));
   digitalWrite(_4Pin, HIGH && (hours_tens & B00000100));
   digitalWrite(_2Pin, HIGH && (hours_tens & B00000010));
   digitalWrite(_1Pin, HIGH && (hours_tens & B00000001));
   delay(1);
   digitalWrite(select_hours_tensPin, LOW);
   
}

void setup  ()  {
  
  pinMode(clock_inPin, INPUT);
  pinMode(_1Pin, OUTPUT);
  pinMode(_2Pin, OUTPUT);
  pinMode(_4Pin, OUTPUT);
  pinMode(_8Pin, OUTPUT);
  pinMode(select_mins_onesPin, OUTPUT);
  pinMode(select_mins_tensPin, OUTPUT);
  pinMode(select_hours_onesPin, OUTPUT);
  pinMode(select_hours_tensPin, OUTPUT);
  
  attachInterrupt( 1, Tick, RISING );
}

void loop  ()  {
  
  display_time();
  set_time  ();
  Tick();
}
    
void Tick  ()  {

  clk_in = clk_in + 1;
  if (clk_in > 256)  {
    seconds = seconds + 1;
    clk_in = 0;
    
  }
    if (seconds > 59)  {
      mins = mins + 1;
      seconds = 0;
  }
    if (mins > 59)  {
      hours = hours + 1;
      mins = 0;
    }
    if (hours > 12) {
      hours = 1;
    }
}

void set_time  ()  {
  
  pinMode(mins_adjustPin, INPUT);
  pinMode(hours_adjustPin, INPUT);
  
  mins_increase = digitalRead(mins_adjustPin);
  hours_increase = digitalRead(hours_adjustPin);
  
  if (mins_increase == LOW)  {
    delay(200);
    mins = mins + 1;
    seconds = 0;
  }
  if (hours_increase == LOW)  {
    delay(200);
    hours = hours + 1;
    seconds = 0;
  }
  
  display_time();
}

1 Tick is intended to be called as an interrupt routine on pin 3 rising. However, you also are calling Tick in your main loop. You don't need to do that (to test this hypothesis, take out the line AttachInterrupt and see if the clock still advances).

2 Look up the volatile keyword in the reference section, you should be using it on variables accessed by interrupts and other means.

3. Did you know that there is a millis() function that can be used as a clock basis, which is also crystal driven?

4 You have some code like:
if (hours >= 10) {
hours_tens = hours / 10 ;
}
else {
hours_tens = 0;
}
What value do you think hours_ten will have if you just write: hours_ten = hours/10 if hours is 0?, 1?, 9? 10? Put in a print statement
to find out.

I hope you have lots of fun with your Arduino.

I think the test should be >= 256, but that doesn't account for being that much out. Are you sure the clock pulse is 256 Hz?

Try making the relevant variables volatile, as they are being used in an ISR:

volatile int clk_in = 0;

volatile unsigned char seconds = 0;
volatile unsigned char mins = 0;
volatile unsigned char hours = 0;

Tick is intended to be called as an interrupt routine on pin 3 rising. However, you also are calling Tick in your main loop.

Oh yeah, that's your main problem.

void loop  ()  {

  display_time();
  set_time  ();
  Tick();
}

...


void set_time  ()  {
...
  display_time();
}

You don't need to call display_time in set_time as you are going to call it anyway a moment later.

Because of the 4 x delay(1) and because you are calling display_time twice per loop, that means you called Tick() an extra time per 8 mS. That means another 125 times per second. So you called Tick() 381 times a second, not 256 times a second, which almost exactly accounts for getting a tick every 40 seconds rather than every 60 seconds.

256/381 * 60 = 40.31 seconds

Hey thanks guys, you both taught me something. I took the call to Tick() out of the loop function and display_time out of set_time. Everything seems to be working now and I'm working on an alarm/snooze addition. More to come...