Skip to content

Make Segment class non-copyable and move-only #4580

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Make Segment class non-copyable and move-only #4580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 54 additions & 5 deletions wled00/FX.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
#ifndef WS2812FX_h
#define WS2812FX_h

#include <array>
#include <vector>
#include "wled.h"

Expand Down Expand Up @@ -385,6 +386,8 @@ typedef enum mapping1D2D {
M12_sPinwheel = 4
} mapping1D2D_t;

struct SegmentSettings;

// segment, 68 bytes
typedef struct Segment {
public:
Expand Down Expand Up @@ -414,7 +417,7 @@ typedef struct Segment {
};
uint8_t grouping, spacing;
uint8_t opacity;
uint32_t colors[NUM_COLORS];
std::array<uint32_t, NUM_COLORS> colors;
uint8_t cct; //0==1900K, 255==10091K
uint8_t custom1, custom2; // custom FX parameters/sliders
struct {
Expand Down Expand Up @@ -565,7 +568,7 @@ typedef struct Segment {
stopY = sStopY;
}

Segment(const Segment &orig); // copy constructor
Segment(const Segment&) = delete;
Segment(Segment &&orig) noexcept; // move constructor

~Segment() {
Expand All @@ -580,7 +583,7 @@ typedef struct Segment {
deallocateData();
}

Segment& operator= (const Segment &orig); // copy assignment
Segment& operator= (const Segment&) = delete;
Segment& operator= (Segment &&orig) noexcept; // move assignment

#ifdef WLED_DEBUG
Expand Down Expand Up @@ -626,7 +629,7 @@ typedef struct Segment {
Segment &setMode(uint8_t fx, bool loadDefaults = false);
Segment &setPalette(uint8_t pal);
Segment &setName(const char* name);
uint8_t differs(const Segment& b) const;
uint8_t differs(const SegmentSettings& b) const;
void refreshLightCapabilities();

// runtime data functions
Expand Down Expand Up @@ -784,6 +787,48 @@ typedef struct Segment {
} segment;
//static int segSize = sizeof(Segment);

struct SegmentSettings
{
explicit SegmentSettings(const Segment& b)
: start{b.start}
, stop{b.stop}
, offset{b.offset}
, grouping{b.grouping}
, spacing{b.spacing}
, opacity{b.opacity}
, mode{b.mode}
, speed{b.speed}
, intensity{b.intensity}
, palette{b.palette}
, custom1{b.custom1}
, custom2{b.custom2}
, custom3{b.custom3}
, startY{b.startY}
, stopY{b.stopY}
, colors{b.colors}
, options{b.options}
{}

uint16_t start;
uint16_t stop;
uint16_t offset;
uint8_t grouping;
uint8_t spacing;
uint8_t opacity;
uint8_t mode;
uint8_t speed;
uint8_t intensity;
uint8_t palette;
uint8_t custom1;
uint8_t custom2;
uint8_t custom3;
// also include check1/2/3 ?
uint8_t startY;
uint8_t stopY;
std::array<uint32_t, NUM_COLORS> colors;
uint16_t options;
};

// main "strip" class
class WS2812FX { // 96 bytes
typedef uint16_t (*mode_ptr)(); // pointer to mode function
Expand All @@ -798,6 +843,10 @@ class WS2812FX { // 96 bytes
static WS2812FX* instance;

public:
WS2812FX(const WS2812FX&) = delete;
WS2812FX(WS2812FX&&) = default;
WS2812FX& operator=(const WS2812FX&) = delete;
WS2812FX& operator=(WS2812FX&&) = default;

WS2812FX() :
paletteBlend(0),
Expand Down Expand Up @@ -883,7 +932,7 @@ class WS2812FX { // 96 bytes
inline void trigger() { _triggered = true; } // Forces the next frame to be computed on all active segments.
inline void setShowCallback(show_callback cb) { _callback = cb; }
inline void setTransition(uint16_t t) { _transitionDur = t; } // sets transition time (in ms)
inline void appendSegment(const Segment &seg = Segment()) { if (_segments.size() < getMaxSegments()) _segments.push_back(seg); }
inline void appendSegment(Segment &&seg = Segment()) { if (_segments.size() < getMaxSegments()) _segments.push_back(std::move(seg)); }
inline void suspend() { _suspend = true; } // will suspend (and canacel) strip.service() execution
inline void resume() { _suspend = false; } // will resume strip.service() execution

Expand Down
50 changes: 11 additions & 39 deletions wled00/FX_fcn.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,18 +91,6 @@ uint8_t Segment::_clipStartY = 0;
uint8_t Segment::_clipStopY = 1;
#endif

// copy constructor
Segment::Segment(const Segment &orig) {
//DEBUG_PRINTF_P(PSTR("-- Copy segment constructor: %p -> %p\n"), &orig, this);
memcpy((void*)this, (void*)&orig, sizeof(Segment));
_t = nullptr; // copied segment cannot be in transition
name = nullptr;
data = nullptr;
_dataLen = 0;
if (orig.name) { name = static_cast<char*>(malloc(strlen(orig.name)+1)); if (name) strcpy(name, orig.name); }
if (orig.data) { if (allocateData(orig._dataLen)) memcpy(data, orig.data, orig._dataLen); }
}

// move constructor
Segment::Segment(Segment &&orig) noexcept {
//DEBUG_PRINTF_P(PSTR("-- Move segment constructor: %p -> %p\n"), &orig, this);
Expand All @@ -113,26 +101,6 @@ Segment::Segment(Segment &&orig) noexcept {
orig._dataLen = 0;
}

// copy assignment
Segment& Segment::operator= (const Segment &orig) {
//DEBUG_PRINTF_P(PSTR("-- Copying segment: %p -> %p\n"), &orig, this);
if (this != &orig) {
// clean destination
if (name) { free(name); name = nullptr; }
stopTransition();
deallocateData();
// copy source
memcpy((void*)this, (void*)&orig, sizeof(Segment));
// erase pointers to allocated data
data = nullptr;
_dataLen = 0;
// copy source data
if (orig.name) { name = static_cast<char*>(malloc(strlen(orig.name)+1)); if (name) strcpy(name, orig.name); }
if (orig.data) { if (allocateData(orig._dataLen)) memcpy(data, orig.data, orig._dataLen); }
}
return *this;
}

// move assignment
Segment& Segment::operator= (Segment &&orig) noexcept {
//DEBUG_PRINTF_P(PSTR("-- Moving segment: %p -> %p\n"), &orig, this);
Expand Down Expand Up @@ -1084,7 +1052,7 @@ uint32_t IRAM_ATTR_YN Segment::getPixelColor(int i) const
return strip.getPixelColor(i);
}

uint8_t Segment::differs(const Segment& b) const {
uint8_t Segment::differs(const SegmentSettings& b) const {
uint8_t d = 0;
if (start != b.start) d |= SEG_DIFFERS_BOUNDS;
if (stop != b.stop) d |= SEG_DIFFERS_BOUNDS;
Expand All @@ -1099,6 +1067,7 @@ uint8_t Segment::differs(const Segment& b) const {
if (custom1 != b.custom1) d |= SEG_DIFFERS_FX;
if (custom2 != b.custom2) d |= SEG_DIFFERS_FX;
if (custom3 != b.custom3) d |= SEG_DIFFERS_FX;
// also consider check1/2/3 ?
if (startY != b.startY) d |= SEG_DIFFERS_BOUNDS;
if (stopY != b.stopY) d |= SEG_DIFFERS_BOUNDS;

Expand Down Expand Up @@ -1786,11 +1755,14 @@ Segment& WS2812FX::getSegment(unsigned id) {
void WS2812FX::resetSegments() {
_segments.clear(); // destructs all Segment as part of clearing
#ifndef WLED_DISABLE_2D
segment seg = isMatrix ? Segment(0, Segment::maxWidth, 0, Segment::maxHeight) : Segment(0, _length);
if (isMatrix) {
_segments.emplace_back(0, Segment::maxWidth, 0, Segment::maxHeight);
} else {
_segments.emplace_back(0, _length);
}
#else
segment seg = Segment(0, _length);
_segments.emplace_back(0, _length);
#endif
_segments.push_back(seg);
_segments.shrink_to_fit(); // just in case ...
_mainSegment = 0;
}
Expand Down Expand Up @@ -1838,12 +1810,12 @@ void WS2812FX::makeAutoSegments(bool forceReset) {
// there is always at least one segment (but we need to differentiate between 1D and 2D)
#ifndef WLED_DISABLE_2D
if (isMatrix)
_segments.push_back(Segment(0, Segment::maxWidth, 0, Segment::maxHeight));
_segments.emplace_back(0, Segment::maxWidth, 0, Segment::maxHeight);
else
#endif
_segments.push_back(Segment(segStarts[0], segStops[0]));
_segments.emplace_back(segStarts[0], segStops[0]);
for (size_t i = 1; i < s; i++) {
_segments.push_back(Segment(segStarts[i], segStops[i]));
_segments.emplace_back(segStarts[i], segStops[i]);
}
DEBUG_PRINTF_P(PSTR("%d auto segments created.\n"), _segments.size());

Expand Down
4 changes: 1 addition & 3 deletions wled00/json.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,7 @@ bool deserializeSegment(JsonObject elem, byte it, byte presetId)

//DEBUG_PRINTLN(F("-- JSON deserialize segment."));
Segment& seg = strip.getSegment(id);
//DEBUG_PRINTF_P(PSTR("-- Original segment: %p (%p)\n"), &seg, seg.data);
const Segment prev = seg; //make a backup so we can tell if something changed (calling copy constructor)
//DEBUG_PRINTF_P(PSTR("-- Duplicate segment: %p (%p)\n"), &prev, prev.data);
const SegmentSettings prev(seg); //make a backup so we can tell if something changed

int start = elem["start"] | seg.start;
if (stop < 0) {
Expand Down
7 changes: 3 additions & 4 deletions wled00/led.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,11 @@ void setValuesFromSegment(uint8_t s)


// applies global legacy values (col, colSec, effectCurrent...)
// problem: if the first selected segment already has the value to be set, other selected segments are not updated
void applyValuesToSelectedSegs()
{
// copy of first selected segment to tell if value was updated
unsigned firstSel = strip.getFirstSelectedSegId();
Segment selsegPrev = strip.getSegment(firstSel);
// compare against first selected segment to tell if value was updated
const unsigned firstSel = strip.getFirstSelectedSegId();
const Segment& selsegPrev = strip.getSegment(firstSel);
for (unsigned i = 0; i < strip.getSegmentsNum(); i++) {
Segment& seg = strip.getSegment(i);
if (i != firstSel && (!seg.isActive() || !seg.isSelected())) continue;
Expand Down