Public variable in class not incrementing

Question

I'm not sure what is wrong with my code, but whenever I call addNewLine, the linesInserted variable is not updating correctly. When I print the value of linesInserted inside of the addNewLine method, it shows as incrementing, but when I print the value inside of the draw method, it shows that it is 0.

I have tried different increment operators (linesInserted += 1 and linesInserted = linesInserted + 1). Oddly enough, when I do linesInserted = 99 instead of linesInserted++, it sets the value correctly. I'm wondering if I am possibly messing up a reference somewhere?

#include <Adafruit_GFX.h>
#include <Adafruit_SSD1331.h>
#include <SPI.h>

class Character {
  public:
    char** lines; // using char for 8 bit integer
    int sizeOfLines;
    char symbol;
    Adafruit_SSD1331* display;
    int width;
    draw(int x, int y, int color);
    addNewLine(char startX, char startY, char endX, char endY);
    int linesInserted = 0;
};

Character::draw(int x, int y, int color) {
  if (linesInserted < sizeOfLines) {
    display->setCursor(0, 39);
    display->setTextColor(0xF800);
    display->setTextSize(1);
    char buf[30];
    sprintf(buf, "SYMBOL %c\nWASTED MEMORY", symbol);
    Serial.println(symbol);
    Serial.print(linesInserted);
    Serial.print(" ");
    Serial.println(sizeOfLines);
    display->print(buf);
  }
  for (int i = 0; i < sizeOfLines; i++) {
    char* line = lines[i];
    display->drawLine(line[0] + x, line[1] + y, line[2] + x, line[3] + y, color);
  }
};

Character::addNewLine(char startX, char startY, char endX, char endY)  {
  linesInserted++;
  if (linesInserted > sizeOfLines) {
    display->setCursor(0, 15);
    display->setTextColor(0xF800);
    display->setTextSize(1);
    char buf[30];
    sprintf(buf, "ERROR\nSYMBOL %c\nTOO MANY LINES", symbol);
    display->print(buf);
  } else {
    lines[linesInserted - 1] = new char[4]{startX, startY, endX, endY};
  }
};

EDIT: Git repo to the project is added - https://github.com/Pyrodron/charger-destination-board

Answer 1

It's difficult to pin down the exact details as to why the problem it's manifesting exactly the way it is. But, in short, it's fair to say you're running out of memory.

First, just a note: The code I'm testing with (because it compiles) is the code out of your github repo, which used a char type variable for linesInserted and had a commented-out println(linesInserted);, so it kind of seems like at one point you'd been trying to print the count as character, specifically non-printable control characters. So that might may have been confusing you. I uncommented it and added a , DEC) parameter to it (rather than casting) to coerce println into treating it as an int.

I tested your code without the display library in a PC environment to allow me to run valgrind on it. It didn't find anything; I wasn't necessarily expecting it to. But, I did crudely instrument your code in this enviroment to see that you're calling malloc()/new 124 times, for a total of 500 bytes, but importantly this does not include the structure heap overhead, which is significant because you're new-ing tiny four-character chunks.

Then I ran your code as compiled for an AVR. Because I don't have your display, and wanted to match your environment as closely as possible, I left the display code in but NOPed out a few function calls that would have screwed up my test rig.

The primary finding is that you're dangerously close to "running out" of memory:

000005e0 | 01 00 02 04  00 01 00 02 - 00 04 00 03  01 03 05 04 | ........ ........
000005f0 | 00 01 03 02  03 04 00 01 - 06 02 06 04  00 00 00 00 | ........ ........
00000600 | 00 04 00 02  00 02 00 04 - 00 01 01 01  01 04 00 00 | ........ ........
00000610 | 02 00 02 04  00 02 02 02 - 02 04 00 01  03 01 03 04 | ........ ........
00000620 | 00 00 04 00  04 04 00 02 - 04 02 04 04  00 01 05 01 | ........ ........
00000630 | 05 04 00 00  06 00 06 04 - 00 02 06 02  06 00 00 00 | ........ ........
                                                      ^^
                                                 [__brkval]

                          [SP]
                           VV               
000006b0 | 00 00 00 00  00 00 b7 09 - 03 01 01 06  02 a5 02 00 | ........ ........
000006c0 | 00 00 00 01  00 57 05 06 - cc 00 03 8f  0f 01 00 00 | .....W.. ........
000006d0 | 00 01 00 00  03 00 03 00 - 00 00 07 02  a4 00 01 00 | ........ ........
000006e0 | 03 02 a4 00  05 f8 0d 06 - f6 00 03 00  00 08 c3 08 | ........ ........
000006f0 | de 00 01 00  01 80 1a ed - 02 04 00 41  a4 02 04 00 | ........ ...A....

There's 120 byte separation between the edge of the heap and the stack pointer. The implementation of the malloc that I ran this under checks to see whether or the the stack pointer is within 128 (__malloc_margin) bytes. As your code was building in my environment, without the counter-printing in, this managed to not-fail to allocate. However adding a single additional call to addNewLine was enough to cause malloc() via new to fail. So you are indeed right on the edge. And you have no checks to see if malloc or new returning nullptr.

You're making one allocation after another without deallocation anything, which is resulting in a fairly simple pattern of the heap edge advancing by the requested size of your allocation plus 2 bytes each time. So that's about 248 bytes waste in just heap structure.

I'm not sure on the exact cause of your counter being overwritten, assuming that's what's actually happening. In fact I was having a hard time finding an case where it would overwrite anything in memory with the code as you have it, but here's a good candidate:

lines[linesInserted - 1] = new char[4]{startX, startY, endX, endY};

With generated code looking like:

0x24c6 [addNewLine+244] ldi    r25, 0x00       ; 0
0x24c8 [addNewLine+246] call   0x3830  ;  0x3830 [malloc]
0x24cc [addNewLine+250] movw   r30, r24
0x24ce [addNewLine+252] st     Z, r10
0x24d0 [addNewLine+254] std    Z+1, r11        ; 0x01
0x24d2 [addNewLine+256] std    Z+2, r17        ; 0x02
0x24d4 [addNewLine+258] std    Z+3, r16        ; 0x03

So, if malloc() fails, apparently the new statement will just assume that it didn't and goes ahead and writes into the memory at 0x0000 (nullptr) anyway, ugh. I think this is because technically you're calling the normal new that is supposed to throw an exception on failure. But there are no exceptions with the way avr-g++ is set up for Arduino. As it exists in ArduinoCore-avr version 1.8.3, the new code just returns what malloc returns and the compiler thinks an exception will be in progress if it fails. Normally you would call the std::nothrow version of new[] that returns nullptr on failure, as in:

lines[linesInserted - 1] = new(std::nothrow) char[4]{startX, startY, endX, endY};
// check lines[linesInserted - 1] against nullptr here

However, there's no facility to do that in 1.8.3. The master as of writing appears to have addressed this, but that's not shipping yet.

---

The short answer to this is basically: Don't do that. That is:

• Avoid dynamic allocation altogether where possible.

• This was mentioned in the comments, but it's worth repeating, use flash where you can. Besides getting your character data into PROGMEM, you also have some string literals that can be moved into flash by way of F().

• To the extent that you're doing dynamic allocation requests:

  • Check to see that they're actually succeeding

  • Make as few requests as possible.

  • Do them up-front at the beginning of the code. The "up-front" part you're more or less already doing, so that much is good.

  • It seems if you're going to dynamically allocate under core version 1.8.3 and you're at all in danger of having an allocation failure, it is important that you not use new, since doing so may cause it to overwrite memory for the initialization/construction part after the allocation proper has failed. Alternately, seriously consider modifying your Arduino core's new.cpp file to do something like call abort() if malloc() returns nullptr, at a minimum, if not something more sensible. Otherwise, this could drive you insane.

Answer 2

It would be good to see how this is used rather than just having a class with no context. But I'd start by removing unnecessary parts, like the display pointer, and you could always use a simple program, like ideone.com to compile quickly and check for errors.

As I read through it I see 2 flaws with memory management. This block is using the size of a char pointer instead of a character. I don't understand what you want this code to do.

(char**) malloc(sizeof(char*) * Char.sizeOfLines);

Try something more like:

(char**) malloc(sizeof(char) * <numberOfCharsPerLine> * Char.sizeOfLines);

But then even though you're allocating memory for a whole block of lines, then each time you add a new one, you're allocating memory for a new line.

lines[linesInserted - 1] = new char[4]{startX, startY, endX, endY};

Instead try something like:

lines[linesInserted - 1][0] = startX;
lines[linesInserted - 1][1] = startY;
...