Problem mit Malloc

[Thomas Bergmüller (Gast)]

Hallo,

ich habe momentan ein Problem mit malloc(), das ich einfach nicht gelöst 
bekomme.

Verwende es in einer Interruptroutine der USART am Atmega 88 bzw. 
Atmega32

Es handelt sich um eine Kommunikationslibrary zwischen PC und uC, die 
Datenrahmen unterschiedlicher Länge (zw. 5 und 262 Bytes) verarbeiten 
muss. Innerhalb dieses Protokolls gibt es einen Dateitransfer, der 
größere Dateien entsprechend auf mehrere Datenpakete aufteilt und im 
Empfänger muss das dann wieder zu einer Datenwurst zusammengesetzt 
werden.

Dabei sind unterschiedliche Protokolle implementiert (Daten, 
Datenbroadcast, Kommando, Kommandobroadcast, Filetransfer usw.)

Bis zu dem Zeitpunkt, an dem ich den Filetransfer implementierte, lief 
das ganze problemlos. Zur Bufferung der Daten (Empfang über USART) wurde 
immer sobald bekannt war wie lange das aktuelle Paket ist (wird 
mitübermittelt) mit malloc ein Speicher allokiert und dieser dann 
befüllt.

Beim Dateitransfer wird ein Ankündigungsframe mit einigen Informationen 
(Dateigröße, Dateiname) übertragen. Der uC antwortet dem PC daraufhin, 
ob er genügend Speicher frei hat um die Datei aufzunehmen usw.

Das ganze wird in einer Funktion abgefragt und die antwort versendet, 
danach wird in die ISR zurückgekehrt.


Nun zum eigentlichen Problem:
Der in der Funktion allokierte Speicher wird teilweise in den 
Speicherbereich von lokalen static-Variablen und sogar globalen 
Variablen gelegt, die entsprechend überschrieben werden.

Ich komm einfach nicht dahinter, warum das geschieht, hat das 
irgendetwas mit der internen Speicherverwaltung von malloc zu tun.

(Achtung Pseudocode)

Funktionierend:

ISR(USART)
{ 
    // Diverse Staticvariablen
    // ....
    // 
    // static unsigned char* workingDataPtr;
    // Warten bis bekannt ist wie lang das gerade empfangende Paket is
    workingDataPtr = (unsigned char*) malloc(FRAMELENGTH);

    // Wenn Paket empfangen und Daten stimmen
    free(global_validDataPtr); // Alte Daten löschen
    global_validDataPtr = workingDataPtr;
}

nicht funktionierend

function FileTransfer(unsigned char* receivedData)
{
   // Diverse Staticvariablen
   // .....
   // static unsigned char* workingFilePtr;
 
   // Wenn Ankündigungsframe mit Infos
    workingFilePtr = (unsigned char*) malloc(FILESIZE);
   
   // Wenn Dateidaten empfangen
   //Schreibe in allkoierten Speicher @ workingFilePtr
   for(i=0;i<framelänge; i++)
   {
      workingFilePtr = receivedData[i];
      workingFilePtr++;
   }

   // Wenn letztes Paket empfangen
   free(global_validFilePtr); // Altes File löschen
   global_validFilePtr = workingFilePtr; (wieder auf erstes Byte rückgesetzt versteht sich)
}


ISR(USART)
{ 
    // Diverse Staticvariablen
    // ....
    // static unsigned char* workingDataPtr;
    //
    // Warten bis bekannt ist wie lang das gerade empfangende Paket is
    workingDataPtr = (unsigned char*) malloc(FRAMELENGTH);

    // Wenn Filetransfer und ein korrektes Paket empfangen wurde
    // call FileTransfer(workingDataPtr);
}

Bei der nicht funktionierenden Variante geht alles was vor 
Implementierung des Filetransfers funktionierte ebenso. Nur sobald ich 
die Filetransferroutine aufrufe, überschreibt der dort allokierte 
Speicher (in workingFilePtr) diverse Staticvariablen in der ISR, der 
Filetransferroutine und auch globale Variablen, fast so, als wüsste die 
Funktion und daraus aufgerufenes malloc nicht, wo bereits Daten liegen.


Bin mit meinem Latein leider am Ende.


Bitte und Danke schonmal im Voraus!
Thomas

[Thomas Bergmüller (Gast)]

Im Anhang noch das Mapfile des gesamten Projekts...

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

Thomas Bergmüller schrieb:
> ich habe momentan ein Problem mit malloc(), das ich einfach nicht gelöst
> bekomme.
>
> Verwende es in einer Interruptroutine der USART am Atmega 88 bzw.
> Atmega32

Ick.  Ich bin gewiss der letzte, der dir von malloc() abraten würde,
schließlich habe ich die aktuelle avr-libc-Implementierung ja
geschrieben ;), aber von malloc() innerhalb einer ISR würde ich dir nun
wirklich abraten.  Erstens kann das Teil vergleichsweise lange laufen,
und zweitens musst du dir verdammt sicher sein, dass du keine race
condition bekommst: wenn du malloc() (oder free()) außerhalb der ISR
rufst, dann wird ein derartiger Aufruf vom Interrupt unterbrochen und
du rufst es in der ISR nochmal auf, dann ist der Teufel los.

[Thomas Bergmüller (Gast)]

Ich rufe es "quasi" nur innerhalb der ISR

Also die ISR ruft entweder direkt oder über einen weiteren 
Funktionsaufruf hinweg (der Rücksprung erfolgt aber wieder in die ISR) 
malloc / free auf.

Das ganze passiert nur, wenn ein Datenpaket fertig empfangen und geprüft 
wurde, und so lange wartet der Sender auf ein Acknowledge, das erst 
gesendet wird, wenn ich mit Malloc fertig bin. Erst dann können neue 
Daten kommen.

Einziges was mir auf die Schnelle einfällt was den Ablauf unterbrechen 
könnte, sind Störungen am Bus (RS485).

Bin aber natürlich auch für Alternativen offen, am ehesten wäre dann ein 
fixer Empfangspuffer mit den 262 Bytes und für die übertragene Dateien 
(die ja von einigen Bytes bis eben Datenspeicher halb voll variieren 
können) sinnvoll oder?

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

Fester Puffer in der ISR.  Wenn du dynamische Datenmengen verwalten
können willst, kann ja der main context dann die empfangenen Daten
anhand ihrer tatsächlichen Länge in einem malloc()-Puffer umkopieren.

Damit hättest du eine kurze und schnelle ISR, die halt genau ein
Datenpaket empfangen kann, danach muss sie warten, bis diese Daten
außerhalb verarbeitet worden sind.

Bitte benutze das malloc() aus der aktuellen avr-libc.  Die Versionen
vor avr-libc-1.7.0 (also beispielsweise die des derzeit letzten WinAVRs)
hatten teilweise noch gravierende Bugs im malloc().

[Peter D. (peda)]

Thomas Bergmüller schrieb:
> Also die ISR ruft entweder direkt oder über einen weiteren
> Funktionsaufruf hinweg (der Rücksprung erfolgt aber wieder in die ISR)
> malloc / free auf.

Dann macht malloc/free keinen Sinn. Das braucht man nur, wenn 
verschiedene Instanzen es ausführen, d.h. bei free die Instanz von 
malloc bereits beendet ist.
Ansonsten kannst Du einfach gleich ne lokale Variable mit der maximal 
benötigten Größe anlegen.

Ich würde allerdings vermeiden, im Interrupt riesige Monsterprotokolle 
zu parsen und sogar noch Callbacks auszuführen. Das Main und andere 
Interrupts haben dann sehr schlechte Karten, auchmal an die Reihe zu 
kommen.

Ich laß den UART-Interrupt nur alles in nen FIFO schmeißen und 
vielleicht noch das Ende des Frame erkennen. Die Auswertung macht dann 
das Main.
Der Sender kann sich die Größe des FIFO holen und ihn mit Frames füllen. 
Danach hat er auf Antwort zu warten, ehe er weiter sendet.


Peter

[Peter D. (peda)]

Jörg Wunsch schrieb:
> Die Versionen
> vor avr-libc-1.7.0 (also beispielsweise die des derzeit letzten WinAVRs)
> hatten teilweise noch gravierende Bugs im malloc().

Autsch!

