Memory issues resolved?

Here is some code. I know it might seem more complicated than it needs to for the job its doing… but I am trying to get my head around how I work with memory in c++

Please critic? THANKS!

WebServer.pde

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

byte mac[] = {0xDE, 0xAD, 0xBE, 0xEF, 0xFE, 0xED};
byte ip[] = {192, 168, 1, 177};
Server server(80);

void InitEthernet()
{
  Ethernet.begin(mac, ip);
  server.begin();
}

void DoEthernet(void (*f)(Client, HTTPRequest))
{
  Client HTTPClient = server.available();
  unsigned int iInputIndex = 0;
  char cReceived[300];
  
  if (HTTPClient)
  {
    boolean currentLineIsBlank = true;
    
    while (HTTPClient.connected())
    {
      if (HTTPClient.available())
      {
        char c = HTTPClient.read();

        cReceived[iInputIndex] = c;
        iInputIndex++;

        if (c == '\n' && currentLineIsBlank)
        {
          HTTPRequest h; //should i do a free here and malloc at char cReceived[..]? Would need init to do malloc at beginning to stop memory error???
          h.Con();
          h.Parse(cReceived);
          f(HTTPClient, h);
          h.Decon();
          break;
        }

        if (c == '\n')
        {
          currentLineIsBlank = true;
        }
        else
        {
          if (c != '\r') { currentLineIsBlank = false; }
        }
      }
    }

    delay(1);
    HTTPClient.stop();
  }
}

void DoResponse(Client _HTTPClient, HTTPRequest _Request)
{
  Page xPage;
  xPage.Con();

  free(xPage.PageTitle);
  xPage.PageTitle = strdup("Title");

  free(xPage.Style);
  xPage.Style = strdup("body{font-family: Arial; font-weight: bold; background-color: #CC0000; color: black;} .title{font-size: 20px;}");

  free(xPage.BodyTitle);
  xPage.BodyTitle = strdup("Beakie Web Server");

  free(xPage.InnerHTML);
  xPage.InnerHTML = strdup(_Request.HTTPHeaderFieldKeys[0]);

  xPage.Render(_HTTPClient);
  
  xPage.Decon();
}

void setup()
{
  InitEthernet();
}

void loop()
{
  DoEthernet(DoResponse);
}

Code.h

int strlen(char* _String);
void AddNameValue(char* Key, char* Value, char*** KeyArray, char*** ValueArray, int* ArrayLength);
void FreeNameValueArray(char*** KeyArray, char*** ValueArray, int* ArrayLength);

struct HTTPRequest
{
public:
  char* URL;

  int HTTPHeaderFieldCount;
  char** HTTPHeaderFieldKeys;
  char** HTTPHeaderFieldValues;

  int QueryStringCount;
  char** QueryStringKeys;
  char** QueryStringValues;

  void Con()
  {
    URL = strdup("");

    HTTPHeaderFieldCount = 0;
    HTTPHeaderFieldKeys = (char**)malloc(0);
    HTTPHeaderFieldValues = (char**)malloc(0);

    QueryStringCount = 0;
    QueryStringKeys = (char**)malloc(0);
    QueryStringValues = (char**)malloc(0);
  };

  void Decon()
  {
    free(URL);
    FreeNameValueArray(&HTTPHeaderFieldKeys, &HTTPHeaderFieldValues, &HTTPHeaderFieldCount);
    FreeNameValueArray(&QueryStringKeys, &QueryStringValues, &QueryStringCount);
  };

  enum HTTPHeaderFieldParsePosition
  {
    POSITION_GET = 1,
    POSITION_KEY = 2,
    POSITION_VALUE = 3
  };

  void Parse(char* _Request)
  {
    AddNameValue("hMYKEY1", "hMYVALUE1", &HTTPHeaderFieldKeys, &HTTPHeaderFieldValues, &HTTPHeaderFieldCount);
    AddNameValue("qMYKEY1", "qMYVALUE1", &QueryStringKeys, &QueryStringValues, &QueryStringCount);
    AddNameValue("hMYKEY2", "hMYVALUE2", &HTTPHeaderFieldKeys, &HTTPHeaderFieldValues, &HTTPHeaderFieldCount);
    AddNameValue("qMYKEY2", "qMYVALUE2", &QueryStringKeys, &QueryStringValues, &QueryStringCount);
    AddNameValue("hMYKEY3", "hMYVALUE3", &HTTPHeaderFieldKeys, &HTTPHeaderFieldValues, &HTTPHeaderFieldCount);
    AddNameValue("qMYKEY3", "qMYVALUE3", &QueryStringKeys, &QueryStringValues, &QueryStringCount);
    AddNameValue("hMYKEY4", "hMYVALUE4", &HTTPHeaderFieldKeys, &HTTPHeaderFieldValues, &HTTPHeaderFieldCount);
    AddNameValue("qMYKEY4", "qMYVALUE4", &QueryStringKeys, &QueryStringValues, &QueryStringCount);
    return;
  }

};

