Make Segment class non-copyable and move-only

[JoaDick]

This is really pure coincidence... Apparently @TripleWhy and me have been working on the same topic at the same time:
Eliminating copies of the Segment class. See the first PR here: #4578

The reasons for this alternative PR - after hesitating for a long time - are the following:

• This PR prohibits copying of the Segment and WS2812FX class entirely.
• It is less intrusive, preserving most of the existing code.
• It does not treat a segment object as raw memory.

The last point of this list is the most important one IMHO. Using memcpy(), reinterpret_cast(), and alike on objects seems very convenient in the beginning. But they are also very prone to becoming a really bad pitfall in the future! And the performance gain - if any - is very disputable.
That is also why many C++ coding standards from the industry (MISRA, AUTOSAR, SEI-CERT ...) do not allow using these; see here for example:
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rc-memset
https://wiki.sei.cmu.edu/confluence/display/cplusplus/OOP57-CPP.+Prefer+special+member+functions+and+overloaded+operators+to+C+Standard+Library+functions

As said, I was hesitating for a long time, and studied the PR and its discussion intensively. Just wait for the other PR to become available? Or incorporate its suggestions into this C++ compliant PR and give credit?

Well... here's the result. I really hope no one is offended by this alternative!

Summary by CodeRabbit

• New Features

  • Enhanced LED segment configuration improves control and responsiveness during visual effects.

• Refactor

  • Streamlined how segments are managed, resulting in more efficient performance and smoother transitions.

• Chores

  • Cleaned up legacy code and removed redundant debug outputs, contributing to increased stability.

[coderabbitai]

Walkthrough

The changes introduce a new structure (SegmentSettings) to encapsulate segment properties and modify how segments are managed. In both header and function source files, the copy constructor and copy assignment operator for Segment have been deleted, and the differs method now accepts a SegmentSettings parameter instead of a Segment. The modifications further update the segment appending mechanism by switching to move semantics, as well as replacing push_back with emplace_back for in-place construction. Additionally, variable constness is enforced in one of the functions to enhance safety.

Changes

Files	Change Summary
wled00/FX.h, wled00/FX_fcn.cpp	- Added new struct SegmentSettings to encapsulate segment properties. - Deleted the copy constructor and copy assignment operator for Segment. - Updated the differs method in Segment to take a SegmentSettings parameter. - Changed appendSegment (in FX.h) to accept an rvalue reference and, in FX_fcn.cpp, replaced push_back with emplace_back.
wled00/json.cpp	- Modified the deserializeSegment function to use a SegmentSettings backup (const SegmentSettings prev(seg);) instead of copying the entire Segment. - Removed commented-out debug print statements.
wled00/led.cpp	- Updated variable declarations in the applyValuesToSelectedSegs function: - Changed unsigned firstSel to const unsigned firstSel. - Changed Segment selsegPrev to a const Segment& selsegPrev, enhancing immutability and safety of the segment data.

✨ Finishing Touches

• 📝 Generate Docstrings

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (3)

wled00/led.cpp (1)

30-32: Use a narrower type for firstSel if suitable.
If getFirstSelectedSegId() returns a smaller type, using a dedicated type (e.g., uint8_t) can enhance clarity and ensure the variable’s range is consistent. Also consider verifying that firstSel is valid if no segment is selected.

wled00/FX_fcn.cpp (1)

1055-1070: Consider adding comparisons for check1, check2, and check3.
The comment at line 1070 hints at these fields possibly affecting segment behavior. If they do, leaving them out could mask meaningful changes.

wled00/FX.h (1)

790-830: New SegmentSettings struct
A helpful encapsulation of segment properties. The comment at line 825 hints at missing check1/check2/check3. If you want complete parity with Segment, consider adding them.

Would you like me to generate a patch integrating these flags here, or open a new issue to track it?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fa2949a and 514688e.

📒 Files selected for processing (4)

