ATMega324 timer1 keeps running

[admiralk]

I am using the timer to time the duration of states in a state machine, so basically it should be acting as a one-shot timer. Once it initially starts however, it never stops running.

The state machine does progress through its states and finish, but the states last for too long and for random times. I will try to cut out most of the trash that accumulated while debugging and unrelated stuff. Apologies in advance if this too disjointed.

Timer.c

Code: [Select]

#include "Timer.h"

 void Timer_Init()
 {
// CTC Mode
TCCR1B |= (1 << WGM12);
// Output compare interrupt A enabled.
TIMSK1 |= (1 << OCIE1A);
// Set compare match.
//OCR1A = 0x3d09; // 15625
Timer_Frequency(1);
// Enable interrupts.
sei();
 }

 void Timer_Start(Timer *t)
 {
//check if timer is already running!
if(t->timer_running)
return;

t->timer_expired = 0;
t->timer_running = 1;

// 64 prescaler.
TCCR1B |= (1 << CS11) | (1 << CS10);
 }
 
 void Timer_Frequency(uint8_t f)
 {
// OCRnA = F_CPU / frequency * 2 * N - 1
OCR1A = (1000000ul / (f * 2 * 64) - 1);
 }
The timer is only used in this function.

Code: [Select]

#include "EscapeBehavior.h"
 //#include "Serial.h"

 uint8_t currentState = 0;

 void Escape(int id, uint8_t *ready, int8_t *r, int8_t *l, Timer *t)
 {
//USART_Transmit(currentState);
switch(currentState)
{
case IDLE:
{
if(ready[id] == 1)
currentState = BACK;

break;
}
case BACK:
{
r[id] = -1;
l[id] = -1;
if(t->timer_running == 0)   // if timer is not running, start it.
Timer_Start(t);

if(t->timer_expired != 1)   // if timer has not reached its end.
break;

currentState = TURN;
break;
}
case TURN:
{
r[id] = -1;
l[id] = 1;
if(t->timer_running == 0)   // if timer is not running, start it.
Timer_Start(t);

if(t->timer_expired != 1)   // if timer has not reached its end.
break;

currentState = FOREWARD;
break;
}
case FOREWARD:
{
r[id] = 1;
l[id] = 1;
if(t->timer_running == 0)   // if timer is not running, start it.
Timer_Start(t);

if(t->timer_expired != 1)    // if timer has not reached its end.
break;

currentState = IDLE;
ready[id] = 0;
break;
}
//default:
//currentState = IDLE;
}
 }
the timer is supposed to be stopped in the ISR, and maybe that is where I am missing something.

Code: [Select]

// Timer interrupt.
ISR(TIMER1_COMPA_vect)
{
// Timer ran its course.
timer.timer_expired = 1;
// Turn off timer.
TCCR1B &= (~(1 << CS11) | ~(1 << CS10) | ~(1 << CS12));
timer.timer_running = 0;
USART_Transmit(times);
times++;
}
Yes, I know using the USART in an interrupt is a bad idea, but it is only for debugging and how I know the timer never stops.

main is simple.

Code: [Select]

uint8_t behavior_ready[NUM_BEHAVIORS];  // Set when behavior is active.
int8_t right_motor_speed[NUM_BEHAVIORS];// Speed of right motor.
int8_t left_motor_speed[NUM_BEHAVIORS]; // Speed of left motor.


int main(void)
{
Timer_Init();
Motor_Init();

USART_Init(MYUBRR);

// Set up pins.
//   IR Sensor
DDRB &= ~(1 << PINB0);
// Set up pcint8.
// Set pin change mask
PCMSK1 |= (1 << PCINT8);
// enable pin change interrupts
PCICR |= (1 << PCIE1);
// enable interrupts.
sei();

    while (1)
    {
CruiseBehavior(CRUISE_ID);
Escape(ESCAPE_ID, behavior_ready, right_motor_speed, left_motor_speed, &timer);
speed = arbitrate();
Motor_Speed(speed.right, speed.left);
    }
}

This supposed to be simple code and only lights LEDs based the direction each motor should be running. As I said, it does do that, just not at the correct times. Once started, the timer never stops and as near as I can tell never misses a beat regardless how often or when Escape() gets re-enabled. My last thought was that I somehow could not clear the register to stop the timer in the interrupt, but putting it in the while loop of main did not make a difference. The variables; timer_running and timer_expired must be getting changed, or the state machine would get stuck once it left idle.

Any thoughts on why the timer continuously runs would be appreciated.

[ledtester]

This will not clear the CS10, CS11 and CS12 bits:

Code: [Select]

