Feature: expose additional effect settings #294
Comments
|
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: 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. |
|
@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.) |
|
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. |
|
There are two types of settings:
Both 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 |
|
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<SettingSpec> 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
<#294 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD34FSUR3NMUAEIE2PQLXR5V5LANCNFSM6AAAAAAYVVFPJY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
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<SettingSpec> 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
> <#294 (comment)>,
> 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 <#294 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF7LATR3OIEG5WLTBU3XR7RIHANCNFSM6AAAAAAYVVFPJY>.
You are receiving this because you are subscribed to this thread.
|
|
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<SettingSpec> 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
> > <
#294 (comment)>,
> > 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 <
#294 (comment)>,
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
<#294 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD32UXVMDDGM5JICHJ2DXR75PPANCNFSM6AAAAAAYVVFPJY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
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<SettingSpec> 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
> > > <
> #294 (comment)>,
>
> > > 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 <
> #294 (comment)>,
> 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
> <#294 (comment)>,
> 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 <#294 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF6XM5BWKDLHLWNDA2DXSAF2NANCNFSM6AAAAAAYVVFPJY>.
You are receiving this because you commented.
|
|
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<SettingSpec> 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
> > > > <
> >
#294 (comment)>,
> >
> > > > 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 <
> >
#294 (comment)>,
> > 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
> > <
#294 (comment)>,
> > 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 <
#294 (comment)>,
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
<#294 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD35CNVQLMDU76PZ5N2LXSAHOXANCNFSM6AAAAAAYVVFPJY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
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<SettingSpec> 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
> > > > > <
> > >
> #294 (comment)>,
>
> > >
> > > > > 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 <
> > >
> #294 (comment)>,
>
> > > 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
> > > <
> #294 (comment)>,
>
> > > 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 <
> #294 (comment)>,
> 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
> <#294 (comment)>,
> 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 <#294 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AA4HCF2YXOYU3BDLXSUDHH3XSAVZ7ANCNFSM6AAAAAAYVVFPJY>.
You are receiving this because you commented.
|
Circling back to this comment, I think I may have caused some confusion with my earlier comment. In an attempt to clear that up:
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 |
|
[ 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
<#294 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACCSD36P4EMWYTR43TCDU6LXSAXQXANCNFSM6AAAAAAYVVFPJY>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
|
@davepl , going back to #294 (comment) 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 #294 (comment) 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. |
|
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? |
|
To be frank, I'm not sure why we are talking about a triangle that has the internal API (as provided by
So: the core C++ API for changing effect settings is provided by If pulling the value for a particular setting of an effect from |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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; thePatternSubscriberseffect 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:
REST_API.mdfile contains information about the effect settings API endpoints.PatternSubscriberseffect provides an example of an effect extending the basic settings exposed byLEDStripEffect.LEDStripEffectincludes a number of helper functions for changing effect settings of "known types". Known types are themselves defined by theSettingSpecstruct intypes.h.The text was updated successfully, but these errors were encountered: