eclipse-archived / smarthome

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

[Hue Binding]: Change/Update the library underneath which actually talks to Hue API #2601

Closed ddudnik closed 7 years ago

ddudnik commented 7 years ago

Currently used library (https://github.com/Q42/Jue) is not maintained any longer since December of 2014 and will become more and more outdated with respect to Hue API. It is important to deal with this issue to be able to support all the newest and future features of Hue API (one example of such a feature is Sensor API).

After a short discussion with @kaikreuzer (https://github.com/openhab/openhab2-addons/issues/1260), here are some ideas to start with:

  1. Take over the Jue library and include the source as a part of Hue binding
  2. Find another library (some research is required, see https://github.com/Q42/hue-libs)
  3. Ask Philips to publish the source code of their SDK under a proper OSI-compliant license :)
  4. .....

P.S. When decision is made, I volunteer myself for the task.

ddudnik commented 7 years ago

Jue library is under MIT License, which I guess permits to take it right away and include into Hue Binding source code.

@kaikreuzer Please correct me if I'm wrong.

kaikreuzer commented 7 years ago

In theory you are right, but we very much avoid to mix licenses within the ESH source repository. Thus I prefer to have the code being contributed under EPL. I will go and check with the author whether this is an option for him.

kaikreuzer commented 7 years ago

Find another library (some research is required, see https://github.com/Q42/hue-libs)

All listed Java ones are dead since 2-4 years...

ddudnik commented 7 years ago

@kaikreuzer Yeah, I also did a brief review of other existing Java libraries (from the list + some from googling) and it seems all of them are not maintained any longer, mostly because Philips provides an official Java SDK now. Makes sense, I guess, since most developers would not required some other alternative than using an official SDK which would work just fine for an Android app etc.

I was also looking at Jue library more closely, it seem to be the most documented and cleanly designed of all, so makes sense to proceed with it if the author is OK with this option.

kaikreuzer commented 7 years ago

Good news: I reached the original author of the lib and he got me in contact with the company who is holding the copyright on it. They are ok with our plan to contribute the sources directly to ESH under EPL!

We should prepare a PR, which refactors the code accordingly and adds the sources instead of the jar. @ddudnik, did I get you right that you volunteered for that?

The sources that must be taken are in https://github.com/qivicon/Jue/commits/get-new-lights-with-serial-number (you see that we already added two commits on that in the past for fixes). We should agree on a Java package name, where to put in those classes. How about directly adding the classes in org.eclipse.smarthome.binding.hue.internal and org.eclipse.smarthome.binding.hue.internal.exceptions? Or shall we create a sub-package org.eclipse.smarthome.binding.hue.internal.jue?

maggu2810 commented 7 years ago

Wouldn't it make sense to keep it in a separated repository but as a subproject of Eclipse SmartHome? So other guys (non-ESH users) can consume the library, too.

kaikreuzer commented 7 years ago

No, I don't think it would be worth the effort. The original lib has only 9 forks (possibly 5 from us) and the interest in it seems pretty low - as @ddudnik pointed out, almost everyone else uses the official Philips SDK instead.

ddudnik commented 7 years ago

@kaikreuzer that's good news! Yes, I volunteered so I'll get to it soon. I guess I'll put the sources directly into org.eclipse.smarthome.binding.hue.internal as you suggested.

kaikreuzer commented 7 years ago

@ddudnik Any update from your end?

ddudnik commented 7 years ago

@kaikreuzer Yes, although I've started just last week. Spent some time setting up the environment and understanding the project structure. Just today I merged the Jue sources retaining the git history and will move forward preparing the PR.

kaikreuzer commented 7 years ago

Perfect then :-)

regnets commented 7 years ago

I am really looking forward to this! If you need any help, just message me.

ddudnik commented 7 years ago

Hi @kaikreuzer , maybe you could help me out a bit.

I god a weird exceptions in org.eclipse.smarthome.config.discovery.test during 'mvn clean install'. I found a similar issue here https://bugs.eclipse.org/bugs/show_bug.cgi?id=469040 but it seems to be caused by something else now. Or can it be related to the fact that I use JDK8 maybe?

Here are the stack traces: java.lang.NoClassDefFoundError: org/eclipse/smarthome/config/setup/test/inbox/InboxOSGITest$_assert_that_approve_adds_all_properties_of_DiscoveryResult_to_Thing_properties_if_no_ConfigDescriptionParameters_for_the_ThingType_are_available_closure43 at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:423) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:336) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:328) at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:160) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.eclipse.smarthome.config.setup.test.inbox.InboxOSGITest.assert that approve adds all properties of DiscoveryResult to Thing properties if no ConfigDescriptionParameters for the ThingType are available(InboxOSGITest.groovy:758)

java.lang.NoClassDefFoundError: org/eclipse/smarthome/config/setup/test/inbox/InboxOSGITest$_assert_that_approve_adds_properties_of_DiscoveryResult_which_are_ConfigDescriptionParameters_as_Thing_Configuration_properties_and_properties_which_are_no_ConfigDescriptionParameters_as_Thing_properties_closure44 at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:423) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:336) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:328) at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:160) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.eclipse.smarthome.config.setup.test.inbox.InboxOSGITest.assert that approve adds properties of DiscoveryResult which are ConfigDescriptionParameters as Thing Configuration properties and properties which are no ConfigDescriptionParameters as Thing properties(InboxOSGITest.groovy:808)

