You may want to consider exponential backoff for the calls to the plm

[iDVB]

The PLM hardware and Insteon infra is pretty slow, but people's expectations for UI are very high.

Consider a UI where the user can use a 0-100 slider bar to control their light.level() in realtime. Not just wait till the user stops dragging and releases.... but control the levels as the scrub the bar.

I've simulated this with my UI and home-controller and I think it's choking on the calls rather then handling them. It seems it just tries to have them all queued up regardless of how many.

[mark-hahn]

It seems it just tries to have them all queued up regardless of how many.

I'm in the middle of implementing the same kind of slider.

I have an idea on how to fix this problem. On each queue entry you have an
extra field, let's call it "uniqueId". When you make a call, and include
this argument, then before the call is added to the queue you remove all
entries with the same uniqueId. In other words there cannot be more than
one entry with this id. The id should be unique across applications, like
a namespace.

You would only use this feature in a call when you only care about the last
one sent. This is exactly what is needed for this problem.

If necessary I'll make a PR for this at the same time I implement the code
that needs it. It would be a safe change because all legacy calls won't
have this argument and will behave the same as before.

On Sun, Apr 26, 2015 at 8:41 PM, iDVB notifications@github.com wrote:

The PLM hardware and Insteon infra is pretty slow, but people's
expectations for UI are very high.

Consider a UI where the user can use a 0-100 slider bar to control their
light.level() in realtime. Not just wait till the user stops dragging and
releases.... but control the levels as the scrub the bar.

I've simulated this with my UI and home-controller and I think it's
choking on the calls rather then handling them. It seems it just tries to
have them all queued up regardless of how many.

—
Reply to this email directly or view it on GitHub
#32.

[brandongoode]

I like the idea of the canceling pending actions over just blocking. I use the queuing to make sure all the commands get executed. For example if I make a single call to several devices, I want to make sure all devices get the update. For my UI, I don't send the next call to the same device until I've received a response (i.e. I throttle manually).

Instead of creating a new "uniqueId" we could just us the device ID.

insteon.cancelPending(deviceId);  // cancel all queued commands for a device
light.cancelPending(); // cancel all queued for the light device object

You could also call the cancelPending() without an ID to cancel all queued actions.

[mark-hahn]

Instead of creating a new "uniqueId" we could just us the device ID.

For this to work you'd have to be sure a device can only have one command
like brightness, which is true in the case of lights. For my relays though
there are different ports. You have to be sure the last command for any
port gets executed. So I think the uniqueId is required. It should be
easy to code and is entirely optional.

The name uniqueId kind of sucks, but I haven't been able to think of
anything better.

I don't send the next call to the same device until I've received a
response (i.e. I throttle manually).

This is still needed of course.

On Mon, Apr 27, 2015 at 4:20 AM, Brandon Goode notifications@github.com
wrote:

I like the idea of the canceling pending actions over just blocking. I use
the queuing to make sure all the commands get executed. For example if I
make a single call to several devices, I want to make sure all devices get
the update. For my UI, I don't send the next call to the same device until
I've received a response (i.e. I throttle manually).

Instead of creating a new "uniqueId" we could just us the device ID.

insteon.cancelPending(deviceId); // cancel all queued commands for a device
light.cancelPending(); // cancel all queued for the light device object

You could also call the cancelPending() without an ID to cancel all
queued actions.

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[brandongoode]

Another option is to have the cancel compare the current pending raw command and look for matches to cancel. Then you could customize it a the type Class.

insteon.cancelPending(regex);  // cancel all queued commands that match the regex
light.cancelPending(); // cancel all queued for the light device object
io.cancelPending(port); // cancel all queued messages for the io device with port as command2

[mark-hahn]

Do you mean to write a custom cancelPending method for each device type?
That seems like a pain.

On Mon, Apr 27, 2015 at 4:23 PM, Brandon Goode notifications@github.com
wrote:

Another option is to have the cancel compare the current pending raw
command and look for matches to cancel. Then you could customize it a the
type Class.

insteon.cancelPending(regex); // cancel all queued commands that match the regex
light.cancelPending(); // cancel all queued for the light device object
io.cancelPending(port); // cancel all queued messages for the io device with port as command2

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[mark-hahn]

But it's your package and your call (no pun intended).

On Mon, Apr 27, 2015 at 4:25 PM, Mark Hahn mark@hahnca.com wrote:

Do you mean to write a custom cancelPending method for each device type?
That seems like a pain.

On Mon, Apr 27, 2015 at 4:23 PM, Brandon Goode notifications@github.com
wrote:

Another option is to have the cancel compare the current pending raw
command and look for matches to cancel. Then you could customize it a the
type Class.

insteon.cancelPending(regex); // cancel all queued commands that match the regex
light.cancelPending(); // cancel all queued for the light device object
io.cancelPending(port); // cancel all queued messages for the io device with port as command2

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[brandongoode]

