EmbDev.net

Forum: µC & Digital Electronics Problem incrementing global int variable in interrut service routine


Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
Hi everyone,

I have a strange Problem: I try to increment a counter inside a 
Interrupt Service Routine, but it seems as if it doesn't work whysoever. 
The ISR is triggered by a Timer Compare. The counter either gets set 
null or gets lost. I tried declaring it volatile, but this didn't work 
either. Here the Code in which i unsuccessfully tried sending the memory 
address of the counter in the loop and in the ISR:

/*
this goes on your arduino
for use with Processing example SimpleSerialArduinoscope
*/
#include <avr/io.h>
#include <avr/interrupt.h>

   volatile int val[512] = { 0 };
int i = 0;
long u;

void setup() {
  //pinMode(0, INPUT);

  Serial.begin(115200);
  cli();                                     // disable interrupts while 
messing with their settings
  TCCR1A = 0x00;                             // clear default timer 
settings, this kills the millis() funciton
  TCCR1B = 0x00;
  TCCR1B |= (1 << WGM12);                    // Configure timer 1 for 
CTC mode
  TCCR1B &= ~(0 << CS12);                    // Set timer prescaling by 
setting 3 bits
  TCCR1B |= (1 << CS11 | 1 << CS10);         // 001=1, 010=8, 011=64, 
101=1024
  TIMSK1 |= (1 << OCIE1B);                   // Enable CTC interrupt
  OCR1B  = 5000;                               // Set CTC compare value

  //ADCSRA |= (1 << ADEN);                     // enable ADC
  ADMUX = B01100000;                         // left adjusted output to 
increase sample rate, 8bit mode
  SMCR |= (1 << SM0);                        // Enable sleep AD 
conversion mode
  SMCR &= ~( (1 << SM2) | (1 << SM1) );
  //SREG |= (1 << ADIE);                       // Enable ADC ready 
interrupt
  ADCSRB |= ( ( 1 << ADTS2 | 1 << ADTS0 ) ); // configure ADC to trigger 
on Timer Compare Match B
  ADCSRB &= ~(1 << ADTS1);
  sei();                                     // turn interrupts back on
  ADCSRA = B11001111;                         // set ADC prescaler to 
128, ADC EN, ADC Start

}

void loop() {
//  i += 1;
//  if(i==512){
//  i=0;
//  }
//u = (long)i;
  Serial.println((HEX)&i);
}

void tx(){
    cli();
    for (int q=0;q<512;q++){
    Serial.print(val[q]);
    Serial.print(",");
    }
    Serial.println();
    i = 0;
  sei();
}

void rx(){
}

ISR(ADC_vect)
{
//Serial.println((int)ADCH);

  val[i] = (int)ADCH;
  i++;
  Serial.println((HEX)&i);
  if(i==512){
    tx();
  }
}

The Platform is a Atmel Atmega 1280. Target is to build a simple USB 
oscilloscope.

Has anybody an idea what the problem could be?

Thanks in advance,

ToKo

Author: Falk (Guest)
Posted on:

Rate this post
0 useful
not useful
@  Tobias K. (tobias_k83)

>I have a strange Problem: I try to increment a counter inside a
>Interrupt Service Routine, but it seems as if it doesn't work whysoever.

i must be volatile too, see article [[Interrupt]].

Regards
Falk

Author: Falk (Guest)
Posted on:

Rate this post
0 useful
not useful
Ahhhh, no links from mikrocontroller.net are working here :-0

http://www.mikrocontroller.net/articles/Interrupt

Author: kbuchegg (Guest)
Posted on:

Rate this post
0 useful
not useful
volatile is one thing.
You need it.


But the other thing is, that you have to make sure, to access i in an 
atomare fashion. You failed to do that.