Was macht man dann als WINAVR-Nutzer und nicht des Compilerbaus fähiger?
Gibt es Workarounds für den WINAVR?


Peter

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

Peter Dannegger schrieb:

> Was macht man dann als WINAVR-Nutzer und nicht des Compilerbaus fähiger?

Um eine Biblithek zu bauen, muss man nicht des Compilerbaus fähig
sein.  Es muss lediglich ein Compiler vorhanden sein.  Viel mehr
braucht man für die avr-libc nicht (sofern man nicht gerade noch die
Doku selbst bauen will).

Ansonsten kannst du dir allemal malloc.c und stdlib_private.h in
dein eigenes Projekt kopieren.

[Thomas Bergmüller (Gast)]

Ok danke werd ich machen..

hab die ISR selbst auf fixe Buffer umgebaut, funktioniert prächtig.

unsigned char devComSpace1[256];
unsigned char devComSpace2[256];

In diesen zwei Bereichen (Adresse 0x87 und 0x18e ) werden die Daten 
gespeichert, in einem Space stehen jeweils gültige Daten, der andere 
wird als Buffer benutzt, wenn gültige Daten drinstehen, werden einfach 
Pointer geswapt

Das malloc-Problem besteht aber weiterhin...

Untenstehender Code zeigt die Routine, die empfangene Pakete in ein File 
schreiben soll.

Sie kann entweder mit pReceive (Parameter 3) = 
DEVCOM_FILETRANSFER_ANNOUNCED oder DEVCOM_FILETRANSFER_RECEIVING 
aufgerufen werden, entsprechend gabelt sich dann der Programmweg.

beim Announce wird der Speicher allokiert und die gesamten 
Static-Variablen, die während des Filetransfers benötigt werden (also 
bis alle Pakete da sind)
initialisiert.

Aufgrund des Return-Werts wird wieder an den PC Rückmeldung gegeben.

Danach bekommt der uC die eigentlichen Dateidaten und baut diese 
Zusammen bis zum letzten Frame, da sollte er dann den 
global_validFilePtr auf den allokierten Speicher setzten und vorher den 
alten Speicher (wo schon eine Datei stand... ) freigeben.

Das Problem: Das Malloc in der unten gezeigten Funktion liefert einen 
Pointer auf einen Speicherbereich bei 0x78, was bei einer im Beispiel 
absolut einprogrammierten Sollgröße von 256 Bytes definitiv in den Space 
1 reinragt und auch diverse Variablen (unter anderen 
global_validFilePtr) überschreibt.

Ist jetzt noch mit der alten libc...

unsigned char devCom_fileReceive(unsigned char* pData, unsigned char pUpperBound, unsigned char pReceiveStatus)
  {
  
    unsigned char i = 0;
    unsigned long packageNumber = 0;
    static unsigned long packageAmount = 0;      // Anzahl der zu erwartenden Datenpakete
    static unsigned char* tmpFilePtr = NULL;      // Working-Dataptr
    static unsigned long lastPackageNumber = 0;    // Letztes empfangenes Paket (nummer)
    static unsigned char* tmpFilePtrSave = NULL;  // Zeigt immer auf erstes Byte des allokierten Speichers  


     packageNumber= ((unsigned long)pData[0] << 24) |  ((unsigned long)pData[1] << 16) | ((unsigned long)pData[2] << 8) | ((unsigned long)pData[3] & 0xff);
     

    // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Vorbereiten auf neues File / Prüfen von Speicherplatz usw.
    if(pReceiveStatus == DEVCOM_FILETRANSFER_ANNOUNCED)
    {
      // TODO: Check Filename, Speichere entsprechend
      
      lastPackageNumber = 0;
      packageAmount = packageNumber; // Speichere Paketnummer, beim Ankündigungsframe ist die Paketnummer die Anzahl der nachfolgenden Pakete

      // Speicher so allokieren, als währen alle ganze Pakete: TODO: Kleinere Speicher...    
      free(tmpFilePtrSave); // ggf. alten nicht benutzten Speicher freigeben..
      
      tmpFilePtr = (unsigned char*) malloc(256);
      tmpFilePtrSave = tmpFilePtr;

      // Prüfe ob genügend Speicher vorhanden war
      if(tmpFilePtr == NULL)
      {
        return(DEVCOM_FILETRANSER_ERROR_NOT_ENOUGH_MEM);
      }
      else
      {
        return(DEVCOM_FILETRANSFER_ERROR_NOERROR);
      }
    }
    // ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ FILE RECEIVING
    else
    {      
      if(packageNumber != (lastPackageNumber+1)) // Wenn es sich nicht um das erwartete nächste Paket handelt
      {
        return(DEVCOM_FILETRANSFER_ERROR_PACKAGE_NUMBER); // ABBRUCH!!
      }
      
      // Werte kopieren
      for(i = 0; i <= pUpperBound; i++) // hinter Rahmendaten platzieren
      {
        *tmpFilePtr = pData[i];                                // Nächstes Speicherelement
        tmpFilePtr++;
      }
  
      if(packageNumber >= packageAmount)
      {
        // File successfully received
         free((void*)global_validFilePtr);     // letzten Datensatz freigeben
        global_validFilePtr = tmpFilePtrSave;   // Datensatz aktualisieren
        global_newFileReceived = 1;
        global_lastReceivedFileSize = (unsigned long)((packageNumber-1)*252 + (unsigned long)i);      
        tmpFilePtrSave = NULL; 
      }    
      else
      {
        lastPackageNumber = packageNumber; // merken für nächsten Aufruf
      }
      
      // Wenn bis hierher ohne Return gekommen, war alles in Ordnung...
      return(DEVCOM_FILETRANSFER_ERROR_NOERROR);
    }  
  }

