while(1) in main wird nicht mehr ausgeführt

[Thomas Frosch (Gast)]

Hi Leute,

bei mir wird manchmal spontan die while(1) schleife in main nicht mehr 
ausgeführt. Allerdings kann ich noch ohne probleme funktionen über die 
UART ausführen. Klar diese führt ein Interrupt aus und anscheinend wird 
ein anderer Interrupt nicht mehr beendet. Habe mir auch schon den 
Artikel "Main-Schleife wird nicht mehr durchlaufen" angeschaut.

Habe alle While schleifen mit einer LED überwacht, sie werden aber alle 
ordnungsgemäß beendet.

z.B.

while ( ADCSRA & (1<<ADSC)) // auf Abschluss der Konvertierung warten
{
    LEDAn;   
}
LEDAus;

Ich benutze einen ATmega 8 und dazu UART, beide INT Eingänge, TWI, ADC 
...

Der Speicher ist zu über 90% voll.

AUSZUG AUS AVRDUDE
======================================================
Program:    7880 bytes (96.2% Full)
(.text + .data + .bootloader)

Data:         52 bytes (5.1% Full)
(.data + .bss + .noinit)
======================================================

Ich komme nicht drauf wo das Problem ist! Wie kann ich nachsehen ob 
eventuell Variablen die im Flash abgespeichert werden evtl. das Programm 
überschreiben. bzw wie viel speicher die Variablen benötigen? Um 
auszuschließen ob es daran liegt dass der Speicher zu voll ist.
Was bedeutet "Data:         52 bytes (5.1% Full)"

Habt ihr noch irgendwelche Ideen um herauszufinden wo das Problem 
auftritt. Wie gesagt ich kann noch über die UART Werte abfragen und 
Aktionen ausführen.

Der UART Interrupt hat ja eine relativ niedrige Priorität darunter ist 
nur noch der Analoge Komparator. Dessen While schleifen werden alle 
ordnungsgemäß beendet. So gehe ich davon aus, dass es nicht an den 
Interrupts liegt.

[Gast (Gast)]

Da fehlt noch etwas Code um irgendwas aussagen zu können.

Also Programm so weit reduzieren und Ballast abwerfen während der Fehler 
weiterhin auftritt und dann mal hier posten.

[Thomas Frosch (Gast)]

Danke fürs schnelle Antworten.

Allerdings ist das Programm relativ groß und der Fehler tritt immer nur 
spontan auf. Kann ihn nur mit viel Glück gewollt verursachen. So ist es 
schwer zu sagen ob der Fehler wirklich weg ist oder nicht. Ich werde es 
trotzdem versuchen, dass Programm abzuspecken.

Trotzdem gibt es noch andere Vorschläge?

[Thomas F. (tfreal10)]

Und...
Was bedeutet "Data:         52 bytes (5.1% Full)"

[Gast (Gast)]

Bei mir* bedeutet diese Angabe wieviel Speicher die von dir verwendeten 
Variablen im RAM belegen.


* AVR Studio + GCC

[Nepi (Gast)]

Vielleicht versuchst du es mal ohne Watchdog. Wenn sich dann was Ändert 
kann es an einem zu langen Delay liegen. Hab sowas mal nach dem 
Hinzufügen einer LCD Routine gehabt.

[Oliver (Gast)]

>Wie kann ich nachsehen ob
>eventuell Variablen die im Flash abgespeichert werden evtl. das Programm
>überschreiben.

Brauchst du nicht. Du wirst keine Variablen im Flash vom Programm aus 
speichern, und die per PROGMEM angelegten Konstanten zählt avr-size brav 
mit.

>Um
>auszuschließen ob es daran liegt dass der Speicher zu voll ist.

"Zu voll" gibt es beim FLASH nicht. Es gibt nur voll oder nicht voll, 
und voll ist das FLASH ist ja noch nicht.

>Was bedeutet "Data:         52 bytes (5.1% Full)"

Das bedeutet, daß zur Compilezeit 52 Byte globale bzw. statische Daten 
bekannt sind und im SRAM reserviert werden. Das ist für ein Programm 
dieser Größe nicht viel. Das bedeutet aber nicht, daß das SRAM nicht 
trotzdem überläuft. Wenn du z.B. größeren Datenfelder o.ä. in einer 
Funktion deklariert hast, mach die zum Test mal static oder ganz global, 
und schau, was dann mit data passiert.

>Der UART Interrupt hat ja eine relativ niedrige Priorität darunter ist
>nur noch der Analoge Komparator.

Die AVR-Interrupts haben in diesem Sinne keine Priorität. First come, 
first serve, und per default wird immer erst eine ISR beendet, bevor die 
nächste drankommt. Die über die Vektor-Reihenfolge definierte Priorität 
kommt nur zum tragen, wenn mehrere Interrupts gleichzeitg anstehen.

>Klar diese führt ein Interrupt aus und anscheinend wird
>ein anderer Interrupt nicht mehr beendet.

Solange du nicht explizit in einer ISR die Interrupts per sei() wieder 
freigibst (und das sollte man nur tun, wenn man genau weiß, was man da 
tut), kann das nicht sein.

Der Grund für das Verhalten kann alles mögliche sein.

- SRAM zu voll
- ISR zu lang (es steht dauernd ein Interrupt an)
- Interrupt ohne zugehörige ISR
- irrlichternde Pointer
- Arraygrenzen überschritten, Fehler bei der Stringbehandlung
- und alle anderen klassischen oder speziellen Programmierfehler

Da musst du jetzt durch...

Oliver

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

> und der Fehler tritt immer nur spontan auf.
Kontrollier mal deine Pointer. Damit meine ich alles, was mit Zeigern, 
Arrays, Strings und Indizes zu tun hat.

[Thomas F. (tfreal10)]

> Solange du nicht explizit in einer ISR die Interrupts per sei() wieder
> freigibst (und das sollte man nur tun, wenn man genau weiß, was man da
> tut), kann das nicht sein.


Wie ist dass gemeint? Soll ich am Anfang jedes ISR´s mit cli() die 
interrupts ausschalten und am ende wieder mit sei einschalten?

also z.B.

ISR(INT1_vect){ // Interrupt für IR_Empfänger
  cli();
         DISABLE_BUTTONS;
  _delay_us(320);
  x.HalfBit = 0;
  x.SendEmpf = 0; //Timer 1 auf Empfangen schalten
  ENABLE_BIT_TIMER;
  bitcounter = 14;
  IR_Folge1 = 0;
  IR_FolgeBuild=0;
  sei();
}

[Oliver (Gast)]

>Wie ist dass gemeint? Soll ich am Anfang jedes ISR´s mit cli() die
>interrupts ausschalten und am ende wieder mit sei einschalten?

Am besten machst du einfach gar nichts. Der AVR löscht das globale 
Interrupt-Flag beim Einsprung in eine ISR, und setzt es wieder beim 
Aussprung, so daß während einer ISR die Interrupts gesperrt sind. Schau 
ins Datenblatt/ den Befehlssatz.

Oliver

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

ISR(INT1_vect){ // Interrupt für IR_Empfänger
   :
   _delay_us(320);

Aua. Was ist das für ein Murks?
In einer Interruptroutine (die ja andere Interrupts blockiert), hat ein 
delay() niemals was zu suchen. Wenn du sowas brauchst, dann stimmt 
dein Softwarekonzept noch nicht.

[Jörg W. (dl8dtl) (Moderator)]

Lothar Miller schrieb:

> In einer Interruptroutine (die ja andere Interrupts blockiert), hat ein
> delay() niemals was zu suchen.

Naja, man soll nie nie sagen, aber 320 µs sieht schon ziemlich böse
aus.  Dann sollte man wohl zumindest die globalen Interrupts wieder
freigeben, das aber kann bei einem Externinterrupt riskant sein.

Ein Delay von bis zu vielleicht 10 µs, wenn es denn zwingend mit
dem möglichst straffen Timing einer ISR gemacht werden muss, würde
ich (wenn es zwingend sein muss) noch für akzeptabel halten.  Aber
der OP lässt ohnehin viel zu wenig durchgucken von seinem Projekt,
und mir scheint, dass man hier mit einem Timer sinnvoller arbeiten
könnte.  Innerhalb von 320 µs hat man ja locker die Extern-ISR
wieder verlassen.

[dhgdsg (Gast)]

Sorry für den Müll-Eintrag (dachte hier wäre ein cpature vorgeschaltet 
und wollte das einem Kollegen zeigen), hier auch noch was produktives:

Das Problem könnte ein überlaufender Stack sein, insbesondere bei 90% 
voll IMHO recht wahrscheinlich.

[Jörg W. (dl8dtl) (Moderator)]

dhgdsg schrieb:
> Sorry für den Müll-Eintrag

Hab ich rausgekickt.

> Das Problem könnte ein überlaufender Stack sein, insbesondere bei 90%
> voll IMHO recht wahrscheinlich.

Der SRAM ist statisch nur zu 5 % voll, nicht zu 90 %.  Damit kann man
das praktisch ausschließen (falls der OP nicht gerade eine endlose
Rekursion implementiert hat ;-).

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

> Naja, man soll nie nie sagen...
Richtig, aber am leichtesten merkt man sich die einfachen Regeln  ;-)
Und das sind klare Ja/Nein Richtlinien.

Wenn ich dann davon abweiche, muß ich die Folgen abwägen. Irgendwann 
bekommt man ein Gefühl dafür, ob solche Abweichungen von der einfachen 
Regel gefährlich sind. Dieses Gefühl fehlt Thomas offenbar noch.

[Thomas F. (tfreal10)]

Hier mal mein Projekt ungekürzt. Ich weiss ich mache sicherlich noch 
viele Fehler. Aber da ich mir das alles selber irgendwie durch 
ausprobieren beibringe kann da auch ein bischen murks dabei sein. Wäre 
schön wenn ihr mich auf große Probleme aufmerksam macht so wie z.B. 
delay im interrupt da war mir zwar schon klar dass dies nicht sonderlich 
gut ist aber es war die einfachste und schnellste Lösung und im großen 
und ganzen funktioniert dass alles auch ganz gut.

[Karl H. (kbuchegg)]

Thomas Frosch schrieb:

> ausprobieren beibringe kann da auch ein bischen murks dabei sein.

Bischen ist gut.

Regel Nummer römisch Eins:
In einer ISR wird nie auf irgendetwas gewartet!

Deine UART Empfangsfunktion ist eine einzige riesengrosse Warteschleife. 
Die macht viel zu viel!

> delay im interrupt da war mir zwar schon klar dass dies nicht sonderlich
> gut ist aber es war die einfachste und schnellste Lösung und im großen
> und ganzen funktioniert dass alles auch ganz gut.

Eben nicht. Es scheint nur am Anfang so, als ob das die einfachste 
Lösung ist. In Wirklichkeit ist das eine Aufforderung zum 'sich selber 
ins Knie schiessen'

Und auch mal ein paar Funktionen einführen, die immer wiederkehrende 
Aufgaben in eine einzige Funktion verfrachten und so den Code kürzer 
machen, sind praktisch immer eine gute Idee. Kürzere Code bedeutet 
nämlich: mehr Übersicht

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

> Deine UART Empfangsfunktion ist eine einzige riesengrosse Warteschleife.
> Die macht viel zu viel!
Den Verdacht hatte ich vor 6 Stunden schon: 
Beitrag "Re: while(1) in main wird nicht mehr ausgeführt"  ;-)

EDIT:
Eine ISR-Routine hat nach spätestens 30 Zeilen fertig zu sein. Sie muß 
auf jeden Fall auf eine Bildschirmseite passen. Und keine dieser Zeilen 
beinhaltet das Keyword while oder eine Funktion mit Namen delay und 
bei einer for-Schleife ist Forsicht angesagt ;-)

[Karl H. (kbuchegg)]

Dein
 struct irgendwas x
werte ich mal als Versuch, Speicher zu sparen. Wenn du allerdings in 
derartigen Speichernöten bist, dass du dir ~20 Bytes globale Variablen 
nicht mehr leisten kannst und diese 20 Bytes deine einzigen globalen 
Variablen sind, hast du ein ganz anderes Problem.

[Jörg W. (dl8dtl) (Moderator)]

Er ist ja gar nicht in Speichernot.  Er hat selbst mit Reserve für
den Stack wohl noch mehrere hundert Byte frei, für deren
Nichtbenutzung er kaum Geld von Atmel zurück bekommen wird. ;-)

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Kleines Detail ziemlich am Ende der while(1)-Schleife:

  _delay_ms(20);

Dazu die Frage: bekommst du Rechenzeit kostenlos?
Oder warum verplemperst du sie so einfach?

[sam (Gast)]

compilierst du das programm mit einer compiler-optimierung?

da werden gerne mal schleifen merkwürdig optimiert, dass 
abbruchbedingungen wegoptimiert werden

[Karl H. (kbuchegg)]

Jörg Wunsch schrieb:
> Er ist ja gar nicht in Speichernot.  Er hat selbst mit Reserve für
> den Stack wohl noch mehrere hundert Byte frei, für deren
> Nichtbenutzung er kaum Geld von Atmel zurück bekommen wird. ;-)

OK. In dubio ...
Ich dachte er wolle etwas Speicher für den Stack freischaufeln.


Aber summa summarum.
Wenn das Programm nicht gar so verkorkst wäre, würd ich sogar mithelfen 
das umzuschreiben. Aber das ist selbst mir jetzt zuviel auf einmal (und 
ich soll ja auch noch meine Programme hier debuggen)

[Jörg W. (dl8dtl) (Moderator)]

Einige der Dinge in struct x werden wohl auch als Interruptflags
benutzt, ohne dass sie als volatile markiert wären.

Aber irgendwie fehlt's mir auch ein wenig an Struktur, um da
durch zu blicken.  Ist alles ein ziemlicher Klumpen...

Eine als "static" deklarierte Funktion wird übrigens, wenn sie nur
ein einziges Mal benutzt wird, vom Compiler in jeder Optimierungsstufe
(außer 0 natürlich) inline eingebaut.  Das vergrößert also den Code
nicht, hilft aber extrem, mehr Übersichtlichkeit hinein zu bringen.

[Karl H. (kbuchegg)]

Jörg Wunsch schrieb:
> Ist alles ein ziemlicher Klumpen...

Ja, das ist das eigentliche Problem. Klumpen.
Irgendwie hat man so gar keinen Ansatzpunkt, wo man anfangen könnte das 
alles zu zerpflücken und das was man gefahrlos rausziehen könnte (im 
wesentlichen bieten sich momentan nur die ganzen Warteschleifen an), 
bringt alles in Bezug auf das eigentliche Problem irgendwie nichts.

Wenn ich anfangen müsste, würde ich mal die ganzen case-label 
Funktionalitäten in eigene Funktionen verbannen (damit der Code mal aus 
dem Weg ist) und jede dieser Funktionen auf mögliche Wartebedingungen 
durchforsten.

Dann die ganze Steuerung in der UART-ISR umstellen auf eine 
Statemachine, die in die Hauptschleife wandert.

Und dann mal weiterschauen. Aber der erste Schritt müsste IMHO sein, mal 
einen Überblick zu bekommen, was da eigentlich alles passiert. Auch 
Funktionalität in eigene Module (Source + Header) auszulagern erscheint 
mir da vielversprechend.

