Unique effect identifier

[Louis-Riel]

I propose that we should have a unique ephemeral id value that gets generated for each effect. For example, the GhostWave effect comes up twice, so I can't use the name as a key. For the moment, I internally keep an internal name for the key, and it's the same as the effect name, except when there are multiples, which it would become GhostWave-1 and GhostWave-2. That makes it difficult to manage the list, but doable. It would make for a better data model for the list of effects.

Thanks, Louis.

[rbergen]

@Louis-Riel Considering the fact that manually maintaining these identifiers is asking for trouble (because humans are humans) and not even possible now that effects can be duplicated at runtime, I'd argue these identifiers would have to be a) automatically generated and b) guaranteed to still be unique (à la GUID/UUID).
Would you agree?

[davepl]

But then the question is what doe that IID represent? Since an effect (like Ghostwave) can appear more than once, it can’t map to the effect, it has to map to that effect entry I think. But what does that mean, its position and settings? Since the class names are unique, we could be using NAMEOF(SomeEffectClass) to ID the class, but that doesn’t account for multiple copies of the same effect. - Dave

…

On Jun 20, 2023, at 10:43 PM, Rutger van Bergen ***@***.***> wrote: @Louis-Riel <https://github.com/Louis-Riel> Considering the fact that manually maintaining these identifiers is asking for trouble (because humans are humans) and not even possible now that effects can be duplicated at runtime, I'd argue these identifiers would have to be a) automatically generated and b) guaranteed to still be unique (à la GUID/UUID). Would you agree? — Reply to this email directly, view it on GitHub <#327 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF34U2KCHU2WJIM47E3XMKCYHANCNFSM6AAAAAAZN7FJTA>. You are receiving this because you are subscribed to this thread.

[rbergen]

As I understand it, the ID would represent one single instance of an effect, on one device. As the devices running NightDriverStrip don't communicate with each other directly (yet), I think the "on one device" thing is something we can immediately forget about again.
Which indeed basically boils down to it representing "effect (type), position and settings".

What I can imagine in a practical sense is that a UUID is generated in the LEDStripEffect constructor. The UUID would then be exposed to the web UI via the /effects endpoint, and also persisted in effects.cfg to cover device restarts while the web UI stays open. Certain endpoints that now accept an effect index could then (also) accept a UUID to indicate what effect they're supposed to work on/with. If I'm honest, I don't think there's any functional scenario that can't also be handled using effect indices alone. That said, I can imagine it being easier to tie web UI logic and/or data to a specific effect using an ID that doesn't change when an effect moves in the effect list - which the effect index now can. This is because we've recently added endpoint with which effects can be moved, duplicated and deleted.

What's critical for the whole thing to work is that the generated IDs are truly unique, again in the context of one device. I'm convinced that if duplicates were to occur, the resulting behavior would be almost impossible to debug. (I once ran into a situation where two NICs in one network had the same MAC address, because one of the cards turned out to be imported from another continent - imagine the odds. That also took ages to figure out.)
Another thing to consider is the way to map a UUID back to its the effect instance. One could obviously just choose the "full table scan" approach, but that may become problematic at some point.

All in all, it would be a matter of adding complexity at the back-end to make things easier at the front-end. I'm not sure exactly how to weigh the benefits of the latter versus the downsides of the former.

[davepl]

Being from Microsoft, I want to put it in the registry :-) What about if we just make the FriendlyName unique? Right now there are two effects named Ghostwave, but I’m about to rename one. Then every built in effect will have a unique name The “FriendlyName” should represent an effect class and a set of default values for that class, I’d argue. And the FriendlyName should be unique, and it should be an err if dupes are attempted. That’s my thinking… keep it simple. Text name, use it as a map key where needed, throw an exception on key duplication. - Dave

…

