Button Firing Interrupt Causes Code To Stop

I am currently interfacing my Arduino with a DS1307 RTC chip attached to Analog Inputs 4 and 5, a DS32kHz oscillator, and a momentary button with a pull-down resistor attached to Digital Pin 2. I would like to increment the number of minutes that the RTC is set to by 1 each time the button is pressed, so I am using this code:

#include <WProgram.h>
#include <Wire.h>
#include <DS1307.h>

int time[3];
int fullTime;
int onTime = 700; // time to turn light on
int offTime = 1900; // time to turn light off
volatile int state = LOW;
bool PM = false;
bool useSerialDebug = true; // set to true to give serial output
int minutes = 0;
int hours = 0;

void setup()
{
  if (useSerialDebug)
    Serial.begin(9600);
  setRTC(0, minutes, hours, 0, 0, 0, 0);
  pinMode(9, OUTPUT);
  attachInterrupt(0, setMins, RISING);
}

void setRTC(int sec, int mins, int hr, int dow, int date, int mth, int yr)
{
  RTC.stop();
  RTC.set(DS1307_SEC, sec);        //set the seconds
  RTC.set(DS1307_MIN, mins);     //set the minutes
  RTC.set(DS1307_HR, hr);       //set the hours
  RTC.set(DS1307_DOW, dow);       //set the day of the week
  RTC.set(DS1307_DATE, date);       //set the date  
  RTC.set(DS1307_MTH, mth);        //set the month
  RTC.set(DS1307_YR, yr);         //set the year
  RTC.start();
}

void setMins()
{
  if (minutes < 60)
    minutes = minutes + 1;
  else if (minutes == 60)
  {
    minutes = 0;
    if (hours < 24)
      hours = hours + 1;
    else if (hours == 24)
      hours = 0;
  }
  setRTC(0, minutes, hours, 0, 0, 0, 0);
}
/*
void setHours()
{
  if (hours < 24)
    hours = hours + 1;
  else if (hours == 24)
    hours = 0;
  setRTC(0, minutes, hours, 0, 0, 0, 0);
}*/

void loop()
{
  // get the time, assign it to the array values
  time[0] = RTC.get(DS1307_HR, true);
  time[1] = RTC.get(DS1307_MIN, false);
  time[2] = RTC.get(DS1307_SEC, false);
  // create a military-time integer
  fullTime = (time[0] * 100) + time[1];

  // adjust for 24h time - subtract 12h and set the PM flag
  if (time[0] > 12)
  {
    time[0] = time[0] - 12;
    PM = true;
  }

  // serial stuff
  if (useSerialDebug)
  {
    for (int i = 0; i < 3; i++)
    {
      if (time[i] < 10)
      {
        Serial.print("0");  
      }

      Serial.print(time[i]);

      if (i < 2)
        Serial.print(":");
    }

    if (PM)
      Serial.println("PM");
    else
      Serial.println("AM");

    Serial.print("Full Time String: ");
    Serial.println(fullTime);  
  }
  // end serial stuff

  // turn off if its earlier than the onTime, or later or equal to the offTime
  if (fullTime < onTime || fullTime >= offTime)
    digitalWrite(9, LOW);
  else
    digitalWrite(9, HIGH);
  delay(1000);
}

The code runs fine and shows serial output until I press the button. Once I press the button, I stop receiving serial output. Pushing the button again and again makes no noticeable difference. I have to restart the Arduino to get it running again. What am I missing?

I don't know the that RTC library but if it uses interrupts then it may be interfering with your button interrupt.

You don't need interrupts, why not simply check to see if the button is pressed in loop (where you can also check for bouncing)

The problem I came across before is that if I didnt add a delay in my checking code, if the button is held down, it will continue looping and returning true, and will do whatever is inside the code block more than once. I havent used interrupts before and figured that maybe that would be a better way to get around the problem I came across before.

EDIT Also, I did a quick CTRL+F through the Wire and DS1307 libraries but didnt have any matches for 'interrupt'

The problem I came across before is that if I didnt add a delay in my checking code, if the button is held down, it will continue looping and returning true, and will do whatever is inside the code block more than once. I havent used interrupts before and figured that maybe that would be a better way to get around the problem I came across before.

Interrupt or no, you need some kind of delay (using delay or millis) to debounce the switch unless you add hardware debouncing. But assuming the switch is debounced, you can keep track of the state (up or down) and only increment the time once when the (debounced) state changes from up to down.

