StijnGroenen / ikea-tradfri-api

A Java library for communicating with IKEA TRÅDFRI devices
Apache License 2.0
28 stars 5 forks source link

Ability to tell what kind of even was triggered #1

Open posicat opened 4 years ago

posicat commented 4 years ago

Wonderful code, I was able to import it and talk to my gateway in minutes! Now I'm working on processing events for the interface, looking at monitoring all my vast (3 so far, I just got the starter kit) array of devices.

It looks like if I register a DeviceEvent it will happily capture the event when it happens, and pass it back to my code at which point I can process the event easily enough. However I can't quite tell WHAT happened when I interrogate the event. Looking at how your DeviceObserver processes the requests it builds an ArrayList of events that it captures and then looks through them for callback events and then calls through to my handle method. I would expect to see in DeviceEvent a list of sub-events that were triggered (aka the changeEvents variable) as a property that I could then read to see what happened, but this data never gets written to that event. I'm probably being lazy and not creating a branch/PR and adding the code myself, but then again I don't want to be "that guy" who submits code changes 3 hours after he looks at it for the first time either.

So, instead, what do you think about DeviceEvent returning at minimal the properties that have changed because of the event. It looks like just moving the oldProperties and newProperties outside of the IF block would allow for a new event to be created at the bottom just before the loop processing all the events a DeviceEvent could be created with the old and new properties that would reflect the change that occurred with the device on that event.

Also it looks like if you reverse the order you're placing the events in the array, starting with the LightChangeOnEvent (and similar) level events, then the LightChangeEvent, LightEvent and finally DeviceEvent you would be able to use Collections. reverse() to send through the most specific match like you are already doing, but build the less specific events at the end after all status data has been collected.

If this sounds good I'd be happy to fire up a branch on my machine do a bit of refactoring on the DeviceObserver and get those changes put together. I think that those changes would make things a bit more robust for future use.

It would also be nice to have a callback handle that could capture ALL device events, rather than having to register one event for each device. For 5-10 devices that's just fine, but the overhead gets larger (ok, not that large!) as the number of devices goes up.

posicat commented 4 years ago

I was able to get some change event data off of the LightChangeEvent using reflection, and comparing the values in old and new properties, I'll have to do similar to get events from plugs, remotes, etc. I think this could be easier to access so that people using this code can just receive an event and know right away what parameters changed.

Here's the code I'm using to bridge it into my automation system : https://github.com/posicat/CattechHomeAutomation/blob/develop/HomeAutomationModules/TradfriInterfaceHandler/src/main/java/org/cattech/HomeAutomation/TradfriInterfaceHandler/TradfriInterfaceHandler.java

posicat commented 4 years ago

I can successfully read and write to the device, I have to use a bit of magic via reflection to get to the properties variables to set them the way I'm hoping to be able to eventually, directly to and from a JSON string. Your code is awesome, thank you so much for the time and effort you put into it!

StijnGroenen commented 4 years ago

I like your idea of adding the specific events to the DeviceEvent (and other less specific events like the LightEvent and the LightChangeEvent) as a list. At the moment it is already possible to listen for a less specific event (like DeviceEvent) and then figure out what the corresponding specific event was (by using instanceof or getClass to check if the DeviceEvent is actually a more specific event since all the events inherit from the DeviceEvent and the DeviceObserver also calls the EventHandlers of parent events). This wouldn't work for multiple changes though because each EventHandler is only called once for every change of the device.    

It would also be nice to have a callback handle that could capture ALL device events, rather than having to register one event for each device. For 5-10 devices that's just fine, but the overhead gets larger (ok, not that large!) as the number of devices goes up.

Unfortunately the enableObserve method needs to be called for every single device since an observe request needs to be send for every device that needs a listener. However it is possible to reuse a single EventHandler for multiple devices by storing it in a variable.    

EventHandler<LightChangeEvent> lightEventHandler = new EventHandler<LightChangeEvent>() {
    @Override public void handle(LightChangeEvent event) {handleDeviceEvent(event);}
};
EventHandler<PlugChangeEvent> plugEventHandler = new EventHandler<PlugChangeEvent>() {
    @Override public void handle(PlugChangeEvent event) {handleDeviceEvent(event);}
};
EventHandler<RemoteEvent> remoteEventHandler = new EventHandler<RemoteEvent>() {
    @Override public void handle(RemoteEvent event) {handleDeviceEvent(event);}
};
EventHandler<MotionSensorEvent> motionEventHandler = new EventHandler<MotionSensorEvent>() {
    @Override public void handle(MotionSensorEvent event) {handleDeviceEvent(event);}
};
device.enableObserve(); // This is necessary for the event handler to work.
device.addEventHandler(lightEventHandler);
device.addEventHandler(plugEventHandler);
device.addEventHandler(remoteEventHandler);
device.addEventHandler(motionEventHandler);

In your code you added an event handler for all of the device types. These event handlers all call the same method in which you check the class type of the event and act accordingly. Because you check the event type in the handleDeviceEvent method, it is also possible to replace these four event handlers with one event handler that listens for a DeviceEvent (which is a parent of all the events you used).    

Wonderful code, I was able to import it and talk to my gateway in minutes!

Your code is awesome, thank you so much for the time and effort you put into it!

Thank you very much!

posicat commented 4 years ago

Oh yeah, that makes sense when I'm reading that and fully awake, at 2am in the morning, that just made it work.

The code I've got now is working really well with your library, but I need to get to some of the properties via reflection. I'll see if I can pull do your code and add a few helper methods to eliminate the need for the reflection, and we can talk up any of the changes to the code so that they fit your design. The main thing that would be helpful from what I've found is the ability to serialize and deserialize the properties objects as JSON, and allow them to be sent directly into the applyUpdates, right now the applyUpdates that takes properties is private, thus the reflection.