Memory Leak?

Hi All,

I'm currently working on controlling an Arduino over the internet with an XBox 360 controller. I have a working ActiveX Object that can poll the state of the controller, format it into a message, send it to the Arduino and wait on a response. The ActiveX isn't perfect yet as it's requiring me to relax my internet security settings. Still working on it. Anyway, the formatted message would look like this...
LX:0|LY:0|RX:0|RY:0|LT:255|RT:255|AB:0|BB:0|XB:0|YB:0|RS:0|LS:0|DU:0|DD:0|DL:0|DR:0|ST:0|BK:0|LJ:0|RJ:0|BG:0
This successfully arrives at the Arduino and is parsed into individual variables. For testing purposes, the sketch is currently taking the trigger values (LT & RT) and sending them back to the web page as vibration values for the left & right vibrating motors. The sketch may iterate the loop 100 times before it seizes. My assumption is a memory leak but I don't know. I'm very new to C++. Coming from a web programming backround (PHP, javascript) I'm finding the way C++ handles variables totally baffling. After some googling, I learned that memory held by variables is released when the function they were declared in loses focus. I tried to arrange my loops and functions to facilitate this but it's still seizing.

My hope is that it's just going to jump out at someone. Without further ado...

My Code:

#include <SPI.h>
#include <Ethernet.h>
#include <string.h>

byte mac[] = { 0x90, 0xA2, 0xDA, 0x0D, 0x07, 0xBC };
IPAddress ip(192,168,1, 111);

EthernetServer server(12345);
EthernetClient client;

int iterations = 0;

void setup() 
{
  Serial.begin(9600);
  Ethernet.begin(mac, ip);
  server.begin();
}

void theState() 
{
  //  GamePad State Variables
  int LX = 0; int LY = 0;
  int RX = 0; int RY = 0;
  int LT = 0; int RT = 0;
  bool AB = false; bool BB = false; bool XB = false; bool YB = false;
  bool RS = false; bool LS = false;
  bool DU = false; bool DD = false; bool DL = false; bool DR = false; 
  bool ST = false; bool BK = false;
  bool LJ = false; bool RJ = false;
  bool BG = false;
  
  char c;
  char theState[128] = "";

  int LV = 0;
  int RV = 0;
  float PS = 0;  
  int i = 0;
  c = client.read();
  while (c != '^')
  {
    theState[i] = c;
    c = client.read();
    i++;
  }
  // Parse the GamePad State
  char *p = theState;
  char *str;
  while ((str = strtok_r(p, "|", &p)) != NULL)
  {
    char *q = str;
    char *var;
    int j = 0;
    String _name;
    String _value;
    while ((var = strtok_r(q, ":", &q)) != NULL)
    {
      //char _var[8] = {char(*var)};
      //_var = char(*var);
      if (j == 0) _name = var; else _value = var;
      j++;
    }
    char name[8];
    _name.toCharArray(name, sizeof(name));
    char value[8];
    _value.toCharArray(value, sizeof(value));
    //Serial.println("Name: " + name);
    //Serial.println("Value: " + value);
    
    if (strcmp(name, "LX") == 0){LX = atoi(value);}      
    if (strcmp(name, "LY") == 0){LY = atoi(value);}
    if (strcmp(name, "RX") == 0){RX = atoi(value);}
    if (strcmp(name, "RY") == 0){RY = atoi(value);}
    if (strcmp(name, "LT") == 0){LT = atoi(value);}
    if (strcmp(name, "RT") == 0){RT = atoi(value);}
    if (strcmp(name, "AB") == 0){AB = (value == "1") ? true : false;}
    if (strcmp(name, "BB") == 0){BB = (value == "1") ? true : false;}
    if (strcmp(name, "XB") == 0){XB = (value == "1") ? true : false;}
    if (strcmp(name, "YB") == 0){YB = (value == "1") ? true : false;}
    if (strcmp(name, "LS") == 0){LS = (value == "1") ? true : false;}
    if (strcmp(name, "RS") == 0){RS = (value == "1") ? true : false;}
    if (strcmp(name, "DU") == 0){DU = (value == "1") ? true : false;}
    if (strcmp(name, "DD") == 0){DD = (value == "1") ? true : false;}
    if (strcmp(name, "DL") == 0){DL = (value == "1") ? true : false;}
    if (strcmp(name, "DR") == 0){DR = (value == "1") ? true : false;}
    if (strcmp(name, "ST") == 0){ST = (value == "1") ? true : false;}
    if (strcmp(name, "BK") == 0){BK = (value == "1") ? true : false;}
    if (strcmp(name, "LJ") == 0){LJ = (value == "1") ? true : false;}
    if (strcmp(name, "RJ") == 0){RJ = (value == "1") ? true : false;}
    if (strcmp(name, "BG") == 0){BG = (value == "1") ? true : false;}
    
  }
  // Set Pins with Variables
  LV = LT; // Proof of Concept...  Triggers controlling vibration
  RV = RT; 
  
  // Read Any Sensors & Assemble the Response
  PS = 14.05;
  char buffer[10];
  String _PS = dtostrf(PS, 5, 2, buffer); 
        
  String _LV = String(LV);
  String _RV = String(RV);
  String _response = "|LV:" + _LV + "|RV:" + _RV + "|PS:" + _PS;
  char response[50];
  _response.toCharArray(response, 50);
  
  // send the response
  client.write(response);
  
  iterations++; 
  Serial.print(iterations);
  Serial.print(" ");
  Serial.println(response);
}

