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).
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
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)
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:
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
}
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 !
