Add UUID id to LEDStripEffect

[rbergen]

Description

This adds an automatically generated and JSON-persisted (UU)ID to LEDStripEffect, exposes it via the /effects endpoint and allows its use as the effect identifier in a number of other endpoints (with the support of a new member function in EffectManager).
This is a possible implementation of the idea discussed in #327, and meant more to develop an opinion on the addition than an actual proposal to include it. For one, the API documentation would have to be updated before this should be merged.
As such, I am primarily looking forward to collaborators' feedback.

Contributing requirements

• I read the contribution guidelines in CONTRIBUTING.md.
• I understand the BlinkenPerBit metric, and maximized it in this PR.
• I selected main as the target branch.
• All code herein is subjected to the license terms in COPYING.txt.

[robertlipe]

OK, I'll stay out of the details. I just cringe at seeing those old style loops as they're so often misused and the index is rarely what you _actually_ want, but maybe it is in this case. Go for it. Projects like this that declare the use of PlatformIO have the liberty of declaring changes in language levels pretty freely. It's not like people are using Clang for VAX or MSVC for MIPS to build this. The language level can be "whatever the PIO-current gcc supports". But again, I won't hard-sell that. You are using an ESP-ism in the code, though. Is it decreed that only ESP32 matters to this project? The base projects allow for the hugely popular STM32 and various RISC-V SOCs like GD32V, CH32 family, BL808, BL60x, BL70x, BL615, etc. The bad news is I don't think there's a relevant C++ standard, but this code is built atop other Arduino libraries and there seems to be UUID code there and in PlatformIO. I have no experience with any of them...and TBC, if this project is intended to run ONLY on (multi-core) ESP32, using code from the ESP libraries is fair game. Consider sorting the struct member order by size to reduce internal wasted space on alignment. Rats. More details. Sorry. What is the expected life cycle of these UUIDs? Do they persist across reboots? (If not, would just the address of the object work?) I see it's serialized in the JSON, so does it become part of an exposed persistent API? Another implementation detail, from_chars() is likely faster, but this looks like a pretty low-volume path. Overall, I find the implementation simple enough. I find UUIDs generally distasteful, but if this solves some problem for you, it's hard to argue against it in terms of complexity. It's a straight-forward implementation, so the core design looks good to me. So I typed a lot of words (I do that...) but I'd approve this.

…

On Fri, Jun 23, 2023 at 12:04 PM Rutger van Bergen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/effectmanager.h <#332 (comment)> : > @@ -649,6 +649,15 @@ class EffectManager : public IJSONSerializable return _iCurrentEffect; } + size_t GetEffectIndexForID(const esp_uuid::UUID& id) const + { + for (size_t i = 0; i++; i < _vEffects.size()) I'm not sure how any of this yields the index of the effect. Your first partial snippet doesn't show how to retrieve the index of the effect the for loop is at. The second snippet closes with a double reference to a variable v that doesn't exist outside of the find_if() lambda, and in that lambda uses a struct_type that doesn't exist at all. In summary, it's not clear to me how to actually translate this to a better implementation of the current loop logic. Maybe the code could be made more efficient by using a range for loop and keeping track of the index using a manually increased i. (That approach could then be simplified somewhat by using a range for loop initializer expression. However, that's a C++20 addition and this project uses C++17. So that's not an option at the moment.) With the number of effects we're likely to look at, I doubt it would make much of a difference though. In any case, we're now engaging in a discussion about the details of the implementation of one function. I opened this PR mainly to get feedback on the "UUID ID" approach at a more conceptual level. I'm curious if you have any input concerning that? — Reply to this email directly, view it on GitHub <#332 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD374PSUIXG5VRSLZB6LXMXEBHANCNFSM6AAAAAAZRQWB4I> . You are receiving this because you commented.Message ID: ***@***.*** com>

[rbergen]

The ESP32 thing is a valid point, and actually part of a conversation that's now been started with the (former) maintainer of the library I used for now, in this PR: protohaus/ESP_UUID#2. As you can see, he already indicated that he's willing to commit code that adds support for the ESP8266. And if that doesn't materialize, there are alternatives that I would seriously consider.

Beyond that, I pledge with my hand on my heart that I will review the code in this PR in full before removing the draft status, and improve it where appropriate. As it stands, I'm still not convinced this UUID addition is even necessary. I'm willing to kill my own code with fire if the final conclusion is that it isn't.

