Problem with timer interrupts and analog comparator

I'm having trouble with a project using both the analog comparator and timer1. I hope someone can give me some advice on what's going wrong.
Hardware: Arduino Uno R3
What I'm trying to do: I want to turn on a fan when a temperature rises above a certain level. When the temperature drops the fan stops after a certain time.
How I'm doing it: I have a thermopile sensor on pin 6 (outputs voltage between 0 and 5 volts) and a reference voltage on pin 7 (about 2 volts). The analog comparator trips an interrupt when the thermopile voltage is greater than the reference voltage. The fan then starts up (set pin 4 to HIGH which is connected to a transistor acting as a switch) and a timer variable (timecounter) is set based on the value of pin A1 (this is a voltage divider with a potentiometer to make the time user configurable).
Timer1 is supposed to trip every second. If the timer variable reaches zero the fan is turned off (pin 4 LOW) otherwise it decrements the timer variable.
The reason for doing it this way is so that the fan remains on while the temperature is high because the analog comparator keeps tripping and resetting the timer variable.
The problem: The analog comparator interrupt routine works fine but the timer interrupt isn't. The fan starts but it never switches off.
Ideas on what is going wrong: It looks a bit like the timer interrupt isn't tripping. I've read a lot of tutorials on timer interrupts and it looks to me I'm doing it right (but obviously I can't be).
Another idea I had was it might be something to do with variable scope for the timer variable (timercounter).
Or could it be the two different kinds of interrupts interfering with each other?

After spending a lot of time on Google I'm still stumped. Any ideas appreciated thanks!

Kerry.

Here is my code:

// Thermopile Activated Fan

// when analog pin rises higher than threshold interrupt to start fan and timer
// every time it passes threshold reset time
// timer interrupt to turn of fan

// Analog comparator
//   If AIN0 rises above AIN1 voltage, trigger interrupt

const int THERMOPILE = 6; // AIN0 pin is D6
const int REFPIN = 7;     // AIN1 pin is reference voltage

const int FAN = 4;  //fan pin
const int TIMESET = A1;  // potentiometer for run timer

const float MAXTIME = 120; // maximum fan run time in seconds

volatile int timecounter = 0;  // countdown in seconds until fan stops

void setup() {
  noInterrupts(); // disable all interrupts
  // analog comparator interrupt
  ACSR = 
     (0<<ACD) |   // Analog Comparator: Enabled
     (0<<ACBG) |   // Analog Comparator Bandgap Select: AIN0 is applied to the positive input
     (0<<ACO) |   // Analog Comparator Output: Off
     (1<<ACI) |   // Analog Comparator Interrupt Flag: Clear Pending Interrupt
     (1<<ACIE) |   // Analog Comparator Interrupt: Enabled
     (0<<ACIC) |   // Analog Comparator Input Capture: Disabled
     (1<<ACIS1) | (1<ACIS0);   // Analog Comparator Interrupt Mode: Comparator Interrupt on Rising Output Edge
  
  // timer interrupt (1 per second)
  TCCR1A = 0;
  TCCR1B = 0;
  OCR1A = 15624; 
  TCCR1B |= (1 << WGM12); 
  TCCR1B |= (1 << CS10);
  TCCR1B |= (1 << CS12); 
  TIMSK1 |= (1 << OCIE1A); 
  interrupts(); // enable all interrupts
  
  pinMode(FAN, OUTPUT);
}

void loop() {
}  

// analog comparator interrupt function
ISR(ANALOG_COMP_vect) {
  noInterrupts();
  digitalWrite(FAN, HIGH);  // turn on the fan
  timecounter = analogRead(TIMESET)/1024 * MAXTIME; // set num of seconds for fan to run based on timer potentiometer
  interrupts();
}

// Timer interrupt trips once per second and counts down timer
ISR(Timer1_COMPA_vect) {
  noInterrupts();
  if(timecounter <= 0) {
    digitalWrite(FAN,LOW); // turn off fan
  } else {
    timecounter--;
  }
  interrupts();
}
// analog comparator interrupt function
ISR(ANALOG_COMP_vect) {
  noInterrupts();
  digitalWrite(FAN, HIGH);  // turn on the fan
  timecounter = analogRead(TIMESET)/1024 * MAXTIME; // set num of seconds for fan to run based on timer potentiometer
  interrupts();
}

It is unnecessary to turn interrupts off and on like that. They are already off when the ISR is entered, and turned on when it leaves.

  timecounter = analogRead(TIMESET)/1024 * MAXTIME; // set num of seconds for fan to run based on timer potentiometer

This is integer arithmetic. analogRead(TIMESET)/1024 will always be zero.

  OCR1A = 15624;

