1
\$\begingroup\$

I'm trying to obtain the input frequency of a square wave using the input capture register of an Atmega328p. So far, it works sporadically -- which is to say, when I input a 75 kHz square wave, the output looks like this:

244 244 75117 74766 75117 75117 79207 80402 82051 82901 84656 85561 87431 244 244 244 88888 90395 244 244 244 -941176 -271186 244 -246153 244 244 244

Does anyone know why this might be the case? I've tried messing with the data types, but otherwise I'm not really sure what the problem could be. The code I've written is below.

// # of overflows
volatile long T1Ovs;

// timestamp variables (store TCNT at time of input capture interrupt)
volatile long Capt1, Capt2;

// capture flag
volatile uint8_t Flag;

volatile long ticks;
volatile double period;
volatile long frequency;

void initTimer1(void)
{
  TCNT1 = 0;
  // initialize timer to 0

  //timer/counter1 control register b
  TCCR1B |= (1<<ICES1);
  // input capture edge select; rising edge triggers capture

  //timer/counter1 interrupt mask register
  TIMSK1 |= (1<<ICIE1);
  // ICIE1: input capture interrupt enable

  TIMSK1 |= (1<<TOIE1);
  // timer/counter1 overflow interrupt enable
}

void startTimer1(void)
{
  TCCR1B = (1<<CS10);
  //start timer with pre-scaler = 1

  sei();
  //enable global interrupts

}

ISR(TIMER1_CAPT_vect) // interrupt handler on input capture match (rising edge in this case)
{
  if (Flag == 0)
  {
    Capt1 = ICR1;
    // save timestamp at interrupt (input capture is updated with the counter (TCNT1)
    // value each time an event occurs on the ICP1 pin (digital pin 8, PINB0)

    T1Ovs = 0;
    // reset overflows
  }

  if (Flag ==1)
  {
    Capt2 = ICR1;
  }

  Flag ++;
}

ISR(TIMER1_OVF_vect) // interrupt handled on timer1 overflow
{
  T1Ovs++; // increment number of overflows
}


void setup()
{
  Serial.begin(9600);


  initTimer1();
  startTimer1();
}

void loop()
{
  if (Flag == 2)
  {
    ticks = Capt2 - Capt1 + T1Ovs * 0x10000L;
    // (second timestamp) - (first stamp) + (# of overflows) * (ticks/overflow = 65535)

    frequency = 16000000/ticks;
    // ticks * seconds/ticks = seconds
    // 1/seconds = Hz

    Flag = 0;
    // reset flags

    T1Ovs = 0;
    // reset overflow count

    TIFR1 = 0b00000000; // clear interrupt registers

    Serial.println(frequency);


    TIMSK1 |= (1 << ICIE1); // enable capture interrupt
    TIMSK1 |= (1 << TOIE1); // enable overflow interrupt

  }
}

Thanks in advance!

UPDATE***********************************

The second iteration of code, using enumerated types to make a state machine:

  typedef enum {
      CAPTURE_1,
      CAPTURE_2,
      WAIT
  } timer_state_t;

  timer_state_t flag = WAIT;
   volatile long Capt1, Capt2;

  volatile long T1Ovs;

  void InitTimer1(void)
  {
     //Set Initial Timer value
     TCNT1=0;
     //First capture on rising edge
     TCCR1B|=(1<<ICES1);
     //Enable input capture and overflow interrupts
     TIMSK1|=(1<<ICIE1)|(1<<TOIE1);
  }
  void StartTimer1(void)
  {
  //Start timer without prescaler
  TCCR1B|=(1<<CS10);
  //Enable global interrutps
     sei();
  }

  ISR(TIMER1_CAPT_vect) {
   switch(flag) {
   case CAPTURE_1:
       Capt1 = ICR1;

       flag = CAPTURE_2;
       break;
   case CAPTURE_2:
       Capt2 = ICR1;

       flag = WAIT;
                    Serial.println(flag);

       break;
      }



  }

  ISR(TIMER1_OVF_vect)
  {
    T1Ovs++;
  }

  void setup()
  {
    Serial.begin(9600);

    InitTimer1();
    StartTimer1();
  }

  void loop() {
    flag = CAPTURE_1;

    while (flag != WAIT);
     Serial.println("loop");

    Serial.println(Capt2 - Capt1 + T1Ovs * 0x10000);
  }
