Timer interrupt wird nur einmal aufgerufen.

[Michael H. (mich_tech)]

Hallo, wenn es die Frage schon gibt, entschuldige ich mich hiermit.


Ich habe das Problem, dass meine Timer Interrupt Routine nur einmal 
aufgerufen wird. Mein Programm soll einfach eine LED  1s einschalten und 
dann 1 Sekunde ausschalten
Hierzu verwende ich den Timer 0 des Prozessors Atmega328p-pu bzw der vom 
Arduino Board. Der Timer ist im normal Mode und  Output Compare Match A 
auf 10 ms  eingestellt. Nach 100 aufrufen der ISR setze ich dann den 
Status der Led (PD2).

 Das Komische daran ist, wenn ich am Ende des Interrupts sei() ausführe, 
dann leuchtet die LED wie gewollt (alles funktioniert also). Wenn ich 
sei() auskommentiere führt er die (ISR) Routine nur einmal aus.

 Ist das so normal oder habe ich irgendwas falsch gemacht, sodass das 
notwendig ist. Dachte der Timer läuft, solange bis man ihn wieder 
deaktiviert.
Anbei habe ich den Code angehängt den ich hoffentlich richtig einfügt 
habe,

#include <avr/io.h>
#define F_CPU 8000000UL
#include <util/delay.h>
#include <stdint-gcc.h>
#include <avr/interrupt.h>
#include <avr/sleep.h>

 static uint16_t volatile count= 0;
   static uint8_t volatile debug_flag= 0;

  int main(void)
{
      DDRD = 0x04; PORTD = 0x08;
  cli();
     //disable all interrupts
       //configure timer interrupt
  TCCR0A &= ~((1 << WGM01 ) |  (1 << WGM00 ));
  TCCR0B &= ~((1 << WGM02 ) ); // normal mode all three 0
     double time= 0.010; // 10 ms for timer
  TCCR0B |= ((1 << CS02)|(0 << CS01)|(1 << CS00)); //prescaler 1024
  TCCR0B &= ~(1 << CS01);
  uint16_t prescaler = 1024;
  uint8_t targetcounter = (uint8_t) (((time*F_CPU)/(prescaler))+1);
  OCR0A = targetcounter;
  TIMSK0 |= (1 << OCIE0A); // Output Compare Match A Interrupt Enable
 set_sleep_mode(SLEEP_MODE_IDLE);
  sei();
    while (1) 
    {
    
    
  sleep_mode();
  if (debug_flag == 1){
    PORTD  |= 0x04; //led state 1
  }
  else {
    PORTD  &= ~(0x04); // led state 0
  }

 }
}

void TIMER0_COMPA_vect(){
  
  count++;
  if(count > 100){
    debug_flag ^= 1;
    count = 0;
  }
  //sei();
}

[Einer K. (Gast)]

> void TIMER0_COMPA_vect(){
?
Nicht eher:
ISR(TIMER0_COMPA_vect){
(oder so ähnlich)

[John Doe (Gast)]

Arduino Fanboy D. schrieb:
>> void TIMER0_COMPA_vect(){
> ?
> Nicht eher:
> ISR(TIMER0_COMPA_vect){
> (oder so ähnlich)

Richtig, denn nur so setzt der Compiler an das Ende der Interruptroutine 
ein RETI und kein RET. Und nur RETI setzt das I-Bit.

[Einer K. (Gast)]

Wichtiger ist fast das sichern des Status Registers.
Das I in RETI ist dann nur der krönende+notwendige Abschluss.
(kann man sich schön im Kompilat anschauen)

[my2ct (Gast)]

Michael H. schrieb:
> if (debug_flag == 1){
>     PORTD  |= 0x04; //led state 1
>   }
>   else {
>     PORTD  &= ~(0x04); // led state 0
>   }

Auch wenn nicht primär das Problem: Wozu das "Dauerfeuer" auf den Port.
Für die Funktionalität würde es völlig reichen, wenn der Zustand genau 
einmal nach jedem count-Überlauf neu gesetzt würde.

Ein simples

if (debug_flag)

 würde hier völlig reichen.

[Rolf Rolfus (Gast)]

my2ct schrieb:
> Michael H. schrieb:
> if (debug_flag == 1){
>     PORTD  |= 0x04; //led state 1
>   }
>   else {
>     PORTD  &= ~(0x04); // led state 0
>   }
>
> Auch wenn nicht primär das Problem: Wozu das "Dauerfeuer" auf den Port.
> Für die Funktionalität würde es völlig reichen, wenn der Zustand genau
> einmal nach jedem count-Überlauf neu gesetzt würde.
>
> Ein simples if (debug_flag) würde hier völlig reichen.

Da ist kein Dauerfeuer. Der uC schläft doch eine Zeile drüber..

[Einer K. (Gast)]

my2ct schrieb:
> Ein simples if (debug_flag) würde hier völlig reichen.

Wenn, dann:

if(debug_flag)
{
 debug_flag = 0;
 PIND = 0x04;
}

!?!?

[Rolf M. (rmagnus)]

my2ct schrieb:
> Michael H. schrieb:
>> if (debug_flag == 1){
>>     PORTD  |= 0x04; //led state 1
>>   }
>>   else {
>>     PORTD  &= ~(0x04); // led state 0
>>   }
>
> Auch wenn nicht primär das Problem: Wozu das "Dauerfeuer" auf den Port.
> Für die Funktionalität würde es völlig reichen, wenn der Zustand genau
> einmal nach jedem count-Überlauf neu gesetzt würde.

Mal angenommen, der µC würde sich nicht wie von meinem Namensvetter 
angemerkt nach jedem Zugriff schlafen legen: Was würde man dadurch 
gewinnen? Der Port nutzt sich durch mehrfaches Beschreiben ja nicht ab. 
Auch laufzeit- oder codegrößenmäßig macht es keinen signifikanten 
Unterschied.

[Stefan F. (Gast)]

Rolf M. schrieb:
> Der Port nutzt sich durch mehrfaches Beschreiben ja nicht ab.

Das stimmt. Wenn ich so einen Quelltext sehe hege ich allerdings Zweifel 
daran, ob der Autor sich ausreichend Gedanken darüber gemacht hat, was 
er da tut. An dieser Stelle ist die kleine Schlampigkeit noch harmlos. 
Woanders vielleicht nicht.

[MaWIN (Gast)]

Stefanus F. schrieb:
> Woanders vielleicht nicht.

Und wann soll es nicht harmlos sein, ein bereits gesetztes Bit zu 
setzen?

[my2ct (Gast)]

MaWIN schrieb:
> Und wann soll es nicht harmlos sein, ein bereits gesetztes Bit zu
> setzen?

Spätestens wenn das unter gewissen Betriebssystemen z.B. zu tausenden 
von Registry-Zugriffen führt, werden solche Unüberlegtheiten zum 
Performance Killer.

[Stefan F. (Gast)]

MaWIN schrieb:
> Stefanus F. schrieb:
>> Woanders vielleicht nicht.
>
> Und wann soll es nicht harmlos sein, ein bereits gesetztes Bit zu
> setzen?

Es geht mir nicht nur um diesen einen Fall. Jemand, der unnötige 
Operationen wiederholt, der denkt vielleicht auch an anderen Stellen 
nicht genug darüber nach, was er da zusammen baut.

Gerade die Programmiersprache C ist eine, wo der Entwickler noch sein 
Hirn benutzen muss.

Generell ist Schlampigkeit in deutschen Unternehmen unerwünscht - auch 
wenn harmlose Sachen sind. Wer schlampig arbeitet, bleibt nicht lange

[Thomas E. (thomase)]

Michael H. schrieb:
> set_sleep_mode(SLEEP_MODE_IDLE);
>   sei();

Wenn du deinen Controller in den Sleep schickst und erst danach die 
Interrupts freigibst, wirst du an selbigen keine große Freude haben.

Michael H. schrieb:
> void TIMER0_COMPA_vect(){

Das Problem wurde ja schon angesprochen.

[Rolf M. (rmagnus)]

my2ct schrieb:
> MaWIN schrieb:
>> Und wann soll es nicht harmlos sein, ein bereits gesetztes Bit zu
>> setzen?
>
> Spätestens wenn das unter gewissen Betriebssystemen z.B. zu tausenden
> von Registry-Zugriffen führt, werden solche Unüberlegtheiten zum
> Performance Killer.

Wir sprechen hier von einem Port-Register eines µCs, wo ein if ggf. mehr 
Takzyklen brauchen kann als der eigentliche Zugriff. Wenn man eine 
komplett andere Situation (Registry-Zugriffe under Windows) nimmt, ist 
es wenig überraschend, dass man da anders herangehen muss.

Stefanus F. schrieb:
> MaWIN schrieb:
>> Stefanus F. schrieb:
>>> Woanders vielleicht nicht.
>>
>> Und wann soll es nicht harmlos sein, ein bereits gesetztes Bit zu
>> setzen?
>
> Es geht mir nicht nur um diesen einen Fall.

Der ist es aber, über den wir sprechen. Dass man in anderen Fällen 
anders vorgehen muss, ist für mich eigentlich klar.

> Jemand, der unnötige Operationen wiederholt, der denkt vielleicht auch an
> anderen Stellen nicht genug darüber nach, was er da zusammen baut.

Man könnte jetzt argumentieren, dass ein zusätzliches if, das dafür 
sorgt, dass der Port nur bei Änderungen beschrieben wird, auch eine 
unnötige Operation wäre.
Machst du das bei Variablen auch? Also vor jedem Schreibzugriff erstmal 
prüfen, ob vielleicht der zu schreibende Wert schon drin steht, um 
sicherzustellen, dass du ihn nicht mit dem selben Wert nochmal 
überschreibst?

[Wolfgang (Gast)]

Rolf M. schrieb:
> Wir sprechen hier von einem Port-Register eines µCs ...

Ich meinte eher die allgemeine Grundhaltung beim Programmieren, speziell 
die unüberlegte Nutzen von irgendwelchen Funktionen.
Schon wenn man den oben genannten Portzugriff mit Arduino Hausmitteln 
umsetzt, wundert man sich schnell, wo die Taktzyklen verbraten werden. 
Der nächste versucht dann per Interrupt einen 10µs Takt zu generieren 
und wundert sich, warum er dafür eine CPU mit 84MHz Clock braucht ;-)

[Stefan F. (Gast)]

Rolf M. schrieb:
> Man könnte jetzt argumentieren

Dein Versuch, die Diskussion ins Lächerliche zu ziehen, klappt nicht. 
Darauf falle ich nicht herein.

[Wolfgang (Gast)]

Rolf M. schrieb:
> Man könnte jetzt argumentieren, dass ein zusätzliches if, das dafür
> sorgt, dass der Port nur bei Änderungen beschrieben wird, auch eine
> unnötige Operation wäre.

Das "zusätzlich if" steht schon da ;-)

Michael H. schrieb:
> if(count > 100){

[Einer K. (Gast)]

Thomas E. schrieb:
> Wenn du deinen Controller in den Sleep schickst und erst danach die
> Interrupts freigibst, wirst du an selbigen keine große Freude haben.

Das solltest du nochmal überdenken!
Also mit der Realität vergleichen.

[MaWIN (Gast)]

my2ct schrieb:
>> Und wann soll es nicht harmlos sein, ein bereits gesetztes Bit zu
>> setzen?
>
> Spätestens wenn das unter gewissen Betriebssystemen z.B. zu tausenden
> von Registry-Zugriffen führt, werden solche Unüberlegtheiten zum
> Performance Killer.

Das Lesen eines Update-Flags und ein bedingter Sprung ist in der Regel 
sehr viel laufzeitintensiver als ein simples Bit-Setzen.

[Rolf M. (rmagnus)]

Stefanus F. schrieb:
> Rolf M. schrieb:
>> Man könnte jetzt argumentieren
>
> Dein Versuch, die Diskussion ins Lächerliche zu ziehen, klappt nicht.
> Darauf falle ich nicht herein.

Wie du meinst. Das war zwar nicht lächerlich gemeint - ich finde so eine 
zusätzliche Abfrage tatsächlich genauso überflüssig wie bei einer 
Variablen, aber dann lassen wir das eben einfach so stehen.

[Hannes J (Gast)]

Sagt mal habt ihr nichts zu tun? Wenn ich das erste mal einen Interrupt 
testen will sieht das Hauptprogramm auch nicht viel anders aus. Was soll 
denn bitte der Vergleich mit Registry-Zugriffen?

Der TO will sich anschauen wie Interrupts benutzt werden. Da kommt einer 
und labert von permanentem Zugriff obwohl direkt davor ein Sleep Aufruf 
steht. Der nächste gibt tolle Tipps weil er den Unterschied zwischen 
Initialisierung des Sleep Modus und Beginn von Sleep() nicht rafft und 
der dritte erzählt was von überflüssigen Operationen in einer Registry. 
Beruhigt mal eure Egos ey ich kriege Augenkrebs..

[Michael H. (mich_tech)]

John Doe schrieb:
> Arduino Fanboy D. schrieb:
>>> void TIMER0_COMPA_vect(){
>> ?
>> Nicht eher:
>> ISR(TIMER0_COMPA_vect){
>> (oder so ähnlich)
>
> Richtig, denn nur so setzt der Compiler an das Ende der Interruptroutine
> ein RETI und kein RET. Und nur RETI setzt das I-Bit.

Arduino Fanboy D. schrieb:
> Wichtiger ist fast das sichern des Status Registers.
> Das I in RETI ist dann nur der krönende+notwendige Abschluss.
> (kann man sich schön im Kompilat anschauen)






Ich bedanke mich für die Hilfe. Es funktioniert wie ich es erwartet 
habe, jetzt werde mich bezüglich ISR vielleicht nochmal einlesen müssen.

Bezüglich der anderen Sache mit den  unnötig Schreiben geht es 
wahrscheinlich effizienter (vielleicht auch in der ISR direkt, oder nur 
wenn das flag 1 ist mit xor den wert switchen), das werde ich mir 
nochmal ansehen. Danke für die Hilfestellungen.

[Michael H. (mich_tech)]

Mittlerweile hab ich es so abgeändert:

if (debug_flag == 1){
    PORTD  ^= 0x04;
    debug_flag = 0;
  }

[Einer K. (Gast)]

> PORTD  ^= 0x04;
PIND  = 0x04;
Würde dir min. einen ganzen Takt einsparen.
Damit dieses Detail mindestens um 100% beschleunigen

> if (debug_flag == 1)
Hier dürfte if(debug_flag) ausreichen, bringt aber wohl keinen Vorteil, 
weil der Optimierer das sicherlich wg. überflüssig streicht.