• wled00/FX.h (9 hunks)
• wled00/FX_fcn.cpp (4 hunks)
• wled00/json.cpp (1 hunks)
• wled00/led.cpp (1 hunks)

🔇 Additional comments (10)

wled00/json.cpp (1)

36-36: Ensure SegmentSettings covers all needed fields.
Switching from copying the entire Segment to SegmentSettings is more efficient and aligns with the move-only approach. However, check if additional fields (e.g., check1/check2/check3) also need to be included to track their changes properly.

wled00/FX_fcn.cpp (1)

1758-1762: Check zero-dimension scenarios in matrix vs. strip mode.
When isMatrix is true, the new code uses Segment::maxWidth and Segment::maxHeight for the default segment. If these values are zero or invalid, it might create an invalid segment. Consider adding validation or fallback logic.

wled00/FX.h (8)

19-19: Include <array> looks good
This inclusion is necessary to use std::array for strongly typed arrays.

---

389-390: Forward declaration of SegmentSettings
Helps keep dependencies ordered and avoids circular inclusion issues.

---

420-420: Use of std::array for colors
Replacing the C-style array with std::array is a solid move toward improved type safety and modern C++ best practices.

---

571-571: Deleted copy constructor for Segment
This aligns with the move-only design strategy, preventing accidental copies.

---

586-586: Deleted copy assignment operator for Segment
Ensures that Segment cannot be copied, reinforcing move-only semantics.

---

846-849: Move-only semantics for WS2812FX
Deleting the copy constructor and assignment operator while defaulting the move counterparts solidifies move-only usage for this class.

---

935-935: Switched appendSegment to take an rvalue reference
Accommodates move semantics, avoiding unnecessary copies of Segment objects.

---

632-632:

❓ Verification inconclusive

differs now expects SegmentSettings
Be sure to update all call sites passing a Segment so they construct or have a SegmentSettings argument.

Please run the following script to detect any remaining references that still use the old signature:

---

🏁 Script executed:

#!/bin/bash
# Searching for code references to differs(Segment
rg "differs\s*\(\s*Segment\b" -A 2

Length of output: 36

---

Action: Verify All 'differs' Call Sites Now Use 'SegmentSettings'

The recent grep search did not return any instances where a Segment is passed to differs. This suggests that the conversion to requiring a SegmentSettings argument has been applied everywhere. However, because the automated search produced no output (which might also indicate that some edge cases were missed), please perform a manual review of the call sites for differs to ensure thorough coverage.

• Verify that no call sites are inadvertently still passing a Segment.
• Confirm that all invocations build or supply a proper SegmentSettings argument.

[TripleWhy]

Hmmm
If you were to make SegmentSettings a member of Segment, and remove the relevant members from Segment, this approach would be clearly better, I think. Could have more advantages, too. However, that's probably a lot of work.

With this current proposal, note, that SegmentSettings stores more than what is needed for applyValuesToSelectedSegs. 🤷

