Upon multiple ctors called from LoadEffectFactories()

[robertlipe]

I thought I understood, but I clearly don't. Let me talk through what I thought I understood and someone let me a hand when I sing out of key.

There is a desire to have multiple "handles" into the same effect when there are similar effects in code that act differently. In the UI, they're distinct (they react to up and down and selections in the we menu) and they have different names (shown in the OS title and the web interface) but they're just the object effect, created with different creation params or the same arg list with different values. This works like operator overloading in C++ works in picking the constructor that matches the most arguments most closely, though I can't remember the term for constructor overloads right now. Such constructors will normally grab and retain their additional arguments, squirreling them away inside the object or acting upon them in some way before they all call a common constructor delegate, though existing code seems inconsistent in this.

For example, we have SpectrumAnalyzerEffect, where the third arg might be a color or it might be an palette:

    SpectrumAnalyzerEffect(const char   * pszFriendlyName,
                           int                    cNumBars = 12,
                           const CRGBPalette16  & palette = spectrumBasicColors,
                           uint16_t           scrollSpeed = 0,
                           uint8_t               fadeRate = 0,
                           float           peak1DecayRate = 1.0,
                           float           peak2DecayRate = 1.0)
        : LEDStripEffect(EFFECT_MATRIX_SPECTRUM_ANALYZER, pszFriendlyName),
OR
    SpectrumAnalyzerEffect(const char   * pszFriendlyName,
                           int                    cNumBars = 12,
                           const CRGB &          baseColor = CRGB::Red,
                           uint8_t                fadeRate = 0,
                           float            peak1DecayRate = 1.0,
                           float            peak2DecayRate = 1.0)
        : LEDStripEffect(EFFECT_MATRIX_SPECTRUM_ANALYZER, pszFriendlyName),

This is jolly good and we might choose between those two entry points by building the following two entries in the table:

        ADD_EFFECT(EFFECT_MATRIX_SPECTRUM_ANALYZER, SpectrumAnalyzerEffect, "AudioWave",  MATRIX_WIDTH,  **CRGB(0,0,40),**        0, 0, 1.25, 1.25);

        ADD_EFFECT(EFFECT_MATRIX_SPECTRUM_ANALYZER, SpectrumAnalyzerEffect, "Spectrum",   NUM_BANDS,     **spectrumBasicColors**, 100, 0, 0.75, 0.75);

That's awesome. I shall clone this for EFFECT_MATRIX_SMNOISE by adding one extra argument that's an enum for the effect type. If missing, it will rotate through them. If given, it will play that forever. So we add such a ctor thusly:

    PatternSMNoise(const char* name, EffectType effect) : LEDStripEffect(EFFECT_MATRIX_SMNOISE, "Lava Lamp") , _name(name), _effect(effect)
    {
            debugA("Funky ctor %d", (int)effect);
    }

    PatternSMNoise() : LEDStripEffect(EFFECT_MATRIX_SMNOISE, "Lava Lamp")
    {
            debugA("Default ctor");
            _effect = EffectType::Unknown;
    }

The first stashes effect in _effect and then delegates to the second. Then we hijack the effects table, stealing the effects from Mesmerizer (for debugging convenience)

        // ADD_EFFECT(EFFECT_STRIP_LANTERN, LanternEffect);
#elif 1
        ADD_EFFECT(EFFECT_MATRIX_SMNOISE,       PatternSMNoise);
        ADD_EFFECT(EFFECT_MATRIX_SMNOISE,       PatternSMNoise, "Lava Rainbow", PatternSMNoise::EffectType::LavaLampRainbow);
        ADD_EFFECT(EFFECT_MATRIX_SMNOISE,       PatternSMNoise, "Lava Rainbow Stripe", PatternSMNoise::EffectType::LavaLampRainbowStripe);
        ADD_EFFECT(EFFECT_MATRIX_SMNOISE,       PatternSMNoise, "Shikon", PatternSMNoise::EffectType::Shikon);
        ADD_EFFECT(EFFECT_MATRIX_SMNOISE,       PatternSMNoise, "ColorCube", PatternSMNoise::EffectType::ColorCube);
        ADD_EFFECT(EFFECT_MATRIX_SMPRISMATA, PatternSMPrismata);                        // 6 Be better if it stuck in the pretty part of the curves
    #elif MESMERIZER

The first matches the existing one, of course, 2-5 match the new one because there's the additional enum hitchhiking along, and the last is there just to show we can rotate through them.

Sadness ensues.

Seventeen instances of SMNoise are created - none with the alternative names. We actually can cycle through to Prismata just to confirm the table isn't totally loopy, though. (Well, we're confirming that it DOES loop, actually...)

I could (and will) keep debugging this, but clearly my expectations are off here somehow, so I'd appreciate a bit of guidance here. This basic recipe is the last thing (applied a few times) ono my "SM" matrix effects submit, so I'd like to get unstuck from the recipe and get on with applying it.

What step am I missing? Clearly this works for, e.g., Spectrum Effects, so the framework isn't flagrantly buggy. Somehow, I'm misunderstanding what I need to sacrifice and feed it.

