ISR bricht ab?

[Weasel (Gast)]

Liebe Kollegen ich stehe gerade vollkommen an.
Ich arbeite an einem LCD-Display welches über RS485 mit einem an MODBus 
angelehnten Protokoll angesteuert wird.

Dazu habe ich einen interrupt der die empfangenen Zeichen in einen 
Buffer schiebt, und am Ende den buffer kurz checkt, und wenn 
Empfangsaddresse und start/ende passen ein ACK zurück schickt. So 
einfach so gut.

Als Sender habe ich vorerst einen Arduino der alle 1s einen Befehl 
schickt. Nur kommt bisher nur nach jedem zweiten Befehl ein ACK zurück. 
Ich habe die Übetragung getestet, passt alles. Ich habe den Fehler auf 
den Interrupt am Atmega(328-P) zurückverfolgen können.

Ich habe den Code so weit beschnitten un gekürzt dass nur noch 2 
Funktionen übrig sind.
Hier der Code der Probleme macht, sonst läuft nichts:

volatile char UartBuffer[UBS];      // UART Buffer
volatile int UartCounter;        // UART Buffer counter, used by interrupt

// UART receive interrupt
ISR(USART_RX_vect){
  char in = UDR0;          // Read input register
  UartBuffer[UartCounter] = in;  // put byte into buffer
  
  if(in == '\r'){
    //RS485_Write(UartBuffer,UartCounter);
    
    char check = 1;
    // check if transmission is ok
    if(UartBuffer[0] != ':'){
      check = 0;                // Start condition not found
    }
    char add1 = UartBuffer[1];          // address char 1
    char add2 = UartBuffer[2];          // address char 2
    if((add1 != ADDRESS[0]) || (add2 != ADDRESS[1])){
      check = 0;                // address is not correct
    }
    
    if((UartBuffer[UartCounter] != '\r') || (UartBuffer[UartCounter-1] != '\n')){
      check = 0;                // End of transmission not correct
    }
    
    char crc1 = UartBuffer[UartCounter-2];    //3 last chars of payload are CRC
    char crc2 = UartBuffer[UartCounter-1];
    // CRC CHECK TO BE IMPLEMENTED !! ! ! ! !! !! ! ! ! !!

    if(check == 1){
      // transmission correct
      RS485_ACK();
      //RS485_AddPayload();
    }
    uart_ClearBuffer();
  }else{
    // keep receiving
    UartCounter++;
    // keep buffer from overflowing
    if(UartCounter > UBS){UartCounter = 0;}
  }
}

// writes a string to the bus
void RS485_Write(char* dat, int len){
  RS_En_Port |= (1<<RS_En_Pin);  // RS485 to send mode
  
  for(int i=0; i<len; i++){
    while ( !( UCSR0A & (1<<UDRE0)) )
    ;              // wait for empty transmit buffer
    UCSR0A |= (1<<TXC0);    // reset transmit complete bit
    UDR0 = dat[i];        // send data
  }
  
  while ( !( UCSR0A & (1<<TXC0)) )
  ;                // wait for transmit complete

  RS_En_Port &= !(1<<RS_En_Pin);  // RS485 to receive mode
}

Ich habe versucht immer wieder den Bufferinhalt aus zu geben, und es 
wirkt oft so als ob gerade beim senden von Daten es einen Abbrucht der 
ISR gibt.
Auch wenn ich nach der EOL abfrage einfach ein "lalablubb" ausgebe kommt 
beim Arduino ein "lalalalalablubb" an .....

Kann mir villeicht bitte einen hinweis geben was hier passieren kann?
Ich komme einfach nicht drauf. Der Buffer ist groß genug, der Interrupt 
hat mehr als genügend Zeit. Er bricht einfach irgendwo ab ....

[Nop (Gast)]

Weasel schrieb:

> volatile char UartBuffer[UBS];
> volatile int UartCounter;

>     UartCounter++;
>     // keep buffer from overflowing
>     if(UartCounter > UBS){UartCounter = 0;}

Das sieht nach potentiellem Buffer-Overflow aus. Wenn Deine Buffergröße 
UBS ist, dann sind die zulässigen Indizes 0..UBS-1. Da könnte beim 
Empfang die Variable überschrieben werden, die nach dem Buffer im 
Speicher liegt, wahrscheinlich der UartCounter selber.

[Weasel P. (philipp_s576)]

Nop schrieb:
> Weasel schrieb:
>
>> volatile char UartBuffer[UBS];
>> volatile int UartCounter;
>
>>     UartCounter++;
>>     // keep buffer from overflowing
>>     if(UartCounter > UBS){UartCounter = 0;}
>
> Das sieht nach potentiellem Buffer-Overflow aus. Wenn Deine Buffergröße
> UBS ist, dann sind die zulässigen Indizes 0..UBS-1. Da könnte beim
> Empfang die Variable überschrieben werden, die nach dem Buffer im
> Speicher liegt, wahrscheinlich der UartCounter selber.

Danke, das muss ich auch noch überarbeiten.
Stimmt das war vorher ein Problem.
Mein Gedanke war dass der Buffer einfach zurückgesetzt wird und dann 
(weil er als falsch erkannt wird) einfach ignoriert wird sobald ein \r 
empfangen wird.
Danach wir der Buffer resettet mit uart_ClearBuffer();
(Die Funktion stellt den counter auf null und setzt alle Zeichen im 
Buffer auf \0)

Aber der Buffer ist hier 30 Zeichen lang, die Befehle hier sind nur 10 
lang.

[Kodex (Gast)]

Es muss zwar nicht die Ursache für Dein Problem sein aber möglichweise 
sollte man das trotzdem mal ändern:

Deine ISR tut eigentlich viel zu viel.

Sie sollte:
 - Das Byte aus UDR in Deinen Puffer schreiben
 - eventuell ein Flag setzen, dass ein neues Byte empfangen wurde.

Sie macht aber:
 - Das Byte lesen
 - Prüfen ob das Telegramm vollständig ist
 - Die Adresse des Telegramms mit der Geräteadresse vergleichen
 - ggf. eine Antwortfunktion mit Warteschleifen aufrufen
 - scheinbar soll noch eine CRC Prüfung mit hinein...

Das ist zumindest schonmal nicht schön.

Desweiteren würde ich aber dringend empfehlen, auch noch eine 
Zeitkonstante für das Zurücksetzen des UartCounter einzubauen, zumal Du 
ja auch davon ausgehst, dass sich in UartBuffer[0] immer das 
Startzeichen befindet, wenn ein Endezeichen '\r' erkannt wurde.
Ist das mal nicht der Fall, weil der Arduino beim Reset z.B. ein 0xFF 
auf die Leitung schickt, hast Du schon ein Problem.

Ich würde also mindestens ein sinnvolles Telegramm-Timeout 
einkalkulieren.
Wenn bspw. seit 5ms kein Byte mehr angekommen ist, dann Zähler auf 0 
zurücksetzen.

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

Kodex schrieb:
> Deine ISR tut eigentlich viel zu viel.
Full ACK!
Diese ISR sollte einfach ein Zeichen einlesen und in einen Ringpuffer 
schreiben. Irgendwann "später" beim nächsten Durchlauf der Mainloop wird 
das dann ausgewertet.

Weasel schrieb:
> Ich habe den Code so weit beschnitten un gekürzt dass nur noch 2
> Funktionen übrig sind.
Was macht denn die da:
> RS485_ACK();

[Peter D. (peda)]

Bei RS-485 kann nicht jeder einfach so lossenden, wie er lustig ist. Es 
gibt im Protokoll eine Mindestzeit, die man dem Master Zeit geben muß, 
um seinen Sender abzuschalten. Erst danach darf man seinen Sender 
einschalten.
Der Slave muß wiederum auch seinen Sender schnellstmöglichst wieder 
abschalten, d.h. kein anderer Interrupt darf das verzögern können.

Und beim Empfang mußt Du natürlich die Echos aussortieren, bzw. besser 
noch testen, ob sie identisch zu den Sendedaten sind.

[Weasel P. (philipp_s576)]

Danke für eure Inputs.
Ich schätze ich werde die Auswertung nochmal komplett überdenken.
Der Ansatz mit dem Ringbuffer mit Timeout ist definitiv intelligenter 
als meine (Bastel-)Lösung.
Mein Gedanke war dass der Slave asap ein ack zurück schickt um den Bus 
so wenig wie möglich zu blockieren.....

> Was macht denn die da:
>> RS485_ACK();
einfach ein RS485_Write(":OK\n\r", 5); mehr nicht

[Peter D. (peda)]

Weasel P. schrieb:
> Der Ansatz mit dem Ringbuffer mit Timeout

