So I've been running into a weird issue while trying to compartmentalize some test code I wrote for an Arduino/NeoPixel application. There are two scenarios: Scenario A, my code before the move, and Scenario B, my code after the move. The test code works as expected in Scenario A (a red light walks across my 8x8 led matrix). The very same code, when moved to a container class (Scenario B) results in odd behavior (A blotch of randomly colored LEDs appears and doesn't move). A simple move of functionality from one place to another doesn't seem like it would be able to cause these kinds of symptoms though, so I'm a bit lost.
Here are some pictures. Scenario A Scenario B
I've attached code for the two different scenarios below. I have removed sections of code and includes that aren't being referenced yet for clarity purposes.
I'm still more or less a hobbyist when it comes to Arduino/C++, so feel free to point minor things out as well.
#include <Arduino.h>
#include "Hardware.h"
Hardware* hardware = new Hardware();
void setup()
{
hardware->Setup();
}
uint8_t i = 0;
void loop()
{
auto screen = hardware->GetScreen();
screen->Clear();
screen->SetLedHSV(i++ % screen->Count(), 0, 255, 255);
screen->Show();
delay(100);
}
#pragma once
#include "Screen.h"
class Hardware
{
private:
Screen screen = Screen(8, 8, 14);
public:
void Setup()
{
screen.Setup();
}
Screen* GetScreen() { return &screen; }
};
#pragma once
#include <Adafruit_NeoPixel.h>
class Screen
{
private:
uint8_t width, height;
uint8_t pin;
Adafruit_NeoPixel pixels;
public:
Screen(uint8_t width, uint8_t height, uint8_t pin) :
width(width), height(height), pin(pin)
{
pixels = Adafruit_NeoPixel(width * height, pin, NEO_GRB + NEO_KHZ800);
}
void Setup()
{
pixels.begin();
pixels.show();
pixels.setBrightness(32);
}
uint16_t Count() { return width * height; }
uint8_t GetWidth() { return width; }
uint8_t GetHeight() { return height; }
void Show()
{
pixels.show();
}
void Clear()
{
pixels.clear();
}
void SetLedHSV(uint16_t i, uint16_t h, uint8_t s, uint8_t v)
{
pixels.setPixelColor(i, pixels.ColorHSV(h, s, v));
}
void SetLedHSV(uint8_t x, uint8_t y, uint16_t h, uint8_t s, uint8_t v)
{
if (x < 0 || x >= width)
return;
if (y < 0 || y >= height)
return;
auto i = x + y * width;
pixels.setPixelColor(i, pixels.ColorHSV(h, s, v));
}
};
#include <Arduino.h>
#include "Hardware.h"
#include "TestApp.h"
unsigned long timestamp;
Hardware* hardware = new Hardware();
TestApp* app = new TestApp(hardware);
void setup()
{
hardware->Setup();
}
void loop()
{
app->Update();
delay(100);
}
Same as above.
Same as above.
#pragma once
#include "Hardware.h"
class TestApp
{
Hardware* hardware = 0;
uint8_t i = 0;
public:
TestApp(Hardware* hardware) : hardware(hardware) {}
void Update()
{
auto screen = hardware->GetScreen();
screen->Clear();
screen->SetLedHSV(i++ % screen->Count(), 0, 255, 255);
screen->Show();
}
};
The issue is apparently in the Screen constructor:
Screen(uint8_t width, uint8_t height, uint8_t pin) :
width(width), height(height), pin(pin)
{
pixels = Adafruit_NeoPixel(width * height, pin, NEO_GRB + NEO_KHZ800);
}
Now it creates "empty" pixels using default constructor and in the code block it creates another instance (now properly initialized) and makes a copy into pixels. As the copy assigment operator is not provided, default one is used, and it makes a shallow copy of pixels storage. And after that second instance goes out of scope and deletes that allocated pointer.
Now you have dangling pointer and if you allocate anything, it'll be allocated in the same space as the dangling pointer is pointing to.
Difference between those two test cases is that you are doing another allocation in the second program:
Hardware* hardware = new Hardware(); // dangling pointer is created here
TestApp* app = new TestApp(hardware); // <<-- allocated in the same space as that dangling pointer
Anyway pixels class member should be initialized in constructor's initializer list (exactly same as the rest of the values):
Screen(uint8_t width, uint8_t height, uint8_t pin) :
width(width), height(height), pin(pin), pixels(width * height, pin, NEO_GRB + NEO_KHZ800)
{
// btw: do you really have to store a "pin" value?
}
And why are you using pointers so much? It could be just like this (or it could be passed as a reference into the app too):
#include <Arduino.h>
#include "Hardware.h"
#include "TestApp.h"
unsigned long timestamp;
Hardware hardware;
TestApp app { &hardware };
void setup()
{
hardware.Setup();
}
void loop()
{
app.Update();
delay(100);
}
Btw: It's caused by Adafruit library, that doesn't follow "Rule of three"/"Rule of five" guideline. It shouldn't allow to make a copy of object at all (as a deep copy could be even worst on small devices)