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

• timemage: Is char** lines; ever initialized?
• DuluthIsSuperior: @timemage - Yes, there's a method that sets lines to (char**) malloc(sizeof(char*) * Char.sizeOfLines);. I'm in the process of converting this from a struct to a class so I haven't gotten around to creating a constructor yet.
• timemage: I suspect you'll eventually find this is a matter of corrupting memory. Without the full code probably away to execute it, there's not much chance of me personally finding anything. Generally to have a better chance of getting help with this, I'd say provide full code to a cut down example that still shows the problem. If possible remove the display code entirely from the example, because that dependency will severely limit the set of people that can reproduce your problem. That said, you're likely to find the answer in the process of doing that.
• Majenko: You should re-think your whole strategy. It looks like you're making a wrapper for some graphical symbols made out of straight line segments. It would be far better to store the data statically in flash and just provide a pointer to it to your class. Then use the pgm_read_* functions to access the data. No need for any dynamic RAM allocation. No limit on the number of lines in a symbol (that count could be static and part of the symbol data). No messing with malloc.
• DuluthIsSuperior: @timemage - I didn't want to make this question long by posting the entire code, but then realized that Git is a thing. I guess that's what I get for posting this at 11pm lol. I edited the question with the repo added.

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.

• timemage: I probably should have pointed out that the register file is memory mapped into the start of data space. So, when malloc() fails underneath new char[4]{...} the more immediate effect of it continuing to attempt initialization is that of screwing up the general purpose registers, which will do who-knows-what from there; nothing good or terribly predictable.
• DuluthIsSuperior: I didn't realize new called malloc underneath. That would explain a lot honestly. I always forget that you don't have a lot of memory to work with on Arduino, so I think I'm just going to move to using flash or an SD card to store the information for the symbols at this point. Seems like it would be less of a headache anyways.
• timemage: Yeah, new (with the excepting the "placement" version) is dynamic allocation in any case. That the it uses malloc is an implementation detail; it needn't strictly be done that way. It typically that is what happens though. So far, it doesn't seem like you need an SD card; just look into using PROGMEM and the rest of pgmspace.h. You may find it a bit confusing at first.
• DuluthIsSuperior: Moving all the graphics data over to flash helped a lot! I'm no longer facing issues. Took a little getting used to PROGMEM but it was definitely worth it.

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;
...

• Juraj: what is wrong with assigning arrays to items in array of pointers?
• Krampster: Assignment isn't the problem. The problem is that he's using a malloc operation to alloc the entire set of lines, but then allocating again each line individually. It's like allocating an entire chessboard, and then ignoring your chessboard, and allocating each row independently as you fill it. You either allocate the board, or each row. Not both.
• Juraj: you say those are pointers what OP allocates and then you show how to allocate it at once. or am I reading it wrong?