ESP8266, char array/String compare and memory leak questions

Using Arduino Programming Questions
1

Hello, first of all thanks for helping me out!

I'm working on a small project using a ESP8266 module (which will act as input for an Arduino which does more stuff).

One problem I have is when comparing the received strings with a certain string, this doesn't work. I guess it's some rookie mistake, would be nice if some one had some tips...
In "checkNewData()" you can see how I read the data, in "doJob()" you find the comparisons...(Data is received properly in the Serial monitor and printed normally). I also tried C++ syntax with == instead of strcmp, this also didn't work.

#include <stdio.h>

#include "ESP8266WiFi.h"

#define MAX_CLIENTS 10
#define MAX_LINE_LENGTH 64
#define PORT 80
#define AP_SSID "HIDDEN"
#define PASSWORD "ALSOHIDDEN"

#define LED_OUTPUT 2
//#define PUMPE_OUTPUT 0





 
const char* ssid = AP_SSID;
const char* password =  PASSWORD;
WiFiServer wifiServer(PORT);
int led = 2;
//int pumpe = PUMPE_OUTPUT;


WiFiClient* clients[MAX_CLIENTS] = {NULL};
char buffers[MAX_CLIENTS][MAX_LINE_LENGTH] = {0};
int newData[MAX_CLIENTS] = {0};
 
void setup() {
  pinMode(led, OUTPUT);
  //pinMode(pumpe, OUTPUT);
  Serial.begin(115200);
 
  delay(1000);
 
  WiFi.begin(ssid, password);
 
  while (WiFi.status() != WL_CONNECTED)
  {
    delay(1000);
    Serial.println("Connecting..");
  }
 
  Serial.print("Connected to WiFi. IP:");
  Serial.println(WiFi.localIP());
 
  wifiServer.begin();
}


void checkNewClients()
{
    WiFiClient newClient = wifiServer.available();
    if (newClient) 
    {
      Serial.println("New client connected");
      // Find the first unused space
      for (int i=0; i < MAX_CLIENTS; ++i) 
      {
          if (clients[i] == NULL) 
          {
              clients[i] = new WiFiClient(newClient);
              break;
          }
       }
    }
}

void checkNewData()
{
    memset(newData, 0, MAX_CLIENTS);
    for (int i = 0 ; i < MAX_CLIENTS ; ++i) 
    {
        if (clients[i] != NULL && clients[i]->available())
        {
            newData[i] = 1;
            memset(buffers[i], 0, MAX_LINE_LENGTH); //reset the old buffer
            int index = 0;
            while (clients[i]->available())
            {
                char current = clients[i]->read();
                if(index < MAX_LINE_LENGTH - 1)//-1 to leave space for '\0'
                  buffers[i][index] = current;
                index++;
            }
            buffers[i][index] = '\0';
            //DEBUG
            Serial.println(buffers[i]);
            //END DEBUG
        }
    }
}

void doJob()
{
    for (int i=0 ; i < MAX_CLIENTS ; ++i)
    {
        if(newData[i])
        {
            if(strcmp(buffers[i], "LED OFF") == 0) //THIS DOESN'T WORK
            {
               
                   digitalWrite(led, LOW); // Turn the LED off
                   Serial.println("LED OFF");
            }
            if(strcmp(buffers[i], "LED ON") == 0) //THIS DOESN'T WORK
            {
               
                   digitalWrite(led, HIGH); // Turn the LED on
                   Serial.println("LED ON");
            }
        }
    }
}

void checkDisconnections()
{
     for (int i=0 ; i<MAX_CLIENTS ; ++i)
     {
            if (clients[i])
            {
               if(!clients[i]->connected())
               {
                  
                  clients[i]->stop();
                  //delete(clients[i]); --> I should expect this is necessary, but Arduino crashes
                  clients[i] = NULL; //--> This should be a memory leak, as the object clients[i] still exists..., but as said delete results in a crash
                  if(clients[i] == NULL)
                    Serial.println("Client was deleted");
                  else
                    Serial.println("Client was not deleted");
               }
            }
     }
}



void doWifiJob()
{
    checkNewClients();
    checkNewData();
    doJob();
    checkDisconnections();
}


void loop() 
{
 
  doWifiJob();
  delay(1000);


}

The second problem I have is being unsure about a potential memory leak:
In checkNewClients(), a new client is created and added to the list of clients if a TCP connection is set up. In checkDisconnections() I want to remove the clients which disconnected, making space for other clients. Now I thought I must remove the WiFiClient object there from memory with "delete"...but if I try this, this results in a crash on runtime when a client disconnects (Stack status is printed in Serial Monitor and ESP8266 reboots). Please advise what to do...when I just set the nullpointer it works of course, but it feels wrong to me.

2 
            int index = 0;
            while (clients[i]->available())
            {
                char current = clients[i]->read();
                if(index < MAX_LINE_LENGTH - 1)//-1 to leave space for '\0'
                  buffers[i][index] = current;
                index++;
            }
            buffers[i][index] = '\0';

That's not correct. Index keeps incrementing as long as the client sends data, and then you write the null character completely out of bounds.

