dresden-elektronik / deconz-rest-plugin-v2

deCONZ REST-API version 2 development repository.
BSD 3-Clause "New" or "Revised" License
17 stars 0 forks source link

Device temperature and config/temperature #19

Open manup opened 6 months ago

manup commented 6 months ago

Historically we butcher the config/temperature to expose the temperature which is measured on the device itself. The value can be very coarse and different from the actual ambient temperature.

The main problem is that users mistake it as ambient temperature. And here UIs like the Phoscon App is mostly guilty to present both values just as "Temperature" ... my fault, but fixable.

Related: https://github.com/dresden-elektronik/deconz-rest-plugin/pull/7650#issuecomment-2028145777

I'd love to remove it all together but that would cause many angry responses.

Proposal:


As API change we could introduce state/device_temperature to make it more clear and deprecate config/temperature. This could be done at C++ level to be consistent. Therefore exposing both items for the time being.

So the open question is do we want that as cleanup step of the API? The main problem "user confusion" is merely a problem which is fixed in the Phoscon App and other UIs but not really a requirement to change the API, since there are already two distinct values.

The main benefit I see for a API change is the proper location in state/ instead of config/, with a deprecation phase this can also be done cleanly without having a breaking change.

Thoughts?

ebaauw commented 6 months ago
  • First easy step in the Phoscon App should be to label the shown value just "Device temperature" not just "Temperature" to prevent confusion.

Yes! That should indeed eliminate most confusion.

  • Other clients may chose to do the same and not expose config/temperature and state/temperature as the same things.

Agreed. Homebridge deCONZ and, previously, Homebridge Hue already ignore config/temperature.

As API change we could introduce state/device_temperature to make it more clear and deprecate config/temperature.

Not opposed, but I'll likely continue to ignore the device temperature. However, state/device_temperature should probably be exposed in a separate sensors resource or subdevice (cf. config/battery vs ZHABattery's state/battery), rather than on each existing resource.

SwoopX commented 6 months ago

First of all, it is worth to note that this primarily (or better to say only?) applies to legacy Xiaomi devices and that it is a self inflicted fallacy or assumption a particular value of the special reporting represents either the ambient or device temperature. Could be right, could be wrong, however, Xiaomi does not use any such thing in their ecosystem, nor is it available.

Having said that, I'm all in favor ditching that item. It is also a good idea to at least be prepared for making device temperatures available via state/device_temperature. Imho, I wouldn't expose such values by default as this tempts people do do strange things with it 🤷‍♂️ So, absolutely with Erik here regarding the tendency to ignore it from the client side. Although I can somehow follow the thought about a seperate subdevice or sensor to have stuff cleanly seperated, I'm really not a big fan of that. Too much of an overhead for my personal taste.

Anyway, we should clearly annouce deprecation of items, better sooner than later. Not sure about the best way, but I could imagine setting a fixed date for an item to be kicked out and let that be constantly a part of the release notes. But also be straight on the removal then. Just my 2 ct.

Kane610 commented 6 months ago

I use it in Home Assistant, some people wanted the internal temperature reported, make sure to communicate the change in good time if you move it to state. I would rather see it removed than moved to its own sensor as its value is quite low.

manup commented 6 months ago

Well we can all vote in favor to deprecate and ditch entirely after period x :) I'd also favor this over adding a whole new sub sensor for a mostly useless value.

DDF items can already be marked as deprecated in JSON, with the start date as deprecated, this is also picket up and shown in the auto docs generator.

{
  "name": "config/temperature",
  "deprecated": "2023-09-17"
}

A small script can be added that reminds us later on automatically if a period x has expired on those, e.g. after a year or so.