The reason I did not forbid copies however is that it currently looks like it's going to be needed again... Which is also why I didn't bother avoiding memcpy, which is used internally by the copy constructor (which doesn't make it less dangerous, just less obvious).

[TripleWhy]

Oh right, what I forgot to mention is that I also only call differs if that would actually do anything, i.e. if stateChanged isn't true already. And I haven't found a way to trigger that from the UI... So all of this copy and differ stuff might actually be obsolete? Does anyone know more about that?

[blazoncek]

So all of this copy and differ stuff might actually be obsolete?

Indeed it may be. differs() is a relic from 0.11 era where there was no stateChanged flag. Tracing the workflow may prove that.

[blazoncek]

@JoaDick what was the driving force for removing/prohibiting segment copy?

BTW I'm working on a feature that depends on segment copy (or at least segment settings + effect data + pixel state data copy).

[blazoncek]

So all of this copy and differ stuff might actually be obsolete?

Checked the code: It isn't. differs() is still valid.

[TripleWhy]

Oh right, what I forgot to mention is that I also only call differs if that would actually do anything, i.e. if stateChanged isn't true already. And I haven't found a way to trigger that from the UI... So all of this copy and differ stuff might actually be obsolete? Does anyone know more about that?

I found the reason:

deserializeSegment calls seg.setOption(SEG_OPTION_ON, getBoolVal(elem["on"], seg.on));, unconditionally, here.
setOption then sets stateChanged (because SEG_OPTION_ON is neither SEG_OPTION_SELECTED nor SEG_OPTION_RESET). It doesn't compare the old and new values.

Ergo: stateChanged is always set, the result of differs never matters, whether all effect parameters should be compared or not is irrelevant, and the copy is never needed. At least if that stays as it is.

[JoaDick]

Hmmm If you were to make SegmentSettings a member of Segment, and remove the relevant members from Segment, this approach would be clearly better, I think. Could have more advantages, too. However, that's probably a lot of work.

Fully agree, and I'd appreciate such a refactoring in the future. For this PR I resisted to do so to keep it less intrusive.

With this current proposal, note, that SegmentSettings stores more than what is needed for applyValuesToSelectedSegs. 🤷

As suggested by @blazoncek, applyValuesToSelectedSegs() now stores just a reference to the first selected segment, and no more a copy or the new SegmentSettings.

The reason I did not forbid copies however is that it currently looks like it's going to be needed again... Which is also why I didn't bother avoiding memcpy, which is used internally by the copy constructor (which doesn't make it less dangerous, just less obvious).

Thanks for this hint; I wasn't aware of that change and will have a closer look at it.

[JoaDick]

@JoaDick what was the driving force for removing/prohibiting segment copy?

The main reason is probably very similar to what @TripleWhy is striving for: providing a possibility that effects can be implemented in terms of a class and not just as a plain function. The current way stems back from very early roots - Kitesurfer's WS2812FX - and is still suffering from the same limitations: keeping data persistent between frames is only possible through this kind of a workaround via SEGENV.
Effect implementations could be made much more simpler and understandable, if the persistent data would just be a class member. Also, certain helper functions could be embedded as class members, which would increase encapsulation.

In my playground, I tried something very similar (but much smaller) as @TripleWhy's proposal from here: #4549
The idea is that these new kind of effects shall be implemented as a class, which derives from a specific base class. They just have to implement a virtual showEffect() method, where all the logic lives. This logic could mostly be migrated 1:1 from the existing method-based implementation into the new showEffect() method - i.e. just a renaming of the function.
An essential claim is also that both, the existing method-based and the new class-based implementations, can coexist and don't require immediate changes.
However, the copying of Segment class and its raw byte-based memory management is making things really difficult or even impossible.
If you're interested, I'd be happy to prepare a draft PR for review.

Another driving force are SW design considerations. In a nutshell, only so called value-classes shall be copyable. Most other kind of classes - in particular logic-classes like Segment - shall not be copyable. Although it might seem convenient sometimes, it will most certainly strike back in the future.

BTW I'm working on a feature that depends on segment copy (or at least segment settings + effect data + pixel state data copy).

Thanks a lot for this hint - I'll have a closer look at that as well. Hopefully we can figure out an idea to bring all that great stuff together!

[TripleWhy]

As suggested by @blazoncek, applyValuesToSelectedSegs() now stores just a reference to the first selected segment, and no more a copy or the new SegmentSettings.

In my understanding that would alter the current behavior in funny ways. Either that, or I'm missing something

[JoaDick]

Ah, now I think I understand what you mean. In deed this if() in applyValuesToSelectedSegs() is weird, and the suggested change might alter the existing behaviour.
Probably the best for this PR is to go back and use SegmentSettings there as well. Although a few surplus bytes are stored - as you pointed out - this shouldn't be a problem, but retain the correct behaviour.
Or we figure out how this if() ought to look like...

[JoaDick]

BTW I'm working on a feature that depends on segment copy (or at least segment settings + effect data + pixel state data copy).

Whoa, that's quite a lot and really impressive!
Hopefully I understood all that correctly in that short time, so please correct me if I got anything wrong.
Apparently it is about this issue, and the copy of the Segment is required here.

As a quick solution, I'd suggest to replace the copy constructor with a clone() method. That might sound like a cheap trick, because from a technical perspective it is doing just a copy. But from a design perspective, it will clearly state the intent when it is used, and accidental copies are avoided.
Nevertheless, this workaround doesn't solve the underlying problem: the raw memory handling of Segment::data, which of course has to be copied as well. And exactly that raw byte copying makes it nearly impossible to use "real" C++ objects and pointers to them inside Segment::data. Such rawly copied pointers still point to the old location 😟 To my understanding, this appears to be a major reason why the ParticleSystem had to implement such a complicated memory management.
However, Segment::data might become obsolete one day when we follow @TripleWhy's or my idea of a class based effect implementation. As a consequence, these new effects would also have to provide a clone() method - but that might be acceptable.

[blazoncek]

Apparently it is about this issue, and the copy of the Segment is required here.

Yes.

Just FYI the current implementation in that branch is working miraculously on ESP8266 yielding 55 FPS on a 16x16 matrix. 40+ FPS when 3 effects are stacked one atop another and blended.

AFAIK @DedeHai needed to implement (complex) memory management since PS needs a lot of memory and running i.e. 3 layers (with transition going on) exhausted available RAM.

As for suggestion: I am still a newbie as far as C++ goes. I may not understand everything you or @TripleWhy are trying to pass on to me.

[TripleWhy]

Nevertheless, this workaround doesn't solve the underlying problem: the raw memory handling of Segment::data, which of course has to be copied as well. And exactly that raw byte copying makes it nearly impossible to use "real" C++ objects and pointers to them inside Segment::data. Such rawly copied pointers still point to the old location 😟 To my understanding, this appears to be a major reason why the ParticleSystem had to implement such a complicated memory management.

Well, you know what would solve all of that, right? ;)
You might not even need to copy the segment with this, or if you do, it should be a simpler avoid memcpy.

However, Segment::data might become obsolete one day when we follow @TripleWhy's or my idea of a class based effect implementation. As a consequence, these new effects would also have to provide a clone() method - but that might be acceptable.

Yeah I thought it could be that way, too. But transitions from old um, states, to new states don't just include the effect and all of its parameters, but also the way you set up segments. These days I don't use segments much, but before 2D setups, you had to define separate presets with completely different segment definitions to achieve a simple color transition from left to right or top to bottom. The transition should still work when switching between those setups. Thus all old and new segment definitions and effect parameters need to be known during the transition.

[JoaDick]

@blazoncek - OK, might have been a bit too much information at once in my last comments... What I wanted to say with those many words:

• With some minor modifications, this PR could be made compatible with your concept branch. Without any performance impact 👍
• Unnecessary and accidental copies of Segment are gone.
• But the existing problems caused by the raw memory management inside Segment still remain.

[JoaDick]

Well, you know what would solve all of that, right? ;) You might not even need to copy the segment with this, or if you do, it should be a simpler avoid memcpy.

