Probleme mit malloc() und free()

Hallo zusammen,

Ich habe mir für meinen Arduino Mega 2560 und das dafür verwendete GSM Modul Sim900 ein Unterprogramm geschrieben, um auf vorhandene SMSen zu überprüfen.
Im Unterprogramm verwende ich dynamischen Speicher, da ich nicht genau weiß wie groß die Antwort bzw. die Nachricht sein wird.

Ich habe jetzt das problem das alles ohne malloc() und free() klarerweise funktioniert. Um das ganze auch langzeitstabil zu machen habe ich malloc() und free() eingeführt.
Verwende ich nur malloc() um den Speicher zu reservieren funktioniert alles weiterhin einwandfrei.
Schreibe ich jedoch auch free() dazu am Ende des Unterprogramms wird das Unterprogramm ausschließlich zwei mal ausgeführt, nicht öfters. Ich schätze es gibt ein Speicherproblem.

Stundenlanges suchen im internet mit unzähligen Möglichkeiten dyn. Speicherverwaltung zu lösen brachten leider kein Ergebnis.
Habe bereits versucht nur den Speicherbereich des Pointer *txt freizugeben, da ich mir dachte, dass durch die Funktion strstr() ja der zweite Pointer *s auf *txt zeigt und somit nur *txt freigegeben werden darf. Zu häufiges oder mehrmaliges aufrufen von free() führt laut Literatur ebenfalls zu Problemen.

Hier mal das Unterprogramm, ich hoffe ihr hättet eine Idee oder irgendeinen Ansatz woher das problem stammen könnte:

Hier der Aufruf im Hauptprogramm:

void loop()
{
  int index = 0;

  if( (index = IsSMSAvailable()) > 0 )
  {
    MEGASerial.print("Ausgabe Hauptprog Unread SMS = ");
    MEGASerial.println(index);
    delay(100);
  }
  
  delay(50000);
}

Und hier das betroffene Unterprogramm, wobei mir readLineISSMSAvailable(GSMSerial) immer eine ganze Zeile der Antwort des GSM Moduls liefert, bis \n kommt. Bei erneutem Aufruf wird die nächste Zeile ermittelt, bis eben keine Nachricht mehr vom GSM Modul ansteht.
READ_BUFFER_SIZE_READLINE wurde als globale Variable mit const unsigned int READ_BUFFER_SIZE_READLINE = 151; deklariert.

int IsSMSAvailable()
{
  /*
   *  17:34:19.605 -> +CMGL: 1,"REC READ","+43660658xxxx","","2019/10/23 17:12:30+08"
      17:34:19.673 -> Neuuuu
      17:34:19.741 -> 
      17:34:19.741 -> +CMGL: 7,"REC UNREAD","+43660658xxxx","","2019/10/23 17:09:48+08"
      17:34:19.808 -> Testtt

   */
  
  int repeatswitch = 1;
  int index;
  int i;
  state_t state = SendCMD; 

  char *txt;
  char *s;

  txt = malloc(sizeof(*txt)*READ_BUFFER_SIZE_READLINE);
  s = malloc(sizeof(*s)*READ_BUFFER_SIZE_READLINE);
  
  MEGASerial.println("- - - Funktion IsSMSAvailable - - -");

  if( (txt != NULL) && (s != NULL) )
  {
    
  while(repeatswitch)
  {
  switch( state ) 
  {
    case SendCMD:
      GSMSerial.write("AT+CMGL=\"REC UNREAD\",1\r");
      state = ReadLine;
      repeatswitch = 1;
      break;
 
    case ReadLine:
      txt = readLineISSMSAvailable(GSMSerial); 
      //Gibt ganze Zeile aus
      //17:34:19.605 -> +CMGL: 1,"REC READ","+43660658xxxx","","2019/10/23 17:12:30+08"
      
      if (txt != nullptr)
      {
        if(strcmp(txt, "\0") != 0) //Wenn im String nur \0 steht, soll nichts am SerialMonitor ausgegeben werden, erst bei richtige Daten
        {
          MEGASerial.print("Empfangen in Unread: ");
          MEGASerial.println(txt);
        }
        if(strstr(txt, "+CMGL:") != 0) //String gefunden im Buffer
        {
          s = strstr(txt, ":");
          if(s != 0)
          {
            index = atoi(s+2);
            MEGASerial.print("Index Nr.: "); 
            MEGASerial.println(index);
            repeatswitch = 0;
            
            flushGSMSerial();
            free(txt); 
            //free(s);
            return index;
          }
        }
        else if(strstr(txt, "OK\r\n") != 0) //Wenn Strings ident
        {
          MEGASerial.println("OK - Keine neue Nachrichten!");
          repeatswitch = 0;

          flushGSMSerial();
          free(txt); 
          //free(s);
          return false;
        }
        else if(strstr(txt, "ERROR") != 0)  //Falls ERROR ausgegeben wird versuche nochmals CMD zu senden 
        {
          state = SendCMD;
          repeatswitch = 1;

          flushGSMSerial();
          MEGASerial.println("ERROR Unread");
        }
        else if(strstr(txt, "UNDER_VOLTAGE") != 0) 
        {
          repeatswitch = 1;
          MEGASerial.println("UNDER_VOLTAGE");
        }
        else if(strstr(txt, "OVER_VOLTAGE") != 0) 
        {
          repeatswitch = 1;
          MEGASerial.println("OVER_VOLTAGE");
        }
      }
      else
      {
        repeatswitch = 1;
      }
      break;
  }
  }
  }
  else
  {
    MEGASerial.println("Achtung: Kein virtueller RAM mehr verfügbar!");
  }
  free(txt); 
//free(s);
}

