-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[HueEmulation] Support Coloured bulbs & other improvements #4216
Conversation
7946bec
to
f961c23
Compare
f961c23
to
357955b
Compare
...penhab.io.hueemulation/OSGI-INF/org.openhab.io.hueemulation.internal.HueEmulationServlet.xml
Outdated
Show resolved
Hide resolved
d3a1957
to
b7f85bd
Compare
* Use OSGI annotations * Use config holder class * Move code into own subclasses instead of having a giant HueEmulationServlet.class * Introduce UDN class * Introduce user management class * Introduce light items management class * Watch the item registry for changes and keep a cache of published items, instead of quering all items from the item registry for each API call. * Implement /config endpoint. Necessary for Hue App etc. * Implement /whitelist entpoint including user delete. * Implement dummy entpoints for nowadays Hue bridges (/group, /scenes etc). * Fix: Return unauthorized /config * Fix: error/success response * Fix: Only allow POST http method for adding a user * Start service late in the process via the ReadyMarkerService * Automatically turn off the pairing mode after a time * Support "extended color light bulbs" with a hue range 0-65535 * Support wall plugs Signed-off-by: David Graeff <david.graeff@web.de>
b7f85bd
to
1796fd5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @davidgraeff, just a little cosmetic left.
addons/io/org.openhab.io.hueemulation/ESH-INF/config/config.xml
Outdated
Show resolved
Hide resolved
Signed-off-by: David Gräff <david.graeff@web.de>
@kaikreuzer Ready for another review round |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/items-with-switchable-tag-are-not-discovered/58250/1 |
#3171 is not fixed by this. Setting to 1 still results in an effective off. And a hearty thanks for this work HSBtype support ---woot! |
Details in #4293 and in the forum post... just noting it here to bring attention to the issues before the release. IMO, removing the ability to use groups needs to be resolved or communicated as a breaking change. There have been several reports of the inability to discover Items tagged as Switchable. I suggest these get remedied before 2.4 is released. |
This is not a regression, as the hue emulation never supported groups before. @openhab-5iver Could you please open an issue for group support :) Regarding switchables. I'm having a look at this right. My plan is to pair an osram plug to an original hue bridge and looking at the rest api response to spot any differences in how switchables are exposed. Before my changes everything was exposed as a light. I don't thing this is the way to go for switchables. If certain Alexa firmware versions or devices (personally I do have dots and echos with the German firmware) are not able to recognise Osram plugs, there is not much that can be done. |
Ok this is the response of the hue bridge for a paired osram plug that works with my local Alexa setup with 1 x 2gen dot and 2 x 2gen Echoes:
The hue bridge additionally sends |
You are basically saying that I have not performed a test run before publishing. That is harsh criticism, I'm sorry but there's no other way to interpret that. And you did it again in your last post. Please just accept the fact that hue essentials works on my phone in my local network and all my Alexa's are happily talking to my openHAB via the hue emulation. And yes parsing of this specific username message is performed by string comparisons in a few hue rest implementations, and that's why it slipped my eyes here. And we totally disagree about coding style apparently. I like a well commented and extendable DTO class, were fields can be updated if required, that is converted to a JSON on demand and you prefer string building which of course also works. But that doesn't reduce maintenance complexity. Have you seen the config class with a dozen fields? That will not look pretty with your string builder pattern. I'm lacking time right now, but I will fix the most crucial bugs like the wrong response. |
I am saying that it looks like a not well tested PR, I never said you didn't do a test run. I have tested this on 3 independent devices, all of them didn't work. There's no way to say that it's been well tested as then the official app and the hue essentials app would work. But if you did indeed a complete test run on a fresh install and this doesn't work on any of the apps right now on my side your or my test scenario seems to be a little odd or the code was changed somewhere along the way. It might work right now for you but if you have to re-setup it you will most likely not be able to, at least not with the hue essentials app (if you are that's really interesting and I'll definitely do further debugging to figure out how that json parser is able to accept something like that and find the correct data). I don't know what the echo devices are doing in this case, I would have to look that up but if I remember correctly we have a proper parser in there that'll fail in this case aswell. I am also not preferring string building but json arrays/objects that get arrays/objects "put'ed" into them and in the end a toString call if it's necessary. To be honest I have no clue right now how i would fix the broken response in your code, I'm definitely looking forward to the new PR to see how that's done. Also I've written in one of the issues that Philips devices (like my TV) don't care about the path advertised for the description.xml file, so they're requesting /description.xml everytime instead of /api/description.xml . It would be great if you could do that small change aswell, I have already looked into that and I could do it aswell but as I said I would prefer to not mess with your code until it's working properly as then neither you nor I know what's going on in there which makes it more difficult to find issues. Thanks for taking time to fix this, I'm looking forward to this! |
There is one way to catch those errors. A test bundle. If you have some time on your hands, but don't want to fiddle with the emulation code right now, you could help a lot by setting up a test bundle :) I will tackle item group support (in a hacky way for this release only) and this wrong response. I still don't know why switchable itema are not working for some people. That would need to be sorted as well before a release. |
|
@Flole998 This is not possible afaik. The hue emulation registers a java servlet on /api and can only serve content beneath this path. If you know a framework api call to register a single file on the root topic, please propose a PR with that change. |
I have made a fix for the wrong response in #4311 |
If it helps, Hue Emulation no longer discovers devices on my network either. I have tried using both Switchable and Lighting (I had always used Lighting previously) in my otherwise unchanged insteon configuration. This is a version 1 binding, so it only uses flat files. I cleared out all of my devices from the Alexa app, have tried setting the binding to try the discovery fix and without. I also changed the timeout to 5 minutes, though this was only for my sanity of not going back into the binding configuration to re-enable it after every test. Using wireshark I am seeing the request to GET /api/description.xml and a 200 is returned, this all being after the ssdp traffic of course. The data returned from the GET is fairly sparse. It contains only information about the emulated hub and a couple icon entries. The very next request looks like this: If the pairing button has been pushed (the binding is in pairing mode), it still gets that error, but then goes on and posts the devicetype "Echo". This returns a 200 with the username, a UUID. It then tries a GET like the previous one only with a new key I am running the latest SNAPSHOT, build 1448, and have about 16 devices that worked previously. The bridge is returning |
Thank you for your report. After one of the last commits the emulation stopped working for me as well. I'm on this right now. |
Updated to M8 today and what should I say: Amazing...... The description.xml made it to /description.xml, so my TV found the OpenHAB Hue Bridge, the lights got detected correctly and I am able to use them as an Ambilight. Unfortunately, there's one small issue: The color does not work, as the xy stuff is not implemented (see https://gist.github.com/popcorn245/30afa0f98eea1c2fd34d for some explanation on how that works) yet, I am looking into this right now to see how this could be done easily (and hopefully that still makes it into the 2.4.0 release then). That's the only thing that I'm missing right now, other than that this is absolutely great!! Thanks for all the work, I really appreciate it!! |
@davidgraeff If you have a second, could you please check my implementation at Flole998/openhab2-addons@b3ea45a and Flole998/openhab2-addons@6f06291 and let me know if that's how it is intended (and maybe also check if it works for you), and if it is feel free to pull it over so we can get this merged as quick as possible :) A valid request looks like this
|
Actually my changes are dumb/unnecessary in some parts, we already have fromXY(x, y) in HSB type, so all that is really necessary is "return HSBType().fromXY(xy[0], xy[1]);" and not copying all that stuff over.... That basically means: Disregard the second commit for now, I'll fix that one. EDIT: Fixed and Updated the previous comment |
Those commits are awesome. I'm not sure if Kai will accept feature commits at this point in time anymore. The release notes have been written already. I'll try anyway. |
Someone just brought to my attention that things are not quite right yet:
|
Could you elaborate on the first list item? |
I've taken some time to work on the slightly inaccurate colors today (aswell as my first xy_inc implementation was wrong), my repo has that fixed now. Technically all this is not "new", but fixed from your incomplete implementation now. The changelog doesn't need to get rewritten, and I'd say even an "untested" implementation is better than none. The first Problem is specific to my implementation (sorry if that wasn't clear): In your Response Handler you have several responses for different instances, the Problem with xy is, that it is a list and there is no case for that (and it is very difficult to implement). I haven't found a way for that yet. (And maybe someone wants to look at my calculations and simplify that a little, I'm sure there's room for optimization....) |
Can you make a PR out of your changes? If you include a test case for xy for either the RestAPI unit test or the OSGI full implementation test, we can get this in super fast. |
PR created, I also made a testcase based on one of yours, I am not checking the results of my color conversion though, hope this is okay like this. |
This pull request has been mentioned on openHAB Community. There might be relevant details there: |
* HueEmulation IO binding: Support Coloured bulbs * Use OSGI annotations * Use config holder class * Move code into own subclasses instead of having a giant HueEmulationServlet.class * Introduce UDN class * Introduce user management class * Introduce light items management class * Watch the item registry for changes and keep a cache of published items, instead of quering all items from the item registry for each API call. * Implement /config endpoint. Necessary for Hue App etc. * Implement /whitelist entpoint including user delete. * Implement dummy entpoints for nowadays Hue bridges (/group, /scenes etc). * Fix: Return unauthorized /config * Fix: error/success response * Fix: Only allow POST http method for adding a user * Start service late in the process via the ReadyMarkerService * Automatically turn off the pairing mode after a time * Support "extended color light bulbs" with a hue range 0-65535 * Support wall plugs Signed-off-by: David Graeff <david.graeff@web.de> Signed-off-by: Pascal Larin <plarin@gmail.com>
* HueEmulation IO binding: Support Coloured bulbs * Use OSGI annotations * Use config holder class * Move code into own subclasses instead of having a giant HueEmulationServlet.class * Introduce UDN class * Introduce user management class * Introduce light items management class * Watch the item registry for changes and keep a cache of published items, instead of quering all items from the item registry for each API call. * Implement /config endpoint. Necessary for Hue App etc. * Implement /whitelist entpoint including user delete. * Implement dummy entpoints for nowadays Hue bridges (/group, /scenes etc). * Fix: Return unauthorized /config * Fix: error/success response * Fix: Only allow POST http method for adding a user * Start service late in the process via the ReadyMarkerService * Automatically turn off the pairing mode after a time * Support "extended color light bulbs" with a hue range 0-65535 * Support wall plugs Signed-off-by: David Graeff <david.graeff@web.de> Signed-off-by: Maximilian Hess <mail@ne0h.de>
instead of quering all items from the item registry for each API call.
Signed-off-by: David Graeff david.graeff@web.de
Fixes #3753
Fixes #3754
Fixes #3118
Fixes #2858
Fixes #2005
Fixes #1317
Fixes #1311
Fixes #1257
Fixes #2881
Fixes #2936
Fixes #3171