I just pushed an update that has a cancelPending that cancels commands based on matching the raw Insteon command. This does not have to be the final solution, just my option. Please feel encouraged to call me out if you would like to see something different.

@mark-hahn - Please continue to debate on this and future issues. - It makes for better code (and more enjoyable coding).

[mark-hahn]

No problem. I love to argue. :-)

Speaking of arguing ...

Having to deal with a raw insteon commands kind of sucks. Only the low
level should even know what that is.

I would like to offer again to code the queue-tagging option as a PR.
Somehow it feels like the clean solution to me. Putting tags in queues is
common practice.

Implementing your scheme to have each device implement a method to cancel
based on the devices need's is second best but much better than what you
have now. I thought about it and every device doesn't have to implement
this. Only ones that can have independent calls like ports. But how would
the device method clear the queue? Again it seems like we would have to
tag the queue.

On Mon, Apr 27, 2015 at 6:58 PM, Brandon Goode notifications@github.com
wrote:

I just pushed an update that has a cancelPending that cancels commands
based on matching the raw Insteon command. This does not have to be the
final solution, just my option. Please feel encouraged to call me out if
you would like to see something different.

@mark-hahn https://github.com/mark-hahn - Please continue to debate on
this and future issues. - It makes for better code (and more enjoyable
coding).

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[brandongoode]

I'm not opposed to it. I think we could even support both options. Please create a PR with the tagging (even if it's only a start). I'm a visual person, so need to see some example code before I really get it.

[mark-hahn]

When thinking through the implementation and usage I realized what you have implemented is all that is needed. After looking at your code I saw that your current device methods use the device id, not the regex (except for IO). So my complaint about higher levels needing to know the hex isn't really valid. The code for IO also looks fine. I didn't realize you had implemented so much.

Sorry for the unnecessary arguing. I was in love with my algorithm instead of just thinking things through.

[iDVB]

Sorry, I'm a bit late to the party on this one but here are my two cents.
I'm thinking from the UX perspective here. Users might want to scrub that slider bar and see the light level-up and down in realtime.... just like this guy....

It sounds like you're planning to use a cancelPending method which might have a debounce effect rather then the intended throttle. You likely only want to cancelPending on "some" of the calls (throttle) but not all.

I think if you cancelPending of all until the last request,....then this module will have IMHO a similar flaw to many other HomeAutomation software, which is they take the easy way out and wait for the user to stop scrubbing the bar before they send out the level.

[mark-hahn]

It sounds like you're planning to use a cancelPending method which might
have a debounceeffect rather then the intended throttle. You likely only
want to cancelPending on "some" of the calls (throttle) but not all.

What you are saying would be true if you used the cancelPending call in a
naive way. But used properly it gives you neither simple debouncing nor
simple throttling.

You call cancelPending immediately before each call to set a value. Then
the latest value is always sent at the soonest possible time. This
achieves the desired speed effect. It allows a slider to behave as
expected.

On Tue, Apr 28, 2015 at 9:13 AM, iDVB notifications@github.com wrote:

Sorry, I'm a bit late to the party on this one but here are my two cents.
I'm thinking from the UX perspective here. Users might want to scrub that
slider bar and see the light level-up and down in realtime.... just like
this guy....

[image: Dimmer]
https://camo.githubusercontent.com/387cb767e5eb3fc19ca2de19c60fa55b61843a75/687474703a2f2f676f6f2e676c2f35366d54474e

It sounds like you're planning to use a cancelPending method which might
have a debounce effect rather then the intended throttle. You likely only
want to cancelPending on "some" of the calls (throttle) but not all.

I think if you cancelPending of all until the last request,....then this
module will have IMHO a similar flaw to many other HomeAutomation software,
which is they take the easy way out and wait for the user to stop scrubbing
the bar before they send out the level.

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[iDVB]

Fair enough. Looking forward to test it out. Maybe if you have a suggestion/example of how to properly use it too? Thanks for your efforts here.

[mark-hahn]

if you have a suggestion/example of how to properly use it

Just place the cancelPending call before every fast-changing call. You can
also do this on every call, not just fast-changing ones. It just runs a
tiny bit slower which can't be perceived. I'm planning on wrapping it in a
local function ...

function setLevel(level) {
  light.cancelPending();
  light.level(level);
}

One could argue that this should just be built-in in the core insteon Light
class. But for other device types this may not work.

On Tue, Apr 28, 2015 at 10:25 AM, iDVB notifications@github.com wrote:

Fair enough. Looking forward to test it out. Maybe if you have a
suggestion/example of how to properly use it too? Thanks for your efforts
here.

—
Reply to this email directly or view it on GitHub
#32 (comment)
.

[iDVB]

It appears the cancelPending() method just returns true/false. Following the format of the other methods.... would it not be a good idea to have it also return a promise?

[brandongoode]

The cancelPending method is synchronous and therefore the overhead of a promise isn't needed. At least that was my logic.