void loop()
{
  client = server.available();
  if (client) 
  {
    if (client.connected() && client.available()) 
    {  
      theState();
    }
  }
  client.flush();
  client.stop();
  delay(1);
}

Any help at all would be greatly appreciated. If anyone is interested in the ActiveX Object and client-side javascript, I'm more than willing to share.

Thanks,

Don

My hope is that it's just going to jump out at someone.

Sure. Your problem is spelled String. Lose the String class.

    while ((var = strtok_r(q, ":", &q)) != NULL)

Do you know the difference between the complicated, large footprint strtok_r() function and the easy to use, smaller footprint strtok() function?

The _r version is thread safe. Do you really think thread safety is an issue on the Arduino, where there is only a single thread?

You could probably parse that in a state machine. Example here:

Other than that, I agree with PaulS. First, there is a bug in dynamic memory allocation which hasn't been fixed yet. Second, even if it was, you are probably fragmenting memory.

Paul,
Thank you for the rapid response! To answer your question, I don't know the difference. My goal was to parse that string into the respective variables and that was the first thing I came across that worked. That was after hours of compile errors about not being able to convert from one type to another or losing precision. It was a frustrating day. I can format that string any way I like before it's passed to the Arduino. I'm certain there's a cleaner way to get the same end result and I'm completely open to suggestions. Is the strtok() function not part of string.h?

thanks,

don

Nick,

Thank you for this. I'm thinking I could incorporate your example into my sketch. I'm off to study it.

Thanks again!

don

My goal was to parse that string into the respective variables and that was the first thing I came across that worked.

The strtok() function can do that, if properly used.

char someData[] = "LX:0|LY:0|RX:0|RY:0|LT:255|RT:255|AB:0|BB:0|XB:0|YB:0|RS:0|LS:0|DU:0|DD:0|DL:0|DR:0|ST:0|BK:0|LJ:0|RJ:0|BG:0";

char *name = strtok(someData, ":");
while(name)
{
   char *valu = strtok(NULL, "|");
   if(valu)
   {
      // Do something with name and valu

      name = strtok(NULL, ":");
   }
   else
      name = NULL;
}

Paul,

That was beautiful. Elegant even. I wasn't comfortable with the inital code mainly because I didn't understand it. Here's the section as it stands now with your input...