Die dynamische Speicherverwaltung ist bei den kleinen (8 Bit) Controllern bekannt schlecht. U.a. liegt das daran, daß bei dem kleinen verfügbaren Speicher die Fragmentierung ein unüberwindliches Problem darstellen kann. Vermutlich ist das bei Dir auch der Fall, wenn Probleme nach free() auftreten.

Zur Abhilfe feste Puffer ausreichender Größe verwenden und auf malloc/free verzichten.

Beim Mega kann man noch externes RAM anschließen, es bliebe aber auszuprobieren, ob das mit malloc/free besser bzw. überhaupt funktioniert.

Der Mega hat 6kB RAM. Da kann man problemlos einen festen Puffer verwenden. Du musst auch nicht den ganzen Text einlesen, sondern immer nur eine Zeile

Thor2018:
Um das ganze auch langzeitstabil zu machen habe ich malloc() und free() eingeführt.

Damit erreichst Du genau das Gegenteil.

Ich verstehe nicht, wozu das malloc() / free() hier überhaupt nützlich sein soll. Wenn Du am Anfang der Funktion einen Speicher mit fester Länge anforderst, und am Ende der Funktion wieder freigibst, kannst Du gleich eine entsprechende lokale Variable anlegen. Der Speicherplatz wird am Ende der Funktion auch wieder freigegeben, und da gibt es keine Fragmentierung.

So ganz komme ich mit deiner Funktion auch noch nicht klar:

Hier

txt = malloc(sizeof(*txt)*READ_BUFFER_SIZE_READLINE);

setzt Du den Pointer txt auf den angeforderten Speicherbereich.

und hier:

txt = readLineISSMSAvailable(GSMSerial);

überschreibst Du den Pointer doch mit dem Ergebnis deiner Funktion readLineISSMSAvailable(GSMSerial). Damit ist doch dein Zeiger auf den Speicherbereich weg, oder? Wie willst Du den dann noch nutzen oder gar wieder freigeben?
Wie ist die readLine-Funktion denn definiert? Was gibt die denn da zurück? Ich vermute einen Pointer auf die Zeile, die aber ganz woanders gespeichert ist.

Serenifly:
Der Mega hat 6kB RAM. ...

Ich erinnere mich daß er 8kB RAM hat.
Grüße Uwe

Das wäre ja ein ganzer UNO Unterschied :wink:

Gruß Tommy

Kein Wunder, daß Der so viele Beinchen hat ... gleich vier Uno auf ein Mal :open_mouth:

:wink:

Gruß Tommy

Danke für eure Antworten!
Das ist ein sehr guter und wichtiger Hinweis, dass die dynamische Speicherverwaltung ist bei den kleinen (8 Bit) Controllern bekanntlich schlecht ist und generell zu wenig RAM besitzt, Danke!

Das heißt ich werde das ganze mit fixer Speicherplatzzuweisung machen.
Bin ich auf den richtigen Pfad unterwegs wenn ich hier anstatt Zeiger ein fixes Array verwende? Wobei Zeiger u Array ja sowieso dasselbe bzw. verwandt sind.

