While loop with ISR

Go To Last Post
9 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I´m using TIMER1 to wait instead using delay_ms that was freezing the program. And now I´m havving a strange behavior and I don´t know what´s happening.

 

Here´s part of my code, or at least what i think it´s relevant for this problem.

 

#define F_CPU 16000000UL

 

static volatile uint8_t counter;

static volatile uint8_t start_counter;

 

int main(void)

{

 

    init(); // initialize the system

 

    while (1)

    {

            // Do stuff

    }

}

 

void init()

{

    // Do some stuff

    // Start timer1

    TCCR1B |= (1 << WGM12);                 // Configure timer 1 for CTC mode

    TIMSK1 |= (1 << OCIE1A);                // Enable CTC interrupt

    OCR1A = 624;                            // Set CTC compare value to 1Hz at 16MHz AVR clock, with a prescaler of 1024

    TCCR1B |= ((1 << CS10) | (1 << CS12));  // Start timer at Fcpu/1024

 

    // Define status and screen

    status = STARTING;

    screen = HOME;

 

    counter = 0; // Set counter to zero

    start_counter = 0; // Set start counter to zero

 

    sei(); //  Enable global interrupts

    while(start_counter <= 2){}; // Loop unitl 2 seconds

    cli(); // stop global interrupts

 

    status = STOPPED;

    counter = 0;

 

    oled_clear();

}

 

ISR(TIMER1_COMPA_vect)

{

    counter++;

    if (counter == 25) // 1 second

    {

        seconds++;

        hours = seconds / 3600;

        counter = 0;

        if (status == STARTING) { start_counter++;}

    }

}

 

If I try with the above code, the program freezes at this loop while(start_counter <= 2){};

 

But when I chenaged it to see what´s going on to a loop like this one:

 

while(start_counter <= 2){} // Loop unitl 2 seconds

    {

         oled_set_font_size(0);

         oled_goto(7,0);

         oled_write_int(start_counter,1);

    }

 

The program just works fine. It show the second counting from 0 to 2 and then move to next step. What´s wrong with  while(start_counter <= 2){}; ???

 

I was trying to use delay_ms(2000) and it was freezing the screen too....

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

for a start I think you have no idea how interrupts work, and second have you tried stepping through you code in your head?

 

you should have seen that once you are in the while loop nothing is incrementing your start_counter so it will never be anything other than 0

 

O and you should not turn on and off interrupts all the time they should be on when you use them ( what is the point of having them otherwise) and only when you have very important time critical things to do you turn them off as short as possible

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

As soon as I read the thread title my thoughts were "Nooooooo!!!"

 

On AVR an ISR handler runs with interrupts disabled so any time spent in an ISR "hogs the CPU" and nothing else can run. So the ISRs should always be designed to be over in microseconds. Just service the peripheral (like pick up UDR in a USART interrupt for example) then return. In a timer interrupt it's likely to be little more than setting a volatile flag to say the event occurred or, at a a pinch, triggering some further behaviour (quickly) if something is particularly timing sensitive.

 

With this in mind you would almost never use while() in an ISR. It implies waiting for some condition to happen. Often that can be something that may take quite some time to occur and during all that time the entire system is held to ransom.

 

So take a step back and say why you think it's necessary to delay inside an ISR.

 

In your code I see some stuff for OLED writing. Well OLEDs are aimed at human beings and human beings can only cope with about 10 things happening per second. In MCU terms that can be 10,000's if not 100,000's of machine cycles. You don't want to waste that many CPU cycles just keeping the shaved ape entertained. Something like display writing can easily be done slowly, at leisure in your main() code. At the moment all you have in main is "// Do stuff" and yet you are NOT doing stuff. Suggest you consider what parts of your operation can be moved there.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the wuick reply.

 

Maybe I should take a step back and see why using delay_ms(2000) is freezing the program.

 

Before this event the program shows a splash screen and I want it to stay there for 2 seconds and then go to the main loop. In the init function it also starts the RTC module (DS1307) and the Oled. There are things happening in the main loop, just didn´t want to put it all here.

 

In the main loop I´m also using the timer1 to keep track of the seconds. So it´s not a good idea right? What you suggest to me? Maybe reading the seconds registers everytime and do whatever I want when it changes? Using another external peripheral?

 

I´m using a 16MHz crystal and I set the lfuse to 0xFF. Here´s my makefile (took that form Make - AVR Programming book):

 

##########------------------------------------------------------##########
##########              Project-specific Details                ##########
##########    Check these every time you start a new project    ##########
##########------------------------------------------------------##########

MCU   = atmega328p
F_CPU = 16000000UL
BAUD  = 9600UL
## Also try BAUD = 19200 or 38400 if you're feeling lucky.

## A directory for common include files and the simple USART library.
## If you move either the current folder or the Library folder, you'll
##  need to change this path to match.
LIBDIR = lib

