Skip to content

Fix crash when float is passed instead of int - #13

Merged
tfriedel merged 3 commits into
tfriedel:masterfrom
OleksandrBerchenko:master
Feb 4, 2019
Merged

tfriedel merged 3 commits into
tfriedel:masterfrom
OleksandrBerchenko:master

Conversation

@OleksandrBerchenko

Copy link
Copy Markdown

No description provided.

@OleksandrBerchenko

Copy link
Copy Markdown
Author

@tfriedel HA integration is ready. If you have time, it would be great if you could test it.

Just take https://github.com/OleksandrBerchenko/home-assistant/blob/osramlightify/homeassistant/components/light/osramlightify.py and put it instead of the original file (on my env it's located in /srv/homeassistant/lib/python3.5/site-packages/homeassistant/components/light/osramlightify.py). Additionally put the latest lightify/__init__.py instead of /srv/homeassistant/lib/python3.5/site-packages/lightify/__init__.py.

Thanks!

@OleksandrBerchenko

OleksandrBerchenko commented Feb 1, 2019 •

Copy link
Copy Markdown
Author

@tfriedel

tfriedel commented Feb 2, 2019 via email

Copy link
Copy Markdown
Owner

@OleksandrBerchenko

Copy link
Copy Markdown
Author

And my code essentially breaks their workaround :)

Well, I need to think how to fix/workaround that. Expect a new PR/update to osramlightify.py later today.

@OleksandrBerchenko

Copy link
Copy Markdown
Author

I have updated this PR, osramlightify.py and documentation (refresh the links above).

What I changed:

  1. The library now exposes light.raw_values() with all the raw values as obtained from gateway. So, even if we parsed them 100% incorrectly, a user may still access original values and implement all the logic on his/her side.

  2. HA module checks whether the device type is "SENSOR" and exposes raw_values as sensor_values attribute. For now I have decided not to expose raw_values for all devices to avoid unneeded noise.

My update still breaks the code from https://community.home-assistant.io/t/using-osram-lightify-motion-sensor-with-ha-triggered-by-change-in-rgb/55517, but now it can be easily fixed by changing attributes.rgb_color[1] (green value) to attributes.sensor_values[4]. I will post there, once my changes are merged.

Does it make sense?

Thanks!

@tfriedel
tfriedel merged commit fc8d56f into tfriedel:master Feb 4, 2019
@OleksandrBerchenko

Copy link
Copy Markdown
Author

Looking forward for your feedback about HA integration. Once everything is verified, could you please trigger a new release on Pypi?

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Oh, I see you have released it already. Thanks!

@tfriedel

tfriedel commented Feb 5, 2019

Copy link
Copy Markdown
Owner

I started testing yesterday. Basic functionality was working fine. I should still add this device type for the one Ikea lamp with id 128.
One thing I noticed, that with two RGB devices (an osram rgbw lamp and an osram lightstrip) blue was not working anymore. Which is probably a coincidence but still weird. So these devices are actually defective now. And for the lightstrip it was working during one of the tests recently. For the other lamp, it might have been defective longer and I just noticed it. If you go to Amazon and read the reviews for these things, there's quite a few people complaining about those lamps failing early. Bummer. Anyway this probably has nothing to do with the changes in the code I hope.
I will test it a bit more the next days.

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Sounds strange. So, blue doesn't work anymore even from the Lightify app, correct?

I don't have RGB bulbs right now (my only one stopped working several months ago) and was not able to do 100% accurate testing. Anyway, I don't think I changed anything related to set_rgb/build_colour functionality.

Thanks!

@tfriedel

tfriedel commented Feb 5, 2019 via email

Copy link
Copy Markdown
Owner

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Possibly, I'm not sure how many on/off cycles is expected for their lifetime.

By the way, you might have noticed that I slightly increased timeouts inside test_lightify.py. Without that my lights sometimes went into unreachable state for a few seconds.

@rbdubz3

rbdubz3 commented Feb 5, 2019

Copy link
Copy Markdown

Tested code from master on light strips and RGBW A19 bulbs since last night - all of them Lightify - I don't see any issues with Blue

@tfriedel

tfriedel commented Feb 5, 2019

Copy link
Copy Markdown
Owner

Ok got a chance to test it a bit more. So far everything works pretty smooth!
I noticed the colors of the icons in HA is now generally correct. Except with an RGBW lamp, there the displayed color is always the last RGB color. However the lamp can be in either of two modes: RGB or tunable white. Depending on what you changed last decides in which mode it is.
For example you set the color to green, then you change the temperature to warm. The color of the icon in HA is green, but the lamp is orange.

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Hm... What is type_id of that lamp? Does it change dynamically?

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Or is it a "usual" RGB lamp?