TCCR1B &= (~(1 << CS11) | ~(1 << CS10) | ~(1 << CS12));

Try:

Code: [Select]

TCCR1B &= ~((1 << CS11) | (1 << CS10) | (1 << CS12));

or simply:

Code: [Select]

TCCR1B &= 0xF8;


[admiralk]

Suggestion one hangs the state machine in state 1; BACK. The timer keeps running.

Suggestion two hangs the state machine in state 2; TURN. The timer keeps running.

I originally did not have the outer ()s. I added them just be specific. I also tried clearing each bit on a separate line. CS12 is just added for completeness, it should never be set to begin with.

Thanks for trying.

[Psi]

Do you have your timer variables declared as volatile.

You are setting them in an interrupt and checking them outside the interrupt, so they need to be volatile.
Otherwise your code could be getting confused and reading the variable values wrong.

Also check you are using the right datasheet,  324P and 324 might not be the same MCU.
I know that 328 and 328P are not the same.

[admiralk]

Do you have your timer variables declared as volatile.

You are setting them in an interrupt and checking them outside the interrupt, so they need to be volatile.
Otherwise your code could be getting confused and reading the variable values wrong.

Also check you are using the right datasheet,  324P and 324 might not be the same MCU.
I know that 328 and 328P are not the same.

I thought about that, but since they are being changed at some point, they are not being optimized out.

Code: [Select]

typedef struct
{
    uint8_t timer_expired;  //!< Set when the timer finishes.
    uint8_t timer_running;  //!< Set when the timer is running.
} Timer;
Changing it to

Code: [Select]

typedef struct
{
    volatile uint8_t timer_expired;  //!< Set when the timer finishes.
    volatile uint8_t timer_running;  //!< Set when the timer is running.
} Timer;
Hangs it in state 1: Back, and never starts the timer. That really confuses me.

And yes, it is the correct datasheet, https://ww1.microchip.com/downloads/en/DeviceDoc/ATmega164P-324P-644P-Data-Sheet-40002071A.pdf. I did have problems originally loading the program because I omitted the P in the command.

[cv007]

You can see why your clearing of cs bits is wrong-
https://godbolt.org/z/Gx1aM7T9M

You can use something like the timer1_restart function + enum in the code above instead of dealing with the CSn bit manipulation multiple times in your code. Making the function static will result in the compiler (most likely) inlining the code so you end up with easy to use code and no loss of code size.
timer1_restart( T1_OFF ); //turn off
timer1_restart( T1_DIV64 ); //turn off, clear tcn1, clear flags, start timer1

[Psi]

Do you have your timer variables declared as volatile.

You are setting them in an interrupt and checking them outside the interrupt, so they need to be volatile.
Otherwise your code could be getting confused and reading the variable values wrong.

Also check you are using the right datasheet,  324P and 324 might not be the same MCU.
I know that 328 and 328P are not the same.

I thought about that, but since they are being changed at some point, they are not being optimized out.

It's also that it might copy the variable value into a CPU register and then use it sometime later not realizing an interrupt happened and the copy is no longer correct.

[admiralk]

Well, now I seem to have broken it. The state machine seems to always lock up and at different states. I think it is time to go back to the drawing board and start over.

I do not completely understand that restart function at the moment, but will look closer on the rewrite, along with other suggestions.

Thanks so far, hopefully it works better by next weekend.

[Psi]

If you have a variable used in an interrupt and also outside the interrupt then make it volatile.
The downside to making it volatile when it does not need to be is minimal vs the problems it will cause if it should be volatile and is not.

[admiralk]

That makes sense. I have just been flipping too many things back and forth that I lost track of where I was going. There are other problems I ran across also. Hopefully I can clear some of that up before having to work around the other code, like I have been trying to do.

[DavidAlfa]

You escape function looks very wrong.

Code: [Select]

ISR(TIMER1_COMPA_vect)
{
timer.timer_expired = 1;
timer.timer_running = 0;
}


Code: [Select]

                         if(t->timer_running == 0)   // This will execute everytime the timer expires and set expired = 0
Timer_Start(t);

if(t->timer_expired != 1)   // so it will always stop here
break;

currentState = TURN;      // And never get there
break;

Perhabs:

Code: [Select]

