Feature: expose additional effect settings

[rbergen]

Recently, the ability was added for effects to expose settings that can be changed via the API. A few basic settings have been exposed by LEDStripEffect; the PatternSubscribers effect adds two more for itself.

Obviously, there are more effects for which it makes sense to expose properties as settings that can be changed via the device API and (later) the device web UI.

When working on this:

• The REST_API.md file contains information about the effect settings API endpoints.
• The aforementioned PatternSubscribers effect provides an example of an effect extending the basic settings exposed by LEDStripEffect.
• LEDStripEffect includes a number of helper functions for changing effect settings of "known types". Known types are themselves defined by the SettingSpec struct in types.h.

[Louis-Riel]

I'm POCing something to save UI effect settings in the browser storage facilities. In this POC, I'm setting an effect image URL option. This is what the effect image feature looks like: The little gear opens a config dialig: The favicon.ico is the default, and the one in the screenshot is the overridden value:

So all and all, this is more for the UIness of this parameter configuration, rather than using browser storage to save effect params. This is not the best, as this parameter will only be effective for the effect on the browser you set the option in. I'm looking to use browser storage to save site options like color schemes and things of the sort. So maybe this effect image url option could be in the chips effect config, and if that is so, the dialog approach can still be used and the prefs saved to the chip.

[rbergen]

@Louis-Riel Would you envisage using this UX for other chip-stored effect settings as well? Because that's what this issue is primarily about.

Concerning the image per effect: I can definitely see that being a chip-stored effect setting, provided we indeed make it a URL (not an actual image upload that is stored to SPIFFS, for instance.)

[robertlipe]

If a setting is changed, how is the effect notified? Is it torn down and recreated (ouch!) where it should pick the config settings out of the received JSON or is there intended to be some kind of notification that emits a std::vector to some kind of receiver that you've registered? An event would allow interactive tuning where the user beats on the up and down keys (or tweaks a slider or whatever) while nuking and restarting the animation would be much more jarring.

To the notion of global settings "effect speed" is a common pattern in the ones I worked on. One of our competitors (and I still can't find who because there's so much copying ...) defined Brightness, Speed, and Scale for every effect, but the result was a UX nightmare as effects would rob bits and overload things to weird meanings. "Scale is 0-100, but odds mean that PacMan is self-driving and the lower digit controls the number of ghosts and the top digit controls the maze/map level". I think it was meant to be drive by an IR remote but I just can't imagine it was usable.

How do our settings interact with the IR remote?

Speed from 0-100 is something we can all feed into a map and use. It's more than just frame rate. Brightness (#284) is probably another global setting that we should broadcast down to the events.

Both of those would be nicely represented by actual keys on remotes like https://www.amazon.com/dp/B0C1Z44XVW - They're also good pleas to NOT teardown and reconstruct the animation.

[rbergen]

There are two types of settings:

• Device settings. These are kept in the DeviceSettings class (declared and implemented in devicesettings.h and devicesettings.cpp), which is made available via g_ptrSystem->DeviceSettings(). Notifications of updates to these settings are not pushed to effects that use them. This means that effects that rely on device settings for their own configuration currently need to pull and check changes to device setting values at intervals that make sense to them. An example of an effect doing this is PatternWeather, which gets a number of settings from DeviceSettings. An effect that just pulls a setting it needs from DeviceSettings whenever it needs it is PatternPongClock.
• Effect settings. Changes to these are pushed to an individual effect directly; LEDStripEffect includes an overloadable SetSetting() member function for this. Examples of an effect that adds its own effect settings to the standard set (that being friendlyName, maximumEffectTime, the read-only hasMaximumEffectTime, and the write-only clearMaximumEffectTime) is PatternSubscriber.

Both DeviceSettings and individual effects should publish the specifications for all settings they keep in a list of SettingSpec instances, and return a list of std::reference_wrappers to them, when asked. Both DeviceSettings and LEDStripEffect include a GetSettingSpecs() member function that can be used as the starting point for exploring exactly how this is currently set up. The SettingSpecs are published via a number of endpoints in the on-board web server's API, for consumption by JSON-savvy humans and/or a semi-dynamic (web) interface. The endpoints are documented in REST_API.md.

I didn't look into it in much detail, but I'd say that things like Speed, Brightness, Scale, etc. are candidates for effect settings. This is also why these properties are persisted to JSON on a per-effect basis.

The IR remote does most of its interaction with EffectManager, via the RemoteControl class - and that class' handle() function as implemented in remotecontrol.cpp specifically. I don't think there's currently any direct IR remote interaction with individual effects. In either case YMMV, but I remember finding the code in RemoteControl::handle() very digestible.

[robertlipe]

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

I have remotes on the way, but knowing I can (presumably) script some testing via curl/wget to honk on networked endpoints makes that practical to exercise now. Having the effects register SettingSpecs structs and when (some future?) web interface or network events push changes to Speed is a good goal. I'm looking at include/effects/matrix/PatternSubscribers.h and don't see that working, though. Ideally, you'd like to register an OnSpeedChanged event and have that listener/observer trigger when the dial is dragged.If you receive the entire list of Settings you register and have to pilfer through them, that's not totally terrible, but it's not ideal. You'd like to register OnSpeedChanged(SettingsValue Old, SettingsValue New) or something so you can just reschedule your timers without looping. Neither SightReader() nor UpdateSubscribers() quite reach that zen. I see where the registration for change notifications ishappening (FillSettingSpecs()), but I don't see where I'd get the callback for OnSettingChanged(SettingSpec old, SettingSpec new) were we can just operate on the change. Is there perhaps a better example, or is this a hole? I'd much rather receive a SettingsValue than a json blob.

Speed and Brightness are probably the easiest to think about for something that can change continuously that needs smooth adjustments and without having to dumpster dive in an entire std::vector to find the one that's changed. I could probably implement an onChanged handler for a Speed event for 80% of the new effects fairly mechanically. For testing, I'd just have a a shell script on the server turning the knob up and down endlessly.