Der ursprüngliche Code für die "Unterunterfunktion" readLineISSMSAvailable(GSMSerial) ist folgender (Danke an Serenifly!)

char* readLineISSMSAvailable(Stream& stream)
{
  /* Typische Antwort auf AT+CMGL="REC UNREAD",1:
   *  17:44:08.902 -> +CMGL: 1,"REC UNREAD","+436606xxxxxx","","2019/10/18 10:34:58+08"
   *  17:44:08.970 -> Neu10
   *  17:44:09.038 -> 
   *  17:44:09.038 -> +CMGL: 2,"REC UNREAD","+436606xxxxxx","","2019/10/18 10:36:52+08"
   *  17:44:09.140 -> Neu11
   */
   
  static byte index;
  static char buffer[READ_BUFFER_SIZE_READLINE];

  while (stream.available())
  {
    char c = stream.read(); //Auslesen der Antwort bis \n kommt, sonst weiter inkrementieren

    if (c == '\n')          //wenn LF eingelesen
    {
      buffer[index] = c;      //Schreibe \n hinein
      buffer[index+1] = '\0';   //String terminieren nach \n mit \0
      index = 0;
      return buffer;        //melden dass String fertig eingelesen wurde und zurückgeben
    }
    else if (index < READ_BUFFER_SIZE_READLINE - 2)   //Solange noch Platz im Puffer ist 
    {
      buffer[index++] = c;    //Zeichen abspeichern und Index inkrementieren
    }
  }
  return nullptr;           //noch nicht fertig, Null-Pointer zurückgeben
}

Habe bereits die pointer durch Arrays ersetz, leider bekomme ich hier eine Fehlermeldung, ich schätze es liegt am Parameter welcher übergeben wird, im speziellen an Stream.
Hierbei erhalte ich folgende Meldung:

Und der angepasste Unterunterprogramm Code (wahrscheinlich fehlerhaft bei Stream)

char readLineISSMSAvailable(Stream& stream)
{
  /* Typische Antwort auf AT+CMGL="REC UNREAD",1:
   *  17:44:08.902 -> +CMGL: 1,"REC UNREAD","+436606xxxxxx","","2019/10/18 10:34:58+08"
   *  17:44:08.970 -> Neu10
   *  17:44:09.038 -> 
   *  17:44:09.038 -> +CMGL: 2,"REC UNREAD","+436606xxxxxx","","2019/10/18 10:36:52+08"
   *  17:44:09.140 -> Neu11
   */
   
  static byte index;
  static char buffer[READ_BUFFER_SIZE_READLINE];

  while (stream.available())
  {
    char c = stream.read(); //Auslesen der Antwort bis \n kommt, sonst weiter inkrementieren

    if (c == '\n')          //wenn LF eingelesen
    {
      buffer[index] = c;      //Schreibe \n hinein
      buffer[index+1] = '\0';   //String terminieren nach \n mit \0
      index = 0;
      return buffer;        //melden dass String fertig eingelesen wurde und zurückgeben
    }
    else if (index < READ_BUFFER_SIZE_READLINE - 2)   //Solange noch Platz im Puffer ist 
    {
      buffer[index++] = c;    //Zeichen abspeichern und Index inkrementieren
    }
  }
  return false;           //noch nicht fertig, Null-Pointer zurückgeben
}

Wieso legst du nochmal global ein Array an? Und sogar zwei. Eins reicht doch.

Das Array wird intern verwaltet. Du bekommst einen Zeiger auf das Array zurück. Entsprechend musst du das in einem char* abspeichern

So:

char* txt = readLine(....)

Und der angepasste Unterunterprogramm Code (wahrscheinlich fehlerhaft bei Stream)

Das hast du vollständig verstümmelt. Wieso soll die Funktionen char zurückgeben? Wenn du char einem Array zuweist kommt klar ein Fehler. Das ist nicht verwunderlich

Man kann das Array auch außerhalb global verwalten. Dann muss man aber bool als Rückgabewert machen und true/false zurückgeben um das Ende zu melden. Das ist bei deiner Verschlimmbesserung nicht konsequent und da hast eine Vermischung der zwei Prinzipien.
char als Rückgabewert. buffer zurück um das Ende zu melden. false ansonsten. Das sind drei verschiedene Dinge die überhaupt nicht zueinanderpassen

Dann eben das Array einmal intern und dann nochmal global.

Wenn du nicht verstehst was du machst dann solltest du die Funktion auch nicht ändern