This would surely be a first step in the right direction 👍

Yeah I thought it could be that way, too. But transitions from old um, states, to new states don't just include the effect and all of its parameters, but also the way you set up segments. [...] Thus all old and new segment definitions and effect parameters need to be known during the transition.

Very well summarized. I think we're having quite the same thoughts on this topic.

Now it would be really good to get feedback if there is general interest in approaching this direction towards class based effects.
We've already had some really good ideas and concepts for that - which might probably be too much for one PR without knowing the general direction. (Especially for a WLED newbie-contributor like me 😅 )

[blazoncek]

I've read that again. Hopefully getting it right.

The main issue is that _data is raw byte array. Right?
Placing objects in it (in current implementation) is prone to errors especially if those objects allocate their own memory. Right?

Rewriting everything from scratch (this or the other PR) aside the easy "solution" is to prohibit memory allocation from within objects placed into _data (i.e. using placement new or other more unsafe methods).

This, of course, contradicts PS approach which does need extra memory and has to come with its own solution.

IMO the solution lies somewhere in the middle. The approach by @TripleWhy is very radical (and probably unfit for every existing effect, especially PS system). If the effect functions are converted into instantiated objects then 2 things are missing: object creation and destruction. When do we create effect object? At JSON parsing time, when effect ID actually change, or later when strip is being servidced? The same goes with destruction. However, when there are transitions active both effects should be running so we cannot destroy previous effect when selecting a new one.