The Wire interrupt handler is the the twi.c file (in the wire/utility directory), its: SIGNAL(TWI_vect)

Here is some code based on the standard arduino debounce example that you can use. Its modified to have a function that returns true once per debounced button press.
The code uses the internal pull-up resistors, if you use external pull-down resistors then make the changes noted in the code.

const int buttonPin = 2;     // the number of the pushbutton pin
int count = 0;

void setup() {
  pinMode(buttonPin, INPUT);
  digitalWrite(buttonPin, HIGH); // turn on internal pull-ups if needed
  Serial.begin(9600);
}

void loop() {
   if(buttonPressed()) {
      count = count + 1;
      Serial.println(count);
   }
}

//returns true if a button is pressed or has been pressed since the last call
// will not return true again until the button is released and pressed again
boolean buttonPressed()
{
static boolean buttonState;             // the current reading from the input pin
static boolean lastButtonState = LOW;   // the previous reading from the input pin
static boolean result = false;       // the value returned by function
static long lastDebounceTime = 0;  // the last time the output pin was toggled
const long debounceDelay = 50;    // the debounce time; increase if the output flickers

  // read the state of the switch into a local variable:
  int reading = digitalRead(buttonPin);
  // If the switch changed, due to noise or pressing:
  if (reading != lastButtonState) { 
    lastDebounceTime = millis();    // reset the debouncing timer
  }   
  if ((millis() - lastDebounceTime) > debounceDelay) {    
    if( buttonState != reading){ // if the state has changed
       if(reading == LOW)  // change this to HIGH if using pull-down resistors
          result = true;   
        buttonState = reading; // stable so in state long enough
    }    
  }
  lastButtonState = reading;
  if( result == true){
      result = false; // only return true once per press
      return true;  
  }
  return false;
}

I modified my button to pull the digital pin low, and removed my pull-down resistor.

This is the code Im using:

#include <WProgram.h>
#include <Wire.h>
#include <DS1307.h> // written by  mattt on the Arduino forum and modified by D. Sjunnesson

int time[3];
int fullTime;
volatile int state = LOW;
volatile int lastState = HIGH;
bool PM = false;
bool useSerialDebug = true; // set to true to give serial output

int onTime = 700; // time to turn light on
int offTime = 1900; // time to turn light off

int lightPin = 12; // light to turn on and off
int minsButtonPin = 11; // button to increment minutes by 1

void setup()
{
  if (useSerialDebug)
    Serial.begin(9600);
  setRTC(0, 0, 0, 0, 0, 0, 0);
  pinMode(lightPin, OUTPUT);
  pinMode(minsButtonPin, INPUT);
  digitalWrite(minsButtonPin, HIGH); // turn on internal pull-up

}

void setRTC(int sec, int mins, int hr, int dow, int date, int mth, int yr)
{
  RTC.stop();
  RTC.set(DS1307_SEC, sec);        //set the seconds
  RTC.set(DS1307_MIN, mins);     //set the minutes
  RTC.set(DS1307_HR, hr);       //set the hours
  RTC.set(DS1307_DOW, dow);       //set the day of the week
  RTC.set(DS1307_DATE, date);       //set the date  
  RTC.set(DS1307_MTH, mth);        //set the month
  RTC.set(DS1307_YR, yr);         //set the year
  RTC.start();
}

void setMins(int minutes, int hours)
{
  if (minutes < 60)
    minutes = minutes + 1;
  else if (minutes == 60)
  {
    minutes = 0;
    if (hours < 24)
      hours = hours + 1;
    else if (hours == 24)
      hours = 0;
  }
  setRTC(0, minutes, hours, 0, 0, 0, 0);
}
/*
void setHours()
 {
 if (hours < 24)
 hours = hours + 1;
 else if (hours == 24)
 hours = 0;
 setRTC(0, minutes, hours, 0, 0, 0, 0);
 }*/

//returns true if a button is pressed or has been pressed since the last call
// will not return true again until the button is released and pressed again
boolean minsButtonPressed()
{
  static boolean buttonState;             // the current reading from the input pin
  static boolean lastButtonState = LOW;   // the previous reading from the input pin
  static boolean result = false;       // the value returned by function
  static long lastDebounceTime = 0;  // the last time the output pin was toggled
  const long debounceDelay = 50;    // the debounce time; increase if the output flickers

  // read the state of the switch into a local variable:
  int reading = digitalRead(minsButtonPin);
  // If the switch changed, due to noise or pressing:
  if (reading != lastButtonState) {
    lastDebounceTime = millis();    // reset the debouncing timer
  }  
  if ((millis() - lastDebounceTime) > debounceDelay) {    
    if( buttonState != reading){ // if the state has changed
      if(reading == LOW)  // change this to HIGH if using pull-down resistors
        result = true;  
      buttonState = reading; // stable so in state long enough
    }    
  }
  lastButtonState = reading;
  if( result == true){
    result = false; // only return true once per press
    return true;  
  }
  return false;
}