Timeout ist nicht nötig, das erschwert Dir nur das Debuggen mit einem 
Terminal.
Ist ein Paket fehlerhaft, wird es verworfen und ein Fehlerzähler zählt 
hoch.
Protokolle, die ein Timeout benötigen, sind Pfui.

[Nop (Gast)]

Peter D. schrieb:

> Timeout ist nicht nötig, das erschwert Dir nur das Debuggen mit einem
> Terminal.

Zu Debugzwecken kann man den Timeout einfach als "sehr groß"(tm) wählen.

> Protokolle, die ein Timeout benötigen, sind Pfui.

Und das schreibst Du ausgerechnet über TCP/IP in ein Forum, ist klar. 
Timeouts sind im Sinne der Robustheit sinnvoll, damit sich die Zustände 
automatisch wieder aufeinander synchronisieren. Sie sollten nur nicht so 
klein gewählt werden, daß sie im halbwegs normalen Betrieb zuschlagen.

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

Weasel P. schrieb:
> Mein Gedanke war dass der Slave asap ein ack zurück schickt um den Bus
> so wenig wie möglich zu blockieren.....
Dafür blockierst du
>> Was macht denn die da:
>>> RS485_ACK();
> einfach ein RS485_Write(":OK\n\r", 5); mehr nicht
Ja, das führt zu einen "wait" in einem Interrupt, denn in der 
RS485_Write() steht:

    while ( !( UCSR0A & (1<<UDRE0)) )
    ;              // wait for empty transmit buffer

Ein böser Showstopper, der beliebige Effekte hervorrufen kann.

Merksatz: in einem Interrupt wird niemals für nennenswerte Zeit oder gar 
auf andere Hardwarekomponenten gewartet.

Weasel P. schrieb:
> Der Ansatz mit dem Ringbuffer mit Timeout ist definitiv intelligenter
> als meine (Bastel-)Lösung.
Zum Glück kommt die Idee mit dem Timeout nicht von mir... ;-)

> Ich schätze ich werde die Auswertung nochmal komplett überdenken.
Tu das. Und zwar so, dass die Protokollauswertung und abhandlung in der 
Mainloop passiert, und nur zum eigentlichen Empfangen (und 
schlimmstenfalls zum Senden) ein Interrupt nötig ist.

[Peter D. (peda)]

Nop schrieb:
> Und das schreibst Du ausgerechnet über TCP/IP in ein Forum, ist klar.

Nur weil etwas verbreitet ist, ist es noch lange nicht gut.
Timeouts kaschieren nur den Fehler und verhindern die Ursachensuche.
Z.B. passiert es mir sporadisch im Firmennetzwerk, wenn ich mit dem 
Explorer ein Verzeichnis öffne, daß das Netzwerk auf meinem PC für exakt 
1min tot ist (keine andere Explorer-Instanz auf Netzlaufwerk oder 
Internetbrowser, alles steht und wird grau). Die IT ist nicht in der 
Lage, den Fehler zu beheben. Ehe man den Debugger angeschmissen hat, ist 
die 1min schon vorbei.

Protokolle mit Timeouts lassen sich auch schlecht über andere Kanäle 
tunneln.
Protokolle mit Statemaschine sind da viel sicherer und flexibler.

[Nop (Gast)]

Peter D. schrieb:

> Protokolle mit Timeouts lassen sich auch schlecht über andere Kanäle
> tunneln.

Das kommt sehr auf die Timeouts an. Außerdem, wenn relativ kurze 
Timeouts aus der Anwendung heraus denn sinnvoll erscheinen, wären Tunnel 
mit hoher Latenz doch ohnehin nicht nutzbar.

> Protokolle mit Statemaschine sind da viel sicherer und flexibler.

Selbstverständlich sollte beides da sein. Die Timeouts sollten, wie ich 
schon schrieb, auch im normalen Betrieb gar nicht zuschlagen, sondern 
als Fallback dienen.

[Pandur S. (jetztnicht)]

Ich wuerde im Interrupt erst mal genau das Byte in den buffer schreiben, 
und ein Flag setzen. Die verarbeitung ausserhalb.

Ich lasse jeweils eine Zustandmaschine mitlaufen. Das bedeutet der 
Interrupt erkennt beginn der Nachricht, nimmt das Laengenbyte und zaehlt 
runter. Wenn die Nachricht im Buffer ist, wird ein flag gesetzt. Die 
Verarbeitung geschieht ausserhalb im Main.