I have mentioned somewhere that current/legacy approach to effects has an issue at the time effect no longer runs. There is no notification to effect function to cleanup after itself (destructor). Currently the call variable is the only way for effect to know when it started (or was restared!!!). Restarting would not need entire creation process (constructor), just reiniting some variables. A definition of when effect starts and notification when effect ceases to run (there will be no more executions of that function) are two missing things to safely store any data from within effect function apart from _data location. Being object or not.

All that being said I am not against the change, however, WLED is a very complex system so any change has to be thought over several times.

[TripleWhy]

Uh... This is the disabled Segment copy PR, I don't think we should go into detail about Effects here, let's head over there if you want to discuss it. However before you copy the comment over there, I'd appreciate if you'd simply take a look at the PR

[JoaDick]

The main issue is that _data is raw byte array. Right? Placing objects in it (in current implementation) is prone to errors especially if those objects allocate their own memory. Right?

Exactly, this is a big drawback.

Rewriting everything from scratch (this or the other PR) aside the easy "solution" is to prohibit memory allocation from within objects placed into _data (i.e. using placement new or other more unsafe methods).

I'd not prohibit that at once, but provide alternatives to be able to move away from it step by step.

This, of course, contradicts PS approach which does need extra memory and has to come with its own solution.

It is essential to remain compatible. Breaking changes that require adjusting "everything" are a no-go. Of course that might make some parts tricky...

[...] then 2 things are missing: object creation and destruction. When do we create effect object? [...] The same goes with destruction. However, when there are transitions active both effects should be running so we cannot destroy previous effect when selecting a new one.

Exactly, these triggers are something I was looking for a long time ago. And gave up back then...
However, after now digging deeper into the code, it should be possible to implement a workaround - although it's probably also going to be tricky...

Restarting would not need entire creation process (constructor), just reiniting some variables. A definition of when effect starts and notification when effect ceases to run (there will be no more executions of that function) are two missing things to safely store any data from within effect function apart from _data location. Being object or not.

This would surely be helpful. But IMHO rather an optimization, and not vital from the very start.

All that being said I am not against the change, however, WLED is a very complex system so any change has to be thought over several times.

That's why I try to keep changes small and less intrusive. Better several small changes one after another than on big bang.
Thanks a lot your feedback, I highly appreciate it!

[blazoncek]

@TripleWhy I can see your point so I'll cut that short: As I'm working on a feature that requires segment copy, this PR does not get a green light from me. Others may vote otherwise though.

IMO there should be a new PR that deals with informing effect function when it is first executed and when it is last executed, prior to any other change. The best place to start that hunt is in resetIfRequired(). Caveat, that function may be invoked several times during a life of effect function. It is sure to be invoked after the last successful run of effect function before new effect takes place. The other place is startTranstion(), this one is sure to be invoked before effect function changes (but can be called prior and after that as well).
That will go hand-in-hand with conversion of effect functions into classes however that is implemented.

[JoaDick]

Ack, that's fine for me. Looking forward to seeing your layering branch on main!
Maybe we can then pick this discussion up again; in particular wrt. the raw memory management - which is the main issue, and not necessarily the copying itself.
And thanks for your hints on the triggers for a possible effect base class. I'll definitely have a closer look at them.