@tfriedel

tfriedel commented Feb 5, 2019

Copy link
Copy Markdown
Owner

type_id is 10. it's an RGBW. this is it:
https://www.amazon.de/Osram-Lightify-Reflektorlampe-Warmwei%C3%9F-tageslicht/dp/B00YSXBMIO
or in the official app it's called "PAR 16 50 RGBW - LIGHTIFY".

@tfriedel

tfriedel commented Feb 5, 2019 •

Copy link
Copy Markdown
Owner

I also wonder if we are dealing with the minimum and maximum temperature correctly. For example the ikea lamp (tunable white) has min. temp 2200. So unless I added a special configuration for it, it was wrong.
Would something bad happen if we set a lamp to a lower value than it is specified ? I would hope the lamp has these values as constants and would ignore values that are out of bounds. In this case maybe a good default would be lowest and highest value ever seen across all lamps?

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Ok, I start recollecting how my own RGB lamp worked... That makes sense: you can't change temperature of white in RGB mode.

Two questions:

  1. When you change the temperature and the lamp transforms from green into orange, the icon remains green because HA (as well as lightify library) still think it's in RGB mode and its color didn't change. But what happens after the next update from the gateway (in 5 seconds or what your scan_interval is)? Does it correctly update the colors to (255, 255, 255) and turn orange? In other words, does the next update from the gateway correct the colors?

  2. Are you aware about any RGB lamps without tunable white functionality?

Thanks!

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Regarding min/max temperature - need to think about it.

@rbdubz3

rbdubz3 commented Feb 5, 2019

Copy link
Copy Markdown

@OleksandrBerchenko - Sylvania Lightify has their Outdoor Garden Spots that are ONLY RGB .. No tunable white - actually bought some and realized after opening that they didn't support the white temperature.. returned them

@tfriedel

tfriedel commented Feb 5, 2019 via email

Copy link
Copy Markdown
Owner

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Ok, so here we have 3 separate issues:

  1. There are RGBW lamps (probably, a majority), which allow to switch to "tunable white" mode and simple RGB lamps, which don't. For simple RGB lamps we would need to disable changing color temperature (to remove temp from supported_features).

    The question is how to distinguish them? I believe it's possible only if RGB lamps have a different type ID (not 10 like RGBW lamps have). Do I understand correctly that none of you have a RGB lamp right now and can check its type ID?

    If my changes to HA are merged, original type ID will be exposed on UI (for unknown devices it will look like 11 (unknown device)) and it should be easier to ask people what device they have. If RGB and RGBW lamps share the same type ID, then we probably can't help here. If RGB lamps have a different type ID, then we will be able to fix that.

  2. RGBW lamps can switch between "color" and "tunable white" modes. The question is how to find what mode is currently used? It looks like the gateway always provides both RGB and temperature values. If we just changed a color or a temperature using the library, then we can probably assume which mode we have right now. But what if it was changed using the mobile app? Or we are initializing a light for the first time? Currently I don't see any way how to do that. Do I miss anything?

    Possibly our last chance is to check alpha value: it's not used at the moment, but it's exposed as the last element of raw_values list. @tfriedel could you please check whether it's being changed when you switch between "color" and "tunable white" modes? For my tunable white lamps and plugs it's always 255.

  3. (to be continued)

@OleksandrBerchenko

OleksandrBerchenko commented Feb 6, 2019 •

Copy link
Copy Markdown
Author
  1. Some 3rd party lamps (like made by Ikea) have a temperature range different from original Osram lamps (2700-6500 for TW and 1900-6500 for RGB).

    I see two issues/risks with setting the widest possible range for all lamps: 1. "Would something bad happen if we set a lamp to a lower value than it is specified?" - I have no idea :) Hopefully not, but I wouldn't introduce additional risks without a real need. Especially after your suspicions that extensive testing might kill your RGB lamps. 2. That would affect user experience with original Osram lamps. Imagine that on HA UI a user sees a temperature slider with current value somewhere in the middle. The user wants to set the coldest temperature and moves the slider to the left end. Nothing happens and in a moment the slider returns to its original position. Looks like a bug, isn't it?

    Maybe we should return to the idea of adding known 3rd party type IDs to the default list? In that case we will fix at least Ikea lamps (possibly the most common ones among 3rd party lamps). Our assumption that different vendors may use the same type IDs is not based on any precedents yet. Probably we can just add all known types (and wait for bug reports). As I mentioned, now it should be easier to report unknown type IDs.

    @tfriedel, what do you think?