Viel Arbeit. Aber das ist der Preis, den jeder irgendwann zahlen muss. 
Ist mir auch so gegangen. Allerdings ist das sehr lehrreich. Dann 
spätestens wenns einem ein drittes mal passiert, achtet man sehr viel 
mehr auf Programmstruktur.

[Thomas F. (tfreal10)]

>Wenn ich anfangen müsste, würde ich mal die ganzen case-label
>Funktionalitäten in eigene Funktionen verbannen (damit der Code mal aus
>dem Weg ist) und jede dieser Funktionen auf mögliche Wartebedingungen
>durchforsten.

Soll ich einfach nur die Case funktionen in eine Funktion schmeißen und 
die dann wiederrum im UART ISR ausführen? bringt zwar mehr übersicht 
aber praktisch ist der UART ISR doch trotzdem noch so aufgeblasen oder?

Andere Möglichkeit wäre mir die Werte die ich durch den UART Empfange zu 
merken und wieder irgendwo ein Bit zu setzen um in der Main while 
schleife dann die ganze case geschichte ablaufen zu lassen oder?

>Dann die ganze Steuerung in der UART-ISR umstellen auf eine
>Statemachine, die in die Hauptschleife wandert.

Was ist Statemachine?

>Kleines Detail ziemlich am Ende der while(1)-Schleife:  _delay_ms(20);
>Dazu die Frage: bekommst du Rechenzeit kostenlos?
>Oder warum verplemperst du sie so einfach?

Ich empfange über einen IR Empfänger RC5 Codes und diese sollen eben 
weiterverarbeitet werden. Allerdings sollen die nicht ständig an den PC 
gesendet werden, da er sonst öfters hintereinander die gleiche Funktion 
ausführen würde obwohl noch gar kein neuer code sondern immer noch der 
alte da ist.

In einem anderen Controller habe ich es so gelöst, dass ich in der While 
schleife einen Zähler bis 30000 oder so hochzähle und am ende wieder 
zurücksetzen lasse und sachen die immer ausgeführt werden müssen bei 
jedem Zählerstand abgearbeitet werden und Sachen die nur einmal 
ausgeführt werden sollen z.B. beim Zählerstand 1000 ausführen lasse. So 
kann ich auch bestimmte Sachen zwingend nacheinander abfolgen lassen. 
Ist dies eine bessere Lösung? Oder wie macht man soetwas richtig?

[Oliver (Gast)]

>Hier mal mein Projekt ungekürzt.

* twi_master.c, line 90: 21: error: General.h: No such file or directory

schade...

Oliver

[Thomas F. (tfreal10)]

die hier

ist nur für True und False da. Hab jetzt True und False durch 1 und 0 
ersetzt und General rausgeschmissen.

[Jörg W. (dl8dtl) (Moderator)]

Thomas Frosch schrieb:
> die hier
>
> ist nur für True und False da. Hab jetzt True und False durch 1 und 0
> ersetzt und General rausgeschmissen.

Besser:

#include <stdbool.h>

Danach hast du den Typen "bool" sowie die Werte "true" und "false"
dafür.  Ist per C99 genormt.

[Oliver (Gast)]

>ist nur für True und False da. Hab jetzt True und False durch 1 und 0
>ersetzt und General rausgeschmissen.

Hatte ich dann auch festgestellt.
Kompilieren tut es, aber da sind so viele "Unschönheiten" drin, da 
durchzusteigen ist ernsthaft nicht möglich.

Ich hab es mal kurz in VMLAB laufen gelassen. Das meckert ein paar Dinge 
an. Zum einen gefällt ihm die ADC-Taktfrequenz nicht, zum anderen wird 
bei Eingaben über die serielle Schnittstelle in der ISR UDR nicht 
gelesen, da die if-Bedingung

