Memory allocation problem

Hello everyone! About a week I am having a nasty bug in my working application. After researching I've found that the bug is corresponding to memory allocation. Below I am going to show the logic (in general).

The problem is that at the time to display(), the data in cl1->inp_data or in cl2->buf becomes broken (depends on input).

Any idea what I may doing wrong violating integrity of arduino's sram? Thanks!

Example:
main.cpp

#include <Arduino.h>
#include <Adafruit_GFX.h>
#include <Adafruit_PCD8544.h>
#include "../lib/class1/class1.h"
#include "../lib/class2/class2.h"

using namespace std;

Adafruit_PCD8544 display = Adafruit_PCD8544(4, 6, 5, 2, 3);

Class1 cl1 = Class1(A0);

Class2 cl2 = Class2(&display, &cl1);

const uint8_t img[] PROGMEM = {0x01, 0xfc, 0x3f, 0x80, 0x01, 0xfc, 0x3f, 0x80};

void setup() {
   Serial.begin(9600);
   pinMode(7, OUTPUT);
   digitalWrite(7, LOW);
   display.begin();
   display.setContrast(50);
   display.setRotation(0);
   display.clearDisplay();
   if(!cl1.init() || !cl2.init()) {
       display.println("Failed to initialize");
       display.display();
       delay(5000);
   }
}

void loop() {
    display.clearDisplay();
    cl1.doCheck();

    switch(cl2.getState()) {
       case 0:
           cl1.addImg(img, "image");
           cl1.regInput(10, "some text");
           // do something
           break;
       case 1:
          cl1.regInput(5, "some text");
          // do something
          break;
       default:
         // do something
         break;
    }

    cl2.display();
    delay(2000);
}

class1.cpp:

#include <Arduino.h>
#include "class1.h"

using namespace std;

 Class1::Class1(uint8_t port) {
     inPort = port;
 }

 bool Class1::init() {
     bool statusFail = false;
     pinMode(inPort, INPUT);
     void * mem = calloc(5, sizeof(char));
     if(mem == nullptr) {
         statusFail = true;
     } else {
         inp_data = (char *)mem;
     }
     void * mem1 = calloc(5, sizeof(char));
     if(mem1 == nullptr) {
         statusFail = true;
     } else {
         op_data = (char *)mem1;
     }
     return statusFail;
 }

 void Class1::doCheck() {
    // collecting input data
    state = 0;
 }

 void Class1::regInput(uint8_t input_length, char const * text) {
      if(strlen(inp_data) < input_length) {
         size_t s = input_length * sizeof(char);
         void * mem = realloc(inp_data, s);
         memset(mem, '\0', s);
         if(mem == nullptr) {
             Serial.println("ERROR");
         }
         inp_data = (char*)mem;
     }
    // do something
    for(uint8_t i = 0; i < strlen(text); i++) {
        *(inp_data + i) = *(text + i);
    }
 }

 char * Class1::getInput() {
    return inp_data;
 }

 uint8_t Class1::getState() {
     return state;
 }

 void Class1::addImg( uint8_t const *img, char const * name) {
     struct1 st1;
     st1.name = name;
     st1.img = img;
     contents.push_back(st1);
 }

class1.h

#ifndef CL1
#define CL1

#include <ArduinoSTL.h>

class Class1 {
public:
    Class1(uint8_t port);
    ~Class1() {
        free(inp_data);
        free(op_data);
    };
    bool init();
    void doCheck();
    void regInput(uint8_t input_length, char const * text);
    char * getInput();
    uint8_t getState();
    void addImg( uint8_t const * img, char const * text);

private:
    uint8_t inPort;
    char * inp_data,
         * op_data;
    uint8_t state;
    struct struct1 {
        const uint8_t * img;
        const char * name;
    };
    std::vector<struct1> contents;
};


#endif

class2.cpp

#include "class2.h"

using namespace std;

 Class2::Class2(Adafruit_PCD8544 * lcd, Class1 * cl1) {
   obj_lcd = lcd;
   obj_class1 = cl1;
 }

 bool Class2::init() {
     bool statusFail = false;
     buf = (char*)calloc(100, sizeof(char));
     if(buf == nullptr) {
         statusFail = true;
     }
     return statusFail;
 }

 uint8_t Class2::getState() {
     // do something assuming cl1->status value
    return obj_class1->getState();
 }

 void Class2::display() {
    // displaying input data from Class1
    strcpy(buf, obj_class1->getInput());
    obj_lcd->setTextSize(1);
    obj_lcd->setTextColor(BLACK);
    obj_lcd->setCursor(0, 0);
    obj_lcd->println(buf);
    obj_lcd->display();
 }

class2.h

 #ifndef CL2
 #define CL2

#include "../class1/class1.h"
#include <Arduino.h>
#include <Adafruit_GFX.h>
#include <Adafruit_PCD8544.h>

class Class2 {
public:
    Class2(Adafruit_PCD8544 * lcd, Class1 * cl1);
    ~Class2() {
        free(buf);
    };
    bool init();
    uint8_t getState();
    void display();

private:
    Adafruit_PCD8544 * obj_lcd;
    Class1 * obj_class1;
    char * buf;
};


#endif

way5:
Below I am going to show the logic (in general).

In which case I will answer in general. Post your code.

(And stop touching the hardware in your constructors.)

I would do that with pleasure but the whole project contains 10 classes each one >1500 lines of code and a bunch of headers that contains PROGMEMed graphics and const data.
I am posting the short but real logic instead. Please take a look at the very first post, I've updated it.
Overall project takes 84.5% of data memory and 75% of progmem on chip.
The problem is when any buffer (op_data, buff, inp_data) is reallocating, other ones becomes a random characters.

Is there any additional rule/tricks in my case for memory allocation/deallocation if compared with native C ? May be it is necessary to wipe out allocated buffers each loop?

Which Arduino are you using? Most have very little RAM, so it's critical to check that any allocation calls are successful. Over time, fragmentation will make a malloc failure more likely.

I note that you're mostly careful to check for failures, but if your realloc fails, you go ahead and use the returned null pointer to write on.

There are a couple of things to mention:

  1. sizeof(inp_data) will always equal to two, because sizeof(char*) is the size of a pointer, which on avr architectures is 2 bytes. So you are always reallocating your memory block -> very inefficient (at least the libc documentation from microchip doesn't tell the details).

  2. Don't allocate memory in the constructor. What if it fails and nobody is looking at the serial output? Instead create a function 'bool initialize' and return the result of the allocation.

way5:
Is there any additional rule/tricks in my case for memory allocation/deallocation if compared with native C ?

Yes, there is one: Just don't. Dynamic memory allocation is very tricky for embedded systems with very limited SRAM. That is, because usually you don't know the size of the dynamic memory needed - what if it needs more than what is available? Especially your code, which doesn't check the allocation result at all (printing it out and continuing doesn't count), is prone to system failure due to bad memory.

In Class2 change buf from char* to char[100] as it doesn't change ever. Same goes for op_data.

  1. cl1.regInput(10, (char *)"some text");
    The cast is unnecessary, change the function prototype of regInput to accept a const char* instead of a char*.

for(uint8_t i = 0; i < strlen(text); i++) {
        *(inp_data + i) = *(text + i);
    }

What if your inp_data buffer is smaller than the text buffer? Which is the case for your case 1 call (length of inp_data = 5, length of text = 10). This will overwrite sram locations directly beyond the inp_data buffer up to the size of the text buffer.

  1. You are missing a destructor in both classes to free the allocated memory.

  2. Realloc will not initialize the memory, when extending the memory block. It only guarantees, that the content of the original memory up to the size of the original memory block is the same as the new one. When you copy the contents of text to inp_data, you are still missing the null-terminator at the end of inp_data (strlen returns the length without the null-terminator). Use this instead and manually assign the last element to '\0'.

void * mem = realloc(inp_data, strlen(text) + 1);

way5:
The problem is when any buffer (op_data, buff, inp_data) is reallocating, other ones becomes a random characters.

buff and op_data never get reallocated.

  1. Why don't you use strings? This will handle the memory allocation for you.

Last Edit: Today at 12:23 pm by way5 Reason: compilable code v2

Please don't do that again. You've created a situation where all the posts from the first to here are now out of context.

wildbill:
Which Arduino are you using? Most have very little RAM, so it's critical to check that any allocation calls are successful. Over time, fragmentation will make a malloc failure more likely.

Hi @wildbill, the chip is ATMEGA328P

@LightuC, first of all thanks for detailed answer. I will add my comments below.

LightuC:

  1. sizeof(inp_data) will always equal to two, because sizeof(char*) is the size of a pointer…

  2. Don't allocate memory in the constructor…

Agreed. Code updated

LightuC:
3. Yes, there is one: Just don't. Dynamic memory allocation is very tricky for embedded systems with very limited SRAM…