struct Page
{
  public:

  char* PageTitle;
  char* Style;
  char* BodyTitle;
  char* InnerHTML;

  void Con()
  {
    PageTitle = strdup("");
    Style = strdup("");
    BodyTitle = strdup("");
    InnerHTML = strdup("");
  };

  void Decon()
  {
    free(PageTitle);
    free(Style);
    free(BodyTitle);
    free(InnerHTML);
  };

  void Render(Client _HTTPClient)
  {
    _HTTPClient.println("<html>");
    _HTTPClient.println("<head>");
    if (strlen(PageTitle) > 0)
    {
      _HTTPClient.print("<title>");
      _HTTPClient.print(PageTitle);
      _HTTPClient.println("</title>");
    }
    if (strlen(Style) > 0)
    {
      _HTTPClient.println("<style type=\"text/css\">");
      _HTTPClient.println(Style);
      _HTTPClient.println("</style>");
    }
    _HTTPClient.println("</head>");
    _HTTPClient.println("<body>");
    if (strlen(BodyTitle) > 0)
    {
      _HTTPClient.println("<p class=\"title\">");
      _HTTPClient.println(BodyTitle);
      _HTTPClient.println("</p>");
    }
    _HTTPClient.println(InnerHTML);
    _HTTPClient.println("</body>");
    _HTTPClient.println("</html>");
  }
};

Code.cpp

#include <Ethernet.h>

int strlen(char* _String)
{
  unsigned int iLoop = 0;
  while(true)
  {
    if (_String[iLoop] == 0)
    {
      return iLoop;
      break;
    }
    
    iLoop++;
  }
}

void AddNameValue(char* Key, char* Value, char*** KeyArray, char*** ValueArray, int* ArrayLength)
{
  char** tmpKeys = (char**)realloc(*KeyArray, (*ArrayLength + 1) * sizeof(char*));
  char** tmpValues = (char**)realloc(*ValueArray, (*ArrayLength + 1) * sizeof(char*));

  if (tmpKeys)
  {
    *KeyArray = tmpKeys;
  }
  if (tmpValues)
  {
    *ValueArray = tmpValues;
  }
//	if (!keys || !values)
//		return false;	// out of memory

  (*KeyArray)[*ArrayLength] = strdup(Key);
  (*ValueArray)[*ArrayLength] = strdup(Value);
  ++(*ArrayLength);
};

void FreeNameValueArray(char*** KeyArray, char*** ValueArray, int* ArrayLength)
{
  for(int i = 0; i < *ArrayLength; i++)
  {
    free(*KeyArray[i]);
    free(*ValueArray[i]);
  }
  free(*KeyArray);
  free(*ValueArray);
}

strlen is already written and debugged for you - why bother reinventing it?

AWOL: strlen is already written and debugged for you - why bother reinventing it?

Practise

I understand you might have written things different from an application point of view. I appreciate it might be overly complicated for the job it does. But, from a code point of view... how is my memory handling please?

You should try practising matching the return type to the type returned.

You say C++ (and the file extension seems to imply it), but what you are actually doing is pretty much C. So what do you actually want to do?

The code.cpp is pretty complex (all those triple pointers) and creating a neat class (actually 2, one for the pairs, one for a set of pairs) could clean that up and make it pretty much straight forward. It would also tell you more easily if you cleaned up memory.

BTW, realloc and strdup may clutter very much the heap, and you don't want to do that on the Arduino...

AWOL: You should try practising matching the return type to the type returned.

Could you please expand?

he means:

int foo() {
   unsigned int bar=0;
   return bar;
}

bar does not match the return type of the function foo() and being lax about such things might bug your code.

scjurgen:
The code.cpp is pretty complex (all those triple pointers) and creating a neat class (actually 2, one for the pairs, one for a set of pairs) could clean that up and make it pretty much straight forward. It would also tell you more easily if you cleaned up memory.