ISR(USART_RXC_vect) { //Empfangen von PC INPUT-> Mode, Byte1, Byte2
  
if (Z_PC != 0)
{...

zumindestens in der Simulation auch mal nicht erfüllt ist. Und das ist 
schlecht.

Zitat aus dem Datenblatt:
>When interrupt-driven data reception is used, the receive complete routine
>must read the received data from UDR in order to clear the RXC Flag, >otherwise a 
new interrupt
>will occur once the interrupt routine terminates.

Das war bestimmt nicht das einzige Problem in der Software, aber es ist 
damit eins weniger.

Ansonsten: Aufräumen. Aber gründlich.

Oliver

Oliver

[Karl H. (kbuchegg)]

Thomas Frosch schrieb:

> Soll ich einfach nur die Case funktionen in eine Funktion schmeißen

Damit würd ich mal anfangen, ja

aus

  switch( irgendwas ) {
    case bla1:
      ...  mächtig viel Code
      break;

    case bla2:
      ... mächtig viel noch mehr Code
      break;
 
    ...
  }

wird

void foo1()
{
  ... mächtig viel Code
}

void foo2()
{
  ... mächtig viel noch mehr Code
}

...

  switch( irgendwas ) {
    case bla1:
      foo1();
      break;

    case bla2:
      foo2();
      break;
 
    ...
  }

> und
> die dann wiederrum im UART ISR ausführen? bringt zwar mehr übersicht
> aber praktisch ist der UART ISR doch trotzdem noch so aufgeblasen oder?

natürlich ist sie das.
Dein Code ist so umfangreich, dass ich mir am Anfang nicht trauen würde 
die Logik gross zu verändern. Aber ich würde mal aufräumen, damit man 
kleinere Einheiten hat, die es zu durchforsten gilt.

Das ist wie in einer Werkstatt:
Ob du deine Werkzeuge alle auf einem Haufen hast, oder säuberlich in 
Schubladen, ändert nichts an den Werkzeugen. Die sind nach wie vor die 
gleichen und wenn der Akkuschrauber defekt ist, dann ist er es auch nach 
der Aufräumaktion.

Wenn aber alles was du weißt die Information ist, dass ein Gerät defekt 
ist, dann tust du dir viel leichter, wenn du in deinem Geräteschrank ein 
Gerät nach dem anderen durchsiehst, als wie wenn du aus dem Haufen 
relativ wahllos eines rausfischt, es überprüfst und es danach wieder in 
den Haufen zurückwirfst.

Dein Code ist in einem Zustand, dass der erste Schritt darin bestehen 
muss, Übersicht zu erhalten.

[Karl H. (kbuchegg)]

Was ich dann auch machen würde:
Funktionalität sinnvoll in eigene Funktionen auslagern.

Mal ein Beispiel

Der Anfang deiner bisherigen ISR:

ISR(USART_RXC_vect) { //Empfangen von PC INPUT-> Mode, Byte1, Byte2
  
  if (Z_PC != 0)
  {
    DISABLE_BUTTONS;
    uint8_t Mode = 0;
    uint8_t Byte1 = 0;
    uint8_t Byte2 = 0;

    Byte1 = UDR;          // Verschlucken von "Startbyte"
    x.WhileCounter = 0;
    while (!(UCSRA & (1<<RXC))){
      x.WhileCounter++;
      if (x.WhileCounter >= 65500)
        {
        ENABLE_BUTTONS;
        return;
        }
    };
    
    Mode = UDR;            // Empfangen von Mode
    x.WhileCounter = 0;
    while (!(UCSRA & (1<<RXC))){
      x.WhileCounter++;
      if (x.WhileCounter >= 65500)
        {
        ENABLE_BUTTONS;
        return;
        }
    };
    
    
    
    Byte1 = UDR;          // Empfangen von Byte1
    x.WhileCounter = 0;
    while (!(UCSRA & (1<<RXC))){
      x.WhileCounter++;
      if (x.WhileCounter >= 65500)
        {
        ENABLE_BUTTONS;
        return;
        }
    };
    Byte2 = UDR;          // Empfangen von Byte2
    ENABLE_BUTTONS;
...

Da ist eine Menge scrollerei notwendig um zu sehen, was da eigentlich 
abgeht. Eine eigenentlich kleine Änderung später:

uint8_t WaitForByte( uint8_t * nextByte )
{
  x.WhileCounter = 0;
  while (!(UCSRA & (1<<RXC))) {
    x.WhileCounter++;
    if (x.WhileCounter >= 65500)
      return 0;
  }

  *nextByte = UDR;
  return 1;
}

ISR(USART_RXC_vect)  //Empfangen von PC INPUT-> Mode, Byte1, Byte2
{
  if (Z_PC != 0)
  {
    DISABLE_BUTTONS;

    uint8_t Mode = 0;
    uint8_t Byte1 = 0;
    uint8_t Byte2 = 0;

    Byte1 = UDR;   // Verschlucken von "Startbyte"

    if( !WaitForByte( &Mode ) ||
        !WaitForByte( &Byte1 ) ||
        !WaitForByte( &Byte2 ) )
    {
      ENABLE_BUTTONS;
      return;
    }

    ENABLE_BUTTONS;

...

... ist schon viel von dieser 'Komplexität' verschwunden. Man sieht in 
der ISR klar und deutlich, dass zunächst ein Byte verworfen wird und 
dann, in dieser Reihenfolge 'Mode', 'Byte1' und 'Byte2' empfangen 
werden. Und zwar ohne dass ich gross scrollen muss. Der Blick auf die 
Reihenfole der Ereignisse wird frei, weil ich die Details des Wartens in 
eine Funktion verbannt habe. Ich kann auch viel leichter kontrollieren, 
ob zb das ENABLE_BUTTONS in allen Fällen korrekt aufgerufen wird. In 
deinem Code muss ich an 3 oder 4 Stellen nachsehen, ob ich das nicht 
vergessen habe. In der kürzeren Form kann man das viel leichter 
überblicken.
Beantworte mal ganz schnell eine Frage in deinem Code: Sind die 
Warteschleifen eigentlich alle gleich, oder gibt es bei einzelnen Bytes 
andere Timeouts? Ähm. Da muss man jetzt Codeabschnitte vergleichen.
Im kurzen Code: Es wird jedesmal dieselbe Funktion aufgerufen. In der 
Funktion gibt es keine Unterscheidung. Daher: Ja, sind alle gleich

Das mein ich mit 'Aufräumen'. Funktional nicht viel verändern (der 
kürzere Code macht immer noch dasselbe, nur die Verteilung im Quellcode 
ist anders), aber für mehr Übersicht sorgen.

Jedesmal, wenn du so einen Kommentar schreiben musst

    Byte2 = UDR;          // Empfangen von Byte2

solltest du dich fragen, ob und wie du deinen Code anders gestalten 
kannst, so dass der Kommentar überflüssig wird. In deinem Code ist der 
Kommentar notwendig, weil das alles entscheidende Abholen des Bytes 
komplett im restlichen Code untergeht.
Und jetzt vergleich mal

    if( !WaitForByte( &Mode ) ||
        !WaitForByte( &Byte1 ) ||
        !WaitForByte( &Byte2 ) )

Brauchst du da einen Kommentar, um zu wissen bzw. zu sehen, was an 
dieser Stelle passiert (ignorier erst mal die ||. Die sind nur dazu da, 
damit beim ersten gemeldeten Fehler aus WaitForByte nicht mehr auf die 
restlichen Bytes gewartet wird, so wie es dein Originalcode auch macht.

Klar, das Alles löst dein Problem zunächst mal nicht. Aber es ist ein 
Schritt in die Richtung, die letztendlich zur Entdeckung des Problems 
führen wird.


Apropos: Wie erreichst du eigentlich wieder eine Synchronisierung des 
Empfängers mit dem Sender, wenn ein Timeout erfolgte? Sagen wir mal 
Byte1 hat zu lange gebraucht. Die Bearbeitung hier wird abgebrochen und 
irgendwann kommt das nächste Zeichen auf der UART. Woher weißt du, das 
das dann ds zu ignorierende Startbyte ist und das nächste Byte das Mode 
Byte, etc.

[Jörg W. (dl8dtl) (Moderator)]

Karl heinz Buchegger schrieb:

> aus
...
> wird
>
>

> void foo1()
> {
>   ... mächtig viel Code
> }
> 
> void foo2()
> {
>   ... mächtig viel noch mehr Code
> }
>

Besser:

static void foo1(void)
{
    ... mächtig viel Code
}

static void foo2(void)
{
    ... mächtig viel noch mehr Code
}

In C ist eine leere Parameterliste was anderes als eine, in der
nur "void" steht, und das "static" sagt dem Compiler, dass das
nie jemand außerhalb des aktuellen Übersetzungsmoduls aufrufen
kann.  Da es nur einmal gerufen wird, kann der Compiler den
Code dieser Funktionen dann bedenkenlos gleich in die switch-
Anweisung einbauen.  Das Ergebnis ist also dasselbe wie beim
Modell ,,Großer Klumpen'', nur der Quelltext ist übersichtlicher.

[Karl H. (kbuchegg)]

Jörg Wunsch schrieb:

> Besser:

Danke.
old habit, bzw. C++ verwöhnt :-)

[Thomas F. (tfreal10)]

@ all
also meine Delay funktionen habe ich jetzt ganz und gar aus meinem 
Programm verbannen können :-) Und ich werde diese auch nicht mehr 
verwenden, da man die meisten solcher Probleme auch anders/besser lösen 
kann.


@ Oliver
Z_PC steht für Zustand PC also ob er an oder aus ist.

es wäre also besser die abfrage if (Z_PC !=0) nach dem Auslesen der 
Bytes abzufragen.

@ Karl heinz Buchegger (Switch)
ok diese Form mach sicherlich Sinn für einige große Teile in der switch 
Anweisung, aber die meisten Anweisungen pro case halten sich eher in 
grenzen. So finde ich ist es schneller lesbar diese zu belassen und nur 
die großen auszuglieder. Jedesmal die passende Funktion dazu zu suchen 
finde ich unpraktisch. Aber natürlich Danke für die Antwort.

Viel mehr würde mich nun aber interessieren wie ich die Switch Anweisung 
komplett aus der USART ISR schmeißen könnte? So wie schon von mir 
beschrieben? Also die eingehenden Bytes in eine Art Puffer schreiben und 
dann in der Main while Schleife abarbeiten?

@ Karl heinz Buchegger (Byte-Einlesefunktion)
Natürlich klar dies so zu lösen hatte ich auch schon vor aber erschien 
mir meistens zu unwichtig mich damit aufzuhalten :-)

Sollte ich die Sache mit dem Puffer machen wie oben beschrieben würde 
sich diese Funktion sowieso auf eine Abfrage verkürzen. Über die Sache 
mit der Synchronisierung habe ich mir ehrlich gesagt gar keine Gedanken 
gemacht! Aber natürlich klar was ist im Fehlerfall? Hat bisher einfach 
zufällig zu gut geklappt! Muss mir dann wie ich es auch schon beim UART 
Senden mache ein Byte für "Starte Byte Folge" oder so ausdenken. Denn da 
sende ich glaube ich eine Tilde am Ende um zu sagen Bytes zwischen Tilde 
(ASCII = 126 da fällt mir auf, dass könnte ich auch '~' so schreiben 
g)  und Tilde gehören zusammen wenn es nicht mehr oder weniger als 3 
sind.

Lauter so Sünden die über die Zeiten des Programmierens immer nur 
teilweise verbessert wurden.

Jetzt die wesentlich wichtigere Frage:
Wie könnte ich die Werte abfragen ohne While schleife in der UART ISR?

Wenn Verbessern dann auch richtig gg

DANKE FÜR DIE SUPER HILFE AN ALLE!!!!!!
Bin natürlich weiter offen für Verbesserungsvorschläge