On Jun 21, 2023, at 8:01 AM, Rutger van Bergen ***@***.***> wrote: As I understand it, the ID would represent one single instance of an effect, on one device. As the devices running NightDriverStrip don't communicate with each other directly (yet), I think the "on one device" thing is something we can immediately forget about again. Which indeed basically boils down to it representing "effect (type), position and settings". What I can imagine in a practical sense is that a UUID is generated in the LEDStripEffect constructor. The UUID would then be exposed to the web UI via the /effects endpoint, and also persisted in effects.cfg to cover device restarts while the web UI stays open. Certain endpoints that now accept an effect index could then (also) accept a UUID to indicate what effect they're supposed to work on/with. If I'm honest, I don't think there's any functional scenario that can't also be handled using effect indices alone. That said, I can imagine it being easier to tie web UI logic and/or data to a specific effect using an ID that doesn't change when an effect moves in the effect list - which the effect index now can. This is because we've recently added endpoint with which effects can be moved, duplicated and deleted. What's critical for the whole thing to work is that the generated IDs are truly unique, again in the context of one device. I'm convinced that if duplicates were to occur, the resulting behavior would be almost impossible to debug. (I once ran into a situation where two NICs in one network had the same MAC address, because one of the cards turned out to be imported from another continent - imagine the odds. That also took ages to figure out.) Another thing to consider is the way to map a UUID back to its the effect instance. One could obviously just choose the "full table scan" approach, but that may become problematic at some point. All in all, it would be a matter of adding complexity at the back-end to make things easier at the front-end. I'm not sure exactly how to weigh the benefits of the latter versus the downsides of the former. — Reply to this email directly, view it on GitHub <#327 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCFYIJE2M6AYT4OYFHDLXMMEFVANCNFSM6AAAAAAZN7FJTA>. You are receiving this because you commented.

[rbergen]

Yes, we could do that. I guess I've just been "raised" differently than the "registry way". 🙂

It was hammered into my system that one should never use a display name as a technical identifier. One reason is that the display name can change, and IMHO renaming an effect should indeed be possible. That means that the display name is not an immutable identifier "by design". I assume - and I'm well aware of the implications of using that word - that having a unique, immutable identifier for an effect instance is the whole point of this. If not then I'd argue again that the index of an effect instance in the effect list is just as good an identifier for that effect instance as its name is.

[robertlipe]

We can check for uniqueness at initialization time and enforce it there.

