Problem with arrays of strings

I’m having a problem manipulating arrays of string. I’m working on an app that needs to send SMS messages, but if the SMS can’t be sent (i.e. no network coverage) then the message needs to be saved to an array to be sent later when the coverage is better. If the array of messages gets full, then I need to discard the oldest message, and add a new one to the ‘new’ end of the array (using ‘addMessage’ and ‘leftShiftArray’). When there is network coverage, the most recent message in the array will be delivered, and the sent message will be removed from the array (using rightNullArray). All the variables and arrays are global. Code snippets are attached.

All of these code snipets work most of the time, but normally after a few iterations (maybe a dozen) either I spot some corruption in the array contents, or it hangs at the line “sms_list[sms_list_size]=strdup(curr_mess);” in addMessage. I’m not sure if I’m using the ‘free’ command correctly, the code didn’t work at all until I added it, but I’ve since learnt from other postings on the forum that ‘free’ should only be used to free memory allocated using ‘memalloc’. Any advice would be appreciated.

Kind Regards

const int sms_len = 160;                            //maximum allowable SMS length
char sms_mess[sms_len+1];                  //full sms message 
const int max_sms_list_size = 10;           //number of unsent SMSs that can be stored
char *sms_list[max_sms_list_size+1]; //array of messages, as yet un-transmitted
char curr_mess[sms_len+1];                       //current sms.

void addMessage()
{
  //add a new message to the array. Records will be added as they arrive from left to right.
  //therefore the rightmost fix should always be the most recent
  
  if (sms_list_size == max_sms_list_size)
  {
    //The array of fixes is already at maximum, therefore discard the oldest fix 
    //to make room for the newest.
    leftShiftArray();
  }

  //add fix to array
  sms_list[sms_list_size]=strdup(curr_mess);
  sms_list_size++;  //increment the counter that records the number of SMSs waiting to be transmitted.
}

void leftShiftArray()
{
  //shift the entire contents of the SMS array left one element. The leftmost element will be lost and
  //a space will be freed up on the righthand side
  free(sms_list[0]);                     //frees up the memory for the 1st (i.e. leftmost element in the array)
  for (int n=0; n<=sms_list_size-1; n++)
  {
    sms_list[n]=sms_list[n+1];
  }
  sms_list[sms_list_size]=0;             //frees up the pointer to the memory
  sms_list_size--;                       //decrement the counter that records the number of fixes waiting to be transmitted.
  sms_lost++;                            //increment the counter that records those SMS's lost due to lack of buffer space
}


void rightNullArray()
{
  //Set the rightmost element (i.e. the most recent message) of the message array to null. This is only used after a 
  //message has been succesfully sent and the space needs to be freed up from the array.

  free(sms_list[sms_list_size-1]); //free up the memory
  sms_list[sms_list_size-1]=0;     //delete/reset the pointer to the memory
  sms_list_size--;                 //decrement the size counter
}

Check your array boundaries carefully, every array seems to have a ...+1 size while not allways needed.

Could you post your complete code?

Furthermore, the fact that strdup() hangs could be caused by not enough free memory to allocate the string. You don't test the return value of strdup(), see http://www.opengroup.org/onlinepubs/009695399/functions/strdup.html and some moments later you could be sending a nullpointer SMS. Definitely the shortest message possible ;)

You could allocate 10 * 160(+1) bytes for the 10 messages and use strcpy instead strdup() & free(). Far less dynamic memory overhead, a bit slower but more robust (in theory).

Rob

Thanks Rob, The arrays are sized with a 'plus 1' to remind me to leave extra space for a null terminator. I'm not sure if it's needed for the pointer array. The full code is too large to fit here, that's why I've just included the edited highlights. I've been using MemoryFree.h (found somewhere on this forum I think) to check for free memory, I'm working with a Mega and appear to have over 5k free so I don't think that's a problem. I'm checking the free memory every iteration, and once the array is full the memory doesn't change. Other than occasionaly hanging, the only visible sign of trouble is occasional corruption (1 or 2 weird characters) in the data in the array. Can anyone confirm if the 'free' command is approriate here? I've read elsewhere that it's only for use after allocating memory explicity using the malloc() comand

Regards

Goodmorning Lima7,

The forum can hold quite an amount of code especially when using # button or the [ code] [/code] tags but when it is stripped to a working version that still contains the bug, it easiest to debug. (often the stripping process itself brightens the mind)

From http://stackoverflow.com/questions/252782/strdup-what-does-it-do-in-c: Strdup() is effectively doing the same as the following code:

char *strdup (const char *s) {
    char *d = (char *)(malloc (strlen (s) + 1)); // Space for length plus nul
    if (d == NULL) return NULL;                  // No memory
    strcpy (d,s);                                // Copy the characters
    return d;                                    // Return the new string
}

So the use of free() seems right to me (no guarantee)

and once the array is full the memory doesn’t change

That’s not true as when SMS’s are sent free() deallocates memory so you get more mem again, there is definitely dynamics in the code. That’s why I proposed to allocate fixed buffers with no free() and malloc() / strdup() . If this alternative buffer implementation gives the same problems the chances that the cause of the trouble lies outside the buffer increase dramatically.

Another problem that might be in the code is when curr_mess == NULL. Don’t know if it can happen but addMessage doesn’t test fotr it:

void addMessage()
{
  if (sms_list_size == max_sms_list_size)
  {
    leftShiftArray();
  }
  sms_list[sms_list_size]=strdup(curr_mess);  [glow]<== HERE[/glow]
  sms_list_size++;  
}

Hi Rob,

I’ve taken your advice and produced a working sketch to demonstrate the problem. It should be possible to run this on any arduino, I’ve removed all reference to custom hardware (GPS, GSM module etc).

It’s currently failing on the line 'sms_list[sms_list_size]=strdup(curr_sms); ’ in addMessage, although this has in the past worked.

const int sms_len = 160;             //maximum allowable SMS length
const int max_sms_list_size = 4;     //number of unsent SMSs that can be stored
char sms_mess[sms_len+1];            //full sms message (header, timestamp, location fixes and checksum)
char *sms_list[max_sms_list_size];   //array of messages, as yet un-transmitted
char curr_sms[sms_len+1];            //current list of fixes.
int mess_seq_num=0;                  //message sequence number
char mess_seq_str[4+1];              //message sequence number expressed as a string
const int max_mess_seq_num=9999;     //maximum message sequence number (before reseting back to 0)
char mess_text[]="MyMessge-";

int sms_list_size=0;                 //actual size of sms_list. (number of populated elements)
long sms_lost=0;                     //counts number of messages lost because the buffer was full.

// random number
long randNumber=0;

void setup()
{
  Serial.begin(115200);//Communication speed for the display
  Serial.println("Starting.........");
}

void loop()
{

  //create the current message
  createMess();
  
  //display the current message
  Serial.print("Current message is <");Serial.print(curr_sms);Serial.println(">");

  //add the message to array of messages
  addMessage();

  //display the whole list
  Serial.print("Whole SMS array follows....."); Serial.print("sms_list_size=<");Serial.print(sms_list_size);Serial.println(">");
  for (int n=0; n<=max_sms_list_size-1; n++)
  {
    Serial.print(n); Serial.print("  ");
    Serial.println(sms_list[n]);
  }

  //When signal strength is good enough, the most recent message should be sent and removed from the
  //array. Simulate this with a one in 10 chance of success.....
  int randNumber = random(10);
  if (randNumber == 1)
  {
    //Remove the rightmost (i.e. the most recent) message from the array.
    rightNullArray();
  }


  //slow things down a bit...
  delay(1000);

}


bool createMess()
{
  int message_size=1;
  
  sprintf(mess_seq_str,"%.4d",mess_seq_num);    //format the sequence number as a four digit string with leading zeros
  
  curr_sms[0]=0; //reset/clear the current message
  
  //create a message 
  for (int i=0; i<message_size ;i++)
  {
    strcat(curr_sms,mess_text);
    strcat(curr_sms,mess_seq_str);
  }

  //increment or reset the counter
  if (mess_seq_num < max_mess_seq_num)
  {
    mess_seq_num++;
  }
  else
  {
    mess_seq_num=0;
  }
}


