Can't get attachInterrupt to work

Hey guys, wonder if someone can help me. I'm trying to implement attachInterrupt using 2 buttons to do 2 different things when each is pressed but just cant seem to figure it out....... Have tried a few examples on line but none have worked for me. I have tested the buttons and wiring with a multimeter to make sure they are not faulty, which they aren't.

Grounds of both buttons are joined together and then to GND on Arduino.
Each + of buttons are wired to pins 18 & 19 individually with no resistors.

If there is no problems with the hardware or the way I have it setup, its got to be something with my sketch.

So if someone fancies throwing their eyes over it and seeing if they can find anything I would be forever great full.

Sketch is here.

It is running on an Arduino mega with a 4 port relay shield plugged into it, RTC DS3231 and 16x2 LCD on I2C.

This is my first proper Arduino project so please go easy on me....

Cheers
Iain

int Delay5 = 3000000;

That's a tight squeeze.

 Serial.println('Interrupt');

Double quotes.

if (Zone3Water == 'YES'){

Very unlikely

Oh yeah, delay and serial I/O don't go in interrupts.

How did you get to 12+ k of code without testing?

AWOL:
How did you get to 12+ k of code without testing?

I have tested, its working as a unit as I am playing with the code. I am only trying to add the buttons to what I assumed was working code. But as this is my first project I was hoping someone would be able to help me with problems I may have missed.

I've only done a bit of python, bash, php up to now, and I do mean a little. So to see the results working I can only go on that and only assume that's where my problem lies.

Cheers Iain

You say it's working, but 3000000 doesn't fit in a sixteen bit "int", and nor does 'YES' (but 'NO' does)
'Interrupt' doesn't even fit in a 32 bit integer.

AWOL:
You say it's working, but 3000000 doesn't fit in a sixteen bit "int", and nor does 'YES' (but 'NO' does)
'Interrupt' doesn't even fit in a 32 bit integer.

I say "its working" because to my un-knowledgeable eyes I'm getting the results I want. I admit the 300000 i havent tested yet, the interrupt neither because i cant get attachInterrupt to work in the first place to come up against that problem to solve that. The 'YES' again, to my un-knowledgeable eyes I'm getting the results I want, it does its routine.

I will go through the input you have provided, thanks.

Cheers
Iain

When the program code is here (in this Forum) I will look at it.

...R

Robin2:
When the program code is here (in this Forum) I will look at it.

...R

Cheers for the offer.
The whole sketch wouldn't fit in the tags so I have trimmed it to fit leaving in as much as possible. Full sketch can still be viewed on github.

Cheers
Iain

#include <DS3232RTC.h>
#include <Streaming.h>
#include <Wire.h>
#include <avr/sleep.h>
#include <avr/power.h>
#include <RTClib.h>
#include <Time.h>
#include <LCD.h>
#include <LiquidCrystal_I2C.h>

const char *string_table[] =
{   
  "   Sleeping",
  "   Watering",
  "    Testing",
  " Zone 1 For 30s ",
  " Zone 2 For 30s ",
  " Zone 3 For 30s ",
  " Zone 4 For 30s ",
  "  Priming pump  ",
  "   Test Mode    ",
  " Manual Run Mode",
  "System Starting",
  "YES",
  "NO",
  "Interrupt",
  };


/*Start of User Defined Settings
*/
// Time1-5 are the hours for program to run (Sensors and watering)
int Time1 = 8;
int Time2 = 14;
int Time3 = 0;
int Time4 = 88;
int Time5 = 88;
// TimeMin is the minute on the hour for program to run.
int TimeMin = 30;
// Water levels are soil moisture settings in percent. So if the moisture in the soil reads less than WaterLevel
// then the program wil run.
int WaterLevel_1 = 60;
int WaterLevel_2 = 60;
int WaterLevel_3 = 60;
int WaterLevel_4 = 60;
int WaterLevel_5 = 60;
// Watering run time's in milliseconds so 1000 is 1sec and 30000 is 30sec.
int Delay1 = 10000; // 10seconds
int Delay2 = 20000; // 20seconds
int Delay3 = 30000; // 30seconds
int Delay4 = 60000; // 60seconds
int Delay5 = 3000000; // 5mins - Used For Refilling Resevoir 
// ZoneSettings------
/* Only user serviceable settings are
   -Zone*Delay = Watering time, Link to Delays above - can be added together using ( brackets) like (Delay2 + Delay3) would give a run
   -- time of 50seconds.
   -Zone*WaterLevel = Moisture Level you want the zone set to, link to WaterLevels above
*/
int Zone1Solenoid = 50;
int Zone1Sensors[] = {A12, A8};
int Zone1Delay = Delay3;
int Zone1SensorValue;
String Zone1Water;
int Zone1WaterLevel= WaterLevel_1;
int Zone2Solenoid = 48;
int Zone2Sensors[] = {A13, A9};
int Zone2Delay = Delay3;
int Zone2SensorValue;
String Zone2Water;
int Zone2WaterLevel = WaterLevel_1;
int Zone3Solenoid = 44;
int Zone3Sensors[] = {A14, A10};
int Zone3Delay = Delay3;
int Zone3SensorValue;
String Zone3Water;
int Zone3WaterLevel = WaterLevel_1;
int Zone4Solenoid = 46;
int Zone4Sensors[] = {A15, A11};
int Zone4Delay = Delay3;
int Zone4SensorValue;
String Zone4Water;
int Zone4WaterLevel = WaterLevel_1;

// End Of User Settings

int programDelay = 100;
int n = 1;
int solenoid;
String water_info;
int delay_time;
int soil1;
int soil2; 
int SensorCount = 2;
int CirculationPump = 5;
int ResevoirPump = 4;
//volatile int buttonstate = HIGH;
#define I2C_ADDR    0x27 
#define BACKLIGHT_PIN     3
#define En_pin  2
#define Rw_pin  1
#define Rs_pin  0
#define D4_pin  4
#define D5_pin  5
#define D6_pin  6
#define D7_pin  7
//int TEST_BUTTON = 19;
//int WATER_BUTTON = 18;
#define DS3231_I2C_ADDRESS 0x68
LiquidCrystal_I2C lcd(I2C_ADDR,En_pin,Rw_pin,Rs_pin,D4_pin,D5_pin,D6_pin,D7_pin);
RTC_DS3231 rtc;
int INTERRUPT_PIN = 22;
volatile int state = LOW;

byte decToBcd(byte val)
{
  return( (val/10*16) + (val%10) );
}
// Convert binary coded decimal to normal decimal numbers
byte bcdToDec(byte val)
{
  return( (val/16*10) + (val%16) );
}

void setup () {
  // Set Time (sec,min,hour,day,date,mon,yr)
  //setDS3231time(00,22,18,5,21,04,16); // Set time
  pinMode(INTERRUPT_PIN, INPUT_PULLUP); 
  pinMode(CirculationPump, OUTPUT);
  pinMode(ResevoirPump, OUTPUT);
  pinMode(Zone1Solenoid, OUTPUT);
  pinMode(Zone2Solenoid, OUTPUT);
  pinMode(Zone3Solenoid, OUTPUT);
  pinMode(Zone4Solenoid, OUTPUT);
//  pinMode(TEST_BUTTON, INPUT);
//  pinMode(WATER_BUTTON, INPUT); 
  digitalWrite(Zone1Solenoid, HIGH);
  digitalWrite(Zone2Solenoid, HIGH);
  digitalWrite(Zone3Solenoid, HIGH);
  digitalWrite(Zone4Solenoid, HIGH); 
  attachInterrupt(19, pin_READ, CHANGE);
  attachInterrupt(18, pin_WATER, CHANGE); 
  Wire.begin();
  rtc.begin();
  lcd.begin (16,2); 
  lcd.setBacklightPin(BACKLIGHT_PIN,POSITIVE);
  lcd.setBacklight(HIGH);
  lcd.home ();
  lcd.print(string_table[10]);
  Serial.begin(57600);
  };

void pin_READ() {
//  buttonstate = digitalRead(TEST_BUTTON);
  lcd.clear();
  lcd.setCursor(0,0);
  lcd.print(string_table[8]);
  delay(Delay3);
  read_sensors();
};

void pin_WATER() {
//  buttonstate = digitalRead(WATER_BUTTON);
  lcd.clear();
  lcd.setCursor(0,0);
  lcd.print(string_table[9]);
  delay(Delay3);
  water_run();
};

void program() {
  DateTime tt = rtc.now();
  int tthour = tt.hour();
  int ttmin = tt.minute();
  Serial.print(tt.hour(), DEC);
  Serial.print(':');
  Serial.print(tt.minute(), DEC);
  Serial.print(':');
  Serial.println(tt.second(), DEC);
  if ((tthour == Time1 || Time2 || Time3 || Time4 || Time5 ) and ttmin == TimeMin ){
    water_run();
  }
  else {
  lcd_time(),
  lcd.print(string_table[0]),
  Serial.flush();
  } 
  };

void water_run(){
    read_sensors();
    lcd.clear();
    lcd.setCursor(0,0);
    lcd.print(string_table[7]);
    if (Zone1SensorValue <= Zone1WaterLevel ) {
      Zone1Water = String(string_table[11]);
      Serial.println(Zone1Water);
    };
    if (Zone2SensorValue <= Zone2WaterLevel ) {
      Zone2Water = String(string_table[11]);
      Serial.println(Zone2Water);
    };
    if (Zone3SensorValue <= Zone3WaterLevel ) {
      Zone3Water = String(string_table[11]);
      Serial.println(Zone3Water);
    };
    if (Zone4SensorValue <= Zone4WaterLevel ) {
      Zone4Water = String(string_table[11]);
      Serial.println(Zone4Water);
    };
    if (Zone1Water || Zone2Water || Zone3Water || Zone4Water == String(string_table[11])){
      digitalWrite(CirculationPump, HIGH);
      delay(1500);
    };
    if (Zone1Water == String(string_table[11])){
      Serial.println(Zone1Water),
      delay_time = Zone1Delay,
      solenoid = Zone1Solenoid,
      water_info = string_table[3], 
      water_routine(); 
    };
    if (Zone2Water == String(string_table[11])){
      delay_time = Zone2Delay,
      solenoid = Zone2Solenoid,
      water_info = string_table[4],
      water_routine();  
    };
    if (Zone3Water == String(string_table[11])){
      delay_time = Zone3Delay,
      solenoid = Zone3Solenoid,
      water_info = string_table[5],
      water_routine();  
    };
    if (Zone4Water == String(string_table[11])){
      delay_time = Zone4Delay,
      solenoid = Zone4Solenoid,
      water_info = string_table[6],
      water_routine();  
    }; 
    digitalWrite(CirculationPump, LOW);
    Zone1Water = String(string_table[12]);
    Zone2Water = String(string_table[12]);
    Zone3Water = String(string_table[12]);
    Zone4Water = String(string_table[12]);
};

void loop(){
  delay(programDelay);
  program();
  };

IainStott:
The whole sketch wouldn't fit in the tags

You can add your .ino file as an attachment if it is too big.

Of course a much better approach would be to write a short program that demonstrates the problem.

...R

This is one of your ISRs

void pin_READ() {
//  buttonstate = digitalRead(TEST_BUTTON);
  lcd.clear();
  lcd.setCursor(0,0);
  lcd.print(string_table[8]);
  delay(Delay3);
  read_sensors();
};

delay() won't work within an ISR and I suspect that lcd.print() won't either, but I am not sure.
ISRs should be short and sweet and certainly should not have a delay() of 30 seconds in them, even if it did work. Flag the fact that the ISR has been triggered, set some variables and exit. Deal with the outcome in loop().

The read_sensors() function may also cause problems when called from an ISR but you have not posted the code.

Cheers for the input and patience guys, will look into the input suggested already. Here is the attached ino file.

Cheers again.
Iain

IrrigationSystem.ino (13.2 KB)

These statements will not work as you think

  if ((tthour == Time1 || Time2 || Time3 || Time4 || Time5 ) and ttmin == TimeMin ){

  if (Zone1Water || Zone2Water || Zone3Water || Zone4Water == String(string_table[11])){

Why has a function called read_sensors() got stuff in it with Serial.print() or lcd.anything(). Make separate functions for the LCD stuff and for the Serial stuff and then the logic will be much clearer and it will be much easier to test each part in isolation.

And there is no need to use interrupts to detect human button presses - humans are very slow. If the rest of the program is properly designed (without any delay()s) you can read the buttons perfectly well by polling.

Have a look at Planning and Implementing a Program

...R

Robin2:
Why has a function called read_sensors() got stuff in it with Serial.print() or lcd.anything(). Make separate functions for the LCD stuff and for the Serial stuff and then the logic will be much clearer and it will be much easier to test each part in isolation.

Because I really don't have a lot of experience with arduino and coding one and have chosen a bigger project than I should for my first lol but who learnt much the easy way...... go big or go home

Robin2:
And there is no need to use interrupts to detect human button presses - humans are very slow. If the rest of the program is properly designed (without any delay()s) you can read the buttons perfectly well by polling.

Have a look at Planning and Implementing a Program

...R

]

Cheers Robin2, thats exactly what I need, will make a cup of tea and read through that now.

Whandall:
These statements will not work as you think

  if ((tthour == Time1 || Time2 || Time3 || Time4 || Time5 ) and ttmin == TimeMin ){

if (Zone1Water || Zone2Water || Zone3Water || Zone4Water == String(string_table[11])){

Why will they not work?? I am not being difficult, just trying to understand better. I am getting the results from the system that i want and it is successfully controlling the irrigation system I am building. I have been building the sketch and testing where I can for about a week. But as with work commitments I don't get a lot of time to dedicate. If you could tell me what the issue will be I can research.

Cheers
Iain

If-statements deal with Boolean expresssions. This means that at runtime, the expression inside the conditional clause of the if-statement must evaluate to a Boolean (true/false) result. If you are not using Boolean values in the expression, they will be automatically converted to Boolean values. This is done by checking them against the value zero (0). If zero, they are converted to false. Otherwise they turn into true.

You have all those TimeX variables defined as numbers. They will be converted to true because none of them are defined to have a zero value. Not equal to zero is true. So the compiler will read your if-statement as if you had written it like so:
if( ((tthour == Time1) || true || true || true || true) and (ttmin == TimeMin)) { ...

Does that even compile?? In C++ there is no "and" operator, there is only "&&" for logical tests or "&" for bitwise manipulations. So that "and" you have in there ought to be "&&" instead.

Since "true || x" for any x will always equal true regardless of the value of x, you are basically saying:
if( true && (ttmin == TimeMin)) { ...

Since "true && x" for any x will always equal x, there is no need to have the "true" in there. So your code simplifies to this:
if( ttmin == TimeMin ) { ...
and all the TimeX variables in there have no effect on the result at all.

Looks like you want to test each TimeX variable against tthour and if any of them match, and also ttmin equals TimeMin, then execute the if-statement's "then" block. To do that you must compare each one with tthour separately. Like this:

if( ( (tthour == Time1) || (tthour == Time2) || (tthour == Time3) || (tthour == Time4) || (tthour == Time5) ) && (ttmin == TimeMin) ) { ...

And likewise for the other if-statement Robin2 pointed out.

Now the == operators will compare each of the numeric values of TimeX against the numeric value of tthour and return a Boolean result (true or false) for each of the parenthesized sub-expressions, which are then used in the "||" computations once they are all figured out, and you get what you wanted.

In C++ there is no "and" operator,

Yes, there is.

AxeMurderer:
Does that even compile??

Yes it compiles and runs, which is to why the code has got to the state its got into. I had Time3, Time4 and Time5 blank at one stage and the sensors ran every hour regardless, then i set them to 88 because its never going to 88 o'clock and now it only runs on the hours specified.

AxeMurderer:
Rest of your post here.......

Understand most of that, give me a bit to read again, and Robin2's link and see where I go. Started the sketch again folling Robin2's post.

Cheers
Iain

Ok so there is an and operator. Never seen that before myself. It must be an arduino ide specific thing. I wouldn't use it though since it is not clear whether you are doing logical or bitwise unless you are familiar with that non-standard syntax. The &/&& makes it perfectly obvious.