\$\endgroup\$
  • \$\begingroup\$ Also, just out of curiosity: the data sheet calls the interrupt vectors "TIM1_CAPT" and things along those lines, but when I try to pass that into the ISR, it doesn't work. I found "TIMER1_CAPT_vect" was used in examples online -- why does this work but the name in the datasheet doesn't? \$\endgroup\$ – user3753934 Jun 15 '15 at 14:29
  • \$\begingroup\$ What the datasheet calls the interrupt vectors is not necessarily what the compiler and standard library call them. avr-libc's ISR names are documented here: nongnu.org/avr-libc/user-manual/group__avr__interrupts.html \$\endgroup\$ – Nick Johnson Jun 15 '15 at 15:11
  • \$\begingroup\$ Gotcha, that makes sense. Thanks for the answer below as well -- still going through it, trying to figure out enums. \$\endgroup\$ – user3753934 Jun 15 '15 at 15:12
  • \$\begingroup\$ I've updated my answer with some examples. \$\endgroup\$ – Nick Johnson Jun 15 '15 at 15:30
1
\$\begingroup\$

Here is your code updated to work, with comments starting with "J:" explaining the changes...

  typedef enum {
      CAPTURE_1,
      CAPTURE_2,
      WAIT
  } timer_state_t;

  volatile timer_state_t flag = WAIT;

  // J:This is a 16-bit timer, so these values will always fit into an unsigned int
  volatile unsigned int Capt1, Capt2, CaptOvr;

  // J:Mind as well make this unsigned and give it 2x range since it can never be negative. 
  volatile unsigned long T1Ovs;

  void InitTimer1(void)
  {
     //Set Initial Timer value
     // J:All measurements against TCNT are relative, so no need to reset
     // TCNT1=0;

     // J: Note we need to set up all the timer control bits because we do not know what state they are in
     // J: If, for example, the WGM bits are set to a PWM mode then the TCNT is going to be resetting out from under us rather than monotonically counting up to MAX

     TCCR1A = 0x00;

     //First capture on rising edge
     TCCR1B =(1<<ICES1);
     //Enable input capture and overflow interrupts
     TIMSK1|=(1<<ICIE1)|(1<<TOIE1);
  }

 // J: Note that it would be ok to start the timer when we assign TCCR1B in InitTimer since nothing will happen when the ISR is called until we set flag to CAPTURE1

  void StartTimer1(void)
  {
  //Start timer without prescaler

  // J: Note that we know that the other CS bits are 0 becuase of the Assignment in InitTimer
  TCCR1B |= (1<<CS10);  

  //Enable global interrutps
  // J: Interrupts are turned on by  Arduino platform startup code
  //  sei();
  }

  ISR(TIMER1_CAPT_vect) {

   switch(flag) {
   case CAPTURE_1:
       Capt1 = ICR1;

       // J: Reset the overflow to 0 each time we start a measurement
       T1Ovs=0;
       doubleOverflowError=0;
       flag = CAPTURE_2;
       break;

   case CAPTURE_2:
       Capt2 = ICR1;

       // J: Grab a snap shot of the overflow count since the timer will keep counting (and overflowing);
       CaptOvr = T1Ovs;    
       flag = WAIT;

       //J: Generally bad to print in ISRs
       //Serial.println(flag);

       break;
      }
  }


  ISR(TIMER1_OVF_vect)
  {
    T1Ovs++;

    // J: Just to be correct, check for overflow of the overflow, otherwise if it overflows we would get an incorrect answer.
    if (!T1Ovs) {
      doubleOverflowError=1;
    }
  }

  void setup()
  {

    Serial.begin(9600);

    InitTimer1();
    StartTimer1();
  }

  void loop() {
    // J: No need to bracket this set with cli() becuase the counter will not be counting until wait is updated

    flag = CAPTURE_1;

    while (flag != WAIT);


    // J: Parenthesis and explicit cast for good luck! ( and to ensure correct size and order for operations) 


     if (doubleOverflowError) {
         Serial.println( "Double Overflow Error! Use a bigger prescaller!");
     } else {
         Serial.println( ( (unsigned long) (Capt2) + (CaptOvr * 0x10000UL) )-Capt1 );
     } 
}
\$\endgroup\$
  • \$\begingroup\$ Note that long int calculations are extremely slow on 8 bit MCUs, so if speed matters at all then it probably makes since to use some conditional code to process the overflow count rather than doing the straightforward way you have. \$\endgroup\$ – bigjosh Jun 15 '15 at 23:38
  • \$\begingroup\$ Thank you so, so much, you've been unbelievably helpful. Do you work with microcontrollers for a living? I'd really like to get more comfortable with them -- do you have any suggestions for resources that could be helpful? Something like an Arduino starter kit but a bit more advanced, either with AVR or PIC? Thanks again! \$\endgroup\$ – user3753934 Jun 16 '15 at 12:23
  • \$\begingroup\$ I think just keep doing what you are doing. Make stuff you are interested, and make mistakes along the way. I see an Arduino as a convenient AVR power supply/programmer and you can do anything on it that you can do with any AVR. Your coding looks great - all the problems you have are just a matter of experience (once you've tracked down a few bugs related to ISRs updating variables, you don't make them any more :)). I also find the AVR data sheets and application notes extremely helpful and well written. PIC too. I'd dig in with some of those that look interesting to you. \$\endgroup\$ – bigjosh Jun 16 '15 at 13:13
  • \$\begingroup\$ Quick question about this comment: "// J: No need to bracket this set with cli() becuase the counter will not be counting until wait is updated" in the loop() function. Does that while (flag != WAIT) loop cause the timer to stop counting as well? I thought that was something that happens automatically, unless you explicitly turn the timer off. \$\endgroup\$ – user3753934 Jun 16 '15 at 17:03
  • \$\begingroup\$ The timer is always running once you set the prescaler to 1. That means that the ISR does get called anytime the input triggers. But if you look at the switch(), if the STATE is wait, then ISR will return without updating any variables. See it? But in the current iteration of the code it does not matter at all because the flag is a unit8, and assignments to single bytes are atomic. It would only matter if you were doing anything non-atomic before starting the timing sequence, so you can safely ignore that comment as long as you remember that the timer is always running once you start. \$\endgroup\$ – bigjosh Jun 16 '15 at 19:59