#include "EscapeBehavior.h"
 //#include "Serial.h"

 uint8_t currentState = 0;

 void Escape(int id, uint8_t *ready, int8_t *r, int8_t *l, Timer *t)
 { :-+
switch(currentState)
{
case IDLE:
{
if(ready[id] == 1) {
currentState = BACK;
                                Timer_Start();
                         }
break;
}
case BACK:
{
r[id] = -1;
l[id] = -1;

if(t->timer_expired == 1) {
currentState = TURN;
                                Timer_Start();
                         }
break;
}
case TURN:
{
r[id] = -1;
l[id] = 1;
                         if(t->timer_expired == 1) {
currentState = FOREWARD;
                                Timer_Start();
                         }
break;
}
case FOREWARD:
{
r[id] = 1;
l[id] = 1;
if(t->timer_expired == 1) {
                                ready[id] = 0;
currentState = IDLE;
                         }
break;
}
}
 }


[admiralk]

You may be right. It would explain why the states were switching randomly, the interrupt happens at just the time. The more I think about it, the more it looks like that might be the problem. I never did like having both those variables there to begin with. Hopefully I will have time to try it out sometime today.

[langwadt]

why all the mess with start and stop and interrupts?  just keep the timer running, read it and calculate when you time has expired and poll that

[admiralk]

Why are interrupts a mess? What if I want to use the timer for something else? How does it do anything else if it is busy polling the timer?

I tried DavidAlfa's suggestions and it almost works. The states are now changing as they should, but about 10 times too fast. The calculation is from the datasheet, so I assume it should be correct. A 1, for 1Hz, should give me 1 second. Changing it to accept a float and passing 0.5 gives me about 1 second.

Code: [Select]

void Timer_Frequency(float f)
 {
// OCRnA = F_CPU / frequency * 2 * N - 1
OCR1A = (1000000ul / (f * 2 * 64) - 1);
 }
Maybe I goofed something in the expression or I am mistaken about the default clock speed?

[DavidAlfa]

The default ATmega324 speed is 8MHz, so why are you using 1MHz for the calcs?
It's much easier to process period instead frequency.
Also skip floats by all means.
I never worked with AVR so some parts might be wrong.

I clearly see something wrong. I don't see where the datasheet says the timer clock is half the system clock?
To me it runs either at CPU clock or through some of the prescalers, which also take the cpu clock?
Otherwise why are you adding the 2 here?

Code: [Select]

OCR1A = (1000000ul / (f * 2 * 64) - 1);
                          ^

OCR1A is 16bit, so the max value it can hold is 65535, which would account for 524ms at pre=64
Better to set the prescaler much higher, unless you really need the precision.
For example, pre=256 (T=32us) will allow 2.1s, and pre=1024 (T=128us) will do 8.4s.

Code: [Select]

void Timer_Init()
 {
    TCCR1B |= (1 << WGM12);                 // CTC Mode
    TIMSK1 |= (1 << OCIE1A);                // Output compare interrupt A enabled.
    Timer_Period(1000000);                  // 1.000.000us = 1s
    sei();
 }

 void Timer_Start(Timer *t)
 {
    if(t->timer_running)                    // Check if timer is already running!
        return;
    TCNT1 = 0;                              // Clear counter
    TCCR1B |= (1<<CS12) | (1<<CS10);        // Enable clock, 1024 prescaler  -> 8MHz/1024 = 7.8KHz, T = 128us
    t->timer_expired = 0;
    t->timer_running = 1;
 }

 void Timer_Period(uint32_t us)
 {
    OCR1A = us>256 ? (us>>8)-1 : 1;         // OCR1A = (us/128)-1 but avoid setting it to 0.
 }

ISR(TIMER1_COMPA_vect)
{
    timer.timer_running = 0;
    timer.timer_expired = 1;
    TCCR1B &= ~( (1<<CS11) | (1<<CS10) | (1<<CS12) );    // Turn off timer.
}


[langwadt]

Why are interrupts a mess? What if I want to use the timer for something else? How does it do anything else if it is busy polling the timer?

For something like this interrupts adds nothing but unnecessary complication. You can't use the timer for something else when you start and stop it, you can poll it from multiple places if it is free running. You are already polling the timer running, polling the timer value isn't any worse

[admiralk]

The default ATmega324 speed is 8MHz, so why are you using 1MHz for the calcs?

I was always under the impression that all ATmegas and ATtinys ran at 1MHz without a crystal or changing it in the setup.

I had to do an image of the equation since it did not copy well from the datasheet. N is the prescaler. My math skills have severely deteriorated, but I am pretty sure that my writing the equation to solve for OCRnA is correct . Using 1MHz for the baud rate calculations worked, unless that was just a fluke?

[ Specified attachment is not available ]

It is not likely that each state will take the same amount of time, so being able to change it is pretty much needed. While I would like to avoid floats, I can live with the truncation if gives a better result. At this point size is not a problem. It will probably take me a couple days to get into the rest of it. So far you have been a great help.

