tfriedel / python-lightify

Python module for Osram lightify. This is a work in progress.
Apache License 2.0
18 stars 9 forks source link

Handle unknown device types better. Fix crash in Python 2.7. #11

Closed OleksandrBerchenko closed 5 years ago

OleksandrBerchenko commented 5 years ago

Summary of changes:

  1. Log warning about unknown device type only once, not during each update (that was actually a bug).
  2. Refactor all the code related to handling device types. Still keep DeviceType class without changes for backward compatibility.
  3. If you are using the library directly, you can pass a dictionary of additional device types into Lightify class constructor like:
lightify = Lightify('192.168.1.46', new_device_types=
            {128: {
                'type': DeviceType.LIGHT,
                'subtype': DeviceSubType.LIGHT_TUNABLE_WHITE,
                'name': 'tradfri tunable white',
                'min_temp': 2700,
                'max_temp': 6500
             }
            }
)
  1. If device type is still unknown, treat it as either LIGHT_TUNABLE_WHITE or LIGHT_RGB, depending on actual (red, green, blue) values. As far as I see, for any non-RGB device Lightify gateway always reports (1, 0, 0). It's very unlikely that somebody may have such a configuration for a real RGB bulb. Anyway, we use that logic only for unknown device types.
  2. I have renamed "switch mini" to "3 button switch" like in Lightify application.
  3. Fix UnicodeDecodeError error in Python 2.7.
tfriedel commented 5 years ago

I ran the tests and everything works.

Regarding 4.: I first thought, comparing if the lamp reports as red to test if it's a tunable white lamp is problematic, but of course red is (255,0,0) and (1,0,0), i.e. a little bit of red is unlikely. But then again, (1,0,0) is not like a pixel on a screen, i.e. basically black. All these lights can't provide a very dim light. The lowest setting, i.e. 1 feels more like 20%. So I think it is maybe somewhat rare, but also not super-rare that someone sets his RGB light to the darkest red possible. Maybe it's better to always assume it's RGB if it's unknown, because if an app reports it as RGB, but the color settings have no effect, it's not as bad as if you have an RGB lamp, and you can only change the white.

OleksandrBerchenko commented 5 years ago

All these lights can't provide a very dim light. The lowest setting, i.e. 1 feels more like 20%.

Yeah, I know. Like 1% of brightness actually feels like 20-30%.

Maybe it's better to always assume it's RGB if it's unknown, because if an app reports it as RGB, but the color settings have no effect, it's not as bad as if you have an RGB lamp, and you can only change the white.

I considered that option. Actually that was the behavior before my latest PR (treat every unknown device as RGB). On the other hand:

I believe in practice we will have very few "false positives" if any. I would propose to leave the fix until the first bug report arrives. Does it make sense?

tfriedel commented 5 years ago

Yes makes sense.

Am Do., 24. Jan. 2019, 16:57 hat OleksandrBerchenko < notifications@github.com> geschrieben:

All these lights can't provide a very dim light. The lowest setting, i.e. 1 feels more like 20%.

Yeah, I know. Like 1% of brightness actually feels like 20-30%.

Maybe it's better to always assume it's RGB if it's unknown, because if an app reports it as RGB, but the color settings have no effect, it's not as bad as if you have an RGB lamp, and you can only change the white.

I considered that option. Actually that was the behavior before my latest PR (treat every unknown device as RGB). On the other hand:

  • The new logic is only applied to unknown devices ("normal" Osram RGB bulbs will be always treated as RGB bulbs). Also I suspect that most of bulbs in the world are actually not RGB :) And (1, 0, 0) is still an unlikely configuration.
  • That may be easily workarounded by setting a bulb to (2, 0, 0). No eye would catch the difference.
  • Without the fix, Home Assistant would display all "RGB" bulbs as being "red" (as it does now for all my Lightify bulbs) + redundant controls.

I believe in practice we will have very few "false positives" if any. I would propose to leave the fix until the first bug report arrives. Does it make sense?

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/tfriedel/python-lightify/pull/11#issuecomment-457249532, or mute the thread https://github.com/notifications/unsubscribe-auth/ABu6xtSUtXNSI2Ol2S7c959wXUef3eBwks5vGdgGgaJpZM4aPdkb .