BTW: even if this is a test programm, your ISR does to much lengthy 
things and corrupts your timing. Output to the RS232 does not belong in 
an ISR.

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
volatile didnt work either... =(

Author: kbuchegg (Guest)
Posted on:

Rate this post
0 useful
not useful
And please:
leave the Interrupt Enable Bits alone inside an ISR! No cli and sei

That is one sure way to shoot yourself in the knee

Author: kbuchegg (Guest)
Posted on:

Rate this post
0 useful
not useful
> volatile didnt work either... =(

volatile ist necessary in that case but it is only half of the story.
atomic access is the other half.

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
okay, atomic access is totally new to me. how do I do that? How can i 
shorten or modify my code so it doesn't corrupt the timing? my plan was 
to fill an array and transmit it when it's full.

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
okay, my idea would be to put the tx() inside an if(i==512)... into 
loop, so that cli/sei can be used (and transmission is not disturbed). 
but what about atomic access? Did you mean the functions of 
<util/atomic.h>?

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
cleaned version without atomic access:

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

volatile int val[512] = { 0 };
volatile int i = 0;

void setup() {
  Serial.begin(115200);
  cli();                                     // disable interrupts while 
messing with their settings
  TCCR1A = 0x00;                             // clear default timer 
settings, this kills the millis() funciton
  TCCR1B = 0x00;
  TCCR1B |= (1 << WGM12);                    // Configure timer 1 for 
CTC mode
  TCCR1B &= ~(0 << CS12);                    // Set timer prescaling by 
setting 3 bits
  TCCR1B |= (1 << CS11 | 1 << CS10);         // 001=1, 010=8, 011=64, 
101=1024
  TIMSK1 |= (1 << OCIE1B);                   // Enable CTC interrupt
  OCR1B  = 5000;                               // Set CTC compare value

  ADMUX = B01100000;                         // left adjusted output to 
increase sample rate, 8bit mode
  SMCR |= (1 << SM0);                        // Enable sleep AD 
conversion mode
  SMCR &= ~( (1 << SM2) | (1 << SM1) );
  //SREG |= (1 << ADIE);                       // Enable ADC ready 
interrupt
  ADCSRB |= ( ( 1 << ADTS2 | 1 << ADTS0 ) ); // configure ADC to trigger 
on Timer Compare Match B
  ADCSRB &= ~(1 << ADTS1);
  sei();                                     // turn interrupts back on
  ADCSRA = B11001111;                         // set ADC prescaler to 
128, ADC EN, ADC Start
}

void loop() {
if(i==512){
}
tx();
}

void tx(){
    cli();
    for (int q=0;q<512;q++){
    Serial.print(val[q]);
    Serial.print(",");
    }
    Serial.println();
    i = 0;
  sei();
}

void rx(){

}

ISR(ADC_vect)
{
  val[i] = (int)ADCH;
  i++;
}

sorry, I am still a bit a kind of noob, is this code better for doing 
the job? According to the link above it would be useful if I reduce the 
array size to 128 as well as the datatype to Byte to workaround using

ATOMIC_BLOCK(ATOMIC_FORCEON)
      {

      }

right?

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
Sorry, I meant an array size of 255 -> byte

Author: Falk (Guest)
Posted on:

Rate this post
0 useful
not useful
@ Tobias K. (tobias_k83)

Iam sure you are a navtive german speaker. So it is much easyer to do so 
. . . ;-)

>okay, my idea would be to put the tx() inside an if(i==512)... into
>loop, so that cli/sei can be used (and transmission is not disturbed).
>but what about atomic access? Did you mean the functions of
><util/atomic.h>?

Read the article.

http://www.mikrocontroller.net/articles/Interrupt

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
Hi Falk,

richtig geraten =) hab den Artikel gelesen, aber so richtig verstanden 
hab ich ihn nicht:

 PINx = (PORTx & 0x0F) ^ lcd_data;

ist laut Artikel die Sollvorgehensweise. Was bedeutet das in meinem 
Fall? ich meine Atomarer als val[i]=ADCH gehts ja nicht, oder soll ich 
da direkt mit Speicheradressen statt mit val[i] arbeiten? Atomar heißt 
ja nur, dass währenddessen nichts anderes ausgeführt werden kann oder?

Author: A. K. (prx)
Posted on:

Rate this post
0 useful
not useful
Tobias K. wrote:

>  PINx = (PORTx & 0x0F) ^ lcd_data;
> ist laut Artikel die Sollvorgehensweise.

Das ist eine mögliche Vorgehensweise, aber nur bei Ports, nicht bei 
Speichervariablen. Bei denen ist eine dieser ATOMIC_BLOCK Klammern genau 
richtig. Und zwar bei jedem Zugriff ausserhalb der ISR.

Nicht atomar kann heissen:
  unteres Byte lesen (alt),
  Interrupt mischt mit und zählt hoch,
  oberes Byte lesen (neu).
Das gibt dann gelesenen Datensalat und ist nur durch temporäre 
Abschaltung des Interrupts zu vermeiden.

Author: Falk (Guest)
Posted on:

Rate this post
0 useful
not useful
@Tobias K. (tobias_k83)