Fehlermeldungen sind auch Texte, die Du kopieren und in Codetags einfügen kannst.

Gruß Tommy

Das zweite Array habe ich angelegt um bei folgender Anweisung

s = strstr(txt, ":");

im Unterprogram IsSMSAvailable() den Pointer/Array txt nicht zu überschreiben, da ich nachfolgende if-Anweisungen mit dem originalen txt durchführen möchte.

Kann ich hier einfach wieder einen Pointer mit

char* txt = readLine(....)

verwenden? Das ist doch kein fix zugewiesener Speicherplatz was doch früher oder später zu Problemen führen kann oder? Theoretisch mit malloc lösbar, hier wie bereits festgestellt wurde am Arduino Mega leider nicht.

Den Zusammenhang zwischen Arrays und Zeigern hast du dann doch nicht verstanden. Das hat sich erst anders angehört.

Ein Zeiger zeigt auf etwas. Er ist eine Adresse auf eine Speicherzelle. Eine Array Variable zeigt auf den ersten Index in einem Array.

Du kannst problemlos sowas haben:

int array[10];
int* ptr = &array[5];  //oder: array + 5

Dann hast du einen Zeiger auf den 5. Index und kannst darüber die Zahl darin ansprechen

Das ist ja das schöne bei den ganzen C String Funktionen. Die liefern Zeiger auf Positionen in dem Array mit dem sie arbeiten. So wird nie extra Speicher benötigt. Du bekommst keine neuen Objekte mit Kopien des Teil-Strings wie bei der String Klasse. Die Algorithmen arbeiten in situ.

Und wieso geht das bei readLine()? Wegen dem static:

static char buffer[READ_BUFFER_SIZE_READLINE];

Statische Variablen behalten ihren Wert von Aufruf zu Aufruf. Wie globale Variablen ist das immer gültig. So kann die Funktionen einen Zeiger auf das Array liefern. Bei nicht-statischen Variablen ist es dagegen tödlich Zeiger auf lokale Variablen zurückzugeben

im Unterprogram IsSMSAvailable() den Pointer/Array txt nicht zu überschreiben, da ich nachfolgende if-Anweisungen mit dem originalen txt durchführen möchte.

strstr() liefert einen Zeiger auf die erste Position des Teil-Strings. Oder NULL wenn der Such-String nicht vorhanden ist. Den Zeiger kannst du keinem Array zuweisen!

Wenn du wirklich eine Kopie eines C String brauchst musst du das mit strcpy()/strncpy() machen. Aber mir scheint du hast dich da sowieso etwas verrannt, weil du nicht verstehst was diese Funktionen wirklich machen

s = strstr(txt, ":");

strstr() ist eigentlich eher dazu da nach ganzen Teil-Strings zu suchen. Für einzelne Zeichen gibt es strchr()

Super Danke für deine Erklärungen! Die waren sehr hilfreich um den unterschied zu verstehen.

Die string Library benutze ich deshalb weil es für mich augenscheinlich erstmal etwas einfacher wirkt mit den Strings zu hantieren. Eben +CMGL: beispielsweise im String zu erkennen und dann weitere Abläufe zu setzen. Würde wahrscheinlich ebenso mit einer einfachen while-Schleife funktionieren nur ist es etwas schneller und einfacher über die Stringlibrary.

Das mit der globalen Variable und txt funktioniert ebenfalls prima, habe ich leider erst danach gesehen, dass ich keinen zusätzlichen Pointer *s benötige.

Verstanden habe ich die Funktionen der String-library nur mit den zeigern bin ich noch nicht ganz Sattelfest.

Das bringt mich noch zu einer anderen Frage, könnte hier ein Speicherproblem bei den Pointern auftauchen da keine Speicherreservierung vorgenommen wurde?
Oder könnte man mit

txt = readLineISSMSAvailable(GSMSerial);

bereits eine Art Reservierung bezeichnen bzw. eine Zuweisung zu einem gültigen Wert/gültige Adresse sodass kein fehler mehr auftreten kann?

Und vor allem brauche ich am Anfang des Unterprogramms keine Initialisierung des Zeigers? Also diesen statt

char *txt;

auf Null zeigen zu lassen?

char *txt = NULL;

Du wirst mit der Klasse String im Dauerbetrieb nicht lange Freude haben, da sie den Speicher stark fragmentiert, auch wenn sie Dir einfacher erscheint.
Infos zu Zeichenketten gibt es hier.