@langwadt If I did not say it in my original post, this is only a simulation to work out problems before trying to add it to the end goal. I have at least 3 other behaviors to add after this. Comparing the value of one variable vs. doing a calculation and then comparing the result is not the same. 

[cv007]

you can poll it from multiple places if it is free running

Ultimately, getting some form of system time means any code can deal with time on its own.

A simple example, where timer1 is free running with its count clk set to 1us (1Mhz/1 or 8Mhz/8), with only the ovf irq in use so a period of 65536us. The system time is in us, so a max usable period of an hour (is more, but is a nice limit) Once you can get time in some form, it can be used in various ways such as the simple task example. In this case, the timer1 irq fires every ~65ms so is not taking up much time unlike a 1ms irq rate, and the tcnt1 register is used as a us count value (added to the overflow count).

https://godbolt.org/z/daKPYjoEx
(untested)

The limit for us count in a uint32_t is ~66 minutes, so a 'task' duration needs to stay within an hour. One can extend the time into ms/sec if needed, but the example was intended to be simple. This example still requires polling for a task completion, but the timer has compare interrupts that could also be put to use to eliminate polling.

For any mcu, one of the first things one should do (or at least think about) is to get some form of system time in place. If the mcu has a low power timer (rtc, lptim, etc.), these can make for good system time sources as they typically can keep running while in sleep modes (and if they run off a 32k xtal, are pretty accurate also).

[langwadt]

The default ATmega324 speed is 8MHz, so why are you using 1MHz for the calcs?

I was always under the impression that all ATmegas and ATtinys ran at 1MHz without a crystal or changing it in the setup.

afaik 8Mhz RC is default

@langwadt If I did not say it in my original post, this is only a simulation to work out problems before trying to add it to the end goal. I have at least 3 other behaviors to add after this. Comparing the value of one variable vs. doing a calculation and then comparing the result is not the same.

subtract and compare is barely anymore code and you can reuse it for a all the wait states and you can use the timer from multiple places

[cv007]

afaik 8Mhz RC is default

The internal 8Mhz rc is default, and also CKDIV8 fuse is programmed by default. So unless the fuses are changed, 1Mhz is the default speed.

This is a common thing in the avr world- being 8 times slower than expected because the CKDIV8 fuse is left unchanged. For those that do not want to deal with the CKDIV8 fuse, one can also change the cpu clock prescale at runtime (CLKPR). It is also not a bad idea to simply set the clock prescale at runtime in any case (harmless if not needed, nice if CKDIV8 fuse bit gets programmed for some reason).

[admiralk]

@langwadt If I did not say it in my original post, this is only a simulation to work out problems before trying to add it to the end goal. I have at least 3 other behaviors to add after this. Comparing the value of one variable vs. doing a calculation and then comparing the result is not the same.

subtract and compare is barely anymore code and you can reuse it for a all the wait states and you can use the timer from multiple places

There is no way to get around checking something. Checking which state to change to in the interrupt would be the worst way to do it. If it only varied between 2 states, then that would be great, just toggle whatever. If I was running an RTOS, good luck on an 8 bitter, I could do this all with threads, which would be a better way all around. As this project developed, I might move to an ARM processor and do that. For now, I have what I have and trying to make due.

The timer does seem to be running continuous, once started, so that is still a problem. Maybe upping the clock speed will help? Maybe not? It is easy enough to try without setting fuses.

[DavidAlfa]

What's the final target of all of this?
You want to step the Escape functiom state every second? Only that?
It's ok to use the timer in interrupt mode, but can be simplified by a lot.
Leaving the timer interrupt running every second causes a negligible impact.
All you need to do is to reset counter after exiting IDLE state and then checking if the update flag was set to proceed to the next step.

Code: [Select]

volatile uint8_t update_state;
void Timer_Init()
{
  TCCR1B |= (1 << WGM12);                 // CTC Mode
  TIMSK1 |= (1 << OCIE1A);                // Output compare interrupt A enabled.   
  TCCR1B |= (1<<CS12) | (1<<CS10);        // Enable clock, 1024 prescaler  -> 8MHz/1024 = 7.8KHz, T = 128us
  Timer_Period(1000000);                  // 1.000.000us = 1s   
  sei();
 }

 void Timer_Start(Timer *t)
 {
  update_state = 0;
  TCNT1 = 0;                              // Clear counter
 }

void Timer_Period(uint32_t us)
{
  if(us<256)  us=256;                     // Ensure OCR1A value is set to > 0
  OCR1A = (us>>8) - 1;                    // OCR1A = (us/128)-1
}

