eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
865 stars 782 forks source link

LIFX binding feature request: implement postCommand and UDP broadcast #2082

Closed cbcoursera1 closed 7 years ago

cbcoursera1 commented 8 years ago

LIFX bulbs currently change (relatively) slowly and serially when making changes to a group. My understanding is that this can be sped up or made simultaneous in two ways:

Implementing postCommand This would allow multiple commands to be sent in parallel via the event bus. sendCommand, currently used, only allows for a single thread in the event bus.

Creating a UDP broadcast Thing This would allow for simultaneous changing of all LIFX bulbs on the network. I've seen this used in other implementations of the LIFX LAN Protocol.

I'm relatively new to this and don't have the knowledge to add these to the binding myself, but if anyone is able I know that it would be appreciated by LIFX users.

kgoderis commented 8 years ago

Pls note that sendCommand and postCommand are only relevant for events from the bulb towards the ESH runtime, not from the runtime to the bulb

On the UDP broadcast it should be noted that even as the command is send to a Group, the ThingHandler get notified individually, eg it does not differentiate between commands send to a group or item directly. So, it would be difficult to group bulbs at the UDP level within the binding, it would also bring complexity as communications have to be shared between ThingHandlers which is something we do not want.

I have to revisit the code to see if we can render the communications bit

Sent from my iPhone

On 30 Aug 2016, at 18:40, cbcoursera1 notifications@github.com wrote:

LIFX bulbs currently change (relatively) slowly and serially when making changes to a group. My understanding is that this can be sped up or made simultaneous in two ways:

Implementing postCommand This would allow multiple commands to be sent in parallel via the event bus. sendCommand, currently used, only allows for a single thread in the event bus.

Creating a UDP broadcast Thing This would allow for simultaneous changing of all LIFX bulbs on the network. I've seen this used in other implementations of the LIFX LAN Protocol.

I'm relatively new to this and don't have the knowledge to add these to the binding myself, but if anyone is able I know that it would be appreciated by LIFX users.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

kgoderis commented 8 years ago

... Concurrent.

To be investigated but I understand your concern

Karel

Sent from my iPhone

On 30 Aug 2016, at 19:12, Karel Goderis karel.goderis@me.com wrote:

Pls note that sendCommand and postCommand are only relevant for events from the bulb towards the ESH runtime, not from the runtime to the bulb

On the UDP broadcast it should be noted that even as the command is send to a Group, the ThingHandler get notified individually, eg it does not differentiate between commands send to a group or item directly. So, it would be difficult to group bulbs at the UDP level within the binding, it would also bring complexity as communications have to be shared between ThingHandlers which is something we do not want.

I have to revisit the code to see if we can render the communications bit

Sent from my iPhone

On 30 Aug 2016, at 18:40, cbcoursera1 notifications@github.com wrote:

LIFX bulbs currently change (relatively) slowly and serially when making changes to a group. My understanding is that this can be sped up or made simultaneous in two ways:

Implementing postCommand This would allow multiple commands to be sent in parallel via the event bus. sendCommand, currently used, only allows for a single thread in the event bus.

Creating a UDP broadcast Thing This would allow for simultaneous changing of all LIFX bulbs on the network. I've seen this used in other implementations of the LIFX LAN Protocol.

I'm relatively new to this and don't have the knowledge to add these to the binding myself, but if anyone is able I know that it would be appreciated by LIFX users.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

cbcoursera1 commented 8 years ago

I'm not following your statement about commands from the bulb to the ESH runtime, maybe I haven't explained myself well.

I'm using OpenHAB and trying to write a rule that will simultaneously change several bulbs, and I'm using sendCommand() in my rule script to make the changes to the bulbs. So my thinking is that this is a command from the ESH runtime to the bulb, but maybe I'm not completely understanding.

My thought on the UDP broadcast Thing was this:

In other LIFX LAN Protocol implementations, I understand that simultaneous bulb changing is sometimes done via UDP broadcast. It's simultaneous because you're utilizing the network infrastructure, and it's fast because it's asynchronous. I was thinking that a Thing could be identified on the network at the UDP broadcast IP, and sending commands to that Thing would propagate them across all LIFX bulbs. But, I do understand that this goes counter to the definition of a Thing as being physical.