java.lang.NoClassDefFoundError: org/eclipse/smarthome/config/setup/test/discovery/DiscoveryServiceRegistryOSGITest$_assert_that_existing_DiscoveryResults_can_be_accessed_by_ExtendedDiscoveryService_implementations_closure43 at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:423) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:336) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:328) at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:160) at java.lang.ClassLoader.loadClass(ClassLoader.java:357) at org.eclipse.smarthome.config.setup.test.discovery.DiscoveryServiceRegistryOSGITest.assert that existing DiscoveryResults can be accessed by ExtendedDiscoveryService implementations(DiscoveryServiceRegistryOSGITest.groovy:431)

Thanks!

ddudnik commented 7 years ago

An update on the issue itself: most work is done. I'll try to test the binding on my home OH2 instance over weekend.

ddudnik commented 7 years ago

@kaikreuzer to make the unit test issue more related to this ticket: I also got the failure when running test for hue, single test fails with:

java.lang.NoClassDefFoundError: org/eclipse/smarthome/binding/hue/test/HueBridgeHandlerOSGiTest$_assert_that_a_status_configuration_message_for_missing_bridge_IP_is_properlyreturned(IP_is_an_empty_string)_closure9 at org.eclipse.osgi.internal.loader.BundleLoader.findClassInternal(BundleLoader.java:423) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:336) at org.eclipse.osgi.internal.loader.BundleLoader.findClass(BundleLoader.java:328) at org.eclipse.osgi.internal.loader.ModuleClassLoader.loadClass(ModuleClassLoader.java:160) at java.lang.ClassLoader.loadClass(ClassLoader.java:358) at org.eclipse.smarthome.binding.hue.test.HueBridgeHandlerOSGiTest.assert that a status configuration message for missing bridge IP is properly returned (IP is an empty string)(HueBridgeHandlerOSGiTest.groovy:222)

Which does not seem to be related to the code changes to me. I also tried running it using Java 7 but no luck.

ddudnik commented 7 years ago

An update: I ran some tests on my local OH2 instance with new Hue bundle, seem to be alright so I'll submit the PR in the next days.

@regnets since you mentioned that you could help with testing, I'm attaching the newly assembled Hue binding jar (please change the extension from zip to jar, GitHub won't allow attaching jar files directly) which you can install manually on your OH2 server. Basically what needs to be checked here is that we do not introduce any regressions and everything works the same way as before. Any feedback will be appreciated! org.eclipse.smarthome.binding.hue-0.9.0-SNAPSHOT.zip

ddudnik commented 7 years ago

@kaikreuzer

An update regarding weird NoClassDefFound errors. After trying different things and some googling I managed to find the root cause of that and it appears to be the Linux encrypted filesystem (which I'm using). Default limit for filename on linux is 255 characters, which is fine. However, when using encrypted FS, this limits becomes lower - 143 characters. The rest is used when encrypting actual filenames.

So just by lowering the length of the test cases names in groovy files I immediately got rid of the aforementioned error and I also checked that the name of the class which was not found was indeed bigger than 143 characters long originally.

I wanted to create an issue about this but then I thought that technically it's not related to smarthome. Although it will create difficulties to any developer using Linux with encrypted drive. The workaround which I intend to try is installing smarthome workspace onto external unencrypted drive.

maggu2810 commented 7 years ago

@ddudnik Please correct me if I am wrong, but this seems to be a limitation of ecryptfs only and not on encrypted filesystems on Linux in general. I am using an encrypted file system for years (on Linux) and never run into such a problem.

ddudnik commented 7 years ago

@maggu2810 that's right, to be precise it is a limitation of eCryptfs when filename encryption is enabled (on by default I think) which is probably used in most Ubuntu installations. I was too soon to generalize :)

Here's a corresponding issue for reference: https://bugs.launchpad.net/ecryptfs/+bug/344878

regnets commented 7 years ago

So far i didn't experienced any issues with your test binding. However i just got 18 Actors and for the most part i am using basic functions (On/Off, Dimming).

I think the sensor api ins't implemented at this time?

ddudnik commented 7 years ago

Hi @regnets

Thank you for providing the feedback! Sensor API is not there yet, it will be a part of #2603.

josor001 commented 7 years ago

Hi folks, just purchased a Hue motion sensor and thought about integrating it in our smart home lab at dortmund's applied sciences university. @ddudnik Is there any progress regarding the Hue API update (especially the Sensor API)? Do you need any help? I may find some student assistant who 'volunteers' ;)

ddudnik commented 7 years ago

Hi @josor001 , Integrating Sensor API is tracked in #2603 So far there was no progress and I will not have time to work on it in the near future. So feel free to "volunteer" some students to tackle it ;)

ddudnik commented 7 years ago

Closing this one to avoid confusion, see related issue #2603