Thanks!

@tfriedel

Copy link
Copy Markdown
Owner

@OleksandrBerchenko

  1. I don't have a simple RGB lamp. I wouldn't disable changing color temperature for them, because I can imagine that the lamp just approximates the orange / bluish colors with it's three R,G,B colours. Since we don't know this yet, we potentially would remove functionality. I also think if there is a temperature slider but it has no effect, it's not so bad. The osram lightify app doesn't allow changing the temperature of the ikea bulbs because it makes wrong assumptions and I would prefer if it didn't do this.
  2. The official lightify app recognizes the two states (temperature vs rgb) even if it is not used to control those parameters. I.e. I use HA to change color, then change color temperature and I observe in the lightify app that it updates the color in the UI accordingly.
    I don't know if there's a direct way to find out what state is in, temperatur or color. But while HA is running we can always keep the values from the last update and compare them with the current update. Then we look what changed, temperature or color and change the mode it's currently in depending on which of those variables changed. This will work almost always and even when the color/temp is changed for example with the official lightify app.
    I did look at the alpha value. No matter what I do, it always stays at 255.
  3. Re: Using type ids to detect temperature range. I'm wondering if not even bulbs from the same manufacturer with the same type id have different temperature ranges.
    Here is another implementation where the author gets the capabilities from the type id by using different bits as feature flags:
        $deviceRGB  = ($type & 8) ? true: false;
        $deviceCCT  = ($type & 2) ? true: false;
        $deviceCLR  = ($type & 4) ? true: false; 

CCT means it has color temperatur. Not sure what CLR means. However the ikea type id 128 doesn't fit this, which explains why the official lightify app can't control it's temperature.
I think we should add known type ids to the default list. For this ikea bulb I have I already added it. Unfortunately I don't have more bulbs / typeids together with temperature ranges, so this would be the only one for now.

@OleksandrBerchenko

Copy link
Copy Markdown
Author

@tfriedel

  1. Agree. And until we have some real type IDs that don't support changing color temperature for sure, there is nothing that we can do here.

  2. What if you don't update colors/temperature neither in HA nor in the app and just turn it on/off? Which state to choose when we initialize a light for the first time? Honestly, I don't see any good fix here.

    In theory, the app may get additional information from the cloud. Cloud communicates with gateway using different protocol that may contain additional bits of data. For example, I was not able to find where the app gets information about battery level from.

    Well... Could you turn on logging.DEBUG and check raw onoff value (before we convert it to boolean)? May be it has more possible values, not only 0/1?

  3. Wow, another Lightify library :) I need to take a look.

@OleksandrBerchenko

Copy link
Copy Markdown
Author

@tfriedel

  1. I can't find what exactly CLR means, but it refers to "fixed white" lamps. And those bitwise operations don't look reliable.

  2. The library is able to connect not only to a local gateway, but also to the cloud. But I don't see anything super useful that may be obtained there.

  3. The only interesting thing that I found is a new type id for RGB lamps without tunable white capabilities:

  const TYPE_LIGHT_COLOR        = 8;  //Fixed White and RGB
  const TYPE_LIGHT_EXT_COLOR    = 10; //Tuneable White and RGBW

  const LABEL_LIGHT_COLOR       = "On|Off Level Colour";
  const LABEL_LIGHT_EXT_COLOR   = "On|Off Level Colour Temperature";

So, probably we can add it to the library (that would address my p. 1 above). What do you think?

Thanks!

@OleksandrBerchenko

Copy link
Copy Markdown
Author

@tfriedel I have created a new PR for RGB/RGBW stuff: #17 . Let me know what you think.

P.S. By the way, that PHP library is not able to recognize temperature/RGB states too. I really doubt that we could leverage onoff, but let me know if you checked that.

Thanks!

@tfriedel

tfriedel commented Feb 16, 2019 via email

Copy link
Copy Markdown
Owner

@tfriedel

tfriedel commented Feb 16, 2019 via email

Copy link
Copy Markdown
Owner

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Agree. Thanks!

@OleksandrBerchenko

Copy link
Copy Markdown
Author

Ok, then we are probably done for now. Could you please create a new build on Pypi? 1.0.7.1

@tfriedel

tfriedel commented Feb 17, 2019 via email

Copy link
Copy Markdown
Owner

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants