0

I have following code in loop() in NodeMCU.

This part is before setup():

String serial_data_read = "";   // for incoming serial string data
String serial_data = "";   // for incoming serial string data

This part is in loop():

if ((last_weight_time < millis() - 1000)) {
   last_weight_time = millis();
   serial_data_read = "";
   Serial.print("P");
   while(Serial.available()) {
      character = Serial.read();
      serial_data_read.concat(character);
   }
   serial_data = serial_data_read;
}

I don't understand C programming that much, but I think it has something to do with memory leaking and eventually running out of memory. It runs for around 20 minutes and then just stops. When I manually restart NodeMCU it runs again fine.

I would be very grateful for any help, how to rewrite my code so it doesn't leak memory. Thank you very much.

  • 4
    Just don't use String, but char arrays instead – chrisl May 18 at 12:06
  • 1
  • 1
    Re if ((last_weight_time < millis() - 1000)): This can fail badly if last_weight_time is right before a millis() rollover event. Use instead the idiom if (millis() - last_weight_time > 1000), which is rollover safe. – Edgar Bonet May 19 at 12:54
1

To write code that will not cause memory leak, you need to understand of where and what caused the memory leak first. The memory leak in this particular case is caused by the line

serial_data_read.concat(character); 

When you instantiate a String object with String serial_data_read = "", Arduino String class create a memory allocation using dynamic memory allocation in the heap, and since your initialised it with "", it therefore create a minimum size of allocation. This is all good if you know how to the dynamic memory is created and how it going to be used.

The problem started when you start doing String concatenation with the line serial_data_read.concat(character), the original allocation for your global variable serial_data_read is no longer have enough space for the concatenated variable, so it create a temporary variable in the heap and do the concatenation, and then eventually put the result back to this newly allocated memory for serial_data_read on the heap, leaving a hole at the heap for the original serial_data_read variable (and the temp variable).

What make this worst is that this is happening for each iteration of the while loop. Theoretically the old serial_data_read prior the concatenation should be free-up at some point (e.g. if your while loop is within a function, the part of the heap might be free-up at the exit of the function), it however does not necessary be the case because your newly created serial_data_read stay on top of piles of to-be-free-up memory that blocking the free up(think of a piece of cheese with a lot of holes on it), so the heap memory get less and less at each iteration.

Short term fix - do not do String concatenation in a loop

String class is not always evil if you know what's going on and how to use it. One of the quick fix for your problem is simply do not do String concatenation within a loop. And if you are using it within a function, keep it local. This will minimise the heap fragmentation.

if ((last_weight_time < millis() - 1000)) {
   last_weight_time = millis();
   if (Serial.available() > 0) {
      String serial_data_read = Serial.readStringUntil('\n');
   }
}

BTW, you can add ESP.getFreeHeap() in your loop() to check if there is still memory leak.

Long term fix - Don't use String class

For a better fix, and especially for some one new to the programming or C/C++, do not use String class and learn how to use c string and array. Here is the version without using String class.

length = 80;
char buffer[length] = {0};
if ((last_weight_time < millis() - 1000)) {
   last_weight_time = millis();
   while(Serial.available()) {
      Serial.readBytesUntil('\n', buffer, length);
   }
}

Other resources to learn about

https://www.cplusplus.com/reference/cstring/ all the functions for c string and array manipulation to replace Arduino String class.

https://hackingmajenkoblog.wordpress.com/2016/02/04/the-evils-of-arduino-strings/

| improve this answer | |
  • Re “[heap allocation] is happening for each iteration of the while loop”: on the ESP8266, it happens once every 16 iterations, as the allocated size is grown by multiples of 16 bytes. Re “if your while loop is within a function, the part of the heap might be free-up at the exit of the function”: when concat() can't manage to grow the string in place, it will allocate a new buffer, copy the existing data, and free the previous buffer right away. This doesn't invalidate your point about the risk of memory fragmentation though. – Edgar Bonet May 19 at 12:49
1

The short answer is, don't use the String class at all. You need to read up on using C strings (which are simply null-terminated arrays of characters. There are C library functions for manipulating C strings. You'll want to avoid printf, scanf, and related functions, as the library that supports those is quite large

The basic idea is to use statically allocated C strings as buffers. Figure out the largest number of characters you need to store, and create a static array of characters at least one character larger (for the terminating null) and use that to collect the output you are building.

| improve this answer | |
  • 1
    printf() and friends don't have much impact on - and cause no fragmentation of - data space. They take up more code-space than Serial.print() but most of us don't write sketches that push the code space limit. – JRobert May 18 at 14:10
  • @Duncan C - Thank you very much for your answer. Can you please add code example how my code would have to be rewritten ? – Frodik May 18 at 14:43
  • 2
    What @JRobert said. Avoiding String is good advice, avoiding printf, scanf is bad advice in this case; they don't cause issues with the heap and that's what this problem is. – romkey May 18 at 14:45
  • Also the ESP8266 has plenty of code space (flash) even on a small variant. On small AVR chips the printf family use a cut-down version with floating point support removed to save space. – Majenko May 18 at 15:58
  • Good to know about the cut-down version of printf/scanf. To the OP: Disregard that part of my answer – Duncan C May 18 at 16:58

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.