Blutiger Anfänger mit Timer Problem

[Mario D. (drmario)]

Hallo Leute,
ich bin ein blutiger Anfänger und trau mich kaum ne Frage über Timer zu 
stellen, aber ich seh den Wald vor lauter Bäumen wohl nicht mehr. Ich 
hab mal meinen kompletten C Code angehängt, aber ich wollte kurz auf die 
interessanten Teile eingehen:

Ich hab nen externen Crystal mit 16GHz an nem Atmega644


Meine Frage ist nun, wenn ich oben in meinem Timer die Kommentare weg 
mache, dann blinken die LEDs alle 3 bis 4 Sekunden auf einmal 
unregelmäßig, danach für 3 Sekunden wieder normal.

Mach ich was Grundlegendes falsch? Mache ich zu viele Berechnungen im 
Timer?


Ich hab den Timer mit Clear Timer on Compare Match eingestellt:

void timerInit(void) {
  TCCR0A = (1 << WGM01); //Mode2: CTC Clear Timer on compare match
  TCCR0B = (1 << CS01) | (1 << CS00); // prescaler = 64
  OCR0A = 250; //250 Takte = 1ms

//  TCCR1A = 0; // kein pwm
  TIMSK0 = (1 << OCIE0A); //Compare Interrupt aktivieren
}

Meine Timer Interrupt Funktion sieht so aus:
ISR (TIMER0_COMPA_vect) {
  if (led1on)
    led1on--;
  millisekunden++;
/*  if(millisekunden == 1000) {
    sekunde++;
    millisekunden = 0;
  }
    if(sekunde == 60) {
         minute++;
         sekunde = 0;
    }
    if(minute == 60) {
      stunde++;
      minute = 0;
    }*/
}

Dann hier noch meine main:
int main(void) {

  uint16_t delay;

  cli();
  systemInit();
  timerInit();
  sei();
  while (1) {
    if (IN(SWITCH_PORT, SWITCH_PIN))
      delay = 500;
    else
      delay = 1000;
    if (!led1on) {
      leds();
      led1on = delay;

    }
  }
}

[Frank L. (franklink)]

Hallo,
ich geb mal einen Tip ohne Gewähr ab, led1on = delay ist kein atomarer 
Zugriff, weil delay ein uint16_t ist.

Mach mal vor der Zuweisung ein cli() und danach ein sei() rein.

Gruß
Frank

[Coder (Gast)]

Ein

volatile uint16_t led1on;

könnte nützlich sein.

[Mario D. (drmario)]

@Coder:
die led1on ist schon volatile! Hatte ich in dem kurzen Abschnitt nicht 
kopiert aber im main.c ist das zu sehen...

@Frank Link
ich hab jetzt mehrere Sachen versucht, aber irgendwie ist das Springen 
der LEDs immer noch da...

Meine main sieht jetzt so aus, oder hatte ich das falsch verstanden mit 
dem sei cli?

int main(void) {
  cli();

  uint16_t delay;

  systemInit();
  timerInit();
  sei();
  while (1) {
    if (IN(SWITCH_PORT, SWITCH_PIN)) {
      cli();
      delay = 500;
      sei();
    } else {
      cli();
      delay = 1000;
      sei();
    }
    if (!led1on) {
      leds();
      led1on = delay;

    }
  }
}

[Frank L. (franklink)]

Jepp,
hast Du.

cli();
led1on = delay;
sei();

Alle anderen sind nutzlos.

Gruß
Frank

[Mario D. (drmario)]

Ok, macht Sinn, sind ja 16bit Operationen...
Aber helfen tut es leider auch nicht :-(

[Karl H. (kbuchegg)]

Mario Dejung schrieb:
> Ok, macht Sinn, sind ja 16bit Operationen...
> Aber helfen tut es leider auch nicht :-(

Das reicht nicht.
Du hast ja noch einen Zugriff auf led1on, den du kapseln musst

    cli();
    uint16_t tmp = led1on;
    sei();

    if (!tmp) {
      leds();
      cli();
      led1on = delay;
      sei();
    }

Du hast natürlich dadurch, dass du die ganzen Zeitvariablen alle
unsigned int( also 16 Bit) gemacht hast, dem µC jede Menge unnötige
Fleissaufgabe aufgebürdet. Die solltest du (bis auf Millisekunden) alle
als uint8_t führen. Das langt dicke für Sekunden, Minuten, Stunden.

[Mario D. (drmario)]

Ok, das scheint des Rätsels Lösung zu sein :-) Danke, auch wenn ich es 
nicht wirklich verstehe... Aber vielleicht kommt da noch bald die 
Routine...

[Rüttiger (Gast)]

das Problem ist halt dass led1on an zwei Stellen verändert/benutzt wird.

wenn jetzt im main vielleicht gerade das erste Byte geladen wurde, funkt 
plötzlich der IRQ dazwischen, ändert beide Bytes des Wortes und 
plötzlich passen im main die beiden Bytes aus denen ein Wort besteht 
nicht mehr zusammen

[Mario D. (drmario)]

Hätte man da dann nicht einfacher Weise die komplette if Abfrage in cli 
und sei setzen können, anstatt noch mal drumrum in ne andere Variable u 
speichern? So ungefähr:

  while (1) {
    if (IN(SWITCH_PORT, SWITCH_PIN)) {
      delay = 500;
    } else {
      delay = 1000;
    }
    cli();
    if (!led1on) {
      leds();
      led1on = delay;
    }
    sei();
  }

[Roland H. (batchman)]

Mario Dejung schrieb:
> Ich hab nen externen Crystal mit 16GHz an nem Atmega644

16 GHz? Hoffentlich wird es da dem atmega644 nicht schwindlig :-)

[Mario D. (drmario)]

Mega Giga, kann man doch mal verwechseln... :-)

By the Way, ich hab die Änderungen mit dem cli und sei gemacht wie in 
meinem letzten Post und das scheint zu funktionieren...

[Karl H. (kbuchegg)]

Mario Dejung schrieb:
> Hätte man da dann nicht einfacher Weise die komplette if Abfrage in cli
> und sei setzen können, anstatt noch mal drumrum in ne andere Variable u
> speichern? So ungefähr:

Natürlich kann man.
Wichtig ist nur, dass weder bei Abfrage noch beim Beschreiben der 
Variable ein Interrupt dazwischen funken kann.
Du musst dich eben fragen, wie lange du die Interrupts gesperrt haben 
willst.

Ein Schlüsselnotdienst, der sofort reagiert, hilft dir nun mal nichts, 
wenn wegen Telefonsperre keiner abhebt.

[Mario D. (drmario)]

Karl Heinz Buchegger schrieb:
> Ein Schlüsselnotdienst, der sofort reagiert, hilft dir nun mal nichts,
> wenn wegen Telefonsperre keiner abhebt.

:-) Danke, den Vergleich hab ich gebraucht! :-)