void loop()
{
  // get the time, assign it to the array values
  time[0] = RTC.get(DS1307_HR, true);
  time[1] = RTC.get(DS1307_MIN, false);
  time[2] = RTC.get(DS1307_SEC, false);
  // create a military-time integer
  fullTime = (time[0] * 100) + time[1];

  if (minsButtonPressed())
  {
    time[1]++;
    setMins(time[1], time[0]);
  }

  // adjust for 24h time - subtract 12h and set the PM flag
  if (time[0] > 12)
  {
    time[0] = time[0] - 12;
    PM = true;
  }

  // serial stuff
  if (useSerialDebug)
  {
    for (int i = 0; i < 3; i++)
    {
      if (time[i] < 10)
      {
        Serial.print("0");  
      }

      Serial.print(time[i]);

      if (i < 2)
        Serial.print(":");
    }

    if (PM)
      Serial.println("PM");
    else
      Serial.println("AM");

    Serial.print("Full Time String: ");
    Serial.println(fullTime);  
  }
  // end serial stuff

  // turn off if its earlier than the onTime, or later or equal to the offTime
  if (fullTime < onTime || fullTime >= offTime)
    digitalWrite(9, LOW);
  else
    digitalWrite(9, HIGH);
  delay(1000);

}

I changed the delay at the bottom of the loop to 100 so as to not to interfere with the button press as much as a 1 second delay.

However, I find that when I press the button, the minutes increment by 2. If I keep holding the button down they dont increment any further. What am I missing?

When the button is pressed you increment time[1] and increment minutes in the setMins function, why are you doing both?

A delay of 1 second in loop is too long – a button press could be missed. I would use a delay similar to the debounce time - 50ms

I have absolutely no idea why I had both increments. I removed the time[1] increment and it works perfectly. Thanks!

So Ive made a couple tweaks, and everything is pretty close to how I want it. However, when I press the hours button when hours is 23, I want it to roll-over to 24, get set back to zero, and update the RTC, just like I do with the other hours. However, at 23:xx, when I press the button, it momentarily changes to 24:xx then back to 23:xx, like this:

Any ideas?

Code:

#include <WProgram.h>
#include <Wire.h>
#include <DS1307.h> // written by  mattt on the Arduino forum and modified by D. Sjunnesson

int time[3];
int fullTime, hours;
volatile int state = LOW;
volatile int lastState = HIGH;
bool PM = false;
bool useSerialDebug = true; // set to true to give serial output

int onTime = 700; // time to turn light on
int offTime = 1900; // time to turn light off

int lightPin = 12; // light to turn on and off
int minsButtonPin = 11; // button to increment minutes by 1
int hoursButtonPin = 10; // button to increment hours by 1
int maxButtonPin = 9;

void setup()
{
  if (useSerialDebug)
    Serial.begin(9600);
  //setRTC(0, 47, 16, 0, 0, 0, 0);
  pinMode(lightPin, OUTPUT);
  pinMode(minsButtonPin, INPUT);
  pinMode(hoursButtonPin, INPUT);
  pinMode(maxButtonPin, INPUT);
  digitalWrite(minsButtonPin, HIGH); // turn on internal pull-up
  digitalWrite(hoursButtonPin, HIGH);
  digitalWrite(maxButtonPin, HIGH);

}

void setRTC(int sec, int mins, int hr, int dow, int date, int mth, int yr)
{
  RTC.stop();
  RTC.set(DS1307_SEC, sec);        //set the seconds
  RTC.set(DS1307_MIN, mins);     //set the minutes
  RTC.set(DS1307_HR, hr);       //set the hours
  RTC.set(DS1307_DOW, dow);       //set the day of the week
  RTC.set(DS1307_DATE, date);       //set the date  
  RTC.set(DS1307_MTH, mth);        //set the month
  RTC.set(DS1307_YR, yr);         //set the year
  RTC.start();
}