kgoderis commented 8 years ago

I'm not following your statement about commands from the bulb to the ESH runtime, maybe I haven't explained myself well.

I'm using OpenHAB and trying to write a rule that will simultaneously change several bulbs, and I'm using sendCommand() in my rule script to make the changes to the bulbs. So my thinking is that this is a command from the ESH runtime to the bulb, but maybe I'm not completely understanding.

Ok that is ok ;-)

I thought you were referring to https://github.com/openhab/openhab/wiki/How-To-Implement-A-Binding , near the end of the page.

kgoderis commented 8 years ago

In other LIFX LAN Protocol implementations, I understand that simultaneous bulb changing is sometimes done via UDP broadcast. It's simultaneous because you're utilizing the network infrastructure, and it's fast because it's asynchronous. I was thinking that a Thing could be identified on the network at the UDP broadcast IP, and sending commands to that Thing would propagate them across all LIFX bulbs. But, I do understand that this goes counter to the definition of a Thing as being physical.

The ThingHandler already uses both broadcast and unicast sockets. The broadcast mainly for service discovery, and the unicast for one to one communications to the bulb. The ThingHandler gets called on handleCommand() by the runtime whenever you do a postCommand in a Rule or alike, but it can not differentiate wether that postCommand was done on a linked Item, or on a Group. So, the ThingHandler has no way to know where the UDP message has to go to, except for its own bulb. Also, ThingHandlers should not know about each other (well, there is a way for them to figure out if there are any siblings but is is bad practice to interweave them).

That being said, all the ThingHandlers have to go through a NetworkThrottler which rate limits the messages to the bulbs (bulbs can only take 20 messages/second, but in practice you can flood the network easily), and secondly, because of the usage of Selectors to get hold of writeable sockets, there is lock() in place, which means that only one ThingHandler at a time can write to the LAN. The main reason to have that lock() is to avoid ConcurrentModificationExceptions on the selector. We could try to place that lock() somewhere else, but it can screw things up.

As a matter of fact I only have 3 LIFX bulbs here at home, so I am not in a good position to test out larger installations. @kaikreuzer Did you notice the behaviour @cbcoursera1 is describing in his initial post?

Karel.

kgoderis commented 8 years ago

s lock() in place, which means that only one ThingHandler at a time can write to the LAN.