void addMessage()
{
  //add a new message to the array. Records will be added as they arrive from left to right.
  //therefore the rightmost fix should allways be the most recent
  
  if (sms_list_size == max_sms_list_size)
  {
    //The array of messsages is already at maximum, therefore discard the oldest fix 
    //to make room for the newest.
    leftShiftArray();
  }

  //add fix to array
  Serial.print("about to insert message <");Serial.print(curr_sms);Serial.print("> into array index <");Serial.print(sms_list_size);Serial.println(">");
  sms_list[sms_list_size]=strdup(curr_sms);  //copy the current message into the array of messages
  sms_list_size++;                           //increment the counter that records the number of SMSs waiting to be transmitted.
  free(curr_sms);                            //not sure if this is required or not. (some help files suggest only need 'free' after using 'memalloc')
  //curr_sms[0]=0;
}


void leftShiftArray()
{
  //shift the entire contents of the SMS array left one element. The leftmost element will be lost and
  //a space will be freed up on the righthand side
  free(sms_list[0]);                     //frees up the memory for the 1st (i.e. leftmost element in the array)
  for (int n=0; n<=sms_list_size-1; n++)
  {
    sms_list[n]=sms_list[n+1];
  }
  sms_list[sms_list_size]=0;             //frees up the pointer to the memory
  sms_list_size--;                       //decrement the counter that records the number of fixes waiting to be transmitted.
  sms_lost++;                            //increment the counter that records those SMS's lost due to lack of buffer space
}


void rightNullArray()
{
  //Set the rightmost element of the message array to null. 

  free(sms_list[sms_list_size-1]); //free up the memory
  sms_list[sms_list_size-1]=0;     //delete/reset the pointer to the memory
  sms_list_size--;                 //decrement the size counter
}

Ran your code => Snippet of the output

Whole SMS array follows.....sms_list_size=<1>
Wh_le SMS array follows.....sms_list_size=<2>
WhOle SMS array follows.....sms_list_size=<3>
Wh?le SMS array follows.....sms_list_size=<4>
Wh?le SMS array follows.....sms_list_size=<4>

There is a string overwritten as Whole is written in 4 ways … So maybe the \0 at the end of a string is missing. I’ll dive into it.

One fatal error is the following call: free(curr_sms);

curr_sms is a static array declared at the top and not allocated by malloc()


bool createMess() does not return a value, that means there can be stack corruption. => make it void.

addMessage() index will go out of bound if sms_list_size == max_sms_list_size

void addMessage() 
{
  if (sms_list_size == max_sms_list_size) leftShiftArray();
  sms_list[sms_list_size] = strdup(curr_sms);  
}

void rightNullArray() does not check if there are messages in the list, so index used may be out of bound.

So there are a number of things to fix. have a look at http://www.arduino.cc/cgi-bin/yabb2/YaBB.pl?num=1265230949 && http://code.google.com/p/alexanderbrevig/downloads/detail?name=SimpleFIFO_1-2.zip&can=2&q=

Thanks Rob, seems to work now. Not sure exactly which bit of tinkering fixed it, but I’ve ended up making several minor changes. Also added a call to a memory checking routine just to confirm that the memory leaks are being cured. It’s run for a couple of hundred iterations now so I think we’ve nailed it. Thanks Rob

Mike

(working code included if you’re interested)

#include <MemoryFree.h>


const int sms_len = 160;             //maximum allowable SMS length
const int max_sms_list_size = 12;     //number of unsent SMSs that can be stored
char sms_mess[sms_len+1];            //full sms message (header, timestamp, location fixes and checksum)
char *sms_list[max_sms_list_size];   //array of messages, as yet un-transmitted
char curr_sms[sms_len+1];            //current sms.
int mess_seq_num=0;                  //message sequence number
char mess_seq_str[4+1];              //message sequence number expressed as a string
const int max_mess_seq_num=9999;     //maximum message sequence number (before reseting back to 0)
char mess_text[]="MyMessge-";

int sms_list_size=0;                 //actual size of sms_list. (number of populated elements)
long sms_lost=0;                     //counts number of messages lost because the buffer was full.