0
\$\begingroup\$

In the ISR, you do a couple of checks that depend on the value of flag, then unconditionally increment it.

In your normal case, what happens is this:

  1. Main routine sets flag to 0
  2. ISR runs, sees flag is 0, stores value in Capt1, increments flag
  3. ISR runs again, sees flag is 1, stores value in Capt2, increments flag
  4. Main routine sees flag is 2, outputs result, resets everything

However, it's entirely possible for the ISR to execute twice before your main routine can check the value of flag. In which case, what happens is this:

  1. Main routine sets flag to 0
  2. ISR runs, sees flag is 0, stores value in Capt1, increments flag
  3. ISR runs again, sees flag is 1, stores value in Capt2, increments flag
  4. ISR runs again, increments flag to 3
  5. Main routine runs, flag is not 2, does nothing
  6. Repeat steps 4 and 5 independently until flag loops around to 2 again in time for main to catch it.

In the meantime, your overflow counter has been counting up and up, so the correct interval between captures gets added to some indeterminate multiple-of-255 of the interval from the overflow count.

The easiest way to fix this (and make the code clearer at the same time) is to make flag an enum, and have it step through three states - capture 1, capture 2, wait - in a state machine that's reset in main. Here's a simplified example:

typedef enum {
    CAPTURE_1,
    CAPTURE_2,
    IDLE
} timer_state_t

timer_state_t flag = IDLE;
volatile long Capt1, Capt2;

ISR(TIMER1_CAPT_vect) {
    switch(flag) {
    case CAPTURE_1:
        Capt1 = ICR1;
        flag = CAPTURE_2;
        break;
    case CAPTURE_2:
        Capt2 = ICR1;
        flag = IDLE;
        break;
    }
}

void main() {
    flag = CAPTURE_1;
    while(flag != IDLE); // Wait until we're done
    // Do something with Capt2 and Capt1
}

Another alternative is to simply store a single count (and overflow value, if necessary), and each time the capture ISR triggers, calculate a new value for the frequency, store it in a global, and reset it - thus providing for continuous measurement. That would look something like this:

volatile long interval = -1;

ISR(TIMER1_CAPT_vect) {
    static long capt = 0; // The value of this variable persists between calls to the ISR
    interval = capt - ICR1;
    capt = ICR1;
}

void main() {
    while(interval == -1); // Wait until we have a useful value
    // Do something with interval
}
\$\endgroup\$
  • \$\begingroup\$ Hm. It seems to be getting stuck in the "while(flag != WAIT);" loop. I inserted a Serial.println(flag) at various points, and it outputted a 0 (first interrupt, Capt1 = ICR1), a 1 (second interrupt, Capt2 = ICR1), and then infinite 2s (flag = WAIT). The interrupt is clearly still working because it continues to print 2s... I just don't understand why it's not exiting the while(flag != WAIT) loop. \$\endgroup\$ – user3753934 Jun 15 '15 at 18:32
  • \$\begingroup\$ I've updated my answer with the second iteration of code, if you get the chance to take a look. Thanks so much! \$\endgroup\$ – user3753934 Jun 15 '15 at 18:40
  • \$\begingroup\$ Actually this is really strange, it seems to be affected by the Serial output. \$\endgroup\$ – user3753934 Jun 15 '15 at 19:16
  • \$\begingroup\$ I tried doing it with global variables, but the resolution isn't high enough... the difference between the interval for 75kHz and 50kHz is only a couple clock ticks. \$\endgroup\$ – user3753934 Jun 15 '15 at 20:35
  • \$\begingroup\$ You shouldn't try and print to serial from an ISR - it'll cause lots of strange things to happen. I don't know what you mean in your last comment, can you clarify? \$\endgroup\$ – Nick Johnson Jun 16 '15 at 11:23

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.