int i = 0;
  c = client.read();
  while (c != '^')
  {
    theState[i] = c;
    c = client.read();
    i++;
  }
  // Parse the GamePad State
  char *name = strtok(theState, ":");
  while(name)
  {
    char *value = strtok(NULL, "|");
    //Serial.print("Name: ");
    //Serial.print(name);
    //Serial.print("\r\nValue: ");
    //Serial.print(value);
    //Serial.print("\r\n");
    
    if (strcmp(name, "LX") == 0){LX = atoi(value);}      
    if (strcmp(name, "LY") == 0){LY = atoi(value);}
    if (strcmp(name, "RX") == 0){RX = atoi(value);}
    if (strcmp(name, "RY") == 0){RY = atoi(value);}
    if (strcmp(name, "LT") == 0){LT = atoi(value);}
    if (strcmp(name, "RT") == 0){RT = atoi(value);}
    if (strcmp(name, "AB") == 0){AB = (value == "1") ? true : false;}
    if (strcmp(name, "BB") == 0){BB = (value == "1") ? true : false;}
    if (strcmp(name, "XB") == 0){XB = (value == "1") ? true : false;}
    if (strcmp(name, "YB") == 0){YB = (value == "1") ? true : false;}
    if (strcmp(name, "LS") == 0){LS = (value == "1") ? true : false;}
    if (strcmp(name, "RS") == 0){RS = (value == "1") ? true : false;}
    if (strcmp(name, "DU") == 0){DU = (value == "1") ? true : false;}
    if (strcmp(name, "DD") == 0){DD = (value == "1") ? true : false;}
    if (strcmp(name, "DL") == 0){DL = (value == "1") ? true : false;}
    if (strcmp(name, "DR") == 0){DR = (value == "1") ? true : false;}
    if (strcmp(name, "ST") == 0){ST = (value == "1") ? true : false;}
    if (strcmp(name, "BK") == 0){BK = (value == "1") ? true : false;}
    if (strcmp(name, "LJ") == 0){LJ = (value == "1") ? true : false;}
    if (strcmp(name, "RJ") == 0){RJ = (value == "1") ? true : false;}
    if (strcmp(name, "BG") == 0){BG = (value == "1") ? true : false;}
    
    name = strtok(NULL, ":");
  }

I commented out string.h as well. Unfortunately, it's still seizing. The first trial got me 114 iterations of the loop. The second only 62. There's always the possibility that the problem could be the ActiveX Object or the javascript in the web page as well. Either way, I'm tickled to have that old code out of there.

Thank you again,

don

One thing that should be noted is that strtok() expects a string. What you are passing it is not a string.