Well, I've tried do not, however in a this particular case I would need to do it once at least in order to have a required functionality.

LightuC:
In Class2 change buf from char* to char[100] as it doesn't change ever. Same goes for op_data.

Agreed. I'll do change this block

LightuC:
4.

cl1.regInput(10, (char *)"some text");

The cast is unnecessary….

Agreed

LightuC:
5.

for(uint8_t i = 0; i < strlen(text); i++) {

*(inp_data + i) = *(text + i);
    }



What if your inp_data buffer …

In the main code there are some procedures to check consistency. Here I left a short description.

LightuC:
6. You are missing a destructor in both classes to free the allocated memory.

Agreed, added to the headers

LightuC:
7. Realloc will not initialize the memory...

8.buff and op_data never get reallocated.

Agreed

LightuC:
9. Why don't you use strings? This will handle the memory allocation for you.

I am aware of using strings in any Arduino project while I can successfully solve the problem with char[]. This increases memory in use, in my case till ~=10%


  1. After refactoring the part of code that I've posted above inside of the main project, I've got that it won't to allocate memory at all. The device halts. I changed malloc -> calloc. Any idea what can happen?

  2. I've also found a reason for buffers to become a random characters earlier. This was happening after doing Class1 : contents.push_back(st1) with st1.img. Feels like that dramatically increases the memory allocated for vector that in turn makes other buffers read rewritten memory blocks.
    Any idea what can be happening here and how to avoid this?

Thanks in advance

way5:
In the main code there are some procedures to check consistency. Here I left a short description.

I don't see any in the provided code, especially this part:

cl1.regInput(5, "some text");

EDIT: Okay, if you fix your if statement, this shouldn't be a problem.

way5:

  1. After refactoring the part of code that I've posted above inside of the main project, I've got that it won't to allocate memory at all. The device halts. I changed malloc -> calloc. Any idea what can happen?

You are not allocating memory but using calloc?

way5:
2. I've also found a reason for buffers to become a random characters earlier. This was happening after doing Class1 : contents.push_back(st1) with st1.img. Feels like that dramatically increases the memory allocated for vector that in turn makes other buffers read rewritten memory blocks.
Any idea what can be happening here and how to avoid this?

There was no vector being used in in your code. Maybe you can upload the files in a zip archive? There might be other things going that we can't see.

LightuC:
There was no vector being used in in your code. Maybe you can upload the files in a zip archive? There might be other things going that we can't see.

please check class1.h: std::vector contents;

Class1 cl1 = Class1(A0);

I try to avoid any allocation outside of the main thread. Your allocating things in basically an unknown environment. Because the order of the allocations is unknown (to you).

I use..

Class1* cl1;  // As your global pointer to an object on the outside.

void start(void) {
  
  cl1 = new Class1(A0);  // Created in known environment.
  .
  .
  .
}

I've been bit by that a few times.

-jim lee

I've

jimLee:
I try to avoid any allocation outside of the main thread. Your allocating things in basically an unknown environment. Because the order of the allocations is unknown (to you).

Ok. Allocation in global can be causing an issue?

Yes it can depending on what the object expects to find during its constructor. Like other globals.

-jim lee

Could you describe the uses of class1 and class2? Maybe we can find a way to avoid the dynamic memory allocation or hide it.

By this day I completely cast out the old buggy code from my project and rewrote it. Now it works fine and I can point on few things that I've learned while programming for AVRs small chips:

  • Do not use memory allocation manually. There are many small tricks that for sure (at least me) will be missed during the code writing/changing that drives to application's unpredictable behaviour.
  • Do not store large data in vectors, use pointers.
  • Do use "String" data types, but limitedly. [thanks @LightuC for insisting :)] Strings even can work as in-class data buffers, but it really matters how much free memory you have.
  • Do use in-class static char buffer arrays (instead of pointers), and do not try to allocate them dynamically. The imaginary benefit to free some memory while it's not needed doesn't cost hours of debugging.

Hope this helps to someone. Thanks everyone for help!

way5:

  • Do use "String" data types, but limitedly. [thanks @LightuC for insisting :)] Strings even can work as in-class data buffers, but it really matters how much free memory you have.

Just a quick note on Strings on the arduino: You have to be very carefull when and how you use them. Although they encapsulate the memory allocation and deallocation, they also fragment the heap when you use basically any operation on them (concat, substring, ...). This is why you should always aim at creating static buffers first, if that is not working you might use String for that. However, if you are already going down the road of dynamic memory allocation you might as well do it on your own. Just remember to call delete for every new.