BTW, realloc and strdup may clutter very much the heap, and you don’t want to do that on the Arduino…

Thanks :slight_smile:

Could I please trouble you for an example?

scjurgen: he means:

int foo() {
   unsigned int bar=0;
   return bar;
}

bar does not match the return type of the function foo() and being lax about such things might bug your code.

Quite right. Human error :) Thanks

Beakie: Could I please trouble you for an example?

Well, I think you would be better of if you go through some C++/OOP tutorial. It is not really very much Arduino specific.

scjurgen: Well, I think you would be better of if you go through some C++/OOP tutorial. It is not really very much Arduino specific.

I wrote some code to do what I wanted... posted it on cplusplus.com and THAT function is what was recommended.

I seem to be getting stuck in this frustrating loop of understanding. I get conflicting advice every time I seem to get advice.

Some people advise I use malloc/free and some people tell me I shouldn't.

If I find tutorials they are seem too basic in program design. I want an HTTPRequest Object and a Page Object but I don't get how I should achieve it.

I am getting really confused about now

Some people advise I use malloc/free

Not many people here would advise the use of malloc/free - it isn't really appropraite on a microcontroller.

scjurgen:
You say C++ (and the file extension seems to imply it), but what you are actually doing is pretty much C.
So what do you actually want to do?

The code.cpp is pretty complex (all those triple pointers) and creating a neat class (actually 2, one for the pairs, one for a set of pairs) could clean that up and make it pretty much straight forward. It would also tell you more easily if you cleaned up memory.

BTW, realloc and strdup may clutter very much the heap, and you don’t want to do that on the Arduino…

What about this?

#include <string.h>

#define DICT_MAXELEMENTS 20
#define DICT_MAXKEYLEN 10
#define DICT_MAXVALUELEN 15

#define TRUE 1
#define FALSE 0
typedef int BOOL;

typedef struct DictEntry
{
	char key[DICT_MAXKEYLEN];
	char value[DICT_MAXVALUELEN];
} DictEntry;

static DictEntry dict[DICT_MAXELEMENTS];
static unsigned dict_size;

BOOL DictIsFull()
{
	return dict_size == DICT_MAXELEMENTS;
}

BOOL DictAddEntry(const char *key, const char *value)
{
	if (dict_size < DICT_MAXELEMENTS)
	{
		strncpy(dict[dict_size].key, key, DICT_MAXKEYLEN);
		strncpy(dict[dict_size].value, value, DICT_MAXVALUELEN);
		++dict_size;
		return TRUE;
	}

	return FALSE;
}

const char *DictLookupEntry(const char *key)
{
	unsigned i;
	for (i = 0; i != dict_size; ++i)
	{
		if (strncmp(dict[i].key, key, DICT_MAXKEYLEN) == 0)
			return dict[i].value;
	}

	return NULL;
}
#define TRUE 1
#define FALSE 0
typedef int BOOL;

Again, this isn't C; C++ has a "bool" data type, and "true" and "false" are reserved words.

AWOL: ```

define TRUE 1

define FALSE 0

typedef int BOOL; ```

Again, this isn't C; C++ has a "bool" data type, and "true" and "false" are reserved words.

Without these 3 lines... hows is that?

Just bought this...

http://www.amazon.co.uk/Effective-Specific-Addison-Wesley-Professional-Computing/dp/0321334876/ref=sr_1_14?ie=UTF8&qid=1300661317&sr=8-14

Just an observation about this:

#define DICT_MAXELEMENTS 20
#define DICT_MAXKEYLEN 10
#define DICT_MAXVALUELEN 15

typedef struct DictEntry
{
    char key[DICT_MAXKEYLEN];
    char value[DICT_MAXVALUELEN];
} DictEntry;

static DictEntry dict[DICT_MAXELEMENTS];

Already you have used up 500 bytes out of the 2048 bytes of RAM on your Arduino, assuming you have a Uno or similar. That's OK so far, but how much RAM does the Ethernet library need?

I understand your confusion about getting conflicting advice, but some advice about using malloc/free or new/delete may be given assuming you have lots of memory. And 2048 bytes isn't lots. And I wonder looking at the snippet above where that dictionary gets populated from? Text processing tends to be memory-intensive. For example, a forum page I just looked at had 96,554 bytes of HTML code. There was a case recently where someone's program hung due to some problem with malloc/free.

As for the triple pointers, I've been programming most of my life, and double pointers is as far as I usually go, and then reluctantly. It's confusing. Consider passing by reference rather than by pointer.