##########------------------------------------------------------##########
##########                 Programmer Defaults                  ##########
##########          Set up once, then forget about it           ##########
##########        (Can override.  See bottom of file.)          ##########
##########------------------------------------------------------##########

PROGRAMMER_TYPE = avrisp
# extra arguments to avrdude: baud rate, chip type, -F flag, etc.
PROGRAMMER_ARGS = -b 19200 -P com5

##########------------------------------------------------------##########
##########                  Program Locations                   ##########
##########     Won't need to change if they're in your PATH     ##########
##########------------------------------------------------------##########

CC = avr-gcc
OBJCOPY = avr-objcopy
OBJDUMP = avr-objdump
AVRSIZE = avr-size
AVRDUDE = avrdude

##########------------------------------------------------------##########
##########                   Makefile Magic!                    ##########
##########         Summary:                                     ##########
##########             We want a .hex file                      ##########
##########        Compile source files into .elf                ##########
##########        Convert .elf file into .hex                   ##########
##########        You shouldn't need to edit below.             ##########
##########------------------------------------------------------##########

## The name of your project (without the .c)
# TARGET = blinkLED
## Or name it automatically after the enclosing directory
TARGET = $(lastword $(subst /, ,$(CURDIR)))

# Object files: will find all .c/.h files in current directory
#  and in LIBDIR.  If you have any other (sub-)directories with code,
#  you can add them in to SOURCES below in the wildcard statement.
SOURCES=$(wildcard *.c $(LIBDIR)/*.c)
OBJECTS=$(SOURCES:.c=.o)
HEADERS=$(SOURCES:.c=.h)

## Compilation options, type man avr-gcc if you're curious.
CPPFLAGS = -DF_CPU=$(F_CPU) -DBAUD=$(BAUD) -I. -I$(LIBDIR)
CFLAGS = -Os -g -std=gnu99 -Wall
## Use short (8-bit) data types
CFLAGS += -funsigned-char -funsigned-bitfields -fpack-struct -fshort-enums
## Splits up object files per function
CFLAGS += -ffunction-sections -fdata-sections
LDFLAGS = -Wl,-Map,$(TARGET).map
## Optional, but often ends up with smaller code
LDFLAGS += -Wl,--gc-sections
## Relax shrinks code even more, but makes disassembly messy
## LDFLAGS += -Wl,--relax
## LDFLAGS += -Wl,-u,vfprintf -lprintf_flt -lm  ## for floating-point printf
## LDFLAGS += -Wl,-u,vfprintf -lprintf_min      ## for smaller printf
TARGET_ARCH = -mmcu=$(MCU)

## Explicit pattern rules:
##  To make .o files from .c files
%.o: %.c $(HEADERS) Makefile
     $(CC) $(CFLAGS) $(CPPFLAGS) $(TARGET_ARCH) -c -o $@ $<;

$(TARGET).elf: $(OBJECTS)
    $(CC) $(LDFLAGS) $(TARGET_ARCH) $^ $(LDLIBS) -o $@

%.hex: %.elf
     $(OBJCOPY) -j .text -j .data -O ihex $< $@

%.eeprom: %.elf
    $(OBJCOPY) -j .eeprom --change-section-lma .eeprom=0 -O ihex $< $@

%.lst: %.elf
    $(OBJDUMP) -S $< > $@

## These targets don't have files named after them
.PHONY: all disassemble disasm eeprom size clean squeaky_clean flash fuses

all: $(TARGET).hex

debug:
    @echo
    @echo "Source files:"   $(SOURCES)
    @echo "MCU, F_CPU, BAUD:"  $(MCU), $(F_CPU), $(BAUD)
    @echo

# Optionally create listing file from .elf
# This creates approximate assembly-language equivalent of your code.
# Useful for debugging time-sensitive bits,
# or making sure the compiler does what you want.
disassemble: $(TARGET).lst

disasm: disassemble

# Optionally show how big the resulting program is
size:  $(TARGET).elf
    $(AVRSIZE) -C --mcu=$(MCU) $(TARGET).elf

clean:
    rm -f $(TARGET).elf $(TARGET).hex $(TARGET).obj \
    $(TARGET).o $(TARGET).d $(TARGET).eep $(TARGET).lst \
    $(TARGET).lss $(TARGET).sym $(TARGET).map $(TARGET)~ \
    $(TARGET).eeprom

squeaky_clean:
    rm -f *.elf *.hex *.obj *.o *.d *.eep *.lst *.lss *.sym *.map *~ *.eeprom

##########------------------------------------------------------##########
##########              Programmer-specific details             ##########
##########           Flashing code to AVR using avrdude         ##########
##########------------------------------------------------------##########

flash: $(TARGET).hex
    $(AVRDUDE) -c $(PROGRAMMER_TYPE) -p $(MCU) $(PROGRAMMER_ARGS) -U flash:w:$<

## An alias
program: flash

flash_eeprom: $(TARGET).eeprom
    $(AVRDUDE) -c $(PROGRAMMER_TYPE) -p $(MCU) $(PROGRAMMER_ARGS) -U eeprom:w:$<

avrdude_terminal:
    $(AVRDUDE) -c $(PROGRAMMER_TYPE) -p $(MCU) $(PROGRAMMER_ARGS) -nt

## If you've got multiple programmers that you use,
## you can define them here so that it's easy to switch.
## To invoke, use something like `make flash_arduinoISP`
flash_usbtiny: PROGRAMMER_TYPE = usbtiny
flash_usbtiny: PROGRAMMER_ARGS =  # USBTiny works with no further arguments
flash_usbtiny: flash

flash_usbasp: PROGRAMMER_TYPE = usbasp
flash_usbasp: PROGRAMMER_ARGS =  # USBasp works with no further arguments
flash_usbasp: flash

flash_arduinoISP: PROGRAMMER_TYPE = avrisp
flash_arduinoISP: PROGRAMMER_ARGS = -b 19200 -P /dev/ttyACM0
## (for windows) flash_arduinoISP: PROGRAMMER_ARGS = -b 19200 -P com5
flash_arduinoISP: flash

flash_109: PROGRAMMER_TYPE = avr109
flash_109: PROGRAMMER_ARGS = -b 9600 -P /dev/ttyUSB0
flash_109: flash

##########------------------------------------------------------##########
##########       Fuse settings and suitable defaults            ##########
##########------------------------------------------------------##########

## Mega 48, 88, 168, 328 default values
LFUSE = 0x62
HFUSE = 0xdf
EFUSE = 0x00

## Generic
FUSE_STRING = -U lfuse:w:$(LFUSE):m -U hfuse:w:$(HFUSE):m -U efuse:w:$(EFUSE):m

fuses:
    $(AVRDUDE) -c $(PROGRAMMER_TYPE) -p $(MCU) \
               $(PROGRAMMER_ARGS) $(FUSE_STRING)
show_fuses:
    $(AVRDUDE) -c $(PROGRAMMER_TYPE) -p $(MCU) $(PROGRAMMER_ARGS) -nv

## Called with no extra definitions, sets to defaults
set_default_fuses:  FUSE_STRING = -U lfuse:w:$(LFUSE):m -U hfuse:w:$(HFUSE):m -U efuse:w:$(EFUSE):m
set_default_fuses:  fuses

## Set the fuse byte for full-speed mode
## Note: can also be set in firmware for modern chips
set_fast_fuse: LFUSE = 0xE2
set_fast_fuse: FUSE_STRING = -U lfuse:w:$(LFUSE):m
set_fast_fuse: fuses

## Set the EESAVE fuse byte to preserve EEPROM across flashes
set_eeprom_save_fuse: HFUSE = 0xD7
set_eeprom_save_fuse: FUSE_STRING = -U hfuse:w:$(HFUSE):m
set_eeprom_save_fuse: fuses

## Clear the EESAVE fuse byte
clear_eeprom_save_fuse: FUSE_STRING = -U hfuse:w:$(HFUSE):m
clear_eeprom_save_fuse: fuses

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Have you also updated the clkdiv8 fuse? else you are still only running at 2MHz instead of 16, so your 2 second delay becomes 16 seconds.......

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I don't see "status" as volatile.  In fact we don't see STARTING or STOPPED either.

 

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Forgot to copy and past them, here they are:

 

#define F_CPU 16000000UL

 

#include <stdlib.h>
#include <avr/interrupt.h>
#include <avr/pgmspace.h>
#include <avr/io.h>
#include <util/delay.h>

 

#include "lib/doutputs.h"
#include "lib/dinputs.h"
#include "lib/eeprom.h"
#include "lib/keypad.h"
#include "lib/math.h"
#include "lib/oled.h"
#include "lib/i2c.h"
#include "lib/rtc.h"

 

// Status definitions
#define STOPPED 0
#define STARTING 1
#define RUNNING 2
#define STANDBY 3
#define FAILURE1 11
#define FAILURE2 12
#define FAILURE3 13

 

// Screen definitions
#define HOME 0

 

char status;
char screen;
static volatile uint8_t counter;
static volatile uint8_t start_counter;
 

 

status is not delcared with volatile. I´ll test it later.

 

What is stange is that using this:

 

while(start_counter <= 2); // Loop unitl 2 seconds
 

Doesn´t work and using this:

 

while(start_counter <= 2)
{
     oled_set_font_size(0);
    oled_goto(7,0);
    oled_write_int(start_counter,1);

}

 

Works fine.

it works fine

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

toscano wrote:

Works fine.

it works fine

yeah probably because those oled calls take a considerable time - so they implement a delay in themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

toscano wrote:
    OCR1A = 624;                            // Set CTC compare value to 1Hz at 16MHz AVR clock, with a prescaler of 1024

How do you get 1Hz from that?