jinnatar / python-cozify

Unofficial Cozify Python bindings and helpers
MIT License
16 stars 4 forks source link

Ambient temperature and color temperature both reported under the same key #15

Open japsu opened 4 years ago

japsu commented 4 years ago

So today I installed some IKEA Trådfri bulbs and immediately my alerts tell me my house is on fire. Like, 4000 degrees on fire. 🔥🚒

Turns out device["state"]["temperature"] can be either ambient temperature (degrees Celsius) or color temperature (degrees Kelvin), depending on the device. They can be distinguished by having either "COLOR_TEMP" or "TEMPERATURE" in device["capabilities"]["values"] (but can there be a device that has both?).

Not sure if this should be fixed in the Cozify API instead of this library, but I bet this is something that affects many users of this library, and having the heuristic in the library would save us from replicating this heuristic everywhere that needs to distinguish ambient temperature from color temperature.

jinnatar commented 4 years ago

Interesting issue! I have the same lights and have indeed not run into it due to the intent filtering. There is an option to do AND filtering which allows you to filter for a set to make it triple sure. I don't believe the two values would ever coexist but I'll have a think of there's an opinionated way to make it less of a pitfall. Ideas are of course welcome.

On Sat, Jul 25, 2020, 17:45 Santtu Pajukanta notifications@github.com wrote:

So today I installed some IKEA Trådfri bulbs and immediately my alerts tell me my house is on fire. Like, 4000 degrees on fire. 🔥🚒

Turns out device["state"]["temperature"] can be either ambient temperature (degrees Celsius) or color temperature (degrees Kelvin), depending on the device. They can be distinguished by having either "COLOR_TEMP" or "TEMPERATURE" in device["capabilities"]["values"] (but can there be a device that has both?).

Not sure if this should be fixed in the Cozify API instead of this library, but I bet this is something that affects many users of this library, and having the heuristic in the library would save us from replicating this heuristic everywhere that needs to distinguish ambient temperature from color temperature.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/Artanicus/python-cozify/issues/15, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAM27KK23NGK7SXWZR23VXDR5LVX3ANCNFSM4PHPVLZQ .

japsu commented 4 years ago

I'm exporting all available values to Prometheus using an exporter I wrote. So in this case it's not viable to ask for only devices that sport capabilities.TEMPERATURE.

I ended up doing this:

            # HACK: Both temperature and color temperature are reported under same key, bad
            capabilities = device.get("capabilities", {}).get("values", [])
            if (
                key == "cozify_temperature" and
                "COLOR_TEMP" in capabilities and
                "TEMPERATURE" not in capabilities
            ):
                key = "cozify_color_temperature"

Here's the whole setup for reference: japsu/kotiscale

jinnatar commented 3 years ago

Looking back at this, I think the tsdb way of dealing with the issue would be to export not only the datapoint but also key variables as tags (edit: i.e. also export the capabilities of the devices). That way you have the full dataset on the Prometheus side and can make sane choices on that side.

But, with that said, it's still annoying that they share a key so I'll keep thinking what can be done about it. Will at least add some documentation.