Sekunden-Task mit CTC-Timer (Atmega88)

[Moritz (Gast)]

Hallo zusammen,

bestimmt habt ihr eine Idee, was ich hier falsch mache:

Ziel:
Auf einem Atmega88 mithilfe des Timer0 im CTC-Modus jede Millisekunde 
einen Compare Match - Interrupt auslösen und somit die Basis für 
zyklisch aufzurufende Funktionen bilden.
Der Interrupt jede Millisekunde klappt soweit.

Problem:
Wenn ich den Inhalt der while(1)-Schleife in die ISR packe (wie man es 
ja eigentlich nicht tun sollte, Stichwort schlanke ISR..) toggelt PD4 
jede Sekunde - das gewünschte Ergebnis.
Wenn der Code jedoch wie hier beschrieben ist (d.h. die task_sekunde()) 
wird aus der while(1) aufgerufen, ergibt sich das Bild wie angehängt.
Jeder vierte Funktionsaufruf passiert zu früh! (Nach ca. 760ms anstatt 
1s).
Weiter unten der Code.

Hat jemand eine Idee? Danke im Voraus, bleibt gesund!

Grüße
Moritz

#define F_CPU 16000000UL

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

void task_sekunde();

volatile uint16_t t = 0;

int main(void)
{  
  DDRD |= (1<<PD4);  // PD4 auf Ausgang einstellen                   
  PORTD |= (1<<PD4);  // PD4 setzen
  
  TCCR0A |= (1<<WGM01);  // CTC - Mode
  TCCR0B |= (1<<CS01) | (1<<CS00);  // Prescaler 1/64
  TIMSK0 |= (1<<OCIE0A);  // Timer Compare Match interrupt enabled
  OCR0A = 249;  // Wert fuer Ueberlauf, Frequenz 500Hz: (F_CPU / (2*500*64) - 1)
  
  sei();
  
  while (1) 
  {
    if (t >= 1000)
    {
      t=0;
      task_sekunde();
    }
  }
}

ISR(TIMER0_COMPA_vect)
{
  t++;  
}

void task_sekunde()
{
  PIND |= (1<<PD4);
}

[hufnala (Gast)]

Hi, hab jetzt nicht alles da, aber Dein |= verodert das Signal und Du 
gibst PIND an.

Probiere mal ob PORTD ^= (1<<PD4) das Problem beseitigt.

//hufnala

[Stefan F. (Gast)]

Du hast da ein Problem mit der 16bit Variable t. Während sie in main() 
gelesen wird, könnte sie durch die ISR verändert werden, was sporadisch 
zu unerwartetem Verhalten führt. Das musst du verhindern.

Siehe 
https://www.nongnu.org/avr-libc/user-manual/group__util__atomic.html

[Michael U. (amiga)]

Hallo,

wenn innerhalb Deiner while(1) Schleife in Interrupt auftritt, ist nicht 
sichergestellt, daß t gültige Werte aufweist. Such mal nach atomic 
Zugriff oder sperre selbst den Interrupt beim Zugriff, wenn das keine 
anderen IRQs stört.
Also

cli();
if ...
{
}
sei();

Um die Sperre möglichst kurz zu halten, wäre es sinnvoll, t in eine 
Hilfsvarable zu übernehmen und damit zu arbeiten:

cli();
uint16_t t1 = t;
sei();
if ...

Gruß aus Berlin
Michael

[OldMan (Gast)]

Du solltest task_sekunde() unter Interruptsperre ausführen.
Und Dir dann das Ergebnis noch einmal anschauen. Du wirst überrascht 
sein.
Die if Abfrage ist mit >= unscharf.
Da du bei 1000 triggern möchtest sollte dies > 999 lauten.

[Moritz (Gast)]

hufnala schrieb:
> Du gibst PIND an
Das ist bei diesem Controller eine einfache Möglichkeit, einen Portpin 
zu toggeln (1 in das PIN-Register schreiben). Deine Schreibweise ist 
aber sicher universeller!


@Michael, Stefan und Oldman:
Danke!! Das war's.
Ich hatte sowas vermutet, den Gedanken dann aber wieder verworfen, weil 
reproduzierbar
- jeder vierte Funktionsaufruf
- nach 760ms
kam.

Grüße
Moritz

[foobar (Gast)]

> Die if Abfrage ist mit >= unscharf.
> Da du bei 1000 triggern möchtest sollte dies > 999 lauten.

Das macht keinen Unterschied.


> Das ist bei diesem Controller eine einfache Möglichkeit, einen Portpin
> zu toggeln (1 in das PIN-Register schreiben).

Durch das Odern schreibst du evtl aber mehr als eine 1 und änderst dann 
mehrere Pins.  Ein "=" reicht.

[foobar (Gast)]

Vergessen: du hast drei Raceconditions: einmal das Auslesen von t, dann 
das Zurücksetzen (beides keine atomaren Zugriffe) und letztlich die Zeit 
zwischen lesen und rücksetzen (da kann dir ein Count verloren gehen).

[Stefan F. (Gast)]

Wenn du gleichmäßige Intervalle brauchst, würde ich t nicht auf 0 zurück 
setzen, sondern 1000 subtrahieren.

Denn stelle Dir mal vor, dein Hauptprogramm verpasst die richtigen 
Zeitpunkte weil es gerade mit anderen Warteschleifen beschäftigt ist 
(z.B. Ausgabe auf ein Display). Dann würde sich dieser Fehler beim 
einfachen Zurücksetzen immer weiter aufaddieren.

Wenn du kannst, dass benutze einen Quarz der "einfache" Zahlen wie 1024 
ermöglicht. Die lassen sich schneller berechnen.

[Moritz (Gast)]

Stimmt, guter Tipp - danke! :-)

Toll, wie schnell man hier qualifizierte Antworten bekommt, wenn man 
sich an ein paar Regeln hält. Lässt einen doch noch hoffen, dass es 
nicht nur Trolls im Internet unterwegs sind.

[foobar (Gast)]

> Wenn du gleichmäßige Intervalle brauchst, würde ich t nicht auf 0 zurück
> setzen, sondern 1000 subtrahieren.

Noch besser: den Timer nie zurücksetzen (ISR erhöht, Anwendung liest 
nur) sondern dein "timeout" (eine extra Variable) um 1000 erhöhen.  So 
kann der einzelne Timer an beliebig vielen Stellen im Programm für 
beliebige Zeiten benutzt werden.  Insb bei Zeiten unterhalb 128 
Timercounts kann man atomar das LSB des Timers lesen und braucht nicht 
mal cli/sei.

Beispiel:

// in header:
extern volatile u32 systick;  // 100Hz
#define systick8 (*(volatile u8*)&systick)

// main:
...
void loop(void)
{
    static u8 t;

    if ((s8)(systick8 - t) >= 50) // .5s
    {
        t += 50;
        BLINK();
        ...
     }
     ...
}

[Stefan F. (Gast)]

foobar schrieb:
> den Timer nie zurücksetzen (ISR erhöht, Anwendung liest
> nur) sondern dein "timeout" (eine extra Variable) um 1000 erhöhen.

Das macht er doch längst so!

foobar schrieb:
> Insb bei Zeiten unterhalb 128
> Timercounts kann man atomar das LSB des Timers lesen und braucht nicht
> mal cli/sei.

Damit kann er TO nichts anfangen, weil er 1000 Millisekunden braucht. 
Die passen in das LSB nicht rein.

[Wolfgang (Gast)]

Stefan ⛄ F. schrieb:
> Das macht er doch längst so!

Und was ist das hier?

Moritz schrieb:
> t=0;

[Stefan F. (Gast)]

Wolfgang schrieb:
> Und was ist das hier?
>> t=0;

Da ist seine Zwischenvariable. Der Timer wäre TCNT0.

[hufnala (Gast)]

Hi, Danke die Möglichkeit kannte kannte ich nicht. Insbesondere dass ein 
|= das bit toggelt, und damit zum exor wird?

Auf das atomic bin ich nicht gekommen, da ich mit einem extra flag 
arbeite und den Zähler nur in der ISR bearbeite. Das Flag wird dann in 
der Hauptschleife abgefragt und zurückgesetztfrage. Kostet halt worst 
case einen weiteren uint oder in einer struct 1 bit und ein paar mehr 
Zugriffe, lässt aber alle ISR weiterlaufen.

Was ist besser/effizienter/sicherer?

//hufnala

[Stefan F. (Gast)]

hufnala schrieb:
> Insbesondere dass ein
> |= das bit toggelt, und damit zum exor wird?

Nein, da wird nichts zum xor.

Die Harware vieler ATmega Controller hat ein besonderes Feature, welches 
einen Ausgang toggelt, wenn man eine 1 in das zugehörige PINx Register 
schreibt. Mit C hat das gar nichts zu tun.

Aber wenn du schreibst "PIND |= 1" dann liest du das PIN Register (also 
den aktuellen Zustand des Portes) ein, setzt das Bit 0 auf 1 und 
schreibst das Ergebnis zurück. Das Ergebnis wird sein, nicht nur Bit 0 
getoggelt wird, sondern auch alle anderen Ausgänge, die vorher HIGH 
waren.

> Was ist besser/effizienter/sicherer?

Kommt auf das Programm an. In dem oben gezeigten Programm würde die 
Interrupts sperren um exklusiv Zugriff auf t zu bekommen. Nur wenn das 
aus irgend einem Grund nicht akzeptabel ist, würde ich es komplizierter 
machen.

[flag (Gast)]

Ein Flag zur Signalisierung des Überlaufs wäre auch mein Ansatz.
Das Flag wird in der ISR gesetzt, im Hauptprogramm getestet und wenn 
gesetzt, dann wird es zurückgesetzt und die Ausgaberoutine läuft 
einmalig durch.

Das entkoppelt die ISR vom Hauptprogramm.
Keine weiteren Klimmzüge erforderlich. Sauber und einfach.

Die verbrauchte Rechenzeit wird fast immer weniger als in der obigen 
Lösung sein. Doch da sind auch andere Szenarien denkbar.

[Stefan F. (Gast)]

Was man anstelle von Interruptsperre auch machen kann: Die Variable t 
mehrmals lesen, bis sie zweimal hintereinander gleich ist. Dann den Wert 
verwenden.

[Chronisches Leid (Gast)]

 while (1)
   {
     if (flag_gesetzt)
     {
       task_sekunde();
       flag_gesetzt = 0;
     }
   }
 }
 
 ISR(TIMER0_COMPA_vect)
 {
   t++;
   If (t >= 1000){
     flag_gesetzt = 1;
     t = 0;
   }
 }
 
 void task_sekunde()
 {
   PIND |= (1<<PD4);
 }

Würde das stabil laufen?
So mache ich das in meinem Uhren Projekt.
Habe aber mehrere Sekunden Abweichung über den Tag. Kannst du den Code 
mit deinem DSO testen?

[hufnala (Gast)]

Danke....

//hufnala

[Stefan F. (Gast)]

Chronisches Leid schrieb:
> Würde das stabil laufen?

Wenn flag_gesetzt volatile und 8bit ist, dann ja.

[Chronisches Leid (Gast)]

Stefan ⛄ F. schrieb:
> Wenn flag_gesetzt volatile und 8bit ist, dann ja.

Ist es. Danke!
Dann liegt's doch am Quarz.
Aber warum müssen es 8 Bit sein?

[Stefan F. (Gast)]

Chronisches Leid schrieb:
> Stefan ⛄ F. schrieb:
>> Wenn flag_gesetzt volatile und 8bit ist, dann ja.
>
> Ist es. Danke!
> Dann liegt's doch am Quarz.
> Aber warum müssen es 8 Bit sein?

Weil 16bit Variablen in zwei Schritten geändert werden. Ok, solange da 
nur 0 oder 1 drin steht, nutzt du effektiv doch nur 1 Byte davon.

[Wolfgang (Gast)]

Stefan ⛄ F. schrieb:
> Da ist seine Zwischenvariable. Der Timer wäre TCNT0.

Nee, nee. t ist defacto eine Softwareerweiterung des Timers.

Stelle dir einfach vor, dass irgendetwas im Hauptprogramm so lange 
dauert, dass die Abfrage auf (t >= 1000) erst bei t=1001 zum Zuge kommt. 
Wenn dann t auf 0 gesetzt wird, entsteht einen Zeitfehler von 1. Das 
passiert bei jedem längeren Durchlauf, so dass sich die Zeitfehler 
akkumulieren. Und genau dieses Aufsammeln wird vermieden, wenn man t 
nicht jedes Mal auf 0 setzt, sondern mit einem jedes mal um 1000 
steigenden Wert vergleicht, i.e. nur die ISR verändert t, sonst niemand.

[Stefan F. (Gast)]

Das mit dem verschleppten Fehler hatte ich bereits vor Dir geschrieben.