TIA!

[rbergen]

Based on this code I don't know how you end up with 17 of them, I can only assume that somehow the ADD_EFFECT macro gets invoked with EFFECT_MATRIX_SMNOISE and PatternSMNoise as its first two parameters 17 times - even if the code snippets you included don't show this.

The problem with the name is almost certainly that you invoke the LEDStripEffect constructor with the default effect name of "Lava Lamp"; whatever gets passed to the second parameter of the LEDStripEffect constructor is what the name will be in the effect table.
The code for the first constructor should thus be as follows:

    PatternSMNoise(const char* name, EffectType effect) : LEDStripEffect(EFFECT_MATRIX_SMNOISE, name), _effect(effect)
    {
            debugA("Funky ctor %d", (int)effect);
    }

If the _name member variable isn't used anywhere else, then I guess it can be dropped altogether.

As the name parameter in the LEDStripEffect constructor is a const String& I'd also make the name parameter of the PatternSMNoise constructor the same type, as we're only passing it along anyway. But that's more a matter of personal taste.

You're not mentioning the JSON constructor anywhere. I'd like to mention ex officio that the _effect member variable needs to be deserialized from JSON in that constructor, and hence serialized to JSON in an overload of SerializeToJSON(). That is, if you want it to retain its value across reboots of the device.

[robertlipe]

On Wed, Aug 16, 2023 at 2:15 AM Rutger van Bergen ***@***.***> wrote: Based on this code I don't know how you end you end up with 17 of them, I can only assume that somehow the

Likewise. I can see they're actually created at startup and not dynamically when the effect "comes alive". (That's surprising to me, but maybe it shoudn't be. I thought they'd be constructed when they came into view in order to not have everything in core all at once. With the obvious debug print in the obvious place, I can see that's actually the ctor that's called 17 times. (W) (InitEffectsManager)(C1) InitEffectsManager... (I) (LoadJSONFile)(C1) Attempting to read JSON file /effects.cfg (I) (InitEffectsManager)(C1) Creating EffectManager from JSON config (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (PatternSMNoise)(C1) JSON ctor (I) (ReadCurrentEffectIndex)(C1) Attempting to read file /current.cfg (I) (InitEffectsManager)(C1) About to call Init from (I) (InitEffectsManager)(C1) Jumping through d Maybe effects.cfg has remnants of previous attempts and deserves a 'clearsettigns' to vaporize it? I definitely should actually serialize that fella out. Let me do that now. One order of virtual bool SerializeToJSON(JsonObject& jsonObject) override coming up! Along with some nipping and tucking around the JSON types elsewhere. That way when we serialize and wee're running one of te five (or so) that's the "same" effect, we'll know which one it is. Splendid idea.

The problem with the name is almost certainly that you invoke the LEDStripEffect constructor with the default effect name of "Lava Lamp"; whatever gets passed to the second parameter of the LEDStripEffect constructor is

Excellent catch. That's the kind of blindness I develop when staring at the same code for too long ... and why I've learned to ask for second opinions before I bloody my head on the wall. Fixing that, however, didn't change the startup sequence or what happens when banging on the remote's next/previous effect key.

If the _name member variable isn't used anywhere else, then I guess it can be dropped altogether.

Isn't that part of the required signature that's used by the web interface and the splash text when it's started?

As the name parameter in the LEDStripEffect constructor is a const String& I'd also make the name parameter

Grumble. OK. String just doesn't come naturally. spectrumeffects actually uses both const char* (SpectrumAnalyzerEffect) and String& (WaveformEffect). I get that nobody likes a tattle tale and I've now fixed mine. :-) Non-trivial code bases of any tenure are rarely fully internally consistent. Maybe a project for a rainy day. (night) You're not mentioning the JSON constructor anywhere. I'd like to mention ex

officio that the _effect member variable needs to be deserialized from JSON in that constructor, and hence serialized to JSON in an overload of SerializeToJSON(). That is, if you want it to retain its value across reboots of the device.

I hadn't mentioned it because I hadn't done anything exciting with it. Attempting to do so with my oh-so-classy class enum that selected which effect results in pain because the json overloads don't know what to do with my int-wearing-groucho-glasses. I'll chase that now. So I'm still getting too many (JSON) ctor calls and I don't know why. That's my task for the morning. Figuring out all the templating going on in this section is a slog. (That's not a criticism; it's just hard to debug templated code when all you really have is debug printing... I sort of have GDB running, but on the hardware that isn't mesmerizer. I'd like to buid an ESP32-S3 mesmerizer (plus a few 2812-capable lines) so I have the best of all worlds all in one hardware. Maybe I should instead focus on getting a "VNC" viewer up to "see" the HUB75 and WS2812's from a browser or app. My half-hearted attempts to collect frames from port 20,000 or whatever it is resulted in random sized packets of gibberish. But for now, I'm not going to get distracted by that. Thanx! RJL

[rbergen]

Ah, click.

Yes, your board is loading its entire effects list from JSON, and thereby ignoring what's in the default effect list as coded in LoadEffectFactories(). It will only add any (new) effects that have a type that's not in the JSON serialized list yet, but not new variations of effect types it already has in the JSON list.