Gruß Tommy

Er redet zwar von einer 'String Library'. Aber so wie es aussieht meint er nicht die String-Klasse, sondern die Standard libc string.h .
Bei der Pointergeschichte fehlt aber offensichtlich noch ein Grundverständnis, was für die Verwendung von C-Strings aber essentiell ist.

Ja genau ich meine die Standard C Bibliothek string.h.

@Tommy56, herrscht hier ebenso ein Speicherproblem bzw. starke Fragmentierung? Da ich gesehen habe du verwendest in dem von dir verlinkten Link ebenso die string.h Bibliothek bei Punkt 1.
Danke für den link, werde ich mir mal Aufmerksam durchlesen.
Dauerbetrieb wäre für mich sehr wichtig, da ich leider immer nach ca. 1 Tag Probleme bekomme, dass meine Steuerung nicht mehr funktioniert bzw. es den Anschein hat als ob der Zustand eingefroren wäre und nichts mehr passiert, weder im Programmablauf noch am SerialMonitor.

Werde mal die genannten Verbesserungen in deinem Link einpflegen und heute noch einen Dauerbetrieb starten - wäre gespannt ob dann alles funktioniert.
Hatte zuvor eine fertige Bibliothek verwendet um bestimmte Funktionen wie SMS senden, empfangen etc. durchzuführen. Klappte leider nicht dauerhaft, deswegen die selbstgeschriebenen Unterprogramme mit der Standard-Bibliothek string.h.

Du musst unterscheiden zwischen string (kleines s), das sind die Funktionen für char-Arrays. Ich verwende dafür lieber den Begriff Zeichenketten.
Und die Klasse String (großes S), die Du bisher verwendest. Diese fragmentiert den RAM.

Gruß Tommy

avr-gcc unterstützt absichtlich nicht die c++ std::string Klasse auf den 8bit-Controllern. Arduino hat, als zweifelhaften Ersatz, dafür die String Objekte erfunden.

Insgesamt besser fährt man aber damit, nur char* und feste text-Arrays zu verwenden.
Den RAM-Speicherplatz braucht man nun mal, und es macht keinen Sinn, den RAM der für eine empfangene SMS erforderlich ist, zeitweise für etwas anderes zur Verfügung zu haben (ein "Bildschirm-Schoner" Spielchen, wenn nichts zu tun ist, etwa?)

Wenn man sich also um den Speicher selbst kümmert, sollte man zwischen dem Definieren und Speicherplatz-Belegen (z.B. mit char msg[160]; ) und Zeigern darauf, wie in

  char* s =strstr(...);

unterscheiden. All diese c-Funktionen str...-Funktion liefern Zeiger auf anderweitig bereits definierte Speicherbereiche zurück.

Mit etwas Übung und ein bisschen Nachdenken kommt man ganz gut mit diesen char* - Zeigern zurecht. Zumal man sich hier nicht um unbekannte / variable Größen eines char kümmern muss.
sizeof (char) ist immer 1. Dein sizeof (*txt) war eher unnötig verwirrend, finde ich.

Zeiger bei der Definition undefiniert zu lassen, sollte man sich übrigens erst gar nicht angewöhnen, auch wenn es erlaubt ist.

Thor2018:
Das mit der globalen Variable und txt funktioniert ebenfalls prima, habe ich leider erst danach gesehen, dass ich keinen zusätzlichen Pointer *s benötige.

Du kannst auch die Funktion so wie ursprünglich verwenden. Auf globale Variablen zu verzichten und die Variablen so weit wie möglich lokal zu machen hat auch seine Vorteile. Dann hat man keinen Zugriff auf das Array an Stellen die eigentlich gar nichts damit zu tun haben. Ich wurde hier auch schon angemeckert, weil ich das in früheren Versionen oft global machte :stuck_out_tongue:

Bei einem globalen Array musst du halt wie gesagt den Rückgabewert richtig machen (mit bool) um zu melden ob das Einlesen schon fertig ist oder nicht

Und vor allem brauche ich am Anfang des Unterprogramms keine Initialisierung des Zeigers?

Das kommt immer auf den Kontext an. Generell werden lokale, nicht-statische Variablen nicht automatisch initialisiert und haben daher willkürliche Werte. Globale und statische Variablen sind automatisch 0. Aber es kann auch sein dass man weiß dass sie vor dem ersten Lesen mit etwas beschrieben wird.