>richtig geraten =) hab den Artikel gelesen, aber so richtig verstanden
>hab ich ihn nicht:

Dann halt nochmal lesen und nachdenken.

> PINx = (PORTx & 0x0F) ^ lcd_data;

>ist laut Artikel die Sollvorgehensweise.

Ist ein Sonderfall für IO-Ports bei neueren AVRs. Ungünstiges Beispiel.

> Was bedeutet das in meinem Fall?
> ich meine Atomarer als val[i]=ADCH gehts ja nicht,

Das ist auch OK, weil es in der ISR ist. Die kann nicht unterbrochen 
werden (wenn man keine Tricks mit verschachtelten Interrupts macht).
Aber warum nicht val[i]=AWDW; ? Das ist ein einfacher, korrekter, 
VOLLSTÄNDIGER Zugriff auf den ADC.

> oder soll ich
>da direkt mit Speicheradressen statt mit val[i] arbeiten?

Nein.

> Atomar heißt
> ja nur, dass währenddessen nichts anderes ausgeführt werden kann oder?

Sicher, aber das ist auch wichtig. Atomar muss der Zugriff im 
Hauptprogramm ein. Warum, steht im Artikel.
AUsserdem ist deine Variante mal wiedr sehr ungünstig. Atomat muss und 
sollte NUR das Kopieren der globalen Variable i (ungünstiger Name!) in 
eine lokale Variable sein. Die Ausgabe sollte NICHT in den atomaren 
Block, weil sie die ganze Zeit deine Interrupts blockiert. Eher so.

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

volatile int val[512] = { 0 };
volatile int i = 0;

void setup() {
  Serial.begin(115200);
  cli();                                     // disable interrupts while
messing with their settings
  TCCR1A = 0x00;                             // clear default timer
settings, this kills the millis() funciton
  TCCR1B = 0x00;
  TCCR1B |= (1 << WGM12);                    // Configure timer 1 for
CTC mode
  TCCR1B &= ~(0 << CS12);                    // Set timer prescaling by
setting 3 bits
  TCCR1B |= (1 << CS11 | 1 << CS10);         // 001=1, 010=8, 011=64,
101=1024
  TIMSK1 |= (1 << OCIE1B);                   // Enable CTC interrupt
  OCR1B  = 5000;                               // Set CTC compare value

  ADMUX = B01100000;                         // left adjusted output to
increase sample rate, 8bit mode
  SMCR |= (1 << SM0);                        // Enable sleep AD
conversion mode
  SMCR &= ~( (1 << SM2) | (1 << SM1) );
  //SREG |= (1 << ADIE);                       // Enable ADC ready
interrupt
  ADCSRB |= ( ( 1 << ADTS2 | 1 << ADTS0 ) ); // configure ADC to trigger
on Timer Compare Match B
  ADCSRB &= ~(1 << ADTS1);
  sei();                                     // turn interrupts back on
  ADCSRA = B11001111;                         // set ADC prescaler to
128, ADC EN, ADC Start
}

void loop() {
if(i==512){
}
tx();
}

void tx(){
  int tmp;

    cli();
    tmp = i;
    i = 0;
    sei();
    for (int q=0;q<512;q++){
      Serial.print(val[q]);
      Serial.print(",");
    }
    Serial.println();
}

void rx(){

}

ISR(ADC_vect)
{
  val[i] = ADCW;
  i++;
}

MfG
Falk

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
okay, cool. Das heißt

ISR(ADC_vect)
{
  val[i] = ADCH;
  i++;
}

passt soweit, nur in der Loop und (?) in tx() kommen die Klammern zum 
Einsatz? Habe die Variablen alle mal auf 8bit runtergeschraubt, was ja 
die Zwangsatomisierung erübrigen sollte, da kam aber auch nix bei rum...

Vielen Dank schonmal für euer feedback!

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
Hi Falk,

ich wollte absichtlich nur das High Register lesen, weil man dadurch die 
Sampling rate hoch bekommt. hab den ADC auch entsprechend left adjusted 
konfiguriert. Werde ich gleich mal probieren, Deine Variante =)

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
BTW: warum ist i als Name ungünstig?

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
mist, kann es sein dass ich auch das globale array in ein lokales 
kopieren muss? In der Form funktionierts noch nicht...

Author: Falk (Guest)
Posted on:

Rate this post
0 useful
not useful
@  Tobias K. (tobias_k83)

>BTW: warum ist i als Name ungünstig?

