Problem with interrupts, can't figure out why it won't work

Using Arduino Programming Questions
1

I'm working on a rather complicated project that involves multiple pushbuttons triggering Pin Change Interrupts and a Timer Interrupt combined with an external RTC chip to keep time (RTC code hasn't been added yet). There's also a 16x4 LCD for output. I'm in the process of building the code up piece by piece (so some things may seem unnecessary right now) but I've hit an error that I cannot explain. My code is as follows:

#include <LiquidCrystal.h>
#include <avr/io.h>
#include <avr/interrupt.h>

#define NO_PIN_ARDUINO

#include <PinChangeInt.h>

//button pin definitions
#define BUTTON_ONE A0
#define BUTTON_TWO 8
#define ROTARY_BUTTON 7
#define ROTARY_ENCODER_A 9
#define ROTARY_ENCODER_B 10

LiquidCrystal lcd(A3, 2, 6, 3, 5, 4);
byte copyright[8] = {
  B01110,
  B10101,
  B11011,
  B11001,
  B11001,
  B11011,
  B10101,
  B01110
};

void setup() {
  // initialize Timer1
  cli();          // disable global interrupts
  TCCR1A = 0;     // set entire TCCR1A register to 0
  TCCR1B = 0;     // same for TCCR1B

  // set compare match register to desired timer count:
  OCR1A = 15624;
  // turn on CTC mode
  TCCR1B |= (1 << WGM12);
  // Set CS10 and CS12 bits for 1024 prescaler:
  TCCR1B |= (1 << CS10);
  TCCR1B |= (1 << CS12);
  // enable timer compare interrupt:
  TIMSK1 |= (1 << OCIE1A);
  sei();          // enable global interrupts
  Serial.begin(9600);
  pinMode(A2, INPUT);
  digitalWrite(A2, HIGH);
  if (digitalRead(A2)) {
    int inputPins[] = {BUTTON_ONE, BUTTON_TWO, ROTARY_BUTTON, \
      ROTARY_ENCODER_A, ROTARY_ENCODER_B, 11, 12, 13, A1, A4, A5};
    for (int i = 0; i < 11; i++) {
      pinMode(inputPins[i], INPUT);
      if (i >= 5)
        digitalWrite(inputPins[i], HIGH);
    }
    lcd.createChar(0, copyright);
    lcd.begin(16, 4);
    lcd.print("  SCORE SEEKER");
    lcdsetCursor(5, 1);
    lcd.write(byte(0));
    lcd.print(" 2014");
    delay1(1500);
    lcdsetCursor(0, 3);
    lcd.print("HOLD LEFT: SETUP");
    delay1(1500);
    attachInterrupt(BUTTON_ONE, func1, CHANGE);
    attachInterrupt(ROTARY_BUTTON, func2, CHANGE);
    attachInterrupt(BUTTON_TWO, func3, CHANGE);
    Serial.println("X");
    lcd.clear();
}

void loop() {
}

void delay1(long interval) {
  long curr = millis();
  long target = curr + interval;
  while (millis() < target) {
  }
}

//This method takes care of the addressing error in 16x4 LCDs
void lcdsetCursor(int col, int row) {
  if (row > 1) {
    lcd.setCursor(col - 4, row);
  } else {
    lcd.setCursor(col, row);
  }
}

I did some testing and discovered that delay() (which is why I implemented delay1) and millis() were causing the problem which I'm guessing means something is wrong with Timer0 but I'm using Timer1 for the Timer Interrupt. I also tried commenting out cli() and sei() and that fixed it so it seems like Timer0 is getting disabled but not re-enabled. Any ideas why that might be? Any alternative possibilities?

2

You simply cannot use delay() in an ISR because it will hang forever. Interrupts
are disabled inside ISRs and delay() busy-waits for an interrupt that cannot happen.
millis() won't advance while in an ISR either for same reason.

Nor should you be wanting to wait a long time in an ISR, lean and mean
is the way to do it.

Yes it is possible to re-enable interrupts in an ISR to allow these things to start
working, but then you have to make your ISR re-entrant.

3

Any ideas why that might be?

Without seeing all of your code, we'd only be guessing. And, I'm guess that the unposted code is doing something stupid.

Any alternative possibilities?

Almost certainly. But, without seeing the stupidly named interrupt handlers, we can't even guess what you are doing, so, suggesting alternatives is outside the realm of possibility.

4

KalebPSpector:
I'm working on a rather complicated project that involves multiple pushbuttons triggering Pin Change Interrupts

Can you describe the project rather than how you are thinking of implementing it.

I can't immediately think of a reason for using interrupts to read push buttons because the human timescale does not work in microseconds.

...R

5

triggering Pin Change Interrupts

    attachInterrupt(BUTTON_ONE, func1, CHANGE);

attachInterrupt(ROTARY_BUTTON, func2, CHANGE);
    attachInterrupt(BUTTON_TWO, func3, CHANGE);

Don't you need "PCintPort::attachInterrupt(2, &quicfunc, CHANGE);" to get the right attachInterrupt() function?
Otherwise you'll get the "core" version of attachInterrupt(), which only works on a couple of pins, and takes a "interrupt number" rather than a pin number as its first argument.

6

Pin change interrupts are fairly easy to set up without a library:

byte prevD = PIND ;
ISR (PCINT2_vect) // portD
{
  // handle changes on portD (pins 0..7)
  byte port = PIND ;
  byte changed_pins = port ^ prevD ;
  prevD = port ;
  ...
}

byte prevB = PINB ;
ISR (PCINT0_vect) // portB
{
  // handle changes on portB (pins 8..13)
  byte port = PINB ;
  byte changed_pins = port ^ prevB ;
  prevB = port ;
  ...
}

void setup ()
{
  PCICR  = 0x05 ; // enable pin change interrupts on ports B, D only
  PCMSK2 = 0xFC ; // enable pin change on only pins 2..7 (port D)
  PCMSK0 = 0x3F ; // enable pin change on pins 8..13 (port B)
  ....
}

On the Uno etc (Mega is different).

pin change channels 0, 1, 2 correspond to ports B, C and D respectively.

I show how to identify pins that have changed since last interrupt (in case you
want to know which one(s) caused the interrupt.

7

westfw:
Don't you need "PCintPort::attachInterrupt(2, &quicfunc, CHANGE);" to get the right attachInterrupt() function?
Otherwise you'll get the "core" version of attachInterrupt(), which only works on a couple of pins, and takes a "interrupt number" rather than a pin number as its first argument.

You're right, thanks for pointing that out. However, the error is occurring before I ever get there.

PaulS:

Any ideas why that might be?

Without seeing all of your code, we'd only be guessing. And, I'm guess that the unposted code is doing something stupid.

Every other line of code I've written is commented out, so what you see is currently all of my code.

Robin2:
Can you describe the project rather than how you are thinking of implementing it.

It's a digital scorekeeper. There are two buttons and a rotary encoder that has a pushbutton knob with a 4x16 LCD for display. I am limited to these I/Os by the enclosure I have. In order to get all the functionality required, I need to be able to detect single presses, double presses, and press-and-holds. I also need to be able to independently keep time, hence the RTC chip.

I suppose I could eliminate the pin change interrupts for the buttons controlling the two scores since those probably won’t need to change that fast, but the button controlling the clock has to be an interrupt because that timing has to be precise.

MarkT:
You simply cannot use delay() in an ISR because it will hang forever. Interrupts
are disabled inside ISRs and delay() busy-waits for an interrupt that cannot happen.
millis() won't advance while in an ISR either for same reason.

I totally understand that delay() and millis() won’t work inside an ISR, but unless I'm missing something blatantly obvious they aren’t occurring inside an ISR, so there’s no reason they shouldn’t work.

8

KalebPSpector:
because that timing has to be precise.

What margin of error would be acceptable - measured in millis or micros?

I totally understand that ... millis() won’t work inside an ISR,

That does not prevent you from saving the current value of millis() while in an ISR. In any case you don't want to be in an ISR for a whole millisecond under any circumstances.

...R

9

KalebPSpector:

PaulS:

Any ideas why that might be?

Without seeing all of your code, we'd only be guessing. ...

Every other line of code I've written is commented out, so what you see is currently all of my code.

Attempting to compile your posted code I get:

sketch_aug22a.ino: In function ‘void setup()’:
sketch_aug22a:65: error: ‘func1’ was not declared in this scope
sketch_aug22a:66: error: ‘func2’ was not declared in this scope
sketch_aug22a:67: error: ‘func3’ was not declared in this scope
sketch_aug22a:72: error: a function-definition is not allowed here before ‘{’ token
sketch_aug22a:75: error: a function-definition is not allowed here before ‘{’ token
sketch_aug22a:89: error: expected `}' at end of input

So you haven't posted all your code.

http://snippets-r-us.com/

10 
void delay1(long interval) {
  long curr = millis();
  long target = curr + interval;
  while (millis() < target) {
  }
}

millis() returns unsigned values so this will eventually fail.

11

Plus your hand-coded "delay" does not handle wrap-around. Why not just use delay() ?

Read this: http://www.gammon.com.au/forum/?id=12127

12

the error is occurring before I ever get there.
// enable timer compare interrupt:
what you see is currently all of my code.

Where is the timer1 compare ISR?
Enabling an interrupt without having an ISR will cause a continuous reset loop; is that what you're seeing (I don't think you ever said exactly what is happening, beyond "it's not working."

13

You're right. I forgot to include the interrupt functions because they aren't really doing much right now. They are as follows:

void func1() {
  Serial.print("Button 1 triggered");
//  processButton(1, PCintPort::pinState);
}

void func2() {
  Serial.print("Button 2 triggered");
//  processButton(2, PCintPort::pinState);
}

void func3() {
  Serial.print("Button 3 triggered");
//  processButton(3, PCintPort::pinState);
}

processButton is the function that will decide what to do depending on which button state was changed and what it was changed to. I've commented it out because it isn't the cause of this problem.

westfw:
Where is the timer1 compare ISR?
Enabling an interrupt without having an ISR will cause a continuous reset loop; is that what you're seeing (I don't think you ever said exactly what is happening, beyond "it's not working."

I was not aware of this and a continuous reset loop sounds a lot like what I might be dealing with, especially since I just checked and the timer ISR is in fact commented out. I'll put it back in and see if that fixes it.

Robin2:
What margin of error would be acceptable - measured in millis or micros?

Given that most scoreboards never display anything smaller than tenths of a second, precision at the milli level would probably be sufficient if doing so would significantly simplify things. Was there something you had in mind?

14

because they aren't really doing much right now.

Except things that they should not be doing.

Serial input and output relies in interrupts functioning. Interrupts do NOT happen while an ISR is running.

Was there something you had in mind?

Yes. Polling the switches instead of using interrupts.

15

PaulS:
Serial input and output relies in interrupts functioning. Interrupts do NOT happen while an ISR is running.

Those statements are only there for debugging purposes and will ultimately be removed. If those won't work, how would you recommend debugging my processButton function which should resolve as part of the button ISRs?

PaulS:
Yes. Polling the switches instead of using interrupts.

I thought of that but with there are situations where someone might push one button and then immediately press another in order to stop the clock. I don't want a clock stop button press to be delayed because the program is processing a press for a different button (I have to wait 300 milliseconds before confirming a press of any button since buttons can be double pressed).

16

I have to wait 300 milliseconds before confirming a press of any button since buttons can be double pressed

I don't understand this. 3/10ths of a second is not a debouncing time. It is a deliberate interval. A far better choice is to use the state change detection example to determine when the switch BECOMES pressed. Take action when the start or stop switches BECOME pressed, not when they ARE pressed.

17

Those statements are only there for debugging purposes and will ultimately be removed. If those won't work, how would you recommend debugging my processButton function which should resolve as part of the button ISRs?

Flash an LED or something if you must. The difficulty of debugging is just one more reason to not use interrupts unless you absolutely have to. You don't have to.

KalebPSpector:

PaulS:
Yes. Polling the switches instead of using interrupts.

I thought of that but with there are situations where someone might push one button and then immediately press another in order to stop the clock. I don't want a clock stop button press to be delayed because the program is processing a press for a different button (I have to wait 300 milliseconds before confirming a press of any button since buttons can be double pressed).

You shouldn't be overloading time-critical buttons like START / STOP. Keep them single-function, and you'll be able to respond to them much faster than 300 ms if the rest of your code is written in a non-blocking way. If you aren't overloading them, but other overloaded buttons are causing interference due to their overloading, you've coded them wrong.

You shouldn't be using delays at all for this kind of code, and you certainly don't need to use ISRs for human-pressed buttons. You might think that a stop clock button needs an ISR to be accurate, but human reaction time is already going to introduce huge amounts of error into the measurement. Human reaction time in on the order of 250 ms. As long as you can respond to a button press an order of magnitude faster than that, there should be no trouble.

You have to ask yourself how fast "immediately" is on a human timescale. It's much much slower than a microprocessor. In an amount of time that a human might consider "immediate", an Arduino microprocessor has enough time to run a million or two instructions. And I'm talking about the 16 MHz ones, not the Due. Microprocessors are a lot faster than people, and that difference in timescales can be difficult to grasp.

18

Jiggy-Ninja:
Microprocessors are a lot faster than people, and that difference in timescales can be difficult to grasp.

Th OP seems to have missed it when this was pointed out in Reply #3.

...R