[C++] Variable array length has strange behavior

Hi all,

I don’t understand why the following code isn’t working:

// byte collectorModuleNumber = 2;

byte* responseBuffer = new byte[2 * collectorModuleNumber];
		
for (byte i = 0; i < collectorModuleNumber; i++)
{
	// getId() return const word
	Serial.print(collectorModules[i]->getId()); // it doesn't print nothing

	const word moduleId = collectorModules[i]->getId();

	if (i == 0)
	{
		responseBuffer[i] = highByte(moduleId); // doesn't assign nothing
		responseBuffer[i + 1] = lowByte(moduleId);
	} 
	else
	{
		responseBuffer[i + 1] = highByte(moduleId);
		responseBuffer[i + 2] = lowByte(moduleId);
	}
}

EDIT:
Complete code:

CollectorModule.h

class CollectorModuleClass
{
 protected:
 	static const word id = 1;

	const word getId() { return id; };
};

MyTest.h

class MyTestClass
{
 protected:
	CollectorModuleClass* collectorModules[];
	byte collectorModuleNumber;

	MyTestClass(const byte& collectorModuleNumber);

	void sendManifest();

	void registerCollector(const byte& i, CollectorModuleClass* cm);
};

MyTest.cpp

#include "MyTest.h"

MyTestClass::MyTestClass(const byte& collectorModuleNumber)
{
	this->collectorModuleNumber = collectorModuleNumber;
};

void MyTestClass::registerCollector(const byte& i, ICollectorModule* cm)
{
	collectorModules[i] = cm;
};

void MyTestClass::sendManifest()
{
	byte* responseBuffer = new byte[2 * collectorModuleNumber];
			
	for (byte i = 0; i < collectorModuleNumber; i++)
	{
		// getId() return const word
		Serial.print(collectorModules[i]->getId()); // it doesn't print nothing

		const word moduleId = collectorModules[i]->getId();

		responseBuffer[2 * i] = highByte(moduleId); // doesn't assign nothing
		responseBuffer[2 * i + 1] = lowByte(moduleId);
	}
};

Sketch.ino

#include "CollectorModule.h"
#include "MyTest.h"

CollectorModuleClass* c1 = new CollectorModuleClass();
CollectorModuleClass* c2 = new CollectorModuleClass();
MyTestClass* m = new MyTestClass(2);

void setup()
{
	m->registerCollector(0, c1);
	m->registerCollector(1, c2);

	m->sendManifest();
}

void loop(){}

Maybe because you are using bytes 0 and 1 for entry 0, bytes 2 and 3 for entry 1, bytes 3 and 4 for entry 2... Each entry past 1 overlaps overlaps the previous entry.

Try:

  responseBuffer[2*i] = highByte(moduleId);
 responseBuffer[2*i + 1] = lowByte(moduleId);

I don't understand what the code is supposed to be doing, nor why you are not multiplying i by 2 to index the array.

johnwasser: Maybe because you are using bytes 0 and 1 for entry 0, bytes 2 and 3 for entry 1, bytes 3 and 4 for entry 2... Each entry past 1 overlaps overlaps the previous entry.

Try:

  responseBuffer[2*i] = highByte(moduleId);
 responseBuffer[2*i + 1] = lowByte(moduleId);

This is right, because I need to split for each entry the word (2 byte each) in 2 separate entry in the array, so entry 0 need to allocate high byte to 0 and low byte to 1, entry 1 need to allocate high byte to 2 and low byte to 3 and so on.

Well lets see

You're showing us what you define for responseBuffer and collectorModuleNumber;

BUT you're printing something to do with collectorModules and getId();

I think you need to be looking Here

Look at edit in the first post.

Everything is working as excepted when I change array size to a constant.

void MyTestClass::sendManifest()
{
	byte responseBuffer[2 * 2]; // this is constant (how it should be with variable size)
			
	for (byte i = 0; i < collectorModuleNumber; i++)
	{
		// getId() return const word
		Serial.print(collectorModules[i]->getId()); // it doesn't print nothing

		const word moduleId = collectorModules[i]->getId();

		responseBuffer[2 * i] = highByte(moduleId); // doesn't assign nothing
		responseBuffer[2 * i + 1] = lowByte(moduleId);
	}
};
void MyTestClass::sendManifest()
{
	byte* responseBuffer = new byte[2 * collectorModuleNumber];
			
	for (byte i = 0; i < collectorModuleNumber; i++)
	{
		// getId() return const word
		Serial.print(collectorModules[i]->getId()); // it doesn't print nothing

		const word moduleId = collectorModules[i]->getId();

		responseBuffer[2 * i] = highByte(moduleId); // doesn't assign nothing
		responseBuffer[2 * i + 1] = lowByte(moduleId);
	}
};

Where does that pointer get deleted?

[quote author=Nick Gammon link=msg=2061387 date=1422306860] Where does that pointer get deleted? [/quote]

You mean for responseBuffer? byte* responseBuffer = new byte[2 * collectorModuleNumber];

I don't do a delete.

delete [] responseBuffer;

This is just a sample test to understand why with the variable size getId() doesn't return nothing.

[quote author=Nick Gammon date=1422306860 link=msg=2061387] Where does that pointer get deleted? [/quote] Looks like a typical example of a memory leak to me. responseBuffer is a local variable to the function void MyTestClass::sendManifest()

Once the function is complete, the variable responseBuffer will cease to exist BUT the memory allocated by that new byte[2 * collectorModuleNumber]; will not be released.

If you ever test the code you would see that deleting the pointer is useless (for this case).

It doesn't change nothing, this is something about the initialization of array with a variable...

By the way, I think that I will find a workaround for that.

Can you zip up the project into one file, so we can test without having to download and reassemble half-a-dozen files? You can add that as an attachment to your post.

How to use this forum

I did too many adaptements to show what's the problem cause. At the moment my code is big, and I should give it also a good clean and optimization.

When I will find a solution that fits good, I will post result here :)

I solved using this, but I still don't understand why it didn't work before...

const word n = 2 * collectorModuleNumber;
byte responseBuffer[n];

In place of this?

 byte* responseBuffer = new byte[2 * collectorModuleNumber];

Well, you are not using "new" for a start.

Plus in your original code you did not test for an error in memory allocation.

eg.

 byte* responseBuffer = new byte[2 * collectorModuleNumber];
        if (responseBuffer == NULL)
            {
            // some sort of error
            }

The fact that you were not deleting because "This is just a sample test" would go a long way to explaining why the memory allocation failed. If you tested a few rounds then indeed it would fail.

When I will publish all of my code as open source (on github probably), you can check it out for errors if you want. At the moment I don't delete any allocated buffer (or array of bytes) and I don't check for any memory allocation error and it works perfectly (I tested by running the program for 2 days consecutive...).