void setMins()
{
  if (time[1] < 60)
    time[1]++;
  else if (time[1] == 60)
  {
    time[1] = 0;
    if (time[0] < 24)
      time[0]++;
    else if (time[0] == 24)
      time[0] = 0;
  }
  setRTC(time[2], time[1], time[0], 0, 0, 0, 0);
}

void setHours()
{
  if (time[0] < 24)
    time[0]++;
  else if (time[0] == 24)
    time[0] = 0;
  setRTC(time[2], time[1], time[0], 0, 0, 0, 0);
}

//returns true if a button is pressed or has been pressed since the last call
// will not return true again until the button is released and pressed again
boolean minsButtonPressed()
{
  static boolean buttonState;             // the current reading from the input pin
  static boolean lastButtonState = LOW;   // the previous reading from the input pin
  static boolean result = false;       // the value returned by function
  static long lastDebounceTime = 0;  // the last time the output pin was toggled
  const long debounceDelay = 50;    // the debounce time; increase if the output flickers

  // read the state of the switch into a local variable:
  int reading = digitalRead(minsButtonPin);
  // If the switch changed, due to noise or pressing:
  if (reading != lastButtonState) {
    lastDebounceTime = millis();    // reset the debouncing timer
  }  
  if ((millis() - lastDebounceTime) > debounceDelay) {    
    if( buttonState != reading){ // if the state has changed
      if(reading == LOW)  // change this to HIGH if using pull-down resistors
        result = true;  
      buttonState = reading; // stable so in state long enough
    }    
  }
  lastButtonState = reading;
  if( result == true){
    result = false; // only return true once per press
    return true;  
  }
  return false;
}

boolean hoursButtonPressed()
{
  static boolean buttonState;             // the current reading from the input pin
  static boolean lastButtonState = LOW;   // the previous reading from the input pin
  static boolean result = false;       // the value returned by function
  static long lastDebounceTime = 0;  // the last time the output pin was toggled
  const long debounceDelay = 50;    // the debounce time; increase if the output flickers

  // read the state of the switch into a local variable:
  int reading = digitalRead(hoursButtonPin);
  // If the switch changed, due to noise or pressing:
  if (reading != lastButtonState) {
    lastDebounceTime = millis();    // reset the debouncing timer
  }  
  if ((millis() - lastDebounceTime) > debounceDelay) {    
    if( buttonState != reading){ // if the state has changed
      if(reading == LOW)  // change this to HIGH if using pull-down resistors
        result = true;  
      buttonState = reading; // stable so in state long enough
    }    
  }
  lastButtonState = reading;
  if( result == true){
    result = false; // only return true once per press
    return true;  
  }
  return false;
}

void loop()
{
  // get the time, assign it to the array values
  time[0] = RTC.get(DS1307_HR, true);
  time[1] = RTC.get(DS1307_MIN, true);
  time[2] = RTC.get(DS1307_SEC, true);
  // create a military-time integer
  fullTime = (time[0] * 100) + time[1];
  // fill in the hours and mins integers

  if (minsButtonPressed())
    setMins();
  if (hoursButtonPressed())
    setHours();

  /*  // adjust for 24h time - subtract 12h and set the PM flag
   if (time[0] > 12)
   {
   time[0] = time[0] - 12;
   PM = true;
   }*/

  if (digitalRead(maxButtonPin) == HIGH)
  {
    // serial stuff
    if (useSerialDebug)
    {
      for (int i = 0; i < 3; i++)
      {
        if (time[i] < 10)
        {
          Serial.print("0");  
        }

        Serial.print(time[i]);

        if (i < 2)
          Serial.print(":");
      }
      /*
      if (PM)
       Serial.println("PM");
       else
       Serial.println("AM");
       */
      Serial.print("Full Time String: ");
      Serial.println(fullTime);  
    }
    // end serial stuff
  }
  // turn off if its earlier than the onTime, or later or equal to the offTime
  if (fullTime < onTime || fullTime >= offTime)
    digitalWrite(lightPin, LOW);
  else
    digitalWrite(lightPin, HIGH);
  delay(20);

}

Bump!

This logic:

    if (time[0] < 24)
      time[0]++;
    else if (time[0] == 24)
      time[0] = 0;

is wrong in both setMinutes() and setHours(), as it only increments the hours if its 23 or less. Think about what happens when time[0] is 23 when it enters that code segment.

you want something like:

time[0]++;
if (time[0] == 24) time[0] = 0;

instead of the block above in both functions.