[Thomas Bergmüller (Gast)]

Debugfenster nach erfolgtem Dateitransfer von 256 Bytes (2 Pakete)

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

Thomas Bergmüller schrieb:

> Das Problem: Das Malloc in der unten gezeigten Funktion liefert einen
> Pointer auf einen Speicherbereich bei 0x78

Zeig mal die Kommandozeilen von Compiler und Linker, mit denen du
compiliert hast.

[Thomas Bergmüller (Gast)]

hm... ich weiß zwar nicht wieso sich der Fehler so äußerte aber es läuft 
jetzt...

es stand definitiv in tmpFilePtrSave ein falscher Wert, was ich jetzt 
geändert habe hat damit nicht viel zu tun ^^

hab die Zählvariable i als unsigned char deklariert, wenn man die 
natürlich bis <= 255 zählen lässt, baut man sich eine Endlosschleife... 
Ich nehme an wenn ich dann auch noch da immer einen Filepointer erhöhe, 
dann zerschieß ich mir den ganzen Speicher mit den sich immer 
wiederholenden Daten :S

Wie dem auch sei, vielen Dank für die Hilfe!!

[Ulrich P. (uprinz)]

Mal so ganz grundsätzlich:

In einer Funktion
{
- Paket mit malloc(size) allozieren
- Paket auf die Reise schicken (sprich be-/abarbeiten)
- Paket freigeben free(paket)
}

Im ISR
{
- IsrPtr = Paket (mit Kopie arbeiten)
- Nur IsrPtr für die Bearbeitung verwenden
}

Durch Semaphoren oder einfache überschaubare Mechanismen kann man beide 
Funktionen noch gegeneinander verriegeln:
- ISR beendet und deaktiviert sich ggf. selbst, wenn Paket==NULL
- ISR steuert ein (globales oder im Header des Pakets vorhandenes) Flag, 
welches der Funktion mitteilt, dass das Paket noch in Bearbeitung ist.
Durch diese Flags kann man die Funktion auch mehrfach aufrufen, sie 
meldet zurück, ob das Paket bearbeitet und freigegeben ist oder nicht.

Im thoretischen Beispiel des TO ist eine böse Falle drin, den hier 
werden global_validFilePtr und workingFilePtr nicht sauber gesetzt und 
free(global_validFilePtr) hat einen anderen Wert angenommen, als malloc 
für workingFilePtr herangezogen hatte.

Daher meine Aufforderung, immer nur mit Kopieen des Pointers zu 
arbeiten, dem per malloc() ein Speicher zugewiesen wurde und das 
Original wieder für free zu verwenden. Kostet ein paar Pointer im RAM, 
spart jede Menge Kopfschmerztabletten.

Gruß, Ulrich