Das ist ein Name für einen (lokalen) Index oder so, was man halt immer 
mal wieder schnell braucht. Globale Variablen sollte man mit 
aussagekräftigen Namen versehen, oder z.B. duch eine Vorsilbe ala gTime 
(g wie global).

>mist, kann es sein dass ich auch das globale array in ein lokales
>kopieren muss?

Sicher, dort wird ja auch in der ISR zugegriffen.

MFG
Falk

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
grrrr... der globale Counter wird immer noch nicht hochgezählt...

Author: Falk (Guest)
Posted on:

Rate this post
0 useful
not useful
Woran erkennst du das?
Poste VOLLSTÄNDIGEN Code als Anhang.

Author: Tobias K. (tobias_k83)
Posted on:
Attached files:

Rate this post
0 useful
not useful
ich sende aus der ISR heraus gCnt

(anhang geht irgendwie nich... hier noch mal gepastet:)

/*
this goes on your arduino
for use with Processing example SimpleSerialArduinoscope
*/
#include <avr/io.h>
#include <avr/interrupt.h>
//#include <util/atomic.h>

volatile int val[255] = { 0 };
volatile byte gCnt = 0;

void setup() {
  Serial.begin(115200);
  cli();                                     // disable interrupts while 
messing with their settings
  TCCR1A = 0x00;                             // clear default timer 
settings, this kills the millis() funciton
  TCCR1B = 0x00;
  TCCR1B |= (1 << WGM12);                    // Configure timer 1 for 
CTC mode
  TCCR1B &= ~(0 << CS12);                    // Set timer prescaling by 
setting 3 bits
  TCCR1B |= (1 << CS11 | 1 << CS10);         // 001=1, 010=8, 011=64, 
101=1024
  TIMSK1 |= (1 << OCIE1B);                   // Enable CTC interrupt
  OCR1B  = 5000;                             // Set CTC compare value

  ADMUX = B01100000;                         // left adjusted output to 
increase sample rate, 8bit mode
  SMCR |= (1 << SM0);                        // Enable sleep AD 
conversion mode
  SMCR &= ~( (1 << SM2) | (1 << SM1) );
  //SREG |= (1 << ADIE);                     // Enable ADC ready 
interrupt
  ADCSRB |= ( ( 1 << ADTS2 | 1 << ADTS0 ) ); // configure ADC to trigger 
on Timer Compare Match B
  ADCSRB &= ~(1 << ADTS1);
  sei();                                     // turn interrupts back on
  ADCSRA = B11001111;                        // set ADC prescaler to 
128, ADC EN, ADC Start
}

void loop() {
        if(gCnt==255){
            tx();
        }
}

void tx(){
 byte tmp;
 int valTmp[255];
    cli();
    tmp = gCnt;
    for(byte i;i<255;i++){
    valTmp[i] = val[i];
    }
    gCnt=0;
    sei();

    for (byte q=0;q<255;q++){
    Serial.print(val[q]);
    Serial.print(",");
    }
    Serial.println();
}

void rx(){

}

ISR(ADC_vect)
{
  val[gCnt] = ADCW;
  Serial.print((int)gCnt);
  gCnt += 1;
}

Author: Falk (Guest)
Posted on:

Rate this post
0 useful
not useful
Öhm, sprach ich nicht von VOLLSTÄNDIGEM Code? Ich sehe kein main()

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
Ja sorry, hab ich vergessen zu sagen: ich programmiere den Atmega mit 
der Arduinoplattform, da gibts meines Wissens kein main.

Author: Tobias K. (tobias_k83)
Posted on:

Rate this post
0 useful
not useful
Okay, ich habe jetzt einfach Ockham's Razor geschwungen und es geht:

ISR(ADC_vect)
{
  Serial.println((int)ADCH);
}

Trotzdem vielen dank für den Exkurs in atomare Interrupt Behandlung =)

Reply

Entering an e-mail address is optional. If you want to receive reply notifications by e-mail, please log in.

Rules — please read before posting

  • Post long source code as attachment, not in the text
  • Posting advertisements is forbidden.

Formatting options

  • [c]C code[/c]
  • [avrasm]AVR assembler code[/avrasm]
  • [code]code in other languages, ASCII drawings[/code]
  • [math]formula (LaTeX syntax)[/math]




Bild automatisch verkleinern, falls nötig
Note: the original post is older than 6 months. Please don't ask any new questions in this thread, but start a new one.