ATmega16 Modulo ISR Problem (Arduino IDE)

[lex7 (Gast)]

Hallo Zusammen,

wenn ich diesen Code ausführe

volatile uint16_t countISRTimer1=0;

void setup() {
//...
}

void loop() {
 if(countISRTimer1%128==0) 
  Serial.println(countISRTimer1);
 countISRTimer1++;
}

funktioniert es wie gewünscht. (Alle 128 wird der aktuelle Wert 
ausgegeben)
Lasse ich den counter in der ISR hochzählen

volatile uint16_t countISRTimer1=0;

ISR( TIMER1_COMPA_vect )                            // every 10ms
{
 countISRTimer1++;
}

void setup() {
//...
}

void loop() {
 if(countISRTimer1%128==0) 
  Serial.println(countISRTimer1);
}

sieht die Ausgabe so aus:
0
4
128
132
256
260
usw.

Was kann hier passieren?

[leo (Gast)]

lex7 schrieb:
> Lasse ich den counter in der ISR hochzählenvolatile uint16_t
> countISRTimer1=0;
>
> ISR( TIMER1_COMPA_vect )                            // every 10ms
> {
>  countISRTimer1++;
> }
>
> void setup() {
> //...
> }
>
> void loop() {
>  if(countISRTimer1%128==0)
>   Serial.println(countISRTimer1);
> }
>
> sieht die Ausgabe so aus:
> 0
> 4
> 128

Das Problem hier ist der nicht-atomare Zugriff auf countISRTimer1 bei 
der Ausgabe. Waehrend die Teile der 16-bit Zahl gelesen werden 
inkrementiert die ISR die Zahl.
S. zB. http://www.gammon.com.au/interrupts (Atomic access)

leo

[lex7 (Gast)]

Guter Punkt.

int16_t getCountISRTimer1( void )         
{
  int16_t val;
  cli();
  val = countISRTimer1; 
  sei();
  return val;
}

void loop() {
 if(getCountISRTimer1()%128==0) {
  Serial.println(countISRTimer1);
  }
}

Leider bleibt das Ergebnis wie oben.
Noch Ideen?

[Dr. Sommer (Gast)]

lex7 schrieb:
> Leider bleibt das Ergebnis wie oben.
> Noch Ideen?

In der ISR die Interrupts abschalten bringt gar nix, weil sie da schon 
aus sind. Sie wieder einzuschalten ist aber gefährlich. Du musst sie in 
der loop abschalten.
Wie wäre es damit, aus der Variablen eine uint8_t zu machen und immer 
nur bis 127 zu zählen und dann auf 0 zu setzen? Dann erledigt sich das 
Problem komplett, denn 8-Bit Typen können atomar gelesen werden. 
Effizienter ist es auch. Aber Vorsicht: der per println ausgegebene Wert 
kann dann anders sein als der in der if Bedingung. Daher am Besten eine 
lokale temporäre Kopie machen.

[Dr. Sommer (Gast)]

Dr. Sommer schrieb:
> In der ISR die Interrupts abschalten bringt gar nix, weil sie da schon
> aus sind

Oh, falsch gelesen! Äh, der letzte Teil meiner Aussage ist hier das 
Problem - die Variable wird 2x ausgelesen, einma im if und einmal im 
println. Und beim letzteren auch ohne Interrupt Sperre.

[Stefan F. (Gast)]

Außerdem soll die Variable als "volatile" gekennzeichnet werden, damit 
bei jedem Zugriff auf das RAM zugegriffen wird und kein Caching in einem 
CPU Register stattfindet.

Das hat lex7 richtig gemacht, ich wollte es in diesem Kontext nur 
nochmal erklären, weil es oft vergessen wird.

[Dr. Sommer (Gast)]

Ich würde es wenn möglich so machen:

volatile uint8_t countISRTimer1=0;

ISR( TIMER1_COMPA_vect )                            // every 10ms
{
  if (countISRTimer1 == 127)
    countISRTimer1 = 0;
  else
    ++countISRTimer1;
}

void setup() {
//...
}

void loop() {
  uint8_t temp = countISRTimer1;
  if (temp==0) 
    Serial.println(temp);
}

Wenn aber wirklich bis 65408 gezählt werden soll, dann so:

volatile uint16_t countISRTimer1=0;

ISR( TIMER1_COMPA_vect )                            // every 10ms
{
  ++countISRTimer1;
}

void setup() {
//...
}