You should never manually use new and delete in your code. They should only be used by library implementers. Raw pointers (such as WiFiClient *) should never own any allocated memory, they can only point to objects that are owned by someone else.
If you need to dynamically allocate objects, use std::unique_ptr and std::make_unique (C++14). This makes memory leaks impossible and shouldn't crash the Arduino (unless there are other problems in your code, of course).

#include <memory>

std::unique_ptr<WiFiClient> clients[MAX_CLIENTS] = {};

...

          if (!clients[i]) {
              clients[i] = std::unique_ptr<WiFiClient>(new WiFiClient(newClient));
              // in C++14 and higher, use:
              //     clients[i] = std::make_unique<WiFiClient>(newClient);
              break;
          }

...

                  clients[i]->stop();
                  clients[i].reset(); // destructs the client and frees the memory

Pieter

3

OK I see I made a mistake with my increment of index, should be inside the if...
And I will try it with std::unique_ptr and report.

Thanks a lot!

4

OK std::unique_ptr works...thanks a lot!

Also strcmp works now, after printing each char as a Hex in Serial Monitor, I saw that the client also send a 0x0D at the end (which is carriage return '\r'). I had expected maybe a new line or a new line and cr, but not only a cr...I now just ignore cr (and will probably add other special chars as well)

Here is the repaired code, I think the mistake with the index is fine now, if not please give me an alert :slight_smile:

#include <memory>

#include "ESP8266WiFi.h"

#define MAX_CLIENTS 10
#define MAX_LINE_LENGTH 64
#define PORT 80
#define AP_SSID "HIDDEN"
#define PASSWORD "ALSOHIDDEN"

#define LED_OUTPUT 2
//#define PUMPE_OUTPUT 0





 
const char* ssid = AP_SSID;
const char* password =  PASSWORD;
WiFiServer wifiServer(PORT);
int led = 2;
//int pumpe = PUMPE_OUTPUT;


std::unique_ptr<WiFiClient> clients[MAX_CLIENTS] = {};
char buffers[MAX_CLIENTS][MAX_LINE_LENGTH] = {0};
int newData[MAX_CLIENTS] = {0};
 
void setup() {
  pinMode(led, OUTPUT);
  //pinMode(pumpe, OUTPUT);
  Serial.begin(115200);
 
  delay(1000);
 
  WiFi.begin(ssid, password);
 
  while (WiFi.status() != WL_CONNECTED)
  {
    delay(1000);
    Serial.println("Connecting..");
  }
 
  Serial.print("Connected to WiFi. IP:");
  Serial.println(WiFi.localIP());
 
  wifiServer.begin();
}


void checkNewClients()
{
    WiFiClient newClient = wifiServer.available();
    if (newClient) 
    {
      Serial.println("New client connected");
      // Find the first unused space
      for (int i=0; i < MAX_CLIENTS; ++i) 
      {
          if (!clients[i]) 
          {
              clients[i] = std::unique_ptr<WiFiClient>(new WiFiClient(newClient));
              break;
          }
       }
    }
}

void checkNewData()
{
    memset(newData, 0, MAX_CLIENTS);
    for (int i = 0 ; i < MAX_CLIENTS ; ++i) 
    {
        if (clients[i] != NULL && clients[i]->available())
        {
            newData[i] = 1;
            memset(buffers[i], 0, MAX_LINE_LENGTH); //reset the old buffer
            int index = 0;
            while (clients[i]->available())
            {
                char current = clients[i]->read();
                if(index < MAX_LINE_LENGTH - 1)//-1 to leave space for '\0'
                {
                  if(current == '\r')
                      continue;
                  buffers[i][index] = current;
                  //DEBUG
                  //Serial.println(current,HEX);
                  index++;
                }
            }
            buffers[i][index] = '\0';
            //DEBUG
            Serial.println(buffers[i]);
        }
    }
}

void doJob()
{
    for (int i=0 ; i < MAX_CLIENTS ; ++i)
    {
        if(newData[i])
        {
            if(strcmp(buffers[i], "LED OFF") == 0)
            {
               
                   digitalWrite(led, LOW); // Turn the LED off
                   Serial.println("LED OFF");
            }
            if(strcmp(buffers[i], "LED ON") == 0)
            {
               
                   digitalWrite(led, HIGH); // Turn the LED on
                   Serial.println("LED ON");
            }
        }
    }
}

void checkDisconnections()
{
     for (int i=0 ; i<MAX_CLIENTS ; ++i)
     {
            if (clients[i])
            {
               if(!clients[i]->connected())
               {
                  
                  clients[i]->stop();
                  clients[i].reset();
                  //DEBUG
                  if(!clients[i])
                    Serial.println("Client was deleted");
                  else
                    Serial.println("Client was not deleted");
               }
            }
     }
}



void doWifiJob()
{
    checkNewClients();
    checkNewData();
    doJob();
    checkDisconnections();
}


void loop() 
{
 
  doWifiJob();
  delay(1000);


}

And what I probably should change i using only 1 buffer instead of a reserving buffer per client (and run the dojob() from the checknewdata() function)...as for me action should be immediatly there is no need to keep track of all buffers...ok for certain usages this could maybe make sense.