[robertlipe]

Night driver today won't run on even 8266. I helped some guy debug S2 and found numerous places in the code that assumed two cores. I'm not convinced that some effects couldn't run on a single core, but probably not well with the current scheduler inside a scheduler implementation. So esp8266 is already an outsider.

…

On Fri, Jun 23, 2023, 2:22 PM Rutger van Bergen ***@***.***> wrote: The ESP32 thing is a valid point, and actually part of a conversation that's now been started with the (former) maintainer of the library I used for now, in this PR: protohaus/ESP_UUID#2 <protohaus/ESP_UUID#2>. As you can see, he already indicated that he's willing to commit code that adds support for the ESP8266. And if that doesn't materialize, there are alternatives that I would seriously consider. Beyond that, I pledge with my hand on my heart that I will review the code in this PR in full before removing the draft status, and improve it where appropriate. As it stands, I'm still not convinced this UUID addition is even necessary. I'm willing to kill my own code with fire if the final conclusion is that it isn't. — Reply to this email directly, view it on GitHub <#332 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD32Y6FGSWXK3RK5T2HDXMXUI3ANCNFSM6AAAAAAZRQWB4I> . You are receiving this because you commented.Message ID: ***@***.***>

[davepl]

As a comment from the peanut gallery, there are no doubt many efficient ways to do things, and perhaps a different type of collection would be easier to search, I dunno. But I’d say premature optimization is the root of a lot of evil, so unless it’s called frequently in a way that impacts perf, I wouldn’t change what isn’t broken, unless it means that we’re inextricably tried to it, and I don’t think we are. So, unless there’s a salient technical objection, no code is the best code! As an aside, I have no objection to anyone upgrading the code to c++20 as long as it passes CI and runs on spectrum, mesmerizer, led strip, and the s3 platform. We just went from 14 to 17 not long ago, and it wasn’t painful. - Dave

…

On Jun 23, 2023, at 10:53 AM, Robert Lipe ***@***.***> wrote: You are using an ESP-ism in the code, though. Is it decreed that only ESP32 matters to this project? The base projects allow for the hugely popular STM32 and various RISC-V SOCs like GD32V, CH32 family, BL808, BL60x, BL70x, BL615, etc. The bad news is I don't think there's a relevant C++ standard, but this code is built atop other Arduino libraries and there seems to be UUID code there and in PlatformIO. I have no experience with any of them...and TBC, if this project is intended to run ONLY on (multi-core) ESP32, using code from the ESP libraries is fair game.

[rbergen]

I think the key point is in the middle of your comment: "no code is the best code". The question we still haven't answered is if we actually need this. As it was @Louis-Riel who suggested (something like) this in #332, I'm curious to his opinion on the discussion(s) we've been having about this.

Just for completeness my two cents on the other things:

• I think the exact implementation of the lookup loop in GetEffectIndexForID() is really a case of po-tay-to/po-tah-to, at least with the number of effects we're likely to see. Switching to a different container for the effects requires a massive overhaul of EffectManager, so we'd need a very good reason to do that.
• I don't really care if we're on C++17 or C++20. I have not yet seen anything in C++20 that I regretted not having available to me in C++17. C++23 on the other hand will introduce some things that I've considered "obvious gaps" in the language/libraries for a long time. I just wouldn't use this particular change/PR as the trigger for such a switch. I couldn't actually make it on my own anyway, because I can't confirm operation of the result on led strip or s3.

(By the way, I actually had to look up the meaning of "peanut gallery". I think it's a North-Americanism that never made it to this side of the Atlantic. 😄)

[rbergen]

Just to tie off some other points in @robertlipe's earlier comment:

• If you look at the code I've written in this project then you'll see that I basically always use range for loops. Unless I need the index, which is indeed the case here.
• I picked this UUID library because it provides built-in support for String, which is what we've settled on as the "preferred string type" within NightDriverStrip - with a few specific exceptions.
• The library was called "ESP UUID" by its author, but I don't actually believe there's any code in there that wouldn't run on other Arduino-based devices. If I remember correctly, its dependencies are Arduino and ArduinoJson.

[rbergen]

I'm closing this after an extended period of no follow-up from the original requester. If the need bubbles up again, we can reopen (provided I remember this is here).