Further sensor API support #4
Conversation
|
Sensors would be a great addition, but I think there are some problems with the implementation you are proposing. First, subclasses are a great idea and will help to make it easy to implement the sensors. However, overriding the 'state' attribute seems like a bad idea to me, because some sensors have more than one relevant 'state' attribute. I think the state attribute shouldn't be changed and should just return the raw state values of the Hue API, so all state values are always available. The idea of adding more properties to specific sensor classes to parse 'raw' values is great and will make the implementation of the sensors easier. Second, you can't make generic classes for both CLIP and ZLL sensors, because the available config options and states are different. For example, instead of a generic 'HuePresenceSensor' you'll need 'ZLLPresenceSensor' and 'CLIPPresenceSensor' classes. The ZLL Presence sensors have config options for sensitivity, whereas the CLIP Presence sensors don't have those options. I've created a pull request (marthoc/aiohue#1) that changes the existing classes, adds support for presence and light level sensors, and implements 'set_config' methods. Some sensor types are not implemented yet. |
|
Thanks @raymondelooff for this. I’ll let @balloob weigh in with his thoughts before I do anything with your PR. I’m not sure I agree with you re the state property - the idea would be that any relevant state items for each sensor class get exposed as property methods themselves, so they will in fact also always be available. Same goes for config variables like battery and reachable. I’ll have to think more about the differences between ZLL and CLIP sensors and how they should be treated in the API before offering a response to your other point. |
|
One design rule for aiohue is that we should have properties named after the values in the raw data which should provide the data as-is. If we are going to interpret the data, we should name the properties differently. That way, people will be able to understand most of aiohue by looking at the Hue docs. |
|
OK so @raymondelooff you are right re "state", that should stay unchanged. If we expose additional properties that mirror the state and config values in name, we can do the conversions there. @raymondelooff I will merge your PR and we can go forward from there. Thanks! |
|
@raymondelooff - should be expose all values from state as properties on the subclasses? I think if we are doing it for e.g. buttonevent, we should do it for lastupdated too (for example on the ZLL switch). |
|
Thanks for merging! Some general properties like I'm not completely sure if it would help to add value formatting in this library. Adding extra methods that almost have the same name as the property could make it very confusing. Maybe it's better to do no conversion in this library, and move all the conversion to Home Assistant. Currently, the conversion of the hue and saturation of the lights is also done by Home Assistant before it's passed to the API via this library. |
|
Yeah, let's not do conversion in this lib. We should keep this lib as simple as possible to empower people to do as much as possible. |
|
@balloob @raymondelooff I've just had a chance to turn my mind back to this PR and have pushed another commit that removes the conversions and units of measure (uom's are not needed if we are just returning the raw, unconverted value, as was previously discussed). @balloob I've left the "conversions" in for the ZGP and ZLL switch classes (from button event numbers to a human-readable name) but if we are taking out conversions for the rest of the classes then these classes should probably just return the raw value returned by the API too. I'm open to your comments! |
aiohue/sensors.py
Outdated
| class GenericCLIPSensor(GenericSensor): | ||
| def __init__(self, id, raw, request): | ||
| super().__init__(id, raw, request) | ||
| self._on = raw['config'].get('on') |
There was a problem hiding this comment.
Since we store raw as instance variable, we should have properties just fetch it from raw. That way we keep a single source of truth.
aiohue/sensors.py
Outdated
| class DaylightSensor(GenericSensor): | ||
| def __init__(self, id, raw, request): | ||
| super().__init__(id, raw, request) | ||
| self._daylight = raw['state'].get('daylight') |
There was a problem hiding this comment.
We do 'get' on everything. We should only do 'get' if the value might not be there.
aiohue/sensors.py
Outdated
| @property | ||
| def buttonevent(self): | ||
| if self._buttonevent == 34: | ||
| return 'BUTTON_1' |
There was a problem hiding this comment.
We should not convert and then we should expose constants ZGP_SWITCH_BUTTON_1 etc.
That way we can do:
from aiohue.sensors import ZGP_SWITCH_BUTTON_1
if sensor.buttonevent == ZGP_SWITCH_BUTTON_1:
…
Also, only use 'get' if a property may not exist in the API result.
aiohue/sensors.py
Outdated
| class ZLLSwitchSensor(GenericZLLSensor): | ||
| def __init__(self, id, raw, request): | ||
| super().__init__(id, raw, request) | ||
| self._buttonevent = raw['state'].get('buttonevent') |
There was a problem hiding this comment.
We shouldn't cache things. we should just fetch from raw directly. Single source of truth.
There was a problem hiding this comment.
I'm right in the middle of a bunch of updates to the sensors framework based on your earlier feedback. All properties will now be retrieved directly from the raw instance variable and not cached.
aiohue/sensors.py
Outdated
|
|
||
| @property | ||
| def buttonevent(self): | ||
| if self._buttonevent == ZLL_SWITCH_BUTTON_1_CLICK: |
There was a problem hiding this comment.
This whole if/else is no longer needed right? We can just return self.raw.get('buttonevent') ?
There was a problem hiding this comment.
I have to admit I was not thinking clearly when I redid this :) . You are right, this should just return self.raw['state']['buttonevent']. Will fix.
Also, reorganize ZGP Switch into a single class (as there is only one ZGP Switch, it doesn't currently make sense to have a Generic ZGP class and then a ZGP Switch class as we can't really be sure what the base ZGP properties would be).
Provides examples of the properties exposed for each sensor type.
|
OK @balloob - now you could review this. Here's what I've done in a nutshell:
To test I created one of each CLIP sensor type on my Hue bridge and ran example.py against it. Seems to work well! |
Hey @balloob: following our discussion at the end of my last PR, I've put some effort into sketching out how I think the Hue sensor API could be further integrated into aiohue.
This initial commit is just a sample so you can take a look at my thinking and give your comments/direction before I go any further.
I've sketched out additional classes (that subclass the GenericSensor), for the HueTapSwitch, the HueDimmerSwitch, a HueGenericSwitch, a HueTemperatureSensor, and a HueDaylightSensor, so you can see my thinking on how each type of sensor (i.e. switch, sensor, binary sensor) could be handled. You'll see that in each specific class I override GenericSensor's state property method to return the main state property of that class of sensor, be it
buttonevent,temperature,daylight, etc. I'm thinking this would made the downstream integration simpler.In the temperature sensor, you'll see an example of how we could handle the conversion necessary to return useful data (e.g. convert the value the Hue API returns into Celsius, or for light level, we would convert the value the Hue API returns into lux) and also provide the unit of measure for downstream use in HA.
We could go further and also return e.g. the mdi icon appropriate for that sensor, but I'm thinking that's on the dividing line between what should be in this library and what HA should handle - interested in your thoughts.
You'll also see the factory method I started sketching out that now gets passed to Sensors' constructor instead of the old Sensor class.
I've added constants for the types of sensor supported by the Hue bridge (including CLIP sensors and the category they fall into based on what their main state property is, e.g. buttonevent, lightlevel, etc), and also constants for the models of Hue sensor where that is/may be relevant to their further treatment in the factory method (specifically for the Hue Tap and Hue Dimmer).
On a side note, my sincere congratulations on the Ubiquiti Networks news! Good luck!