void loop() {
  cli ();
  uint16_t temp = countISRTimer1;
  sei ();
  if (temp % 128 == 0) 
    Serial.println(temp);
}

[lex7 (Gast)]

Besten Dank an alle, da hatte ich wohl einen Knoten im Kopf.


hiermit bekomme ich auch eine doppelte Ausgabe:
Dr. Sommer schrieb:
> Wenn aber wirklich bis 65408 gezählt werden soll, dann so:
>

volatile 
> uint16_t countISRTimer1=0;
> 
> ISR( TIMER1_COMPA_vect )                            // every 10ms
> {
>   ++countISRTimer1;
> }
> 
> void setup() {
> //...
> }
> 
> void loop() {
>   cli ();
>   uint16_t temp = countISRTimer1;
>   sei ();
>   if (temp % 128 == 0)
>     Serial.println(temp);
> }

0
0
128
128
256
256 usw.

Aber klar, die ISR wird nur alle 10ms ausgeführt, da kann der loop() 
doppelt durchlaufen werden.

[Dr. Sommer (Gast)]

lex7 schrieb:
> hiermit bekomme ich auch eine doppelte Ausgabe:

Die andere Version mit uint8_t liefert genauso doppelte Ausgaben - die 
sind halt nur immer "0" ;-) Daher ist das println(temp) da auch etwas 
sinnlos, weil temp da eh immer 0 ist.

lex7 schrieb:
> Aber klar, die ISR wird nur alle 10ms ausgeführt, da kann der loop()
> doppelt durchlaufen werden.

Ja, eigentlich sogar noch viel schneller. Das println bremst es 
wahrscheinlich.

[Falk B. (falk)]

Dr. Sommer schrieb:
> lex7 schrieb:
>> hiermit bekomme ich auch eine doppelte Ausgabe:
>
> Die andere Version mit uint8_t liefert genauso doppelte Ausgaben - die
> sind halt nur immer "0" ;-) Daher ist das println(temp) da auch etwas
> sinnlos, weil temp da eh immer 0 ist.
>
> lex7 schrieb:
>> Aber klar, die ISR wird nur alle 10ms ausgeführt, da kann der loop()
>> doppelt durchlaufen werden.
>
> Ja, eigentlich sogar noch viel schneller. Das println bremst es
> wahrscheinlich.

Sicher, aber das Grundkonzept ist schon Unsinn. Wenn man einen Vorgang 
mit einer Frequenz X anstoßen will, dann DIREKT!

volatile uint16_t countISRTimer1=0;
volatile uint8_t  isr_flag=0;

ISR( TIMER1_COMPA_vect )                            // every 10ms
{
   ++countISRTimer1;
  if ( (countISRTimer1 & 0x7F) == 0) isr_flag = 1;   // fast modulo operation
}

void setup() {
//...
}

void loop() {

  if (isr_flag) {
    isr_flag=0;
    cli ();
    uint16_t temp = countISRTimer1;
    sei ();
    Serial.println(temp);
  }
}

Das ist eindeutig und laufzeitunabhängig (es sei denn, die Ausgabe per 
UART läuft mit 10 Baud).

[Dr. Sommer (Gast)]

Falk B. schrieb:
> Sicher, aber das Grundkonzept ist schon Unsinn. Wenn man einen Vorgang
> mit einer Frequenz X anstoßen will, dann DIREKT!

Da kann man sich die ISR aber auch sparen und es direkt so machen:

uint8_t countISRTimer1 = 0;
void loop() {
  if (TIFR & (1 << TOV1)) {
    TIFR = 1 << TOV1;
    if (countISRTimer1 == 128) {
      countISRTimer1 = 0;
      Serial.println ("Overflow\n");
    } else
      ++countISRTimer1;
    
  }
}

Spart die Fummelei mit cli/sei, volatile, Überlegung wo die ISR den 
eigentlichen Programmfluss unterbrechen kann...

[Falk B. (falk)]

Dr. Sommer schrieb:
> Falk B. schrieb:
>> Sicher, aber das Grundkonzept ist schon Unsinn. Wenn man einen Vorgang
>> mit einer Frequenz X anstoßen will, dann DIREKT!
>
> Da kann man sich die ISR aber auch sparen und es direkt so machen:

Naja, das geht aber nur, wenn die Überlauffrequenz eher klein ist und 
die Abfrage des Überlaufbits ausreichend schnell. Das ist schon wieder 
ein Sonderfall.

"Ich bin dafür, die Dinge soweit wie möglich zu vereinfachen, aber nicht 
weiter."

Albert Einstein