Improve Tradfri binding #4299
Improve Tradfri binding #4299
Conversation
| @@ -226,7 +236,12 @@ private void handleBrightnessCommand(Command command) { | |||
|
|
|||
| private void handleColorTemperatureCommand(Command command) { | |||
| if (command instanceof PercentType) { | |||
| setColorTemperature((PercentType) command); | |||
| PercentType newValue = (PercentType) command; | |||
| if (!state.getColorTemperature().equals(newValue)) { | |||
There was a problem hiding this comment.
If we had sent some other Color command meanwhile, is it guaranteed that the state.getColorTemperature() is changed? What value does it e.g. take if I send a "green" to the bulb?
There was a problem hiding this comment.
The state is re-created on each message received from the bridge so - it should be up-to-date.
For changing color - I think this is not supported (yet) by the binding - only color temperature can be set
There was a problem hiding this comment.
@ivivanov-bg Color is supported now: #4271
There was a problem hiding this comment.
OK, Point taken, will check how it behaves tomorrow - but are you sure that the RGB light supports setting color temperature
|
Did you come across any real issue in this context? So far, we hardly do such checks in any of our bindings - it should always be valid to send out a command, even if we believe that it might not have any effect (but in the end it should be up to the device to decide on that). |
|
Yes - I did - if you send 2 commands (one for brightness and one for color temperature) in a short time (less than a second) - the second command is ignored by the bulb no matter if the first command actually change anything. So - this change will prevent flooding the bulb with unnecessary commands and will improve it's responsiveness |
|
Don't you assume with this change that the binding is the only one in the universe interacting with the bulb? How is this protected against race conditions with e.g. the native app? |
|
Messages are send by the bridge asynchronously no matter where does the change come from - if you change any value using the remote control, the IKEA app or the binding - the COAP message is always transmitted and the binding always receives it (and updates it's sate and items) |
|
But there is a time delay. So with your change the binding might swallow incorrectly a command. I don't think this is the right way to compensate for a limitation on hardware side... |
|
The binding is the 'driver' to the hardware - so if there are any hardware limitations - the binding is the place where this should be handled. eg. - if it's known that the device can not handle more than 1 command per second - the binding should not allow any user (application) to send more than 1 command each second. It should either ignore the request, or cache them in a queue, or whatever else but for sure the commands should not arrive to the device. (at least this is my point of view for the bindings) |
|
The LIFX Binding uses a throttling mechanism with locking/unlocking to make sure at most one message is sent to a light within a certain interval. This results in the command being queued instead of being ignored. |
|
Something like a queue sounds better.
|
|
In this case I think it would be even better to use the protocol in synchronious way when sending commands. |
|
Related problem: |
Exactly this I think should be handled in the binding and not in each application which uses it. Updated the PR to use a queue and send commands with a 200 milliseconds delay after each other. |
|
Just for discussion: |
I think this is a good idea - this way it will be per command. Currently each light handler has it's own coap handler - so it is already per light, but having it per command will be more flexible BTW - I tested also with the RGB bulb and didn't notice any issues with the 200ms delay. Both color and brightness were set properly |
|
I see, that's cool. And yes, today color+brightness setting works for me even with 200ms, but with freshly rebooted gateway. Nevertheless a dynamic delay would be great for further improvements, lets say, a configurable |
|
If I see this correctly, would that start a thread with a |
|
Yes - that's right. But the blocking queue will ensure that the thread is not running if there is no data to be processed |
|
It might be possible to have only one loop (in the bridge handler), but this will require quite big refactoring as each bulb should not directly send coap messages, but ask the bridge to do it instead |
|
Indeed. Sorry, should have read the code more carefully. |
| logger = LoggerFactory.getLogger(getClass()); | ||
|
|
||
| commandsQueue = new LinkedBlockingQueue<>(); | ||
| commandExecutorThread = new Thread(() -> { |
There was a problem hiding this comment.
Why do you create a new Thread on your own? According to our guidelines this is not allowed. Please use the scheduler and its submit method to have it run immediately.
Also if this is now the second binding after the Lifx one that implements such a throttle mechanism, whether it might be better to offer a general throttling implementation that can be used by both bindings?
There was a problem hiding this comment.
Why do you create a new Thread on your own?
Using scheduler is suitable for one time jobs or short actions (in a way fire and forget). The idea of this thread is to be a long running operation (start it in the initialization of the handler and stops it on dispose).
The scheduler will also introduce some limitations.
- you can't check the state (if it's running or not)
- You can't restart it (unless you submit a new job - which will have new instance of the command queue)
- You can't wait for it to finish
There was a problem hiding this comment.
Keeping a thread occupied with nothing for most of the time is not a good idea though. Especially not on constrained hardware.
How about something like this:
private final Queue<Command> queue = //...
private Future<?> job;
void sendCommand(Command cmd) {
queue.add(cmd);
if (job == null || job.isDone()) {
job = scheduler.submit(() -> {
while (!queue.isEmpty()) {
Command command = queue.poll();
// do send the command...
try {
Thread.sleep(200);
} catch (InterruptedException e) {
e.printStackTrace();
}
}
});
}
}
void destroy() {
if (job != null) {
job.cancel(true);
}
}
There was a problem hiding this comment.
Shouldn't we use a blocking queue instead of that thread.sleep usage?
There was a problem hiding this comment.
How about something like this
I guess it is also possible but the if statement: job == null || job.isDone() is not atomic which means that in some rare cases we might skip running the job and the command will not be sent.
Shouldn't we use a blocking queue instead of that thread.sleep usage?
The thread.sleep has nothing to do with the blocking, it's used to allow the device to process the command before sending a new one.
There was a problem hiding this comment.
Shouldn't we use a blocking queue
If you can make the queue block for at least 200ms...? I don't think this works though. But maybe I didn't fully get your idea...
but the if statement: job == null || job.isDone() is not atomic
Of course not, you are right! The above code was meant to illustrate the mechnism ("something like this"), not the final implementation. Otherwise I would have created a PR myself, right? Please add proper synchronization, logging and whatever else you see is necessary in order to make this robust and stable.
There was a problem hiding this comment.
@wborn since you mentioned that the Lifx uses a throttling mechanism and @kaikreuzer
mentioned that @kgoderis had to use such a mechanism for the Tesla binding because the web API locks you out if you fire too many requests at once, do you guys think that we should come up with a general solution and provide it in the framework? Do you think it would be possible to generalize all use cases that we currently have?
There was a problem hiding this comment.
Yes I've also been thinking about having a general solution in the framework.
The LIFX lights can be addressed via unicast and multicast (UDP broadcast). The LIFX throttling implementation makes sure that a lock is obtained on all lights or single lights. What I don't like about the implementation is that it uses Thread.sleep. But since the packet interval is just 50ms it this is not really an issue.
The Nest API also has rate limits which deny access to the API when you overload it with too many requests.
Perhaps the framework can provide some kind of mechanism where you can create groups (and possibly sub-groups) with objects (devices). These groups then have properties like time based limits and allow for operations (jobs) that take into account the defined group limits.
There was a problem hiding this comment.
The details of such a generic throttling mechanism can be a little tricky...
There are easy ones, e.g. the minimal delay which could be made configurable.
Then there is to distinguish whether the throttling should happen per bridge or per thing - still doable if it is completely agnostic to what it actually does, this would have to be handed over by Runnables.
Then you will have to distinguish whether whatever-the-throttles-is-instructed-to-do should really happen delayed (if a delay is required at all) or rather some of the executions should be skipped. While delaying might make sense in some cases (e.g. if I switch on a group of lights, I want every light to be on in the end, even if that's not possible at the same time) while sometimes it is rather not wanted (e.g. if I toggle a lamp a couple of times quickly, I might not want it to blink for the next minute but rather as soon as possible reach the final state). Unless I want to morse to my neighbors, then of course the blinking is intended 😉
Just skipping the congested executions also might not be a suitable solution, because maybe only some command equal out each other. Others might be important.
This all might still be doable with configurable implementations of different optimization strategies and callbacks for doing the "equalling out" calculations.
But all that seems a little overdone compared to the few lines of code that are similar in LIFX and Trådfri. I would recommend waiting a little, collecting other use-cases before suggesting to implement a generic solution. Especially since I really consider this a bug in the Trådfri gateway that ideally should be fixed there - don't get me wrong, I'm kind of okay with having the workaround here, but that's what it is. I would not want to establish this as "best practice" or anything.
I totally agree that this should be a temporary workaround until the FW of the bridge is fixed. Usually I also prefer the pool executors but in this case I think it will only introduce overhead as the thread will be occupate
I can't think of a way to do what you suggest (to get new thread of the pool when one command is send and when the second command arrives to reuse the same thread if it's not yet finished). That's because the state of the thread is not under control. Further more - there is chance that two (or more) threads are retrieved from the pool at the same time - then there is no way to ensure the required delay between two commands |
Signed-off-by: Ivaylo Ivanov <ivivanov.bg@gmail.com>
|
I think I figured it out how to synchronize the threads. Now the scheduler is used in kind of fire and forget way and if there is no command - there will be no blocking thread waiting for commands. (hopefully @SJKA will be happy with this implementation :)) |
Send command to the bulb only if there is actually a change on the value for brightness or color temperature
Signed-off-by: Ivaylo Ivanov ivivanov.bg@gmail.com