I suspect we'll agree that the IR remote probably SHOULD wiggle at least speed and brightness. Let's pencil that in. Making that work for an effect or two will probably speak to us on how those interfaces should look. (That's probably not JSON and not a full array...) A pair of hashes for changed keynames would be better, but just the SettingsValues that changed would be better still.

Subscribers may be a poor example for the data I'm thinking of. Those values will be a humungo key copy/paste into place once every few years.

OnSpeedChanged(SettingsValue Old, SettingsValue New) is better than OnSettingsChanged(SettingsValue[] Old, SettingsValue[] New) is better than OnSettingsChanged(const JsonObjectConst& new) Loop over and see what changed()

I already have code like PatternSMFire2021.h: static constexpr uint8_t Speed = 150; // 1-252 ...why is not 255?! // Setting PatternSMFire2021.h: static constexpr uint8_t Scale = 9; // 1-99 is palette and scale // Setting PatternSMSpiro.h: static constexpr uint8_t Speed = 46; // 1-255 is length Setting PatternSMSpiro.h: static constexpr uint8_t Scale = 3; // 1-100 is palette Setting

that's just begging to be hooked up to code like this.

I'll report back when I'm more ready to tackle this head-on. We may need to team up on the sender side and the receiver side if that's OK.

On Tue, Jul 25, 2023 at 2:05 AM Rutger van Bergen @.***> wrote:

There are two types of settings:

• Device settings. These are kept in the DeviceSettings class (declared and implemented in devicesettings.h and devicesettings.cpp), which is made available via g_ptrSystem->DeviceSettings(). Notifications of updates to these settings are not pushed to effects that use them. This means that effects that rely on device settings for their own configuration currently need to pull and check changes to device setting values at intervals that make sense to them. An example of an effect doing this is PatternWeather, which gets a number of settings from DeviceSettings. An effect that just pulls a setting it needs from DeviceSettings whenever it needs it is PatternPongClock.
• Effect settings. Changes to these are pushed to an individual effect directly; LEDStripEffect includes an overloadable SetSetting() member function for this. Examples of an effect that adds its own effect settings to the standard set (that being friendlyName, maximumEffectTime, the read-only hasMaximumEffectTime, and the write-only clearMaximumEffectTime) is PatternSubscriber.

Both DeviceSettings and individual effects should publish the specifications for all settings they keep in a list of SettingSpec instances, and return a list of std::reference_wrappers to them, when asked. Both DeviceSettings and LEDStripEffect include a GetSettingSpecs() member function that can be used as the starting point for exploring exactly how this is currently set up. The SettingSpecs are published via a number of endpoints in the on-board web server's API, for consumption by JSON-savvy humans and/or a semi-dynamic (web) interface. The endpoints are documented in REST_API.md https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md .

I didn't look into it in much detail, but I'd say that things like Speed, Brightness, Scale, etc. are candidates for effect settings. This is also why these properties are persisted to JSON on a per-effect basis.

The IR remote does most of its interaction with EffectManager, via the RemoteControl class - and that class' handle() function as implemented in remotecontrol.cpp specifically. I don't think there's currently any direct IR remote interaction with individual effects. In either case YMMV, but I remember finding the code in RemoteControl::handle() very digestible.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1649244494, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD34FSUR3NMUAEIE2PQLXR5V5LANCNFSM6AAAAAAYVVFPJY . You are receiving this because you commented.Message ID: @.***>

[davepl]

Just musing here:

One option would be, when an effect has its settings changed, is that we call Start(). That’s normally only called when an effect is first shown and is for more expensive one-time operations.

As long as an app tore down and recreated any settings-dependent resources in Start(), it would all just work!

So, if we institute a policy of “Start()” may indicate new settings, I don’t think any callbacks would be needed.

Changing matrix size or number of channels might be a deal breaker no matter how we do it, at least for this version! But the settings we’ve already defined are easily managed in Start() by patterns like Weather.

• Dave

On Jul 25, 2023, at 8:32 AM, Robert Lipe @.***> wrote:

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

I have remotes on the way, but knowing I can (presumably) script some testing via curl/wget to honk on networked endpoints makes that practical to exercise now. Having the effects register SettingSpecs structs and when (some future?) web interface or network events push changes to Speed is a good goal. I'm looking at include/effects/matrix/PatternSubscribers.h and don't see that working, though. Ideally, you'd like to register an OnSpeedChanged event and have that listener/observer trigger when the dial is dragged.If you receive the entire list of Settings you register and have to pilfer through them, that's not totally terrible, but it's not ideal. You'd like to register OnSpeedChanged(SettingsValue Old, SettingsValue New) or something so you can just reschedule your timers without looping. Neither SightReader() nor UpdateSubscribers() quite reach that zen. I see where the registration for change notifications ishappening (FillSettingSpecs()), but I don't see where I'd get the callback for OnSettingChanged(SettingSpec old, SettingSpec new) were we can just operate on the change. Is there perhaps a better example, or is this a hole? I'd much rather receive a SettingsValue than a json blob.

Speed and Brightness are probably the easiest to think about for something that can change continuously that needs smooth adjustments and without having to dumpster dive in an entire std::vector to find the one that's changed. I could probably implement an onChanged handler for a Speed event for 80% of the new effects fairly mechanically. For testing, I'd just have a a shell script on the server turning the knob up and down endlessly.

I suspect we'll agree that the IR remote probably SHOULD wiggle at least speed and brightness. Let's pencil that in. Making that work for an effect or two will probably speak to us on how those interfaces should look. (That's probably not JSON and not a full array...) A pair of hashes for changed keynames would be better, but just the SettingsValues that changed would be better still.

Subscribers may be a poor example for the data I'm thinking of. Those values will be a humungo key copy/paste into place once every few years.

OnSpeedChanged(SettingsValue Old, SettingsValue New) is better than OnSettingsChanged(SettingsValue[] Old, SettingsValue[] New) is better than OnSettingsChanged(const JsonObjectConst& new) Loop over and see what changed()

I already have code like PatternSMFire2021.h: static constexpr uint8_t Speed = 150; // 1-252 ...why is not 255?! // Setting PatternSMFire2021.h: static constexpr uint8_t Scale = 9; // 1-99 is palette and scale // Setting PatternSMSpiro.h: static constexpr uint8_t Speed = 46; // 1-255 is length Setting PatternSMSpiro.h: static constexpr uint8_t Scale = 3; // 1-100 is palette Setting

that's just begging to be hooked up to code like this.

I'll report back when I'm more ready to tackle this head-on. We may need to team up on the sender side and the receiver side if that's OK.

On Tue, Jul 25, 2023 at 2:05 AM Rutger van Bergen @.***> wrote:

There are two types of settings:

• Device settings. These are kept in the DeviceSettings class (declared and implemented in devicesettings.h and devicesettings.cpp), which is made available via g_ptrSystem->DeviceSettings(). Notifications of updates to these settings are not pushed to effects that use them. This means that effects that rely on device settings for their own configuration currently need to pull and check changes to device setting values at intervals that make sense to them. An example of an effect doing this is PatternWeather, which gets a number of settings from DeviceSettings. An effect that just pulls a setting it needs from DeviceSettings whenever it needs it is PatternPongClock.
• Effect settings. Changes to these are pushed to an individual effect directly; LEDStripEffect includes an overloadable SetSetting() member function for this. Examples of an effect that adds its own effect settings to the standard set (that being friendlyName, maximumEffectTime, the read-only hasMaximumEffectTime, and the write-only clearMaximumEffectTime) is PatternSubscriber.

Both DeviceSettings and individual effects should publish the specifications for all settings they keep in a list of SettingSpec instances, and return a list of std::reference_wrappers to them, when asked. Both DeviceSettings and LEDStripEffect include a GetSettingSpecs() member function that can be used as the starting point for exploring exactly how this is currently set up. The SettingSpecs are published via a number of endpoints in the on-board web server's API, for consumption by JSON-savvy humans and/or a semi-dynamic (web) interface. The endpoints are documented in REST_API.md https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md .

I didn't look into it in much detail, but I'd say that things like Speed, Brightness, Scale, etc. are candidates for effect settings. This is also why these properties are persisted to JSON on a per-effect basis.

The IR remote does most of its interaction with EffectManager, via the RemoteControl class - and that class' handle() function as implemented in remotecontrol.cpp specifically. I don't think there's currently any direct IR remote interaction with individual effects. In either case YMMV, but I remember finding the code in RemoteControl::handle() very digestible.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1649244494, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD34FSUR3NMUAEIE2PQLXR5V5LANCNFSM6AAAAAAYVVFPJY . You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650066580, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCF7LATR3OIEG5WLTBU3XR7RIHANCNFSM6AAAAAAYVVFPJY. You are receiving this because you are subscribed to this thread.

[robertlipe]

I agree that changing matrix size or strip physical configuration should be a heavy-weight process that basically destroys and recreates the effect. Let's shelve those.

Your proposal of just reusing Start() would work, but you hinted at the problem with it. It's a heavyweight process. For something like Weather where your zip code changes quite rarely, that's OK. Imagine someone is grabbing a slider and dragging it or is beating on the brightness button on a remote. You probably want to send many tens of OnSliderMoving() per second to allow a smooth, interactive feel. Ideally, you'd receive a key/value pair of just what changed.

The ctor (currently) gives you a big old Javascript object to parse. The effect is then responsible for decomposing that JSON to the SettingValue array, looping over it, noticing what's changed, and acting on it. All the app really needs is "integer brightness was 73, now it's 78". A single SettingSpec (which I quite like) would be lovely. An array of dozens/hundreds of SettingSpecs encoded in JSON isn't as awesome. Is the JSON there a historical artifact that predates SettingSpecs or is there some other kind of data that can be smuggled in that? (I get that JSON, like a hash map, is kind of a universal arglist of key/value pairs.)

If we reuse start, we have to touch every effect. Almost every one of them (absolutely every one that I've worked on) call g()->Clear() as a very early step and resets all kinds of internal state. An (unmodified) effect that's not interested in effects changes is now going to flash wildly when that effect UI element is dragged. Maybe all the effects get modified with an 'if (!running++)' at the top that brackets the body. Some apps similar to ours have our Start in the body of Draw() with a special case (now tested on every Draw()) that hoists that out. Every effect will need to special case the first Start() from all the others.

In summary, it would "just work" and it'd be fine for settings that change infrequently, but I think there are issues if you start doing this at 10Hz or something.

Am I misunderstanding?

On Tue, Jul 25, 2023 at 12:16 PM David W Plummer @.***> wrote:

Just musing here:

One option would be, when an effect has its settings changed, is that we call Start(). That’s normally only called when an effect is first shown and is for more expensive one-time operations.

As long as an app tore down and recreated any settings-dependent resources in Start(), it would all just work!

So, if we institute a policy of “Start()” may indicate new settings, I don’t think any callbacks would be needed.

Changing matrix size or number of channels might be a deal breaker no matter how we do it, at least for this version! But the settings we’ve already defined are easily managed in Start() by patterns like Weather.

• Dave

On Jul 25, 2023, at 8:32 AM, Robert Lipe @.***> wrote:

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

I have remotes on the way, but knowing I can (presumably) script some testing via curl/wget to honk on networked endpoints makes that practical to exercise now. Having the effects register SettingSpecs structs and when (some future?) web interface or network events push changes to Speed is a good goal. I'm looking at include/effects/matrix/PatternSubscribers.h and don't see that working, though. Ideally, you'd like to register an OnSpeedChanged event and have that listener/observer trigger when the dial is dragged.If you receive the entire list of Settings you register and have to pilfer through them, that's not totally terrible, but it's not ideal. You'd like to register OnSpeedChanged(SettingsValue Old, SettingsValue New) or something so you can just reschedule your timers without looping. Neither SightReader() nor UpdateSubscribers() quite reach that zen. I see where the registration for change notifications ishappening (FillSettingSpecs()), but I don't see where I'd get the callback for OnSettingChanged(SettingSpec old, SettingSpec new) were we can just operate on the change. Is there perhaps a better example, or is this a hole? I'd much rather receive a SettingsValue than a json blob.

Speed and Brightness are probably the easiest to think about for something that can change continuously that needs smooth adjustments and without having to dumpster dive in an entire std::vector to find the one that's changed. I could probably implement an onChanged handler for a Speed event for 80% of the new effects fairly mechanically. For testing, I'd just have a a shell script on the server turning the knob up and down endlessly.

I suspect we'll agree that the IR remote probably SHOULD wiggle at least speed and brightness. Let's pencil that in. Making that work for an effect or two will probably speak to us on how those interfaces should look. (That's probably not JSON and not a full array...) A pair of hashes for changed keynames would be better, but just the SettingsValues that changed would be better still.

Subscribers may be a poor example for the data I'm thinking of. Those values will be a humungo key copy/paste into place once every few years.

OnSpeedChanged(SettingsValue Old, SettingsValue New) is better than OnSettingsChanged(SettingsValue[] Old, SettingsValue[] New) is better than OnSettingsChanged(const JsonObjectConst& new) Loop over and see what changed()

I already have code like PatternSMFire2021.h: static constexpr uint8_t Speed = 150; // 1-252 ...why is not 255?! // Setting PatternSMFire2021.h: static constexpr uint8_t Scale = 9; // 1-99 is palette and scale // Setting PatternSMSpiro.h: static constexpr uint8_t Speed = 46; // 1-255 is length Setting PatternSMSpiro.h: static constexpr uint8_t Scale = 3; // 1-100 is palette Setting

that's just begging to be hooked up to code like this.

I'll report back when I'm more ready to tackle this head-on. We may need to team up on the sender side and the receiver side if that's OK.

On Tue, Jul 25, 2023 at 2:05 AM Rutger van Bergen @.***> wrote:

There are two types of settings:

• Device settings. These are kept in the DeviceSettings class (declared and implemented in devicesettings.h and devicesettings.cpp), which is made available via g_ptrSystem->DeviceSettings(). Notifications of updates to these settings are not pushed to effects that use them. This means that effects that rely on device settings for their own configuration currently need to pull and check changes to device setting values at intervals that make sense to them. An example of an effect doing this is PatternWeather, which gets a number of settings from DeviceSettings. An effect that just pulls a setting it needs from DeviceSettings whenever it needs it is PatternPongClock.
• Effect settings. Changes to these are pushed to an individual effect directly; LEDStripEffect includes an overloadable SetSetting() member function for this. Examples of an effect that adds its own effect settings to the standard set (that being friendlyName, maximumEffectTime, the read-only hasMaximumEffectTime, and the write-only clearMaximumEffectTime) is PatternSubscriber.

Both DeviceSettings and individual effects should publish the specifications for all settings they keep in a list of SettingSpec instances, and return a list of std::reference_wrappers to them, when asked. Both DeviceSettings and LEDStripEffect include a GetSettingSpecs() member function that can be used as the starting point for exploring exactly how this is currently set up. The SettingSpecs are published via a number of endpoints in the on-board web server's API, for consumption by JSON-savvy humans and/or a semi-dynamic (web) interface. The endpoints are documented in REST_API.md < https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md>

.

I didn't look into it in much detail, but I'd say that things like Speed, Brightness, Scale, etc. are candidates for effect settings. This is also why these properties are persisted to JSON on a per-effect basis.

The IR remote does most of its interaction with EffectManager, via the RemoteControl class - and that class' handle() function as implemented in remotecontrol.cpp specifically. I don't think there's currently any direct IR remote interaction with individual effects. In either case YMMV, but I remember finding the code in RemoteControl::handle() very digestible.

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1649244494>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCSD34FSUR3NMUAEIE2PQLXR5V5LANCNFSM6AAAAAAYVVFPJY>

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650066580>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCF7LATR3OIEG5WLTBU3XR7RIHANCNFSM6AAAAAAYVVFPJY>.

You are receiving this because you are subscribed to this thread.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650228803, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD32UXVMDDGM5JICHJ2DXR75PPANCNFSM6AAAAAAYVVFPJY . You are receiving this because you commented.Message ID: @.***>

[davepl]

Oh ye of little faith! A well written effect would do heavyweight allocations in the constructor and reserve Start() for anything that was settings dependent. If it’s only doing the work required for the settings change, then, it’s the same workload regardless of whether delivered by Start() or some delegate mechanism.

We have to touch every effect to make sure it respects changed effects anyway, and reorging settings-dependent allocations in Start() seems to me like the least code thrash and solves the problem efficiently.

The only way I could see a 10Hz change would be maybe holding down the “change palette” key on the remote, but even then, if the effect only does the needed work, there’s no less it can do.

• Dave

On Jul 25, 2023, at 11:27 AM, Robert Lipe @.***> wrote:

I agree that changing matrix size or strip physical configuration should be a heavy-weight process that basically destroys and recreates the effect. Let's shelve those.

Your proposal of just reusing Start() would work, but you hinted at the problem with it. It's a heavyweight process. For something like Weather where your zip code changes quite rarely, that's OK. Imagine someone is grabbing a slider and dragging it or is beating on the brightness button on a remote. You probably want to send many tens of OnSliderMoving() per second to allow a smooth, interactive feel. Ideally, you'd receive a key/value pair of just what changed.

The ctor (currently) gives you a big old Javascript object to parse. The effect is then responsible for decomposing that JSON to the SettingValue array, looping over it, noticing what's changed, and acting on it. All the app really needs is "integer brightness was 73, now it's 78". A single SettingSpec (which I quite like) would be lovely. An array of dozens/hundreds of SettingSpecs encoded in JSON isn't as awesome. Is the JSON there a historical artifact that predates SettingSpecs or is there some other kind of data that can be smuggled in that? (I get that JSON, like a hash map, is kind of a universal arglist of key/value pairs.)

If we reuse start, we have to touch every effect. Almost every one of them (absolutely every one that I've worked on) call g()->Clear() as a very early step and resets all kinds of internal state. An (unmodified) effect that's not interested in effects changes is now going to flash wildly when that effect UI element is dragged. Maybe all the effects get modified with an 'if (!running++)' at the top that brackets the body. Some apps similar to ours have our Start in the body of Draw() with a special case (now tested on every Draw()) that hoists that out. Every effect will need to special case the first Start() from all the others.

In summary, it would "just work" and it'd be fine for settings that change infrequently, but I think there are issues if you start doing this at 10Hz or something.

Am I misunderstanding?

On Tue, Jul 25, 2023 at 12:16 PM David W Plummer @.***> wrote:

Just musing here:

One option would be, when an effect has its settings changed, is that we call Start(). That’s normally only called when an effect is first shown and is for more expensive one-time operations.

As long as an app tore down and recreated any settings-dependent resources in Start(), it would all just work!

So, if we institute a policy of “Start()” may indicate new settings, I don’t think any callbacks would be needed.

Changing matrix size or number of channels might be a deal breaker no matter how we do it, at least for this version! But the settings we’ve already defined are easily managed in Start() by patterns like Weather.

• Dave

On Jul 25, 2023, at 8:32 AM, Robert Lipe @.***> wrote:

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

I have remotes on the way, but knowing I can (presumably) script some testing via curl/wget to honk on networked endpoints makes that practical to exercise now. Having the effects register SettingSpecs structs and when (some future?) web interface or network events push changes to Speed is a good goal. I'm looking at include/effects/matrix/PatternSubscribers.h and don't see that working, though. Ideally, you'd like to register an OnSpeedChanged event and have that listener/observer trigger when the dial is dragged.If you receive the entire list of Settings you register and have to pilfer through them, that's not totally terrible, but it's not ideal. You'd like to register OnSpeedChanged(SettingsValue Old, SettingsValue New) or something so you can just reschedule your timers without looping. Neither SightReader() nor UpdateSubscribers() quite reach that zen. I see where the registration for change notifications ishappening (FillSettingSpecs()), but I don't see where I'd get the callback for OnSettingChanged(SettingSpec old, SettingSpec new) were we can just operate on the change. Is there perhaps a better example, or is this a hole? I'd much rather receive a SettingsValue than a json blob.

Speed and Brightness are probably the easiest to think about for something that can change continuously that needs smooth adjustments and without having to dumpster dive in an entire std::vector to find the one that's changed. I could probably implement an onChanged handler for a Speed event for 80% of the new effects fairly mechanically. For testing, I'd just have a a shell script on the server turning the knob up and down endlessly.

I suspect we'll agree that the IR remote probably SHOULD wiggle at least speed and brightness. Let's pencil that in. Making that work for an effect or two will probably speak to us on how those interfaces should look. (That's probably not JSON and not a full array...) A pair of hashes for changed keynames would be better, but just the SettingsValues that changed would be better still.

Subscribers may be a poor example for the data I'm thinking of. Those values will be a humungo key copy/paste into place once every few years.

OnSpeedChanged(SettingsValue Old, SettingsValue New) is better than OnSettingsChanged(SettingsValue[] Old, SettingsValue[] New) is better than OnSettingsChanged(const JsonObjectConst& new) Loop over and see what changed()

I already have code like PatternSMFire2021.h: static constexpr uint8_t Speed = 150; // 1-252 ...why is not 255?! // Setting PatternSMFire2021.h: static constexpr uint8_t Scale = 9; // 1-99 is palette and scale // Setting PatternSMSpiro.h: static constexpr uint8_t Speed = 46; // 1-255 is length Setting PatternSMSpiro.h: static constexpr uint8_t Scale = 3; // 1-100 is palette Setting

that's just begging to be hooked up to code like this.

I'll report back when I'm more ready to tackle this head-on. We may need to team up on the sender side and the receiver side if that's OK.

On Tue, Jul 25, 2023 at 2:05 AM Rutger van Bergen @.***> wrote:

There are two types of settings:

• Device settings. These are kept in the DeviceSettings class (declared and implemented in devicesettings.h and devicesettings.cpp), which is made available via g_ptrSystem->DeviceSettings(). Notifications of updates to these settings are not pushed to effects that use them. This means that effects that rely on device settings for their own configuration currently need to pull and check changes to device setting values at intervals that make sense to them. An example of an effect doing this is PatternWeather, which gets a number of settings from DeviceSettings. An effect that just pulls a setting it needs from DeviceSettings whenever it needs it is PatternPongClock.
• Effect settings. Changes to these are pushed to an individual effect directly; LEDStripEffect includes an overloadable SetSetting() member function for this. Examples of an effect that adds its own effect settings to the standard set (that being friendlyName, maximumEffectTime, the read-only hasMaximumEffectTime, and the write-only clearMaximumEffectTime) is PatternSubscriber.

Both DeviceSettings and individual effects should publish the specifications for all settings they keep in a list of SettingSpec instances, and return a list of std::reference_wrappers to them, when asked. Both DeviceSettings and LEDStripEffect include a GetSettingSpecs() member function that can be used as the starting point for exploring exactly how this is currently set up. The SettingSpecs are published via a number of endpoints in the on-board web server's API, for consumption by JSON-savvy humans and/or a semi-dynamic (web) interface. The endpoints are documented in REST_API.md < https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md>

.

I didn't look into it in much detail, but I'd say that things like Speed, Brightness, Scale, etc. are candidates for effect settings. This is also why these properties are persisted to JSON on a per-effect basis.

The IR remote does most of its interaction with EffectManager, via the RemoteControl class - and that class' handle() function as implemented in remotecontrol.cpp specifically. I don't think there's currently any direct IR remote interaction with individual effects. In either case YMMV, but I remember finding the code in RemoteControl::handle() very digestible.

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1649244494>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCSD34FSUR3NMUAEIE2PQLXR5V5LANCNFSM6AAAAAAYVVFPJY>

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650066580>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCF7LATR3OIEG5WLTBU3XR7RIHANCNFSM6AAAAAAYVVFPJY>.

You are receiving this because you are subscribed to this thread.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650228803, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD32UXVMDDGM5JICHJ2DXR75PPANCNFSM6AAAAAAYVVFPJY . You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650331301, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCF6XM5BWKDLHLWNDA2DXSAF2NANCNFSM6AAAAAAYVVFPJY. You are receiving this because you commented.

[robertlipe]

Little faith, but lots of experience! :-) I may be the new guy, but I've stared at a LOT of effects lately.

It may be the intent that Start() is lightweight. It's not our current reality. We can change that...

PatternAlienText.h's Start() calls g()->Clear(); That's probably bad in scrubber move. PatternBalls.h: initializes the array of the balls. PatternBounce.h: will probably lead the _boids[] (maybe its a smartptr) and reset all the balls. PatternFlowField.h: Ditto PatternMandala.h: will reset x and y for all displayed objects

... and so on. Since these were the examples I learned from, you can guess that a ton of my 62 new ones do th same. But this is OK - it's a yank and put to move those as all the bodies are small and easy.

So we basically repurpose Start(). Instead of Start(JsonObjectConst& stuff), can we make the new one Start(SettingsValue&)

This way we don't have to parse JSON and walk an array for the one that may have changed. We get only the changes and we get them in a reasonably machine-readable format this way. (Yeah, I know machines CAN read JSON, but it's clearly big and slow...)

Who is responsible for these hitting storage? Flash is slow and this could be called in a per-frame context. Best to not block the renderer for that. Are these serialized in the common code (yes, please) or down in the individual effects? We probably don't want anything JSON-ish in a performance path or we'll get stutters.

In the name of progress, I can take the task of moving the guts of the Starts up into the base constructors. That's a needed change if we're going to reuse Start() like this. Is that an agreed upon direction?

RJL

On Tue, Jul 25, 2023 at 1:41 PM David W Plummer @.***> wrote:

Oh ye of little faith! A well written effect would do heavyweight allocations in the constructor and reserve Start() for anything that was settings dependent. If it’s only doing the work required for the settings change, then, it’s the same workload regardless of whether delivered by Start() or some delegate mechanism.

We have to touch every effect to make sure it respects changed effects anyway, and reorging settings-dependent allocations in Start() seems to me like the least code thrash and solves the problem efficiently.

The only way I could see a 10Hz change would be maybe holding down the “change palette” key on the remote, but even then, if the effect only does the needed work, there’s no less it can do.

• Dave

On Jul 25, 2023, at 11:27 AM, Robert Lipe @.***> wrote:

I agree that changing matrix size or strip physical configuration should be a heavy-weight process that basically destroys and recreates the effect. Let's shelve those.

Your proposal of just reusing Start() would work, but you hinted at the problem with it. It's a heavyweight process. For something like Weather where your zip code changes quite rarely, that's OK. Imagine someone is grabbing a slider and dragging it or is beating on the brightness button on a remote. You probably want to send many tens of OnSliderMoving() per second to allow a smooth, interactive feel. Ideally, you'd receive a key/value pair of just what changed.

The ctor (currently) gives you a big old Javascript object to parse. The effect is then responsible for decomposing that JSON to the SettingValue array, looping over it, noticing what's changed, and acting on it. All the app really needs is "integer brightness was 73, now it's 78". A single SettingSpec (which I quite like) would be lovely. An array of dozens/hundreds of SettingSpecs encoded in JSON isn't as awesome. Is the JSON there a historical artifact that predates SettingSpecs or is there some other kind of data that can be smuggled in that? (I get that JSON, like a hash map, is kind of a universal arglist of key/value pairs.)

If we reuse start, we have to touch every effect. Almost every one of them (absolutely every one that I've worked on) call g()->Clear() as a very early step and resets all kinds of internal state. An (unmodified) effect that's not interested in effects changes is now going to flash wildly when that effect UI element is dragged. Maybe all the effects get modified with an 'if (!running++)' at the top that brackets the body. Some apps similar to ours have our Start in the body of Draw() with a special case (now tested on every Draw()) that hoists that out. Every effect will need to special case the first Start() from all the others.

In summary, it would "just work" and it'd be fine for settings that change infrequently, but I think there are issues if you start doing this at 10Hz or something.

Am I misunderstanding?

On Tue, Jul 25, 2023 at 12:16 PM David W Plummer @.***> wrote:

Just musing here:

One option would be, when an effect has its settings changed, is that we call Start(). That’s normally only called when an effect is first shown and is for more expensive one-time operations.

As long as an app tore down and recreated any settings-dependent resources in Start(), it would all just work!

So, if we institute a policy of “Start()” may indicate new settings, I don’t think any callbacks would be needed.

Changing matrix size or number of channels might be a deal breaker no matter how we do it, at least for this version! But the settings we’ve already defined are easily managed in Start() by patterns like Weather.

• Dave

On Jul 25, 2023, at 8:32 AM, Robert Lipe @.***> wrote:

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

I have remotes on the way, but knowing I can (presumably) script some testing via curl/wget to honk on networked endpoints makes that practical to exercise now. Having the effects register SettingSpecs structs and when (some future?) web interface or network events push changes to Speed is a good goal. I'm looking at include/effects/matrix/PatternSubscribers.h and don't see that working, though. Ideally, you'd like to register an OnSpeedChanged event and have that listener/observer trigger when the dial is dragged.If you receive the entire list of Settings you register and have to pilfer through them, that's not totally terrible, but it's not ideal. You'd like to register OnSpeedChanged(SettingsValue Old, SettingsValue New) or something so you can just reschedule your timers without looping. Neither SightReader() nor UpdateSubscribers() quite reach that zen. I see where the registration for change notifications ishappening (FillSettingSpecs()), but I don't see where I'd get the callback for OnSettingChanged(SettingSpec old, SettingSpec new) were we can just operate on the change. Is there perhaps a better example, or is this a hole? I'd much rather receive a SettingsValue than a json blob.

Speed and Brightness are probably the easiest to think about for something that can change continuously that needs smooth adjustments and without having to dumpster dive in an entire std::vector to find the one that's changed. I could probably implement an onChanged handler for a Speed event for 80% of the new effects fairly mechanically. For testing, I'd just have a a shell script on the server turning the knob up and down endlessly.

I suspect we'll agree that the IR remote probably SHOULD wiggle at least speed and brightness. Let's pencil that in. Making that work for an effect or two will probably speak to us on how those interfaces should look. (That's probably not JSON and not a full array...) A pair of hashes for changed keynames would be better, but just the SettingsValues that changed would be better still.

Subscribers may be a poor example for the data I'm thinking of. Those values will be a humungo key copy/paste into place once every few years.

OnSpeedChanged(SettingsValue Old, SettingsValue New) is better than OnSettingsChanged(SettingsValue[] Old, SettingsValue[] New) is better than OnSettingsChanged(const JsonObjectConst& new) Loop over and see what changed()

I already have code like PatternSMFire2021.h: static constexpr uint8_t Speed = 150; // 1-252 ...why is not 255?! // Setting PatternSMFire2021.h: static constexpr uint8_t Scale = 9; // 1-99 is palette and scale // Setting PatternSMSpiro.h: static constexpr uint8_t Speed = 46; // 1-255 is length Setting PatternSMSpiro.h: static constexpr uint8_t Scale = 3; // 1-100 is palette Setting

that's just begging to be hooked up to code like this.

I'll report back when I'm more ready to tackle this head-on. We may need to team up on the sender side and the receiver side if that's OK.

On Tue, Jul 25, 2023 at 2:05 AM Rutger van Bergen @.***> wrote:

There are two types of settings:

• Device settings. These are kept in the DeviceSettings class (declared and implemented in devicesettings.h and devicesettings.cpp), which is made available via g_ptrSystem->DeviceSettings(). Notifications of updates to these settings are not pushed to effects that use them. This means that effects that rely on device settings for their own configuration currently need to pull and check changes to device setting values at intervals that make sense to them. An example of an effect doing this is PatternWeather, which gets a number of settings from DeviceSettings. An effect that just pulls a setting it needs from DeviceSettings whenever it needs it is PatternPongClock.
• Effect settings. Changes to these are pushed to an individual effect directly; LEDStripEffect includes an overloadable SetSetting() member function for this. Examples of an effect that adds its own effect settings to the standard set (that being friendlyName, maximumEffectTime, the read-only hasMaximumEffectTime, and the write-only clearMaximumEffectTime) is PatternSubscriber.

Both DeviceSettings and individual effects should publish the specifications for all settings they keep in a list of SettingSpec instances, and return a list of std::reference_wrappers to them, when asked. Both DeviceSettings and LEDStripEffect include a GetSettingSpecs() member function that can be used as the starting point for exploring exactly how this is currently set up. The SettingSpecs are published via a number of endpoints in the on-board web server's API, for consumption by JSON-savvy humans and/or a semi-dynamic (web) interface. The endpoints are documented in REST_API.md <

https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md>

.

I didn't look into it in much detail, but I'd say that things like Speed, Brightness, Scale, etc. are candidates for effect settings. This is also why these properties are persisted to JSON on a per-effect basis.

The IR remote does most of its interaction with EffectManager, via the RemoteControl class - and that class' handle() function as implemented in remotecontrol.cpp specifically. I don't think there's currently any direct IR remote interaction with individual effects. In either case YMMV, but I remember finding the code in RemoteControl::handle() very digestible.

— Reply to this email directly, view it on GitHub <

https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1649244494>,

or unsubscribe <

https://github.com/notifications/unsubscribe-auth/ACCSD34FSUR3NMUAEIE2PQLXR5V5LANCNFSM6AAAAAAYVVFPJY>

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub <

https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650066580>,

or unsubscribe <

https://github.com/notifications/unsubscribe-auth/AA4HCF7LATR3OIEG5WLTBU3XR7RIHANCNFSM6AAAAAAYVVFPJY>.

You are receiving this because you are subscribed to this thread.

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650228803>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCSD32UXVMDDGM5JICHJ2DXR75PPANCNFSM6AAAAAAYVVFPJY>

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650331301>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCF6XM5BWKDLHLWNDA2DXSAF2NANCNFSM6AAAAAAYVVFPJY>.

You are receiving this because you commented.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650350353, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD35CNVQLMDU76PZ5N2LXSAHOXANCNFSM6AAAAAAYVVFPJY . You are receiving this because you commented.Message ID: @.***>

[davepl]

Sure, I’d be happy to have you follow the following basic approach:

1) If it’s something that is allocated based on “fixed” variable constants like matrix size or channel count, do it in the constructor 2) If it’s something that needs to be recreated as settings change, put it in Start()

I think that’d be a great use of time, I appreciate you taking a look at it!

PS: Where possible, if something is used infrequently or if it’s used in sequential (vs random) access, put it in PSRAM as done in other places. For high perf data or randomly accessed data, like compression stuff, put it in normal RAM. But that should be fairly rare.

• Dave

On Jul 25, 2023, at 1:44 PM, Robert Lipe @.***> wrote:

Little faith, but lots of experience! :-) I may be the new guy, but I've stared at a LOT of effects lately.

It may be the intent that Start() is lightweight. It's not our current reality. We can change that...

PatternAlienText.h's Start() calls g()->Clear(); That's probably bad in scrubber move. PatternBalls.h: initializes the array of the balls. PatternBounce.h: will probably lead the _boids[] (maybe its a smartptr) and reset all the balls. PatternFlowField.h: Ditto PatternMandala.h: will reset x and y for all displayed objects

... and so on. Since these were the examples I learned from, you can guess that a ton of my 62 new ones do th same. But this is OK - it's a yank and put to move those as all the bodies are small and easy.

So we basically repurpose Start(). Instead of Start(JsonObjectConst& stuff), can we make the new one Start(SettingsValue&)

This way we don't have to parse JSON and walk an array for the one that may have changed. We get only the changes and we get them in a reasonably machine-readable format this way. (Yeah, I know machines CAN read JSON, but it's clearly big and slow...)

Who is responsible for these hitting storage? Flash is slow and this could be called in a per-frame context. Best to not block the renderer for that. Are these serialized in the common code (yes, please) or down in the individual effects? We probably don't want anything JSON-ish in a performance path or we'll get stutters.

In the name of progress, I can take the task of moving the guts of the Starts up into the base constructors. That's a needed change if we're going to reuse Start() like this. Is that an agreed upon direction?

RJL

On Tue, Jul 25, 2023 at 1:41 PM David W Plummer @.***> wrote:

Oh ye of little faith! A well written effect would do heavyweight allocations in the constructor and reserve Start() for anything that was settings dependent. If it’s only doing the work required for the settings change, then, it’s the same workload regardless of whether delivered by Start() or some delegate mechanism.

We have to touch every effect to make sure it respects changed effects anyway, and reorging settings-dependent allocations in Start() seems to me like the least code thrash and solves the problem efficiently.

The only way I could see a 10Hz change would be maybe holding down the “change palette” key on the remote, but even then, if the effect only does the needed work, there’s no less it can do.

• Dave

On Jul 25, 2023, at 11:27 AM, Robert Lipe @.***> wrote:

I agree that changing matrix size or strip physical configuration should be a heavy-weight process that basically destroys and recreates the effect. Let's shelve those.

Your proposal of just reusing Start() would work, but you hinted at the problem with it. It's a heavyweight process. For something like Weather where your zip code changes quite rarely, that's OK. Imagine someone is grabbing a slider and dragging it or is beating on the brightness button on a remote. You probably want to send many tens of OnSliderMoving() per second to allow a smooth, interactive feel. Ideally, you'd receive a key/value pair of just what changed.

The ctor (currently) gives you a big old Javascript object to parse. The effect is then responsible for decomposing that JSON to the SettingValue array, looping over it, noticing what's changed, and acting on it. All the app really needs is "integer brightness was 73, now it's 78". A single SettingSpec (which I quite like) would be lovely. An array of dozens/hundreds of SettingSpecs encoded in JSON isn't as awesome. Is the JSON there a historical artifact that predates SettingSpecs or is there some other kind of data that can be smuggled in that? (I get that JSON, like a hash map, is kind of a universal arglist of key/value pairs.)

If we reuse start, we have to touch every effect. Almost every one of them (absolutely every one that I've worked on) call g()->Clear() as a very early step and resets all kinds of internal state. An (unmodified) effect that's not interested in effects changes is now going to flash wildly when that effect UI element is dragged. Maybe all the effects get modified with an 'if (!running++)' at the top that brackets the body. Some apps similar to ours have our Start in the body of Draw() with a special case (now tested on every Draw()) that hoists that out. Every effect will need to special case the first Start() from all the others.

In summary, it would "just work" and it'd be fine for settings that change infrequently, but I think there are issues if you start doing this at 10Hz or something.

Am I misunderstanding?

On Tue, Jul 25, 2023 at 12:16 PM David W Plummer @.***> wrote:

Just musing here:

One option would be, when an effect has its settings changed, is that we call Start(). That’s normally only called when an effect is first shown and is for more expensive one-time operations.

As long as an app tore down and recreated any settings-dependent resources in Start(), it would all just work!

So, if we institute a policy of “Start()” may indicate new settings, I don’t think any callbacks would be needed.

Changing matrix size or number of channels might be a deal breaker no matter how we do it, at least for this version! But the settings we’ve already defined are easily managed in Start() by patterns like Weather.

• Dave

On Jul 25, 2023, at 8:32 AM, Robert Lipe @.***> wrote:

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

I have remotes on the way, but knowing I can (presumably) script some testing via curl/wget to honk on networked endpoints makes that practical to exercise now. Having the effects register SettingSpecs structs and when (some future?) web interface or network events push changes to Speed is a good goal. I'm looking at include/effects/matrix/PatternSubscribers.h and don't see that working, though. Ideally, you'd like to register an OnSpeedChanged event and have that listener/observer trigger when the dial is dragged.If you receive the entire list of Settings you register and have to pilfer through them, that's not totally terrible, but it's not ideal. You'd like to register OnSpeedChanged(SettingsValue Old, SettingsValue New) or something so you can just reschedule your timers without looping. Neither SightReader() nor UpdateSubscribers() quite reach that zen. I see where the registration for change notifications ishappening (FillSettingSpecs()), but I don't see where I'd get the callback for OnSettingChanged(SettingSpec old, SettingSpec new) were we can just operate on the change. Is there perhaps a better example, or is this a hole? I'd much rather receive a SettingsValue than a json blob.

Speed and Brightness are probably the easiest to think about for something that can change continuously that needs smooth adjustments and without having to dumpster dive in an entire std::vector to find the one that's changed. I could probably implement an onChanged handler for a Speed event for 80% of the new effects fairly mechanically. For testing, I'd just have a a shell script on the server turning the knob up and down endlessly.

I suspect we'll agree that the IR remote probably SHOULD wiggle at least speed and brightness. Let's pencil that in. Making that work for an effect or two will probably speak to us on how those interfaces should look. (That's probably not JSON and not a full array...) A pair of hashes for changed keynames would be better, but just the SettingsValues that changed would be better still.

Subscribers may be a poor example for the data I'm thinking of. Those values will be a humungo key copy/paste into place once every few years.

OnSpeedChanged(SettingsValue Old, SettingsValue New) is better than OnSettingsChanged(SettingsValue[] Old, SettingsValue[] New) is better than OnSettingsChanged(const JsonObjectConst& new) Loop over and see what changed()

I already have code like PatternSMFire2021.h: static constexpr uint8_t Speed = 150; // 1-252 ...why is not 255?! // Setting PatternSMFire2021.h: static constexpr uint8_t Scale = 9; // 1-99 is palette and scale // Setting PatternSMSpiro.h: static constexpr uint8_t Speed = 46; // 1-255 is length Setting PatternSMSpiro.h: static constexpr uint8_t Scale = 3; // 1-100 is palette Setting

that's just begging to be hooked up to code like this.

I'll report back when I'm more ready to tackle this head-on. We may need to team up on the sender side and the receiver side if that's OK.

On Tue, Jul 25, 2023 at 2:05 AM Rutger van Bergen @.***> wrote:

There are two types of settings:

• Device settings. These are kept in the DeviceSettings class (declared and implemented in devicesettings.h and devicesettings.cpp), which is made available via g_ptrSystem->DeviceSettings(). Notifications of updates to these settings are not pushed to effects that use them. This means that effects that rely on device settings for their own configuration currently need to pull and check changes to device setting values at intervals that make sense to them. An example of an effect doing this is PatternWeather, which gets a number of settings from DeviceSettings. An effect that just pulls a setting it needs from DeviceSettings whenever it needs it is PatternPongClock.
• Effect settings. Changes to these are pushed to an individual effect directly; LEDStripEffect includes an overloadable SetSetting() member function for this. Examples of an effect that adds its own effect settings to the standard set (that being friendlyName, maximumEffectTime, the read-only hasMaximumEffectTime, and the write-only clearMaximumEffectTime) is PatternSubscriber.

Both DeviceSettings and individual effects should publish the specifications for all settings they keep in a list of SettingSpec instances, and return a list of std::reference_wrappers to them, when asked. Both DeviceSettings and LEDStripEffect include a GetSettingSpecs() member function that can be used as the starting point for exploring exactly how this is currently set up. The SettingSpecs are published via a number of endpoints in the on-board web server's API, for consumption by JSON-savvy humans and/or a semi-dynamic (web) interface. The endpoints are documented in REST_API.md <

https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md>

.

I didn't look into it in much detail, but I'd say that things like Speed, Brightness, Scale, etc. are candidates for effect settings. This is also why these properties are persisted to JSON on a per-effect basis.

The IR remote does most of its interaction with EffectManager, via the RemoteControl class - and that class' handle() function as implemented in remotecontrol.cpp specifically. I don't think there's currently any direct IR remote interaction with individual effects. In either case YMMV, but I remember finding the code in RemoteControl::handle() very digestible.

— Reply to this email directly, view it on GitHub <

https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1649244494>,

or unsubscribe <

https://github.com/notifications/unsubscribe-auth/ACCSD34FSUR3NMUAEIE2PQLXR5V5LANCNFSM6AAAAAAYVVFPJY>

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub <

https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650066580>,

or unsubscribe <

https://github.com/notifications/unsubscribe-auth/AA4HCF7LATR3OIEG5WLTBU3XR7RIHANCNFSM6AAAAAAYVVFPJY>.

You are receiving this because you are subscribed to this thread.

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650228803>,

or unsubscribe < https://github.com/notifications/unsubscribe-auth/ACCSD32UXVMDDGM5JICHJ2DXR75PPANCNFSM6AAAAAAYVVFPJY>

. You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub < https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650331301>, or unsubscribe < https://github.com/notifications/unsubscribe-auth/AA4HCF6XM5BWKDLHLWNDA2DXSAF2NANCNFSM6AAAAAAYVVFPJY>.

You are receiving this because you commented.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650350353, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD35CNVQLMDU76PZ5N2LXSAHOXANCNFSM6AAAAAAYVVFPJY . You are receiving this because you commented.Message ID: @.***>

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650534701, or unsubscribe https://github.com/notifications/unsubscribe-auth/AA4HCF2YXOYU3BDLXSUDHH3XSAVZ7ANCNFSM6AAAAAAYVVFPJY. You are receiving this because you commented.

[rbergen]

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

Circling back to this comment, I think I may have caused some confusion with my earlier comment. In an attempt to clear that up:

• SettingSpec objects are "just" metadata specifications for the actual settings that can be be retrieved and set at the device or effect level. They are meant to tell a future web interface what settings exist for an effect, what their description is, what's their type and if there are lower and upper boundaries to their values. As we seem to be focusing on effect settings in this conversation, I will not mention device settings any further. In any case, the key thing here is that the SettingSpec blobs do not play any role in the actual retrieval or changing of settings.
• The actual setting( value)s for an effect can be retrieved with LEDStipEffect's SerializeSettingsToJSON() member function - which does yield something that's very much intended to be turned into a JSON blob. They can be set with the same class' SetSetting() member function. The latter can be used to set the value for one effect setting at a time. As this function is called on the effect class' instance when an effect setting's value is changed, the effect can decide on the spot what needs to be interpreted/reinitialized/redrawn/reset/etc.
• The aforementioned member functions are exposed in the on-board web server's API using the Retrieve effect settings and Change effect settings endpoints.

How I see things is that any remote control's interactions with individual effects would not be routed through the web server's API, but through the RemoteControl::handle() function - and then somehow the member functions of LEDStripEffect I just mentioned. A first practical challenge I see there is finding remote control buttons that are available for use. On the remote controls I have at my disposal (those admittedly being the very simple ones that were "randomly thrown in" with some of the LED devices I acquired), the vast majority of buttons has already been assigned a function.

[robertlipe]

[ Sorry. Bumped the send key ]

Sorry that I kind of dropped this, Rutger. Hoping to bring this closer to a design plan:

Yes, I think we've agreed that device/strip/display level settings are kind of different than effects-specific settings, even though it would be nice if they shared a lot. I was indeed focused on the per-effect settings here as we could have a LOT of them in play. There are a few things that are implemented per-effect (speed and brightness being the most obvious) that are so common that we should probably just default to these existing.

I lobbied for OnSettingsChanged() (which we've agreed to not call it that, but to reuse Start() by changing its semantics from what's currently implemented (which wasn't apparently the intention...) and giving it a new arg list. I'd hoped the effects would receive ONLY the changed settings, preferably with an old and new but I'm malleable on that, as a SettingsObject and NOT a JSON blob. I'm not totally sure if I've lost that argument yet, but history is pretty clear. I really just want the effects to receive a glorified std::Variant and to know which one it was ("menu level", "13") and not a kilobyte of ascii text json to parse. That's a SettingsValue and not a JsonObjectConst where I have to parse the whole thing and figure out what's changed.

My rationale was that these may be scrolled somewhat rapidly via a user beating on a remote key or dragging a slider in some kind of future UI. We may get streams of dozens of these in a short time. Enough to paralyze a 80Mhz device? Maybe not, but stutters are easy to imagine and doing as little work as we can seems wise. So you'll never see that rate of change for editing a big-ole YouTube ID key, but dragging a speed or brightness button could certainly make for a lot of traffic.

That REST API is really pretty nifty. Even if we don't have a fancy way to access it, one of us (and it doesn't need to be someone familiar with the guts of embedded programming) should make a tiny web page - it can run on a real computer and just send requests to the NDL device(s) - for testing and development. I've been scripting with curl and that's pretty barbaric. This is something that a little PHP or something could make pretty awesome while a fancy UI is in development.

curl "http://192.168.2.165:/effects" {"currentEffect":19,"millisecondsRemaining":39761,"effectInterval":120000,"Effects":[{"name":"Audiograph","enabled":true,"core":true},{"name":"GhostWave","enabl

I agree with the overload (underload?) of physical remote buttons. I first started with an LED remote that had too few buttons to be useful at all. Then I took an old random TV remote (yard sales, junk bins, scrapyard, etc.) and just butchered ./include/remotecontrol.h to do something useful with as many of the buttons as I could find...and kept a physical piece of paper (sigh) within reach to remember that "satellite up", "change CD pack", "fast forward" or whatever mapped to something I found useful. I run much closer to a stock remotecontrol.h these days, but I should totally enable the 44-pin variation.

Since that time, I've picked up a number of strips that come with controllers and corresponding remotes like https://www.amazon.com/dp/B0C1ZM751X that include remotes with more buttons than even make sense. 16 colors, most of which I can't name and other buttons that are easy to repurpose. Now that I look at the code, the key44

define may be just the ticket on those. The same (?) remote + controller

is under $2 on Ali https://www.aliexpress.us/item/3256802133498118.html, as you'd kind of expect. (Side note: what country are you in? I'm in the U.S. in the Nashville area...) The controller itself is kind of useless on WS2812's as it wants the RGB style, but "tossing" the controller and reusing the remote is totally legit. Several of my strip acquisitions have included these remotes, so NOW I'm drowning in an ocean of remotes + controllers that really don't work very well in random combinations or with the light configurations I care about. I need to start putting stickers on things as soon as they hit the door.

Now I have to figure out how products like https://www.amazon.com/dp/B0BG3935QY have two lines leaving the "controller", but 3 lines (VCC, GND, DATA) running down the strip. Maybe they're using a decoupler to extract the "AC" signal from the power rails? I dunno yet. But that general class of products is totally worth raiding as a source of bulbs and remotes. (I don't particularly recommend it for the breadboarding crowd...) Put the lights in a place where you don't need the remote and steal the remote. :-) I now have an embarrassing number of crappy little remotes that I'm not totally sure which controller is their soulmate. The danger (for us) about random Amazon--ish products is that about half of the ones labeld "individually controllable" or that show pictures of the same are NOT Neopixel-grade hardware at all; they're RGB. All the reds are in parallel, all the greens, and such. So they may show 16M colors, but not all at once. Some products are advertised that are physically impossible to recreate the pictures on the box. It's bad!

IMO, we could probably combine these two problems and have a "remote" web interface that does all the things so you don't need IR remotes at all and can just beat on keypresses in a web interface.

So, our TLDR/Next steps: 1) I've agreed to rearrange most of the code in Setup into the ctors. 2) Someone needs to change the arglist to Start to take an arglist of ... something. I vote for a SettingsEffect array, but I have a feeling it'll be a JSON blob. I'm willing to help with this, but need someone able to set official project description on the desired API. 3) Remotes are hard. Shop carefully. Plea for help for a web version to reduce/eliminate need. 4) We should probably automate some kind of test fixture for cycling through effects using networking. I've worked on effects that did Bad Things when scrolling through things and it resulted in a /div0, for example. 5) For our workers buying "cheap" products, be aware that LOTS of Amazon listings are just plain wrong. Not everything claiming to be pixel addressable actually IS.

So, yes, I agree we should do more in this area and I'm willing to help put my code where my mouth is.

RJL

On Tue, Jul 25, 2023 at 3:58 PM Rutger van Bergen @.***> wrote:

Thank you. This is helpful. Since I have a BUNCH of effects in L1 cerebral cache right now, I was thinking about the interactions.

Cycling back to this comment, I think I may have caused some confusion with my earlier comment. In an attempt to clear that up:

• SettingSpec objects are "just" metadata specifications for the actual settings that can be be retrieved and set at the device or effect level. They are meant to tell a future web interface what settings exist for an effect, what their description is, what's their type and if there are lower and upper boundaries to their values. As we seem to be focusing on effect settings in this conversation, I will not mention device settings any further. In any case, the key thing here is that the SettingSpec blobs do not play any role in the actual retrieval or changing of settings.
• The actual setting( value)s for an effect can be retrieved with LEDStipEffect's SerializeSettingsToJSON() member function - which does yield a JSON blob. They can be set with the same class' SetSetting() member function. The latter can be used to set the value for one effect setting at a time. As this function is called on the effect class' instance when an effect setting's value is changed, the effect can decide on the spot what needs to be interpreted/reinitialized/redrawn/reset/etc.
• The aforementioned member functions are exposed in the on-board web server's API using the Retrieve effect settings https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md#retrieve-effect-settings and [Change effect settings]( https://github.com/PlummersSoftwareLLC/NightDriverStrip/blob/main/REST_API.md#change-effect-settings] endpoints.

How I see things is that any remote control's interactions with individual effects would not be routed through the web server's API, but through the RemoteControl::handle() function - and then somehow the member functions of LEDStripEffect I just mentioned. A first practical challenge I see there is finding remote control buttons that are available for use. On the remote controls I have at my disposal (those admittedly being the very simple ones that were "randomly thrown in" with some of the LED devices I acquired), the vast majority of buttons has already been assigned a function.

— Reply to this email directly, view it on GitHub https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650551193, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACCSD36P4EMWYTR43TCDU6LXSAXQXANCNFSM6AAAAAAYVVFPJY . You are receiving this because you commented.Message ID: @.***>

[robertlipe]

@davepl , going back to https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650538291

We have some dependency issues that make this not quite mechanical. For example, patternBounce is using the _boids that are in g(). (I didn't even know that g() had Boids[]. I just created and managed my own, such as https://github.com/robertlipe/NightDriverStrip/blob/469e24808063a42bc40c2bd10adad9690e0de142/include/effects/matrix/PatternSMBoidExplosion.h#L31 - maybe those should be psram allocated, but that's not for here. It's a weird thing to stash in gfxbase.)

So we can't just blindly move the bodies of Starts into Inits() that are called by all the ctors. Maybe you know the precise construction order, but for whatever reason, g() is invalid at constructor time.

Likewise, we have a bunch of g()->Clear calls in Starts. Maybe those shouldn't be there at all to allow cross fades, but it's not UNreasonable for an effect to want to start with a clear screen. (Some of the ones I worked on really do require it because they immediately start smearing or blobbing the contents of the pixel buffer and droppings from a previous effect weird them out.) Other existing effects are accessing g()->GetNoise() inside Start - that won't work in a constructor and it would be unsightly to do it on an effect change. PongClock takes a similar nullptr deref if we move that up to a ctor, but it's doing work that needs rethought in an effects system anyway. I didn't analyze the one in Spectrumeffects, but it probably will crash too - I just didn't get through all the other crashes, I think.

Of the existing eleven Starts, eight of them would cause visual weirdities if called at runtime. This is why we were trying to push them into the ctors.

Therefore, it seems reasonable to have an entry point that's after the ctors, after we have a global g() state, and before the first Draw(). I propose we call that entry point Start() :-).

That puts us needing a new entry point in the effects. It's going to need an argument, so we'd be changing the footprint of Start even if we got through the above. I propose an OnSettingsChanged(SettingSpec[] specs)

I suppose an alternative, if you really want Start() to mean both "effect is ready to run and, well Start" and to mean "there has been an effects change" that most Starts will need to maintain an internal flags for first invocation and treat itself specially on that invocation before the first Draw (so it can access g()->GetNoise or whatever) and then act differently and look at its new argument on subsequent calls. I'm not a fan of that "on the first call to read() call open()" kind of design.

Again, I'm willing to do or help with the work. I'm just saying that (my understanding of) the proposal in https://github.com/PlummersSoftwareLLC/NightDriverStrip/issues/294#issuecomment-1650538291 seems to hit an unexpected iceberg. The Rest interface (and Remote) "just" need to emit/broadcast these for speed and brightness and probably just the Rest interface (which I hope is used by the web and mobile apps) need to emit it for specific changes in the app.

Is this a reasonable path forward on the top-level goal in this issue?

P.S. I've gotta get a debugger on this thing. Debugging constructor crashes from the assembly - on a processor I don't really speak - is no fun. Is a JTAG pod the way to go? That'd help upload speeds, but would really hurt my portability.

[robertlipe]

Oof, but we might have USE_REMOTE but not USE_WEBSERVER and include/webserver.h is where all the SettingSpec manipulation is done.

I don't know enough about how remote and REST/web interact to propose an API that's ideal. Rutger, is that your area?

[rbergen]

To be frank, I'm not sure why we are talking about a triangle that has the internal API (as provided by LEDStripEffect), the web server and the IR remote as its corners. In my view:

• The LEDStripEffect::SerializeSettingsToJSON and LEDStripEffect::SetSetting member functions are the actual C++ API (or ABI, if you will) for getting and pushing settings values from and to an individual effect. The name SerializeSettingsToJSON is actually mildly misleading. The settings end up in a JSONObject, which is not (yet) a JSON blob. It's more of a "dynamic object structure", with the clever conversions to and from a range of (primitive) types one would want. Those can also be extended with conversions to and from more complex types; this has already been done for CRGB and CRGBPalette16 in jsonserializer.h.
• What the web server does is "convert" the internal API to and from the language the Internet speaks, i.e. a semi-RESTful API. This is where JSONObjects get converted to and from the actual JSON blobs. Also, the SettingSpecs are externally published by the web server, to aid in the "dynamic implementation" of code that allows human users to interpret and change the settings - this is what the SettingSpecs were created for.
• If we want the IR remote to modify effect settings, then I think RemoteServer should interact directly with the functions mentioned in the first bullet. It should most definitely remain totally unaware of the fact that the web server even exists. In this situation, the SettingSpecs objects could be used to see if the current effect publishes a particular setting that RemoteServer is aware of, and manipulate its value when a particular button is pressed.

So: the core C++ API for changing effect settings is provided by LEDStripEffect, the webserver takes care of translating that to and from "web lingo", and the IR remote server would do the translation from IR button codes. SettingSpecs can be used to discover if a particular effect publishes a particular setting. Concerning the latter point, as I'm typing this I'm inclined to say that RemoteServer could probably blindly invoke SetSetting when a particular effect setting button is pushed on the remote. SetSetting's declaration is such that it accepts a const String& setting name and a const String& setting value, and should just ignore any name/value combinations it doesn't understand. This is what the current implementations in LEDStripEffect and PatternSubscribers do as well.

If pulling the value for a particular setting of an effect from JSONObject is too cumbersome, then we could extend the LEDStripEffect API/ABI with a "GetSetting" function that accepts a const String& name and a String& value as parameters. It could then return a bool to indicate if it actually recognized the setting name, and thus put the setting value in the second parameter. That would be a pretty simple thing to add. It does mean that an effect that exposes settings has to return them in two ways - through the JSONObject and individually. That we could then address by only implementing GetSetting in the effect, and moving the composition of the JSONObject to the web server. The reason I'm reluctant to do that is that it would increase the number of function calls between the webserver and the effect - but maybe I'm worrying too much about the impact of that.