// random number
long randNumber=0;

void setup()
{
  Serial.begin(115200);//Communication speed for the display
  Serial.println("Starting.........");
}

void loop()
{

  Serial.println();Serial.print("freeMemory reports <");Serial.print( freeMemory() );Serial.println("> (bytes) free");

  
  //create the current message
  createMess();
  
  //display the current message
  Serial.print("Current message is <");Serial.print(curr_sms);Serial.println(">");

  //add the message to array of messages
  addMessage();

  //display the whole list
  Serial.print("Whole SMS array follows....."); Serial.print("sms_list_size=<");Serial.print(sms_list_size);Serial.println(">");
  for (int n=0; n<=max_sms_list_size-1; n++)
  {
    Serial.print(n); Serial.print("  ");
    Serial.println(sms_list[n]);
  }

  //When signal strength is good enough, the most recent message should be sent and removed from the
  //array. Simulate this with a one in 10 chance of success.....
  int randNumber = random(10);
  if (randNumber == 1)
  {
    //Remove the rightmost (i.e. the most recent) message from the array.
    rightNullArray();
  }


  //slow things down a bit...
  delay(1000);

}


void createMess()
{
  int message_size=4;
  
  sprintf(mess_seq_str,"%.4d",mess_seq_num);    //format the sequence number as a four digit string with leading zeros
  
  curr_sms[0]=0; //reset/clear the current message
  
  //create a message 
  for (int i=0; i<message_size ;i++)
  {
    strcat(curr_sms,mess_text);
    strcat(curr_sms,mess_seq_str);
  }

  //increment or reset the counter
  if (mess_seq_num < max_mess_seq_num)
  {
    mess_seq_num++;
  }
  else
  {
    mess_seq_num=0;
  }
}


void addMessage()
{
  //add a new message to the array. Records will be added as they arrive from left to right.
  //therefore the rightmost fix should allways be the most recent
  
  if (sms_list_size == max_sms_list_size)
  {
    //The array of messsages is already at maximum, therefore discard the oldest fix 
    //to make room for the newest.
    leftShiftArray();
  }

  //add fix to array
  Serial.print("about to insert message <");Serial.print(curr_sms);Serial.print("> into array index <");Serial.print(sms_list_size);Serial.println(">");
  sms_list[sms_list_size]=strdup(curr_sms);  //copy the current message into the array of messages. Note this uses malloc, and will therefore
                                             //increase the amount of memory being used.
  sms_list_size++;                           //increment the counter that records the number of SMSs waiting to be transmitted.
  curr_sms[0]=0;                             //clear / reset the current SMS message
}


void leftShiftArray()
{
  //shift the entire contents of the SMS array left one element. The leftmost element will be lost and
  //a space will be freed up on the righthand side

  free(sms_list[0]);                     //frees up the memory for the 1st (i.e. leftmost element in the array). Note - this
                                         //should free up the same amount of memory taken up with the strdup command earlier.
                                         //so there should be no net increase in memory usage.
  sms_list[0]=0;
  for (int n=0; n<=sms_list_size; n++)
  {
    sms_list[n]=sms_list[n+1];
  }
  sms_list[sms_list_size-1]=0;           //frees up the pointer to the 'newest' element in memory
  sms_list_size--;                       //decrement the counter that records the number of fixes waiting to be transmitted.
  sms_lost++;                            //increment the counter that records those SMS's lost due to lack of buffer space
}


void rightNullArray()
{
  //Set the rightmost element of the message array to null. 
  
  if (sms_list_size >= 1)            //Check index will be within bounds
  {
    free(sms_list[sms_list_size-1]); //free up the memory
    sms_list[sms_list_size-1]=0;     //delete/reset the pointer to the memory
    sms_list_size--;                 //decrement the size counter
  }
}

Good to hear its running, but thats no guarantee it is free of bugs yet. If time permits I will have a look at the new code tomorrow. Think it will be a good thing to put the whole message buffer in a separate class, so it will be reusable, can be tested seperately etc. I hope to hear when the final version of the project is ready. Maybe you could write an article for the playground !

Rob