And then the ones you are loading from JSON will all suffer from amnesia concerning their _effect value, because that's not (de)serialized yet.
(I will now accept a slap on the fingers for stating the bleeding obvious, but I would cast it to int when serializing and back to EffectType when deserializing in the JSON constructor.)

I think the best thing to do here is make a POST request to http://<your board ip>/reset with effectsConfig and board both set to 1. That'll delete the effect JSON file from SPIFFS and reboot the board. It will then recreate the effects list using the default effect factories at startup.

[rbergen]

I just noticed I missed one concrete question. My apologies.

If the _name member variable isn't used anywhere else, then I guess it can be dropped altogether.

Isn't that part of the required signature that's used by the web interface and the splash text when it's started?

No, they both use the _friendlyName that's a member of the LEDStripEffect base class.

[robertlipe]

On Wed, Aug 16, 2023 at 10:31 AM Rutger van Bergen ***@***.***> wrote: Ah, click. Yes, your board is loading its entire device list from JSON, and thereby ignoring what's in the default effect list as coded in LoadEffectFactories(). It will only add any (new) effects that have a *type* that's not in the JSON serialized list yet, but not new variations of effect types it already has in the JSON list.

Good to confirm as I had come to a similar conclusion, only with less clarity. I'd confirmed that a * Debug: Command received: clearsettings (processRemoteDebugCmd)(C1) Removing persisted settings.... In the telnet debugger thingy changed a LOT for me. I now have a sensible number of objects created, each with the chosen names showing in the web interface, the ability to flip between them, and such. That's .... not at all obvious behavior, but I think I see how I got here. i probably had scraps of this effect accumulated in the JSON and they just accumulated until they could exact their revenge. I'm not sure how that bodes for an end-user accumulating upgrades over time or if it just affects developers. Most users won't encounter ctor signature changes on a regular basis, but it's probably something we should think about if we expect people to be able to OTA daily builds while we (and I'm sure it rarely happens) change signatures in the code. I guess we have kind of the database schema problem where the JSON is ffectively locked to an effects.cpp fingerprint and may need an onUpgrade/onDowngrade kind of cleanup pass. But that's a distraction for me now. I think I have the needed keys to forge on. 1) Stupidly passed through string literal name. 2) Not properly handling JSON serialization of "you are THIS" member. 3) Zombie JSON keys giving me too many instances of the effect. 4) JSON overloading not playing fully nice with class enum. Each is something to solve and each a little clearer with your help, so Thank You. And then the ones you *are* loading from JSON will all suffer from amnesia

concerning their _effect value, because that's not (de)serialized yet.

Yep. Confirmed that what was in the JSON had nonsense.

(I will now accept a slap on the fingers for stating the bleeding obvious, but I would cast it to int when serializing and back to EffectType when deserializing in the JSON constructor.)