void Timer_Period(uint32_t us)
{
  OCR1A = us>256 ? (us>>8)-1 : 1;         // OCR1A = (us/128)-1 but avoid setting it to 0.
}

Interrupt:

Code: [Select]

ISR(TIMER1_COMPA_vect)
{
   update_state = 1;
}

Escape functions:

Code: [Select]

uint8_t currentState;
volatile uint8_t newState;

}
void Escape(int id, uint8_t *ready, int8_t *r, int8_t *l)
{
  extern volatile uint8_t update_state;   
  if(currentState != IDLE && update_state !=1 )         // If not in idle and update_state didn't change, nothing to do
    return;

  switch(currentState)
  {
    case IDLE:
      if(ready[id] == 1) {
        currentState = BACK;
        Timer_Start();
      }   
      break;
       
    case BACK:
      r[id] = -1;
      l[id] = -1;     
      currentState = TURN;
      break;
     
    case TURN:
      r[id] = -1;
      l[id] = 1;
      currentState = FOREWARD;
      break;
     
     case FOREWARD:
      r[id] = 1;
      l[id] = 1;
      ready[id] = 0;
      currentState = IDLE;
      break;
  }
}


[DavidAlfa]

But for now, find out the correct frequency calculation.
Disable Escape() funcion and instead toggle a led:

Code: [Select]

void handle_led(void)
{
  if(update_state)
  {
    update_state=0;
    // Toggle led code here
  }
}

void main(void)
{
  // init, etc ...
  while(1)
  {
    handle_led();
  }
}

When it toggles every second, then continue with the other stuff.

[admiralk]

What's the final target of all of this?

The final target is the beginnings of a robot. The r and l arrays are for speeds, basically the duty cycle for the PWM. The arbitrator picks the active behavior with the highest priority. Eventually, the motor control will most likely be separated out to its own MCU. Sensor groups will probably have their own MCUs as well. I am using the 324 because I had a couple and they have the most I/O of the others I have on hand.

It will probably be Friday before I have a chance to do much with it now. I am getting too old for 10 hr. workdays.   I need a grey beard smiley. 

[admiralk]

OK, I think it is working now. I am not sure what the problem was, but after rewriting it I can now stop the timer. My original way still did not work, but this did, even if it did not before.

Code: [Select]

TCCR1B &= ~((1 << CS10) | (1 << CS11) | (1 << CS12));It is still running too fast, but since I am not concerned with what the actual frequency is, it does not matter. Just that I can change it.

Much thanks Dave! You gave a bunch of ideas on improving it in the final version. I still have some experimenting to do, but should be able to start on real code later this weekend.

[cv007]

It is still running too fast

The code link in post #17 shows how you can set the fuses, post #19 mentions the CKDIV8 fuse and also that you can set the prescaler at runtime regardless of what the CKDIV8 fuse happens to be.

The timer clock will be a known speed- you know the source clock, the cpu clock prescaler either via CKDIV8 fuse (/1 or /8) or set on your own via CLKPR, and the timer prescaler (/64 it appears you want). Your OCR1A calculation also has to be correct, which by your original post will be 2x faster than it should be (an extra /2 included because you were reading a toggle freq formula).

Without much work, you can create a simple test by toggling an led with the help of the timer so it will be easy to tell if the timer is running as expected. Not much fun chasing unknown clock speeds, so may as well get desired==actual before you get too deep into code that has magic values inserted to 'make it work'.

A simple modification of previously linked code-
https://godbolt.org/z/b5o8Eq678
which should blink an led at 1Hz- it either does or does not. If not, something is wrong. Create your own test as you see fit to get the timer running as expected.

[admiralk]

(an extra /2 included because you were reading a toggle freq formula)

That kind of struck a bell. I am not sure which way is right, but taking out the *2 made it run too slow; about 1.5 sec compared to about 2/3 sec. Maybe I am just counting wrong, but again, it does not really matter. The motors, in the real project, are an open loop drive. I will need to find the real time by trial and error. Someone mentioned setting a higher prescaler would give me more resolution. That is one of the experiments I need to try.

At this point, I forget why I picked a 64 prescaler. It is certainly not written in stone. My main objective was to get the timer to stop in the interrupt so that the states would all run at the same time, regardless of when the interrupt was triggered. Originally, the BACK state could last for any fraction of the timer. It could have been the whole time, if the timer was just starting. Or, it could have been a blink of an eye, if the timer was almost done. Now that that has been accomplished, I can work on how long each state needs to run at a given motor speed. It is much easier to change that than the motor speed. And more sensible.

I do appreciate your input just the same.