Have to take this back after re-reading my own code. The lock() is private to the Handler, so it is not blocking other Handlers. In short, I have no clue why there is a delay between switching bulbs :-(

cbcoursera1 commented 8 years ago

I'm not able to use postCommand() at all, actually. I wonder why you are?

Here the the two rules I'm using

rule "Switch test 1"
when
    Item FF_CB_Bed_Lights_Sw received command
then
    sendCommand(LIFX_00_color, receivedCommand)
end

rule "Switch test 2"
when
    Item FF_CB_Bed_Lights_Sw1 received command
then
    postCommand(LIFX_00_color, receivedCommand)
end

The second first rule works fine, the second rule has no effect on the bulb and the Designer identifies it as being an undefined method.

EDIT: Just realized you deleted the post comparing speed of sendCommand() and postCommand()

kaikreuzer commented 8 years ago

@kaikreuzer Did you notice the behaviour @cbcoursera1 is describing in his initial post?

No, but I also didn't have the chance to test it yet. Anyhow, I also only have 3 LIFX bulbs, so my testing won't be any better than yours...

kgoderis commented 8 years ago

EDIT: Just realized you deleted the post comparing speed of sendCommand() and postCommand()

yes, I made a mistake in all my hurry. You only have sendCommand in the Rules.

When switching a bulb on/off, in fact, the binding is sending a series of LIFX protocol messages to the bulb. Yesterday I noticed that in fact, the binding sends all messages for a given bulb, before sending the messages of the next bulb, instead of interleaving the messages (e.g. first set the color on all bulbs, then secondly request a status update on all bulbs).

tavalin commented 8 years ago

How do groups work in LIFX? Does it have any native group mechanism for addressing multiple bulbs with a single request/packet?

cbcoursera1 commented 8 years ago

From my experimenting, sending a command to a group (including LIFX bulbs and Orvibo outlets) produced the same response as iterating through the items in the group and sending commands individually.

On Aug 31, 2016 10:11 AM, "Daniel Walters" notifications@github.com wrote:

How do groups work in LIFX? Does it have any native group mechanism for addressing multiple bulbs with a single request/packet?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse/smarthome/issues/2082#issuecomment-243776650, or mute the thread https://github.com/notifications/unsubscribe-auth/AK4tejqSF0gTucwZhNEklELLpzlON98Aks5qlYujgaJpZM4JwuzI .

tavalin commented 8 years ago

@cbcoursera1 that wasn't what I meant.

As an example, I have a couple of MiLights which are assigned as part of the same "group" on the MiLight hub. OH sees this as one Thing, and as such control of the "group" is done is done via one UDP packet to the MiLight hub rather than multiple packets. I was wondering if the LIFX API allowed native grouping such that it could be modeled as one Thing within the framework.

cbcoursera1 commented 8 years ago

Ah I see. Yes, it does, I'm not familiar with how it works but there's a discussion here:

https://community.lifx.com/t/lan-protocol-group-support/253

On Aug 31, 2016 12:59 PM, "Daniel Walters" notifications@github.com wrote:

@cbcoursera1 https://github.com/cbcoursera1 that wasn't what I meant.

As an example, I have a couple of MiLights which are assigned as part of the same "group" on the MiLight hub. OH sees this as one Thing, and as such control of the "group" is done is done via one UDP packet to the MiLight hub rather than multiple packets. I was wondering if the LIFX API allowed native grouping such that it could be modeled as one Thing within the framework.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse/smarthome/issues/2082#issuecomment-243820731, or mute the thread https://github.com/notifications/unsubscribe-auth/AK4tetnMYkcb_zznAr4DWBsDJij-Y8kzks5qlauJgaJpZM4JwuzI .

wborn commented 8 years ago

I have quite a few LIFX bulbs and also want things to be more speedy with the LIFX binding. After looking at the code to me it seems the main issue is that the ThingManager does not concurrently dispatch commands/updates to different Things.

ThingManager.receiveCommand uses the SafeMethodCaller. I wrote a simple test mimicking the SafeMethodCaller behavior. Most of the time it will just execute calls sequentially.

The SafeMethodCaller.callAsynchronous method name is a bit misleading. Usually there is no asynchronous call. It submits the task to the executor and immediately starts waiting for the result of the Future... unless it takes more than 5 seconds to complete. Only then there will be asynchronous behavior. A call that takes more than 5 seconds will not be cancelled. The thread that submitted the call to the executor will just not wait for the result and continue. The executor has a pool 3 threads by default. So you can just have a few calls running out-of-control.

LIFX bulb commands are not such calls because they take about 200ms or so. You can clearly see they are executed sequentially by the ThingManager when you enable debug logging on the ThingManager and add a new info log statement to the beginning and end of LifxLightHandler.handleCommand.

To preserve the order of commands and updates I think they should not run in parallel until they are received by ThingManager receiveCommand/receiveUpdate. Only there the ThingManager should asynchronously execute calls on different Things. Currently the order of commands and updates may already get messed up when for some reason their execution is asynchronous due to the behavior of the SafeMethodCaller.

Such a change may have a big impact on other bindings and peoples rules because it introduces more concurrency. This concurrency is actually already there (just sometimes) when calls take longer than 5 seconds.

The alternative to this would be that each binding has to implement it's own concurrency (if necessary). That could make bindings more complex and buggy.

Are these correct observations @kaikreuzer , @kgoderis or is there something I do not see?

kgoderis commented 8 years ago

@cbcoursera1 Note that the group concept in the LIFX protocol spec is different than the groups in openHAB. They are not necessarily the same, and I presume the LIFX groups match the groups as defined in the LIFX iOS app. AFAIK this is not something we can use, unless groups are equally defined in the iOS app and openHAB. This is a dangerous assumption.

kgoderis commented 8 years ago

@wborn I have also revisited the LIFX code, and the communication bit for each LIFX bulb stands separate from other bulbs, so the solution/problem lies outside of the binding, I think.

cbcoursera1 commented 8 years ago

I definitely didn't want to make any assumptions, just providing some information.

Thanks for having a look at how concurrent changing could be implemented.

On Sep 16, 2016 7:56 AM, "Karel Goderis" notifications@github.com wrote:

@cbcoursera1 https://github.com/cbcoursera1 Note that the group concept in the LIFX protocol spec is different than the groups in openHAB. They are not necessarily the same, and I presume the LIFX groups match the groups as defined in the LIFX iOS app. AFAIK this is not something we can use, unless groups are equally defined in the iOS app and openHAB. This is a dangerous assumption.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/eclipse/smarthome/issues/2082#issuecomment-247582624, or mute the thread https://github.com/notifications/unsubscribe-auth/AK4teiH40T5KbGNqWZp2uNdS4eCtyvD_ks5qqoPigaJpZM4JwuzI .

wborn commented 8 years ago

I implemented a proof of concept to create more concurrency in the command/update dispatching to Things. I've tested it with 6 LIFX bulbs in a room which now update near instantaneous instead of one by one. :-)

Besides LIFX I don't have other bindings with a lot of Things. So I can't test what the behavior with those are.

I've attached the proof of concept JAR which I tested with openHAB 2 beta 4. It creates 8 single thread executors and distributes all Things evenly across them. Commands/updates are then dispatched to the executor associated with the Thing. The order of events per Thing is thus maintained.

To test it:

  1. Download org.eclipse.smarthome.core.thing-0.9.0-SNAPSHOT.zip and rename extension to .jar (github does not like JARs)
  2. Start openHAB 2 beta 4
  3. Find the ID of 'Eclipse SmartHome Core Thing' with: bundle:list
  4. If it's 102, update it using something like bundle:update 102 file:///home/username/downloads/org.eclipse.smarthome.core.thing-0.9.0-SNAPSHOT.jar
  5. Restart openHAB, and test the more concurrent command/update dispatching
kgoderis commented 8 years ago

@wborn Can you share the URL of your repo/branch where the code is sitting?

I have the KNX binding with >1500 Things in it, so that might be a good stress test

kgoderis commented 8 years ago

@maggu2810 @sbussweiler @kaikreuzer Pls join the discussion on concurrency in the .core.thing

wborn commented 8 years ago

@kgoderis I just commited the code of the POC with c8d704f113120819c9108d780dc77ab44b539147.

It still uses the SafeMethodCaller which has its own threadpool that may become a bottleneck. That pool can be easily increased in runtime/etc/services.cfg.

kaikreuzer commented 8 years ago

Pls join the discussion on concurrency in the .core.thing

I would suggest to wait for https://github.com/eclipse/smarthome/pull/2087 to be merged as @sbussweiler did a lot of changes around that (and if I remember correctly, we also spoke about not having to wait for one handler to return before the next one is informed about an event). I still need to find the time to review and test #2087, but as you seem to be very interested in the Thing handler functionality, please feel free to test it yourself as well and provide feedback on it!

wborn commented 8 years ago

@kaikreuzer I just had a look at the ThingManager in that PR, but the behavior of receiveCommand and receiveUpdate are unchanged.

kaikreuzer commented 8 years ago

Hm, pity, I would have hope so... Nonetheless, I would suggest to first merge #2087 before creating further PRs for that class, in order to avoid merge conflicts.

maggu2810 commented 8 years ago

I have had a look at #2087 and added my review comments. After it has been merged let's have a look at this (and the other PRs waiting for #2087).

kaikreuzer commented 8 years ago

FYI, I have entered https://github.com/eclipse/smarthome/issues/2221 to deal with the ThingManager improvements specifically.

wborn commented 7 years ago

@cbcoursera1 PR #2366 should have significantly improved switching large groups of LIFX lights. Have you found some time to retest it? Sending packets to lights is now done using a thread pool which allows for more concurrency. Therefor I think this issue can be closed.

kaikreuzer commented 7 years ago

As there wasn't any further feedback, let's close this issue.