I accept the obvious when I'm asking for help. :-) There was some reason that didn't play out well (maybe the signature was no longer 'unique enough' - I've forgotten) but I just reverted back to a plain ole C enum for now. Ah. At least part of the problem was that the num values and the method names were the same, so once I demoted them to C enums, the signatures collided with the enums, so I had to go bak to C enums AND change the names. Not traumatic, but enough to let me forge on. So after a clearsettings: (I) (InitEffectsManager)(C1) Creating EffectManager using default effects (I) (InitEffectsManager)(C1) We have an effect manager. Calling LoadEffects (PatternSMNoise)(C1) Default ctor (PatternSMNoise)(C1) Funky ctor 1 (PatternSMNoise)(C1) Funky ctor 2 (PatternSMNoise)(C1) Funky ctor 3 (PatternSMNoise)(C1) Funky ctor 4 (I) (InitEffectsManager)(C1) Back from LoadFX (I) (InitEffectsManager)(C1) About to call Init from (I) (InitEffectsManager)(C1) Jumping through And on the next boot: (I) (LoadJSONFile)(C1) Attempting to read JSON file /effects.cfg (I) (InitEffectsManager)(C1) Creating EffectManager from JSON config (PatternSMNoise)(C1) JSON ctor 0 (PatternSMNoise)(C1) JSON ctor 1 (PatternSMNoise)(C1) JSON ctor 2 (PatternSMNoise)(C1) JSON ctor 3 (PatternSMNoise)(C1) JSON ctor 4 (I) (ReadCurrentEffectIndex)(C1) Attempting to read file /current.cfg (I) (InitEffectsManager)(C1) About to call Init from So now I just have to do something appropriately responsible in the JSON to persist the relationship between all the ctor mutations and handle them across upgrades and such. (And, yeah, probably script that POST with curl or something....) I think the best thing to do here is make a POST request to http://<your

board ip>/reset with effectsConfig and board both set to 1. That'll delete the effect JSON file from SPIFFS and reboot the board. It

Nice tip. Thanx! RJL

…

Message ID: <PlummersSoftwareLLC/NightDriverStrip/repo-discussions/407/comments/6742212 @github.com>

[davepl]

I can’t claim to have read this whole chain, but thought I’d throw in a comment anyway: it wouldn’t hurt my feelings if we reset user settings every time the main effects table changed. That should only happen with a new firmware update, and at this point I’d be OK with users losing their preferences on an upgrade. Even Task Manager resets to defaults in that scenario! Of course it’s preferable to seamlessly migrate them. What my idea situation would be that when the effects table changes, it updates the order and list to match the main table while preserving individual settings for effects that have been customized. But if that’s more than a bit of work, not sure it’s that important. That way, when there’s a new effect table, instead of adding new effects to the end, everyone gets the same default order. - Dave

…

On Aug 16, 2023, at 9:21 AM, Robert Lipe ***@***.***> wrote: On Wed, Aug 16, 2023 at 10:31 AM Rutger van Bergen ***@***.***> wrote: > Ah, click. > > Yes, your board is loading its entire device list from JSON, and thereby > ignoring what's in the default effect list as coded in > LoadEffectFactories(). It will only add any (new) effects that have a > *type* that's not in the JSON serialized list yet, but not new variations > of effect types it already has in the JSON list. > Good to confirm as I had come to a similar conclusion, only with less clarity. I'd confirmed that a * Debug: Command received: clearsettings (processRemoteDebugCmd)(C1) Removing persisted settings.... In the telnet debugger thingy changed a LOT for me. I now have a sensible number of objects created, each with the chosen names showing in the web interface, the ability to flip between them, and such. That's .... not at all obvious behavior, but I think I see how I got here. i probably had scraps of this effect accumulated in the JSON and they just accumulated until they could exact their revenge. I'm not sure how that bodes for an end-user accumulating upgrades over time or if it just affects developers. Most users won't encounter ctor signature changes on a regular basis, but it's probably something we should think about if we expect people to be able to OTA daily builds while we (and I'm sure it rarely happens) change signatures in the code. I guess we have kind of the database schema problem where the JSON is ffectively locked to an effects.cpp fingerprint and may need an onUpgrade/onDowngrade kind of cleanup pass. But that's a distraction for me now. I think I have the needed keys to forge on. 1) Stupidly passed through string literal name. 2) Not properly handling JSON serialization of "you are THIS" member. 3) Zombie JSON keys giving me too many instances of the effect. 4) JSON overloading not playing fully nice with class enum. Each is something to solve and each a little clearer with your help, so Thank You. And then the ones you *are* loading from JSON will all suffer from amnesia > concerning their _effect value, because that's not (de)serialized yet. > Yep. Confirmed that what was in the JSON had nonsense. > (I will now accept a slap on the fingers for stating the bleeding obvious, > but I would cast it to int when serializing and back to EffectType when > deserializing in the JSON constructor.) > I accept the obvious when I'm asking for help. :-) There was some reason that didn't play out well (maybe the signature was no longer 'unique enough' - I've forgotten) but I just reverted back to a plain ole C enum for now. Ah. At least part of the problem was that the num values and the method names were the same, so once I demoted them to C enums, the signatures collided with the enums, so I had to go bak to C enums AND change the names. Not traumatic, but enough to let me forge on. So after a clearsettings: (I) (InitEffectsManager)(C1) Creating EffectManager using default effects (I) (InitEffectsManager)(C1) We have an effect manager. Calling LoadEffects (PatternSMNoise)(C1) Default ctor (PatternSMNoise)(C1) Funky ctor 1 (PatternSMNoise)(C1) Funky ctor 2 (PatternSMNoise)(C1) Funky ctor 3 (PatternSMNoise)(C1) Funky ctor 4 (I) (InitEffectsManager)(C1) Back from LoadFX (I) (InitEffectsManager)(C1) About to call Init from (I) (InitEffectsManager)(C1) Jumping through And on the next boot: (I) (LoadJSONFile)(C1) Attempting to read JSON file /effects.cfg (I) (InitEffectsManager)(C1) Creating EffectManager from JSON config (PatternSMNoise)(C1) JSON ctor 0 (PatternSMNoise)(C1) JSON ctor 1 (PatternSMNoise)(C1) JSON ctor 2 (PatternSMNoise)(C1) JSON ctor 3 (PatternSMNoise)(C1) JSON ctor 4 (I) (ReadCurrentEffectIndex)(C1) Attempting to read file /current.cfg (I) (InitEffectsManager)(C1) About to call Init from So now I just have to do something appropriately responsible in the JSON to persist the relationship between all the ctor mutations and handle them across upgrades and such. (And, yeah, probably script that POST with curl or something....) I think the best thing to do here is make a POST request to http://<your > board ip>/reset with effectsConfig and board both set to 1. That'll > delete the effect JSON file from SPIFFS and reboot the board. It > Nice tip. Thanx! RJL > Message ID: > <PlummersSoftwareLLC/NightDriverStrip/repo-discussions/407/comments/6742212 > @github.com> > — Reply to this email directly, view it on GitHub <#407 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF6DOKK4CIY4MCNVJELXVTXRRANCNFSM6AAAAAA3R2XLBA>. You are receiving this because you are subscribed to this thread.

[rbergen]

I assume you're only talking about the effects preferences, not the device setting stuff? If you include the latter, I'll already have to disagree more than a little. I've had to reconfigure my location (zip/city and country), use Celsius flag, use 24hr time flag, etc. a few times over the past period, and it gets old real fast.

Concerning the effect list I agree that not much harm will be done now. People can change the effect order, duplicate them and change effect settings - but until there is a an actual web UI to add a UX on top of the API endpoints that are already there, nobody will. The fact it's possible does mean that it's practically impossible to guarantee smooth integration of new effects "in the right spot", though. For all we know, a user could have duplicated half the standard effect set and reversed its order, thereby negating any logic there is to the location of the new effect in the standard cycle.

Once there is a UI to drive the API and more effects allow themselves to be configured to the user's taste, I'm not convinced a "blind erase" of their setup is still responsible. I think we would also have to become much more disciplined in updating the firmware's version number. Making the EffectManager ignore a stored JSON list if its firmware number differs from the one in the actual firmware is work, but not much.

Maybe on Mesmerizer we can ask the user to choose whether to retain or reload/refresh their effect list at start-up if a mismatch between JSON version and firmware version is detected, using the remote control as the Yes/No answering device? We'd then need to decide on what to do if no answer is given, but it would leave the choice up to the user in principle.

[robertlipe]

On Wed, Aug 16, 2023 at 11:33 AM David W Plummer ***@***.***> wrote: I can’t claim to have read this whole chain, but thought I’d throw in a comment anyway: it wouldn’t hurt my feelings if we reset user settings every time the main effects table changed. That should only happen with a new firmware update, and at this point I’d be OK with users losing their preferences on an upgrade.

A checksum on the table changing and tossing those settings on change doesn't sound *bad*. Unless there are changes in effects.cc (and all that goes into it) we shouldn't casually vaporize the settings. If Rutger doesn't beat me to that, I'll put that on my run queue after getting these multi-ctor effects landed. That is, of course, predicated on understanding and solving this. ...If there's anything to actually "solve" if your Mesmerizer hasn't lived exactly the same life as mine with "random" changes. We have non-effect settings other settings in other places, right? The settings for your remote type (soon) and the settings for the URL to check for automatic update (soon) and whether the VU meter and mic are on (soon) such are in a different table, aren't they? I mean, there are settings other than for individual effects, right? Actually, the value of that aforementioned checksum would be such a persisted value. P.S. for any others in similar shoes, the recipe for Rutger's potion is: curl --verbose --data "effectsConfig=1" --data "board=1" http://192.168.2.165/reset That'll probably appear in tools/ in some form soon.

[robertlipe]

We'd implicitly been doing Dave's suggestion until a week or so ago anyway. We'd been uploading a blank user partition on every upgrade. Poof, settings reset. I thus think we'd gotten here rather accidentally. By not clobbering the user partition on every load, we've had to deal for the first time with mismatched settings. I don't think that was intentionally planned as much as it just happened.

…

On Wed, Aug 16, 2023 at 12:03 PM Robert Lipe ***@***.***> wrote: On Wed, Aug 16, 2023 at 11:33 AM David W Plummer ***@***.***> wrote: > I can’t claim to have read this whole chain, but thought I’d throw in a > comment anyway: it wouldn’t hurt my feelings if we reset user settings > every time the main effects table changed. That should only happen with a > new firmware update, and at this point I’d be OK with users losing their > preferences on an upgrade. A checksum on the table changing and tossing those settings on change doesn't sound *bad*. Unless there are changes in effects.cc (and all that goes into it) we shouldn't casually vaporize the settings. If Rutger doesn't beat me to that, I'll put that on my run queue after getting these multi-ctor effects landed. That is, of course, predicated on understanding and solving this. ...If there's anything to actually "solve" if your Mesmerizer hasn't lived exactly the same life as mine with "random" changes. We have non-effect settings other settings in other places, right? The settings for your remote type (soon) and the settings for the URL to check for automatic update (soon) and whether the VU meter and mic are on (soon) such are in a different table, aren't they? I mean, there are settings other than for individual effects, right? Actually, the value of that aforementioned checksum would be such a persisted value. P.S. for any others in similar shoes, the recipe for Rutger's potion is: curl --verbose --data "effectsConfig=1" --data "board=1" http://192.168.2.165/reset That'll probably appear in tools/ in some form soon.

[rbergen]

Clearing everything did happen if someone flashed a new version using the Web Installer, or made blanking the entire firmware or the SPIFFS partition part of their local flash flow. I deliberately changed the Web Installer to no longer do this as standard. As I said in my previous comment, losing settings one cares about can be really, really annoying.

Which is why I'm not yet convinced we should just flush a user's effect setup too light-heartedly - once there is a usable means to put such a setup together. Once people start putting time and effort in configuring their individual effect cycle, they'll start caring about losing it.

[robertlipe]

I lacked the vocabulary for the right examples, but I think we're agreeing. Let's see: There are several settings that are worth keeping. There will soon be more. (Because I plan on adding more. Remote control type, settable via web interface, is one of my distant pet projects...I have a TON of remotes for testing.) There are two more API keys that are in another branch of mine under development. Sure, defalults in secrets.h are possible, but even for a developer-oriented product, recompiling to change an API key is a bit silly. That should be configurable in the web api somewhere. I've assumed/hoped that the effects settings are in the JSON stuff and the device settings are in the key store <https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/storage/nvs_flash.html> in the NVS partition dedicated to that. The effects settings are such a pain to configure and there's so little there (today - these might both change in the future) that they're not worth stressing about. In the future, we probably need a more disciplined, strongly versioned approach to those.. I've been assuming that Mesmerizer/Spectrum, etc. cannot be assumed to have a remote control and thus have no UI other than the web UI...which may or may not be built in. Is that true? The option of asking yes or no via remote is intriguing, but if we block on an answer for a person that doesn't have the remote or the little IR dongle (I wish I'd thrown more KY-022's in my recent order...) that would be insanely annoying. (We actually do have one button we can use as an input on Mesmerizer. It's just a game of chicken because it's right next to the unlabeled Reset button. :-) So, are we agreeing? System-wide settings should be treated as precious, even now and Effects settings in JSON we shouldn't be flippant about destroying, but we shouldn't be debugging 17 calls to the constructor because something changed. Are we aligned?

…

On Wed, Aug 16, 2023 at 12:16 PM Rutger van Bergen ***@***.***> wrote: Clearing everything did happen if someone flashed a new version using the Web Installer, or made blanking the entire firmware or the SPIFFS partition part of their local flash flow. I deliberately changed the Web Installer to no longer do this as standard. As I said in my previous comment, losing settings one cares about can be really, really annoying. Which is why I'm not yet convinced we should just flush a user's effect setup too light-heartedly - once there is a usable means to put such a setup together. Once people start putting time and effort in configuring their individual effect cycle, they'll start caring about losing it. — Reply to this email directly, view it on GitHub <#407 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD3YTLFUD22MQL7FEI3LXVT563ANCNFSM6AAAAAA3R2XLBA> . You are receiving this because you authored the thread.Message ID: <PlummersSoftwareLLC/NightDriverStrip/repo-discussions/407/comments/6743171 @github.com>

[rbergen]

Device settings are separated from the effect list configuration, but they are both stored on the SPIFFS partition. The only thing that's currently stored in NVS are the WiFi creds. The device settings can be reset separately; there's also a "deviceConfig=1" option in the /reset endpoint.

And yes, of course we'd have to have an answer for situations where people cannot be expected to have an IR (or they've indicated in their device settings that they don't). Using an IR remote with Spectrum on the M5StickC Plus is possible, but not without soldering things together - one can argue if people doing that is a reasonable expectation or not. In my view this is largely a matter of defaults: on (which) devices that can support IR remote controls do we assume there is one, and what do we choose if no choice is made by the user within a not-too-annoying amount of time?
The latter question basically means we still have to make up our minds about what our choice is, anyway.

Next to what I've already said about this, there are two things that mildly bug me:

1. Applying a bit of reverse reasoning: if we assume nobody cares about what we're persisting about the effects list, then why are we storing it to begin with? Possible response: that assumption may be for a certain time only.
2. I strongly feel we'd have an entirely different conversation if we had a complete web UI that would a) allow users to customize their effect flow, b) make it easy to reload the default set, and c) maybe even gently point out to its user that a new standard effect set is available "and would you kindly click this button if you want to load it?" Possible response: nice academic argument, but that web UI simply doesn't exist yet.

Rolling all that up, I agree with blindly loading the default set if it has changed between firmware versions:

• permanently on all devices that don't have the web server enabled
• on other devices until there are user-friendly means to leave the decision to the user

The latter point means that I think we need to be ready up-front to back out of this decision for those devices once we have such a provision in place.

About triggering the "blind reload" of the defaults: I think we should version the default effect list separately, and track that version in the effect list JSON file. That way we make the decision to reset users' devices to the default effect list an explicit decision during development and review, and decouple it from other changes that may lead to a bump of the overall firmware version.

Even though I brought it up, the IR thing is one I'm happy to park until we no longer know what do with our time, which means I also accept that idea may never see the light of day.

[davepl]

You should assume a remote is part of all setups. It’s the primary interface! And every one I’ve sent out of late includes the remote and receiver. When I make the kits available, they’ll also include both. Thanks Dave

…

On Aug 16, 2023, at 11:02 AM, Robert Lipe ***@***.***> wrote: I lacked the vocabulary for the right examples, but I think we're agreeing. Let's see: There are several settings that are worth keeping. There will soon be more. (Because I plan on adding more. Remote control type, settable via web interface, is one of my distant pet projects...I have a TON of remotes for testing.) There are two more API keys that are in another branch of mine under development. Sure, defalults in secrets.h are possible, but even for a developer-oriented product, recompiling to change an API key is a bit silly. That should be configurable in the web api somewhere. I've assumed/hoped that the effects settings are in the JSON stuff and the device settings are in the key store <https://docs.espressif.com/projects/esp-idf/en/latest/esp32/api-reference/storage/nvs_flash.html> in the NVS partition dedicated to that. The effects settings are such a pain to configure and there's so little there (today - these might both change in the future) that they're not worth stressing about. In the future, we probably need a more disciplined, strongly versioned approach to those.. I've been assuming that Mesmerizer/Spectrum, etc. cannot be assumed to have a remote control and thus have no UI other than the web UI...which may or may not be built in. Is that true? The option of asking yes or no via remote is intriguing, but if we block on an answer for a person that doesn't have the remote or the little IR dongle (I wish I'd thrown more KY-022's in my recent order...) that would be insanely annoying. (We actually do have one button we can use as an input on Mesmerizer. It's just a game of chicken because it's right next to the unlabeled Reset button. :-) So, are we agreeing? System-wide settings should be treated as precious, even now and Effects settings in JSON we shouldn't be flippant about destroying, but we shouldn't be debugging 17 calls to the constructor because something changed. Are we aligned? On Wed, Aug 16, 2023 at 12:16 PM Rutger van Bergen ***@***.***> wrote: > Clearing everything did happen if someone flashed a new version using the > Web Installer, or made blanking the entire firmware or the SPIFFS partition > part of their local flash flow. I deliberately changed the Web Installer to > no longer do this as standard. As I said in my previous comment, losing > settings one cares about can be really, really annoying. > > Which is why I'm not yet convinced we should just flush a user's effect > setup too light-heartedly - once there is a usable means to put such a > setup together. Once people start putting time and effort in configuring > their individual effect cycle, they'll start caring about losing it. > > — > Reply to this email directly, view it on GitHub > <#407 (reply in thread)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ACCSD3YTLFUD22MQL7FEI3LXVT563ANCNFSM6AAAAAA3R2XLBA> > . > You are receiving this because you authored the thread.Message ID: > <PlummersSoftwareLLC/NightDriverStrip/repo-discussions/407/comments/6743171 > @github.com> > — Reply to this email directly, view it on GitHub <#407 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF2HEGSMB42L2S6527TXVUDKXANCNFSM6AAAAAA3R2XLBA>. You are receiving this because you commented.

[rbergen]

Ok, I'll rephrase: one question we need to answer is what we do on (non-Mesmerizer) devices that have no means to display a question to a user. 🙂

Bottom line: we still need to make up our minds about this for part of the project's users.

[robertlipe]

I'd highlight one point you made with a big red circle, Rutger: In multiple conversations (dynamic resizing of partitions, here, recently in another thread) we've had a lot of hand-wringing over preserving state (which is good - it's a problem we SHOULD care about) that just doesn't exist at this point. Once we have releases and version numbers and users with reasonable expectations that their customizations will be preserved, we'll have a different set of problems than we have right now. Right now, blowing persistent state is annoying to a couple of devs, but a day is coming where we'll need to think transactionally across versioned upgrades and such. Back to the primary topic here, though, with Rutger's guidance, I think I now have PatternSMNoise handling multiple constructor entry points sensibly enough to implement Dave's requirement <https://github.com/robertlipe/NightDriverStrip/issues> for merging that effect and thus think I now have the rhythm to pull it off for the others on that list <https://github.com/robertlipe/NightDriverStrip/issues>. I also see THAT <robertlipe#4 (comment)> is where I asked this question last week. :-/

…

On Wed, Aug 16, 2023 at 1:44 PM Rutger van Bergen ***@***.***> wrote: Ok, I'll rephrase: one question we need to answer is what we do on (non-Mesmerizer) devices that have no means to display a question to a user. 🙂 Bottom line: we still need to make up our minds about this for part of the project's users. — Reply to this email directly, view it on GitHub <#407 (reply in thread)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD334YZYCX5QIDL7MYDLXVUIH7ANCNFSM6AAAAAA3R2XLBA> . You are receiving this because you authored the thread.Message ID: <PlummersSoftwareLLC/NightDriverStrip/repo-discussions/407/comments/6743915 @github.com>

[rbergen]

I think you're making my point, at least to a point - and maybe that's your intention. :)

In any case, I think our mindset should already be where we want things to be, and that's a place where state should be preserved. That's why I'm so hesitant to throw away persistence willy-nilly regardless of how few people currently suffer.
At the same time, I can't deny that from the perspective of the now things are not black and white - are they ever? - and arguments can be made that are temporarily valid.

In fact, our conversation about the partitions (still) covers a scenario we agree needs preservation already, which is the storage of device settings. So, I'd say that conversation and its conclusions are still valid, for one.

...all that said and being as it may, there is still the quote that states that "Those who never change their minds, never change anything."
That will happen too. :)

[davepl]

Well, given that I’ve shipped out a dozen boards this week, we should have some new folks looking for things to work on. It might not make V1, but perhaps we could add a basic text menu UI onscreen. So it’s a “special” effect like the Slash effect, invoked by the settings button on the remote (which we define). So you’d have a scrolling text UI perhaps something like this: NETWORK Bananaman IP ADDRESS 192.168.8.157 MEMORY 32768K CPU 65% 77% [User Settings] [Exit] --------------- TIMEZONE GMT-8 > POSTALCODE 98074 > WEATHERKEY 01a2b59d4f5 [Reset Settings] [Restart] [Back] ——————— That would at a minimum require alphanumeric entry of some kind. Not something I think the 3 of us should invest in, but if new blood is willing, any thoughts on it as a general idea? _ Dave

…

On Aug 16, 2023, at 2:11 PM, Rutger van Bergen ***@***.***> wrote: I think you're making my point, at least to a point - and maybe that's your intention. :) In any case, I think our mindset should already be where we want things to be, and that's a place where state should be preserved. That's why I'm so hesitant to throw away persistence willy-nilly regardless of how few people currently suffer. At the same time, I can't deny that from the perspective of the now things are not black and white - are they ever? - and arguments can be made that are temporarily valid. In fact, our conversation about the partitions (still) covers a scenario we agree needs preservation already, which is the storage of device settings. So, I'd say that conversation and its conclusions are still valid, for one. ...all that said and being as it may, there is still the quote that states that "Those who never change their minds, never change anything." That will happen too. :) — Reply to this email directly, view it on GitHub <#407 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF452MG6JJXSCDJAPJ3XVUZOVANCNFSM6AAAAAA3R2XLBA>. You are receiving this because you commented.

[robertlipe]

On Wed, Aug 16, 2023 at 4:11 PM Rutger van Bergen ***@***.***> wrote: I think you're making my point, at least to a point - and maybe that's your intention. :)

I think we're mostly agreeing. I may not hold back my 62 effects CL because some intermediate version of my build triggers weirdness (that I still don't think we understand) in effectsmgr that leads to duplicate effects, but I agree this has emphasized a need for some more rigor around the contracts of persistence (such as it is) and UI (sigh). We have some frailness here.

In any case, I think our mindset should already be where we want things to be, and that's a place where state should be preserved. That's why I'm so hesitant to throw away persistence willy-nilly regardless of how few people currently suffer.

That may mean I need your help figuring out where those duplicates came from. Oh, wait, I nuked my cfg table. Can I repro it in captivity? Maybe.

At the same time, I can't deny that from the perspective of the now things are not black and white - are they ever? - and arguments can be made that are *temporarily* valid.

See above on blocking. (Totally realizing that it sounds like the bug *I* encountered justifies dumping it, but everyone else's bug is worth stopping development and tackling the harder problem. I don't feel great about that.) In fact, our conversation about the partitions (still) covers a scenario we

agree needs preservation already, which is the storage of device settings. So, I'd say that conversation and its conclusions are still valid, for one. ...all that said and being as it may, there is still the quote that states that "Those who never change their minds, never change anything." That *will* happen too. :)

FWIW, you already changed my mind on the partition thing and that's why it's on ice until I can give it my full attention. I already chased some of the related problems all the way up to Espressif engineering to help get solutions. You convinced me that there may be some data on the filesystems worth preserving. I think we can save the, what, tens of kilobytes of data and STILL automatically manage filesystem size...and just do NOTHING when there's data beyond those two (three?) files on the drive. You convinced/educated me. (I didn't realize what was IN those filesystems yet.) I backed up and regrouped. This isn't 1977 IBM or AT&T.

[robertlipe]

work on. It might not make V1, but perhaps we could add a basic text menu UI onscreen. So it’s a “special” effect

Let's have that as a separate discussion. What is V1? What features are missing? Does state preservation make sense for developer builds vs. release builds? Remember, this immediate discussion is in the context of avoiding a bug, but we know state matters at the edges of partition tables, filesystems (and we know SPIFFS is about to come to an end in the IDF tools) and SOME contents (if they loaded 3MB of GIFs onto it and upgrade, we should be careful to leave it...), user settings, and firmware upgrades. But we can't leave their GIFs if we upgrade Platformio and SPIFFS is no longer there. We have to migrate them somehow. For example (and I'm not throwing Dave under the bus here...) adding OTA and then changing the partition table a few times on Mesmerizer hosed the state. Nobody blinked. I spent a few hours trying to figure out why I kept changing code but not seeing changes only to find I was writing executables to an address the device wasn't loading code from. So you’d have a scrolling text UI perhaps something like this: [ ... ]

Not something I think the 3 of us should invest in, but if new blood is willing, any thoughts on it as a general idea?|

Gag me. (Sorry, Dave. :-) If we can't get a spiffy javascripty UI for that, I'd rather have a CGI form doing a POST of some FORMs that are parsed and read into settings objects for zip code, api keys, and such. I'd modify the existing telnet interface thingy to parse strings in a command line interface before I wrote a page telling someone to look at their 44 key remote and imagine a 10-key numeric keypad starting where red is with 1 being red and 3 being Blue and light blue (the bottom one - because the two light blue keys are the same color) is "enter". Blech. We should seriously think about the command line settings thing, though. That's actually how things like your thermostat and router back up their settings. There's a 1.0 feature - network backup and restore. That's a nice tasty, self-contained chunk for new blood. Ditto for unit tests for upgrades across 'frozen' versions.

…

Message ID: <PlummersSoftwareLLC/NightDriverStrip/repo-discussions/407/comments/6745086 @github.com>