Wichtig ist im Main die Moeglichkeit zu haben die freie Zeit zu pruefen. 
Dies geschieht, indem man einen Pin pulst. Jeden Durchgang im Main kommt 
ein Puls. so kann man am Oszillskop sehen, wie beschaeftigt der 
Prozessor ist. Dannn kann man zb bei jeder kompleten Meldung einen Pin 
Pulsen und schauen, ob, wie of das geschieht, und ob es mit der freien 
Zeit zusammenhaengt.

[Weasel P. (philipp_s576)]

Hallo Kollegen,

ich habe jetzt den Interrupt neu geschrieben.
Diemal wird während ein Befehl Empfangen wird, die Bytes in ein Buffer 
geschrieben.
Ist die End of Line erkannt wird dieser Buffer in einen Zweiten Buffer 
kopiert und ein Flag gesetzt.
Die Main bearbeitet dann ebendiesen zweiten Buffer und sendet auch die 
Antwort.

Und ... es funktioniert einwandfrei! Auch die Antwort kommt immer mehr 
als ausreichend schnell zurück obwohl die bearbeitung "erst später" in 
der Main passiert.

Was wurde gelernt?
Interrupt so kurz wie nur irgendwie möglich zu halten. Ich hatte 
angenommen "ach, sonst passiert ja nichts und die Übertragung ist ja 
auch arschlangsam. Wird schon hinhauen".... war trotzdem zu viel.

Nachdem das jetzt geht mach ich mich einmal dran einen Timeout zu 
implementieren.


Vielen Dank für eure Hilfe!

[Peter D. (peda)]

Weasel P. schrieb:
> Wird schon hinhauen".... war trotzdem zu viel.

Im Gegenteil, der Interrupt war zu schnell. Du hast die Mindestzeit 
nicht eingehalten, die der Master für die Richtungsumschaltung benötigt.

Besser als die Codeausführung im Main als undefiniertes Delay zu 
benutzen, ist natürlich, ein definiertes Delay mit einem Timer zu 
erzeugen.

[Weasel P. (philipp_s576)]

Peter D. schrieb:
> Weasel P. schrieb:
>> Wird schon hinhauen".... war trotzdem zu viel.
>
> Im Gegenteil, der Interrupt war zu schnell. Du hast die Mindestzeit
> nicht eingehalten, die der Master für die Richtungsumschaltung benötigt.
>
> Besser als die Codeausführung im Main als undefiniertes Delay zu
> benutzen, ist natürlich, ein definiertes Delay mit einem Timer zu
> erzeugen.

Aber wie kann es dann sein dass der Master dann trotzdem die Antwort 
bekommt und anzeigt?
Beim Alten Programm wenn ich zb. als Antwort ein "lala1234" schreibe 
kommt manchmal beim Master "lalalalala1234" an als ob es immer 
abgebrochen wird.

Mir kam eher vor dass die ISR mit dem Buffer nicht richtig klar gekommen 
ist, oder als ob es beim schreiben Probleme gab....?


Mit der aufteilung auf 2 Buffer und der Verarbeitung in der Main 
funktioniert es jetzt zwar tadellos, warum dieser Fehler aber 
hervorgerufen wurde währe dennoch interessant.

[Falk B. (falk)]

Weasel P. schrieb:
> Ist die End of Line erkannt wird dieser Buffer in einen Zweiten Buffer
> kopiert und ein Flag gesetzt.

Kann man machen, ist bei wenigen Bytes meistens auch OK. Wenn die 
Datensätze aber größer werden, wird das Umkopieren ggf. zu langsam oder 
sinnlos. Da ist es besser, die Daten im Puffer liegen zu lassen und nur 
zwei Pointer auf die Puffer zu wechseln. Damit spart man besonders bei 
großen und sehr großen Datensätzen, z.B. Bilder, massiv CPU-Leistung.

https://www.mikrocontroller.net/articles/Soft-PWM#Intelligenter_L.C3.B6sungsansatz

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

Pandur S. schrieb:
> Ich wuerde im Interrupt erst mal genau das Byte in den buffer schreiben,
> und ein Flag setzen.
So ein Flag (samt der dadurch möglichen Semaphorenproblematik: "was 
passiert, wenn das Flag 'gleichzeitig' gesetzt und zurückgesetzt wird?") 
brauchst du bei einem funktionierenden Ringpuffer nicht. Denn da ist es 
einfach so, dass etwas im Puffer ist, wenn Lesezeiger!=Schreibzeiger:
http://www.lothar-miller.de/s9y/categories/51-Fifo