A string is an array of chars (which you have) that is NULL terminated (which you do not do.

Try:

  while (c != '^')
  {
    theState[i++] = c;
    theState[i] = '\0'; // NULL terminate the array
    c = client.read();
  }

Sure. Your problem is spelled String. Lose the String class.

Problem solved!

Unfortunately, it's still seizing.

"Your problem is spelled String." Guess not! :slight_smile:

If I'm understanding you correctly, shouldn't the code look like this...

while (c != '^')
  {
    theState[i] = c;
    c = client.read();
    i++;
  }
theState[i] = '\0';

which would make only the last item in the array a NULL?

I prefer to keep the array NULL terminated at all times. You do as you wish.

This is a good example of why not to do big string processing on a small MCU.

This is the message coming in over serial:

LX:0|LY:0|RX:0|RY:0|LT:255|RT:255|AB:0|BB:0|XB:0|YB:0|RS:0|LS:0|DU:0|DD:0|DL:0|DR:0|ST:0|BK:0|LJ:0|RJ:0|BG:0

If all you did was write a sketch that watched each letter come in, add it to a character array and counted micros until the next one and compare that to the total run time to get the message in that way, you might be amazed at how much idle time you'd be looking at.

Simple fact, the characters come in 1 at a time and 42 of those are delimiters.

You can process each part of the message as it comes in without using more than a 4-byte buffer and no commands that use string.h whatsoever. You can do that -faster- than the message comes in. I have already gotten one member here over that hurdle and his message strings from his IRDA are over 110 bytes long.

As you process each part you can be sending pieces of the return message and put the vibration motors into action. The interval is going to be short enough you shouldn't notice but just to make it shorter, try at least 57600 baud serial or good old 115200 which is over 10x the 9600 you use.

Nick Gammon hinted at the process by telling you State Engine, btw.

You might use something like below with your incomming strings to handle each data set individully. Try using serial monitor and your incomming string.

//zoomkat 3-5-12 simple delimited ',' string parce 
//from serial port input (via serial monitor)
//and print result out serial port
// CR/LF could also be a delimiter

String readString;

void setup() {
  Serial.begin(9600);
  Serial.println("serial delimit test 1.0"); // so I can keep track of what is loaded
}

void loop() {

  //expect a string like wer,qwe rty,123 456,hyre kjhg,
  //or like hello world,who are you?,bye!,
  
  if (Serial.available())  {
    char c = Serial.read();  //gets one byte from serial buffer
    if (c == '|') {
      //do stuff
      Serial.println(readString); //prints string to serial port out
      readString=""; //clears variable for new input      
     }  
    else {     
      readString += c; //makes the string readString
    }
  }
}

Hi guys,

Sorry for the tardiness. It took a while to put the time together to try the state machine and change the ActiveX dll that sends the data to the Arduino. The data now arrives like this... A0B255C0D0E1F1... etc. I thought about the state machine code and how you have the opportunity to do other things while the data is still coming in a byte at a time. I decided I wanted all variables set before I acted on any of them and for that reason I altered the loop to look like this...

void loop ()
{
  client = server.available();
  if (client)
  {
    Serial.println("Client Connected");
    while (client.connected() && client.available())
    {
      processIncomingByte(client.read());
    }
    applyVariables();
    char* response = getResponse();
    client.write(response);
    client.flush();
    client.stop();
    Serial.println("\r\nClient Disconnected");
  }
}

It seems ok as long as I'm manually sending a single controller state at a time. The Arduino seizes within a few iterations of the loop if I'm looping the client. There's a loop in the javascript that polls the state of the gamepad and only if it's different from the last state, is it sent to the Arduino. If you were to set the controller down, it's state would remain unchanged and the Arduino would be waiting for a client to connect. I'm not sure if blocking and non-blocking are terms that need to be brought up here or not but I'm thinking I have it set up so that there's no way the client can send another set of data while the Arduino is still dealing with the last one. the code from the
ActiveX dll that communicates with the Arduino looks like this...

        private string ArduinoConnect(string IPAddress, Int32 Port, string theState)
        {
            Socket client = new Socket(AddressFamily.InterNetwork, SocketType.Stream, ProtocolType.Tcp);
            System.Net.IPAddress ipAdd = System.Net.IPAddress.Parse(IPAddress);
            System.Net.IPEndPoint arduinoEP = new System.Net.IPEndPoint(ipAdd, Port);
            client.Connect(arduinoEP);
            byte[] _theState = System.Text.Encoding.ASCII.GetBytes(theState);
            client.Send(_theState);
            byte[] _response = new byte[128];
            int iRx = client.Receive(_response);
            string response = System.Text.Encoding.ASCII.GetString(_response, 0, iRx);
            client.Close();
            return response;
        }

Prior to this, the dll code looked like this and reacted in exactly the same way ...

        private string ArduinoConnect(string IPAddress, Int32 Port, string theState)
        {
            string responseData = String.Empty;
            try
            {
                theState += "\0";
                TcpClient client = new TcpClient(IPAddress, Port);
                Byte[] data = System.Text.Encoding.ASCII.GetBytes(theState);
                NetworkStream stream = client.GetStream();
                stream.Write(data, 0, data.Length);
                //Console.WriteLine("Sent: {0}", data);
                data = new Byte[64];
                Int32 bytes = stream.Read(data, 0, data.Length);
                responseData = System.Text.Encoding.ASCII.GetString(data, 0, bytes);
                //Console.WriteLine("Received: {0}", responseData);
                Thread.Sleep(100);
                stream.Close();
                client.Close();
            }
            catch (ArgumentNullException e)
            {
                responseData = "ArgumentNullException: " + e;
                //Console.WriteLine(responseData);
            }
            catch (SocketException e)
            {
                responseData = "SocketException: " + e;
                //Console.WriteLine(responseData);
            }
            //Console.WriteLine("\n Press Enter to Continue...");
            //Console.Read();
            return responseData;
        }
*/

At least in my newbie mind, it looks like the dll sends the data then waits for a response. The response is returned to the javascript and only when there's a state change, is the next message sent to the Arduino. On the Arduino side, I'm thinking it's the same way. Nothing happens until the entire incoming message is read. Then the variables are acted upon, a response is created and sent back to the dll. Again, a newbie's perspective. Unfortunately, I'm still at a loss. Until I can solve the communication issue, I can't build the evil robot of my dreams.

Again, any help at all would be greatly appreciated.

Thanks again,

don

    applyVariables();
    char* response = getResponse();

I'm a little doubtful about this. Can you show all your code?

Sure...

#include <SPI.h>
#include <Ethernet.h>

byte mac[] = { 0x90, 0xA2, 0xDA, 0x0D, 0x07, 0xBC };
IPAddress ip(192,168,3, 111);

EthernetServer server(12821);
EthernetClient client;

typedef enum { NONE, _LX, _LY, _RX, _RY, _LT, _RT, _AB, _BB, _XB, _YB, _LS, _RS, _ST, _BK, _DU, _DD, _DL, _DR, _LJ, _RJ, _BG } states;
states state = NONE;
unsigned int currentValue;
int _currentValue;

//  GamePad State Variables
int LX = 0; int LY = 0;
int RX = 0; int RY = 0;
int LT = 0; int RT = 0;
bool AB = false; bool BB = false; bool XB = false; bool YB = false;
bool RS = false; bool LS = false;
bool DU = false; bool DD = false; bool DL = false; bool DR = false; 
bool ST = false; bool BK = false;
bool LJ = false; bool RJ = false;
bool BG = false;

 //  Response Variables
int LV = 0;
int RV = 0;
float PS = 0;  

void setup()
{
  Serial.begin (115200);
  state = NONE;
  Ethernet.begin(mac, ip);
  server.begin();
  Serial.println("Debugging...");
}

void applyVariables()
{
  LV = LT;
  RV = RT;
}

char* getResponse()
{
  //  Gather sensor data here
  PS = 13.25;
  char buff[10];
  String _PS = dtostrf(PS, 5, 2, buff);
  String _LV = String(LV);
  String _RV = String(RV);
  String _response = "|LV:" + _LV + "|RV:" + _RV + "|PS:" + _PS + "\0";
  char response[50];
  _response.toCharArray(response, 50);
  return response;
}

void handlePreviousState()
{
  switch (state)
  {
    case _LX:
      _currentValue = currentValue - 255;
      LX = _currentValue;
      break;
    case _LY:
      _currentValue = currentValue - 255;
      LY = _currentValue;
      break;
    case _RX:
      _currentValue = currentValue - 255;
      RX = _currentValue;
      break;
    case _RY:
      _currentValue = currentValue - 255;
      RY = _currentValue;
      break;
    case _LT:
      LT = currentValue;
      break;
    case _RT:
      RT = currentValue;
      break;
    case _AB:
      AB = (currentValue == 1) ? true : false;
      break;
    case _BB:
      BB = (currentValue == 1) ? true : false;
      break;
    case _XB:
      XB = (currentValue == 1) ? true : false;
      break;
    case _YB:
      YB = (currentValue == 1) ? true : false;
      break;
    case _LS:
      LS = (currentValue == 1) ? true : false;
      break;
    case _RS:
      RS = (currentValue == 1) ? true : false;
      break;
    case _ST:
      ST = (currentValue == 1) ? true : false;
      break;
    case _BK:
      BK = (currentValue == 1) ? true : false;
      break;
    case _DU:
      DU = (currentValue == 1) ? true : false;
      break;
    case _DD:
      DD = (currentValue == 1) ? true : false;
      break;
    case _DL:
      DL = (currentValue == 1) ? true : false;
      break;
    case _DR:
      DR = (currentValue == 1) ? true : false;
      break;
    case _LJ:
      LJ = (currentValue == 1) ? true : false;
      break;
    case _RJ:
      RJ = (currentValue == 1) ? true : false;
      break;
    case _BG:
      BG = (currentValue == 1) ? true : false;
      break;
  }
  //Serial.write(state);
  //Serial.print(": ");
  Serial.print(currentValue);
  Serial.print(":");
  currentValue = 0;
}

void processIncomingByte (const byte c)
{
  //Serial.write(c);
  if (isdigit (c))
  {
    currentValue *= 10;
    currentValue += c - '0';
  }
  else
  {
    handlePreviousState();
    switch (c)
    {
      case 'A':
        state = _LX;
        break;
      case 'B':
        state = _LY;
        break;
      case 'C':
        state = _RX;
        break;
      case 'D':
        state = _RY;
        break;
      case 'E':
        state = _LT;
        break;
      case 'F':
        state = _RT;
        break;
      case 'G':
        state = _AB;
        break;
      case 'H':
        state = _BB;
        break;
      case 'I':
        state = _XB;
        break;
      case 'J':
        state = _YB;
        break;
      case 'K':
        state = _LS;
        break;
      case 'L':
        state = _RS;
        break;
      case 'M':
        state = _ST;
        break;
      case 'N':
        state = _BK;
        break;
      case 'O':
        state = _DU;
        break;
      case 'P':
        state = _DD;
        break;
      case 'Q':
        state = _DL;
        break;
      case 'R':
        state = _DR;
        break;
      case 'S':
        state = _LJ;
        break;
      case 'T':
        state = _RJ;
        break;
      case 'U':
        state = _BG;
        break;
    }
  }
}

void loop ()
{
  client = server.available();
  if (client)
  {
    Serial.println("Client Connected");
    while (client.connected() && client.available())
    {
      processIncomingByte(client.read());
    }
    applyVariables();
    char* response = getResponse();
    client.write(response);
    client.flush();
    client.stop();
    Serial.println("\r\nClient Disconnected");
  }
}
char* getResponse()
{
...
  char response[50];
  _response.toCharArray(response, 50);
  return response;
}

This isn't valid. You can't return a pointer to a variable inside a function like that.


Also all those Strings:

  String _PS = dtostrf(PS, 5, 2, buff);
  String _LV = String(LV);
  String _RV = String(RV);
  String _response = "|LV:" + _LV + "|RV:" + _RV + "|PS:" + _PS + "\0";

If that doesn't fragment memory, I don't know what will.

Newman!

I have to tell you that I've done more googling of compile errors involving variables than anything else on my journey into the C++ world. Baffling. I'd be up for reading a primer on variables in C++ if you happened to have a decent link.

And thanks again for taking the time.

don

The word to look up in that case is 'scope'.

GoForSmoke:
The word to look up in that case is 'scope'.

Or we could just tell him.

When you do this:

char *myFunc()
{
  char junk[50];

  // blah blah blah

  return junk; // BAD BAD BAD
}

You are allocating an array called junk on "the stack." Without getting into the details of how a function is called, the stack is some memory that exists only while the function is executing. When the function returns, the stack is no longer guaranteed to be there. It may be. It may not be. It may be sometimes and not other times. For more details, google "function calling conventions" or something similar if you want much more detail.

If you want to do what you were doing, you need to pass the pointer into the function. Good practice is that the space be allocated one way or another from outside the function:

char myCharArray[50]; // Allocate an array
strcpy(myCharArray, "ABCDEFG"); // Stuff something into it - this will be null terminated

// Pass a pointer to the array down to a function so the array can be processed.
myFunc(myCharArray); // omiting the [] leaves you with a pointer to the first byte of the array

void myFunc(char *pStr)
{
  // pStr now is a pointer to your array

  // Manipulate your char array here

  pStr[0] = 'A'; // or whatever you want to do

  // blah blah blah
}

Fair warning, I have not passed this through a compiler and its pretty late / early for me here, but you should get the idea.