I'm not a fan of either registry keys OR GUIDs if a simple index in the array will do. Do you NEED a UID or is it enough to know that it's the effect in the sixth slot of the table? (And that it can move from build to build or maybe even at runtime. You can enforce that the dynamic case only appends and instead of a delete there's an 'active' flag that goes in the table or is enforced via accessors, etc.)

[davepl]

I was kidding about the registry! Tough room! Technically, the FriendlyName can and should be internal. The text we display to the user should be localized to the current locale. That’s a bug/feature. So I think it’s good and well to make the unique name the key… Dave

…

On Jun 21, 2023, at 10:20 AM, Rutger van Bergen ***@***.***> wrote: Yes, we could do that. I guess I've just been "raised" differently than the "registry way". 🙂 It was hammered into my system that one should never use a display name as a technical identifier. One reason is that the display name can change, and IMHO renaming an effect should indeed be possible. That means that the display name is not an immutable identifier "by design". I assume - and I'm well aware of the implications of using that word - that having a unique, immutable identifier for an effect instance is the whole point of this. If not then I'd argue again that the index of an effect instance in the effect list is just as good an identifier for that effect instance as its name is. — Reply to this email directly, view it on GitHub <#327 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF3HVI5WX7VW4CVZKKLXMMUMPANCNFSM6AAAAAAZN7FJTA>. You are receiving this because you commented.

[rbergen]

If the FriendlyName is made internal then we could indeed make that the unique identifier. It does mean that when an effect is duplicated at runtime, we'll have to force a unique FriendlyName to be chosen; this is currently not a requirement. Also, as already mentioned, there'll be some deferred maintenance to take care of when it comes to existing FriendlyNames. For one, I believe the Spectrum project has quite a few effect instantiations that are now named "Spectrum Standard". And indeed, a new "external" display name should then be added - localized or not.

That said, I think the core question (still) is if a unique identifier is really necessary. Looking at it from one angle, we've been doing ok without one up to this point.

[Louis-Riel]

That works for me. I was not really thinking of adding loginc per say to the unique identifier, just have it generated as part of the initial creation of the effect, and not use it for lookups or anything. Just have it there. This would allow clients to know, when they get the list of effects, if the difference is from:

• effect at position 5 got deleted
• effect at position 13 got added with the same properties as 5
  or is it just that 5 got moved to 13. All and all, it's really just client implementation details. I'm doing that on the client side at the moment.

[rbergen]

@Louis-Riel Could you maybe take a look at #332, and let me know if that would constitute an implementation of your request?

[robertlipe]

In pseudo-c++: std::string ids = ""; int id = 0; while hash_map.contains(final_name = friendly_name + id) { id++; id = to_string(id); } // now it's known to be unique. Save it. hash_map.insert(final_name); Just stick an incrementing number on the end until it fits. If you run out of space before you run out of integers, you have other problems. '4.2 billion variations of a "unique" effect name ought to be enough for anyone' -- another tough room. :-) Or just remove the "too clever" incrementing names, make it an assert, and let the developer figure it out. But if you're going to allow runtime copies and still key by the name, you're probably going to end up with something like that anyway. Won't most of these names melt when displayed on many of the neopixel grids anyway? Does it do a marquee thing so you can read it 2 characters at a time (if) when you display it on an 8x8 like https://www.aliexpress.us/item/3256804155801767.html ? Or does "matrix" inside NightDriver really mean "hub75" which tends to start around 32x32? And I get it - if it doesn't fit, it doesn't fit. That's not a reason NOT to do this. You're not exactly going to make YouTube counter or such ever look right on a display like that. I'm still trying to understand the lingo internal to the project. Sometimes words don't mean quite the common use.

…

On Wed, Jun 21, 2023 at 7:37 PM Rutger van Bergen ***@***.***> wrote: If the FriendlyName is made internal then we could indeed make that the unique identifier. It does mean that when an effect is duplicated at runtime, we'll have to force a unique FriendlyName to be chosen; this is currently not a requirement. Also, as already mentioned, there'll be some deferred maintenance to take care of when it comes to existing FriendlyNames. For one, I believe the Spectrum project has quite a few effect instantiations that are now named "Spectrum Standard". And indeed, a new "external" display name should then be added - localized or not. That said, I think the core question (still) is if a unique identifier is really necessary. Looking at it from one angle, we've been doing ok without one up to this point. — Reply to this email directly, view it on GitHub <#327 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD3ZWQBC6FA2SBK4DPPTXMOHUJANCNFSM6AAAAAAZN7FJTA> . You are receiving this because you commented.Message ID: <PlummersSoftwareLLC/NightDriverStrip/repo-discussions/327/comments/6247072 @github.com>

[rbergen]

Yeah, I though of that - except this is not what the user may want the new FriendlyName to be, and if it's something we allow a user to change after it's been set then it's no longer "stable".
In any case, the FriendlyName is not an internal thing in practice atm. It's not only used in the web UI, but (indeed) also shown on the matrix. Turning it into a proper internal identifier - and replacing it with something else "external" - is quite a bit of work, which is why I thought introducing a new identifier that nobody sees and is unique by its nature would be an easier option... if we need such an identifier at all. But now I'm starting to sound like a scratched record.

[rbergen]

I've opened draft PR #332 with a UUID-based implementation of this idea, mainly to be able to continue the discussion with a possible implementation to look at. I think this implementation is actually pretty clean, but I am biased of course. As such, one and all are invited to review, love or hate, and be honest.