EmbDev.net

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


von Tobias K. (tobias_k83)


Rate this post
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

von Falk (Guest)


Rate this post
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

von Falk (Guest)


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

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

von kbuchegg (Guest)


Rate this post
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.

von Tobias K. (tobias_k83)


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

von kbuchegg (Guest)


Rate this post
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

von kbuchegg (Guest)


Rate this post
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.

von Tobias K. (tobias_k83)


Rate this post
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.

von Tobias K. (tobias_k83)


Rate this post
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>?

von Tobias K. (tobias_k83)


Rate this post
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?

von Tobias K. (tobias_k83)


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

von Falk (Guest)


Rate this post
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

von Tobias K. (tobias_k83)


Rate this post
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?

von (prx) A. K. (prx)


Rate this post
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.

von Falk (Guest)


Rate this post
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.

1
#include <avr/io.h>
2
#include <avr/interrupt.h>
3
4
volatile int val[512] = { 0 };
5
volatile int i = 0;
6
7
void setup() {
8
  Serial.begin(115200);
9
  cli();                                     // disable interrupts while
10
messing with their settings
11
  TCCR1A = 0x00;                             // clear default timer
12
settings, this kills the millis() funciton
13
  TCCR1B = 0x00;
14
  TCCR1B |= (1 << WGM12);                    // Configure timer 1 for
15
CTC mode
16
  TCCR1B &= ~(0 << CS12);                    // Set timer prescaling by
17
setting 3 bits
18
  TCCR1B |= (1 << CS11 | 1 << CS10);         // 001=1, 010=8, 011=64,
19
101=1024
20
  TIMSK1 |= (1 << OCIE1B);                   // Enable CTC interrupt
21
  OCR1B  = 5000;                               // Set CTC compare value
22
23
  ADMUX = B01100000;                         // left adjusted output to
24
increase sample rate, 8bit mode
25
  SMCR |= (1 << SM0);                        // Enable sleep AD
26
conversion mode
27
  SMCR &= ~( (1 << SM2) | (1 << SM1) );
28
  //SREG |= (1 << ADIE);                       // Enable ADC ready
29
interrupt
30
  ADCSRB |= ( ( 1 << ADTS2 | 1 << ADTS0 ) ); // configure ADC to trigger
31
on Timer Compare Match B
32
  ADCSRB &= ~(1 << ADTS1);
33
  sei();                                     // turn interrupts back on
34
  ADCSRA = B11001111;                         // set ADC prescaler to
35
128, ADC EN, ADC Start
36
}
37
38
void loop() {
39
if(i==512){
40
}
41
tx();
42
}
43
44
void tx(){
45
  int tmp;
46
47
    cli();
48
    tmp = i;
49
    i = 0;
50
    sei();
51
    for (int q=0;q<512;q++){
52
      Serial.print(val[q]);
53
      Serial.print(",");
54
    }
55
    Serial.println();
56
}
57
58
void rx(){
59
60
}
61
62
ISR(ADC_vect)
63
{
64
  val[i] = ADCW;
65
  i++;
66
}

MfG
Falk

von Tobias K. (tobias_k83)


Rate this post
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!

von Tobias K. (tobias_k83)


Rate this post
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 =)

von Tobias K. (tobias_k83)


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

von Tobias K. (tobias_k83)


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

von Falk (Guest)


Rate this post
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

von Tobias K. (tobias_k83)


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

von Falk (Guest)


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

von Tobias K. (tobias_k83)


Attached files:

Rate this post
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;
}

von Falk (Guest)


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

von Tobias K. (tobias_k83)


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

von Tobias K. (tobias_k83)


Rate this post
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 =)

Please log in before posting. Registration is free and takes only a minute.
Existing account
Do you have a Google/GoogleMail account? No registration required!
Log in with Google account
No account? Register here.