Try moving that line down to after the timer is started.

My timers page:

My page about the analog comparator:

timecounter = analogRead(TIMESET)/1024 * MAXTIME;
This is integer arithmetic. analogRead(TIMESET)/1024 will always be zero.

I wondered about that so I tested it and it appears to work I think because MAXTIME is float so the the equation is calculated as float but then saved as an INT so I get a nice round (non zero) timecounter.

Thanks for the tip about turning interrupts off and on. I wasn't sure so I added those lines just to make sure.

I'll try moving the OCR1A line and I'll check out your links and report back.
Thanks.

This line will cause trouble:

ISR(Timer1_COMPA_vect) {

The compiler doesn't seem to recognize the mixed-case identification of Timer1, but it never mentions it. Instead, it puts a jump instruction to location 0, the reset vector, in the interrupt table at the TIMER1 COMPA location, as if there were no ISR defined for it. In my experience, a misidentified ISR name will cause unexpected results, but it won't trigger an error message.

This will work better:

ISR(TIMER1_COMPA_vect) {

As I understand it, the proper name for an ISR is the case-sensitive name shown in the "Reset and Interrupt Vectors in ATmega328 and ATmega328P" table in the data sheet, with underline characters substituted for any spaces, followed by these characters: _vect

Using some Serial.print()'s while a sketch is under development will help you see what's going on. Serial.print()ing an initialization message will make repeated resets obvious. When I add these two lines to your sketch -

  Serial.begin(115200);
  Serial.println("OK");
  • the initialization message repeats.

You are quite right, I can't believe I missed that. I checked the spelling too, but not the capitalization. :slight_smile:

Possibly you haven't made exactly this error yourself, and possibly you haven't done it a dozen times. If not, then I have the advantage of you in this game.

Do we know why there's no error message for misnaming an ISR?

tmd3:
Do we know why there's no error message for misnaming an ISR?

No. On the face of it, it would seem like an error.

I'm happy to say it's working now!

Tmd3 you were right about the case. The tutorial I was working from made the same mistake so it was copied from there.

Nick, you were also right about the integer arithmetic. I had tested it but then made a typo in my actual code. This is what it should be:

timecounter = analogRead(TIMESET)/1024.0 * MAXTIME;

Thank you very much for your help.

Kerry.

I am going to guess that both spellings are defined, but only one substitutes the correct value.

Or, try it in a .cpp and see if it is another "feature" of the IDE .ino file munging.

kbakernz:
I'm happy to say it's working now!

Glad to hear that it almost works. If you haven't changed other things in the code, I suspect that it doesn't do this:

... the fan remains on while the temperature is high because the analog comparator keeps tripping and resetting the timer variable.

That sounds like you want the timer variable to stay at its original setting until the temperature falls below the reference level, and then start timing out. That means that the fan will run until the temperature drops below the setpoint, and then run for an additional period determined by the potentiometer setting. I don't think that the program you posted, corrected as you describe, will do that.

KeithRB:
... guess that both spellings are defined, but only one substitutes the correct value.

My tests don't support that notion. I used this as the Timer1 overflow ISR:

ISR(TIMER1_OVF_vect) {
  asm volatile (
    "neg   %[ctr]"           "\n\t"
    "subi  %[ctr], 1"        "\n\t"
    "neg   %[ctr]"           "\n\t"
    :   [ctr] "=&r" (ctr)
    :  "[ctr]"      (ctr)
    :
  );
  flag = (ctr == 0);
}

Timer 1 was set to normal mode, no prescaler, overflow interrupt enabled. loop() printed a short message whenever it saw flag nonzero. ctr and flag were volatile uint8_t's. The purpose of the peculiar assembler code was to get a couple of "neg" instructions into the disassembled output, to make it easy to find this ISR in the assembler listing. The output was as expected - a message at roughly 1 per second. "neg" instructions bracketing a "subi r24, 1" appeared in the assembler listing, as expected. The ISR was 58 bytes long. I'm happy to post all the code if anyone wants it, but the ISR was the only interesting part.

Changing the ISR definition to this -
ISR(Melbourne) {- in honor of our friend Nick, yielded a long string of initialization messages as output, suggesting repeated resets. The Timer1 interrupt vector contained a jump to a "jmp 0" instruction, with the notation, "<__bad_interrupt>", the same code and notation that appears for all the other interrupts for which there's no ISR. That's not surprising, since nothing in the code suggested that the ISR was for Timer1 overflow. The only interrupts with real jump instructions were Timer0 overflow, and two USART interrupts. There were no "neg" instructions in the assembler listing, suggesting that the ISR didn't get into the program at all. The sketch size was 58 bytes less than the correct version, suggesting that the ISR was simply omitted.

I tried naming the ISR "Sydney" and "Canberra." It took me three tries to get the capital of Australia right, but the results were all the same: no hint of the ISR in the code, and a smaller sketch size by 58 bytes. The IDE never mentioned it, even after I asked it to be verbose about both compiling and uploading. It does hiccup on special characters and spaces, though, making me suspect that it wants something that would be a legal variable name.

It seems unlikely that major Australian cities are defined somewhere. It looks like anything that will pass muster as a variable name will get past the compiler, but only the correct names will generate ISR's in the code.

I hesitate to call this a bug without input from experts. Defining an ISR may tell the compiler to expect an external something-or-other that never materializes, and this behavior may be proper under the standards. A warning message at compile time might be a spiritual act of mercy, though, considering the challenges of troubleshooting ISR's even when they're named correctly.

Changing the ISR definition to this -

ISR(Melbourne) {
  • in honor of our friend Nick,
    ...
    I tried naming the ISR "Sydney" and "Canberra." It took me three tries to get the capital of Australia right, but the results were all the same:

Better let everyone know the right answer, eh? :stuck_out_tongue:

It looks very much as if the compiler silently discards an interrupt handler which doesn't match its table of known numbers. This seems odd behaviour to me.


Hmm, I think I've worked out why that happens. First, look at ISR:

#  define __INTR_ATTRS used, externally_visible
...
#  define ISR(vector, ...)            \
    extern "C" void vector (void) __attribute__ ((signal,__INTR_ATTRS)) __VA_ARGS__; \

OK, given that this basically substitutes your wanted ISR vector here, let's try replacing:

extern "C" void TIMER1_OVF_vect (void) __attribute__ ((signal, used, externally_visible)) __VA_ARGS__; \

So now we have a "C" function with attributes: signal, used, externally_visible

So far so good. But TIMER1_OVF_vect is also a define:

#define TIMER1_OVF_vect   _VECTOR(13)  /* Timer/Counter1 Overflow */

So now we have:

extern "C" void _VECTOR(13)  (void) __attribute__ ((signal, used, externally_visible)) __VA_ARGS__; \

But _VECTOR is also a define:

#define _VECTOR(N) __vector_ ## N

So finally the code looks like this:

extern "C" void __vector_13  (void) __attribute__ ((signal, used, externally_visible)) __VA_ARGS__; \

Now at this stage I got lost, but I am guessing that __vector_13 tells the compiler to put a jump to the function in the 13th vector position at the start of the code block.

But with the case wrong, the last few steps get skipped and you just end up with:

extern "C" void Timer1_OVF_vect (void) __attribute__ ((signal, used, externally_visible)) __VA_ARGS__; \

It's just a function, named Timer1_OVF_vect, which we claim is a "signal" type, but without the extra processing of the name the compiler doesn't add a jump to it, nor does it care. You may as well call it:

extern "C" void foo (void) __attribute__ ((signal, used, externally_visible)) __VA_ARGS__; \

There is no rule saying every function has to be used somewhere. In fact, this might even be some sort of linker behaviour.

The compiler actually has nothing to do with the jump table. The startup module for the C run-time library defines the vector table with weak references. If the linker finds a strong reference that is used. If it does not then the weak reference is used. The compiler's role is to generate a function with a special epilogue / prologue. The name, __vector_13, means nothing special to the compiler.

There is no rule saying every function has to be used somewhere. In fact, this might even be some sort of linker behaviour.

Exactly!

Looks like I was getting close. Where is this "startup module"?

If you have verbose output turned on, it does mention ISR errors, but they seem to get demoted to warnings.
For example, if you try to compile this:

volatile byte a;
void setup(){}
void loop(){}

ISR(Bob_vect){
  a++;
}

You get:
sketch_aug23a.cpp:16: warning: 'Bob_vect' appears to be a misspelled signal handler

They would be part of Libc. Very likely an assembly file (dot-S). Typically they have "CRT0" (or lower-case) in the filename. I don't have dot-S files on my computer so I'm guessing the Arduino IDE does not include the Libc source.

Looks like the object files are named crt[processor].o

[quote author=Tom Carpenter link=topic=183841.msg1363150#msg1363150 date=1377243994]If you have verbose output turned on, it does mention ISR errors[/quote]Indeed. I find it in 1.0.5. I can't get it to show up in 1.5.2.

[quote author=Nick Gammon link=topic=183841.msg1363096#msg1363096 date=1377239882]Better let everyone know the right answer, eh?[/quote]I think it's something that ends in "-ong." Or maybe "-illa?"