-
Notifications
You must be signed in to change notification settings - Fork 116
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Characteristic improvements #73
Changes from 4 commits
e555607
78f6758
2505401
b25f052
9c26ce3
9269b42
4bcf00c
2dafa0f
79de11f
1d94c34
228d9f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,10 @@ | |
A Characteristic is the smallest unit of the smart home, e.g. | ||
a temperature measuring or a device status. | ||
""" | ||
import logging | ||
|
||
_LOGGER = logging.getLogger(__name__) | ||
|
||
|
||
class HAP_FORMAT: | ||
BOOL = 'bool' | ||
|
@@ -56,21 +60,7 @@ class CharacteristicError(Exception): | |
pass | ||
|
||
|
||
class NotConfiguredError(Exception): | ||
"""Raised when an operation is attempted on a characteristic that has not been | ||
fully configured. | ||
""" | ||
pass | ||
|
||
|
||
_HAP_NUMERIC_FIELDS = {"maxValue", "minValue", "minStep", "unit"} | ||
"""Fields that should be included in the HAP representation of the characteristic. | ||
|
||
That is, if they are present in the specification of a numeric-value characteristic. | ||
""" | ||
|
||
|
||
class Characteristic(object): | ||
class Characteristic: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
"""Represents a HAP characteristic, the smallest unit of the smart home. | ||
|
||
A HAP characteristic is some measurement or state, like battery status or | ||
|
@@ -79,7 +69,10 @@ class Characteristic(object): | |
like format, min and max values, valid values and others. | ||
""" | ||
|
||
def __init__(self, display_name, type_id, properties, value=None, broker=None): | ||
__slots__ = ('display_name', 'type_id', 'properties', 'broker', | ||
'setter_callback', 'value') | ||
|
||
def __init__(self, display_name, type_id, properties): | ||
"""Initialise with the given properties. | ||
|
||
:param display_name: Name that will be displayed for this characteristic, i.e. | ||
|
@@ -91,61 +84,57 @@ def __init__(self, display_name, type_id, properties, value=None, broker=None): | |
|
||
:param properties: A dict of properties, such as Format, ValidValues, etc. | ||
:type properties: dict | ||
|
||
:param value: The initial value to set to this characteristic. If no value is given, | ||
the assigned value happens as: | ||
- if there is a ValidValue property, use some value from it. | ||
- else use `HAP_FORMAT.DEFAULT` for the format of this characteristic. | ||
:type value: Depends on `properties["Format"]` | ||
""" | ||
assert "Format" in properties and "Permissions" in properties | ||
self.display_name = display_name | ||
self.type_id = type_id | ||
self.properties = properties | ||
if value: | ||
self.value = value | ||
else: | ||
if self.properties.get('ValidValues'): | ||
self.value = min(self.properties['ValidValues'].values()) | ||
else: | ||
self.value = HAP_FORMAT.DEFAULT[properties["Format"]] | ||
self.broker = broker | ||
self.broker = None | ||
self.setter_callback = None | ||
self.value = self._get_default_value() | ||
|
||
def __repr__(self): | ||
"""Return the representation of the characteristic.""" | ||
return "<characteristic display_name='{}' value={} properties={}>" \ | ||
return '<characteristic display_name={} value={} properties={}>' \ | ||
.format(self.display_name, self.value, self.properties) | ||
|
||
def set_value(self, value, should_notify=True, should_callback=True): | ||
def set_value(self, value, should_notify=True): | ||
"""Set the given raw value. It is checked if it is a valid value. | ||
|
||
:param value: The value to assign as this Characteristic's value. | ||
:type value: Depends on properties["Format"] | ||
|
||
:param should_notify: Whether a the change should be sent to subscribed clients. | ||
The notification is called _after_ the setter callback. Notify will be | ||
performed if and only if the broker is set, i.e. not None. | ||
:param should_notify: Whether a the change should be sent to | ||
subscribed clients. Notify will be performed if the broker is set. | ||
:type should_notify: bool | ||
|
||
:param should_callback: Whether to invoke the callback, if such is set. This | ||
is useful in cases where you and HAP clients can both update the value and | ||
you don't want your callback called when you set the value, but want it | ||
called when clients do. Defaults to True. | ||
:type should_callback: bool | ||
|
||
:raise ValueError: When the value being assigned is not one of the valid values | ||
for this Characteristic. | ||
""" | ||
if self.properties.get('ValidValues') and \ | ||
value not in self.properties['ValidValues'].values(): | ||
raise ValueError | ||
_LOGGER.debug('%s: Set value to %s', self.display_name, value) | ||
value = self.to_valid_value(value) | ||
self.value = value | ||
if self.setter_callback is not None and should_callback: | ||
self.setter_callback(value) | ||
if should_notify and self.broker is not None: | ||
if should_notify and self.broker: | ||
self.notify() | ||
|
||
def _get_default_value(self): | ||
"""Helper method. Return default value for format.""" | ||
if self.properties.get('ValidValues'): | ||
return min(self.properties['ValidValues'].values()) | ||
else: | ||
value = HAP_FORMAT.DEFAULT[self.properties['Format']] | ||
return self.to_valid_value(value) | ||
|
||
def to_valid_value(self, value): | ||
"""Perform validation and conversion to valid value""" | ||
if self.properties.get('ValidValues'): | ||
if value not in self.properties['ValidValues'].values(): | ||
raise ValueError('%s is not a valid value'.format(value)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ikalchev I was thinking about this. What would you think about an error log statement instead of an ValueError? The log statement would be easier to debug. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do you think so? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With errors you get the whole traceback and it might seem unintentional. Whereas with error log message this would be handled much cleaner. I started thinking in which cases you would use an As an implementation for us: I thought about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that breaks HomeKit. I tried it some time ago and it seemed to cause issues. I believe if the value doesn't get updated, that should be hint enough to think something is wrong here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why would you not want an Exception and traceback? If the value is invalid then the caller should capture it, and either log it or ignore it (py-HAP is just providing an API after all, not then entire application. The demo server could be updated to capture and log the exception). If the iOS client makes a call and sets an invalid value, then the exception should also be raised, caught by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should had over the responsibility for that. Especially because we would need a try block for every For |
||
elif self.properties['Format'] == HAP_FORMAT.STRING: | ||
value = str(value)[:256] | ||
elif self.properties['Format'] == HAP_FORMAT.BOOL: | ||
value = bool(value) | ||
elif self.properties['Format'] in HAP_FORMAT.NUMERIC: | ||
value = min(self.properties.get('maxValue', value), value) | ||
value = max(self.properties.get('minValue', value), value) | ||
return value | ||
|
||
def override_properties(self, properties=None, valid_values=None): | ||
"""Override characteristic property values and valid values. | ||
|
||
|
@@ -163,70 +152,50 @@ def override_properties(self, properties=None, valid_values=None): | |
if valid_values: | ||
self.properties['ValidValues'] = valid_values | ||
|
||
def get_hap_value(self): | ||
"""Get the value of the characteristic, constrained with the HAP properties. | ||
""" | ||
val = self.value | ||
if self.properties["Format"] == HAP_FORMAT.STRING: | ||
val = val[:256] | ||
elif self.properties["Format"] in HAP_FORMAT.NUMERIC: | ||
if "maxValue" in self.properties: | ||
val = min(self.properties["maxValue"], val) | ||
if "minValue" in self.properties: | ||
val = max(self.properties["minValue"], val) | ||
return val | ||
|
||
def notify(self): | ||
"""Notify clients about a value change. | ||
"""Notify clients about a value change. Sends the value. | ||
|
||
.. note:: Non-blocking, i.e. does not wait for the update to be sent. | ||
.. note:: Uses the `get_hap_value`, i.e. sends the HAP value. | ||
.. seealso:: accessory.publish | ||
.. seealso:: accessory_driver.publish | ||
""" | ||
self.broker.publish(self.value, self) | ||
|
||
:raise NotConfiguredError: When the broker is not set. | ||
def client_update_value(self, value): | ||
"""Called from broker for value change in Home app. | ||
|
||
Change self.value to value and call callback. | ||
""" | ||
if self.broker is None: | ||
raise NotConfiguredError("Attempted to notify when `broker` is None. " | ||
"Consider adding the characteristic to a " | ||
"Service and then to an Accessory.") | ||
|
||
data = { | ||
"type_id": self.type_id, | ||
"value": self.get_hap_value(), | ||
} | ||
self.broker.publish(data, self) | ||
_LOGGER.debug('%s: Client update value to %s', | ||
self.display_name, value) | ||
self.value = value | ||
self.notify() | ||
if self.setter_callback: | ||
self.setter_callback(value) | ||
|
||
def to_HAP(self, iid_manager): | ||
def to_HAP(self): | ||
"""Create a HAP representation of this Characteristic. | ||
|
||
.. note:: Uses the `get_hap_value`, i.e. sends the HAP value. | ||
|
||
:param iid_manager: IID manager to query for this object's IID. | ||
:type iid_manager: IIDManager | ||
Used for json serialization. | ||
|
||
:return: A HAP representation. | ||
:rtype: dict | ||
""" | ||
hap_rep = { | ||
"iid": iid_manager.get_iid(self), | ||
"type": str(self.type_id).upper(), | ||
"description": self.display_name, | ||
"perms": self.properties["Permissions"], | ||
"format": self.properties["Format"], | ||
'iid': self.broker.iid_manager.get_iid(self), | ||
'type': str(self.type_id).upper(), | ||
'description': self.display_name, | ||
'perms': self.properties['Permissions'], | ||
'format': self.properties['Format'], | ||
} | ||
|
||
if self.properties["Format"] in HAP_FORMAT.NUMERIC: | ||
value_info = {k: self.properties[k] for k in | ||
self.properties.keys() & _HAP_NUMERIC_FIELDS} | ||
else: | ||
value_info = dict() | ||
|
||
val = self.get_hap_value() | ||
if self.properties["Format"] == HAP_FORMAT.STRING: | ||
if len(val) > 64: | ||
value_info["maxLen"] = min(len(val), 256) | ||
if HAP_PERMISSIONS.READ in self.properties["Permissions"]: | ||
value_info["value"] = val | ||
if self.properties['Format'] in HAP_FORMAT.NUMERIC: | ||
hap_rep.update({k: self.properties[k] for k in | ||
self.properties.keys() & | ||
('maxValue', 'minValue', 'minStep', 'unit')}) | ||
elif self.properties['Format'] == HAP_FORMAT.STRING: | ||
if len(self.value) > 64: | ||
hap_rep['maxLen'] = min(len(self.value), 256) | ||
if HAP_PERMISSIONS.READ in self.properties['Permissions']: | ||
hap_rep['value'] = self.value | ||
|
||
hap_rep.update(value_info) | ||
return hap_rep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this is pythonic and all that but using
logger
instead is also fairly common and easier to write. What do you think?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure on that one.
logger
indicates to me that it's a changing variable, something that could be passed to a class or function._LOGGER
on the other hand indicates a constant, that should only be accessible for this module, so exactly what this is.Currently it's
logger
in all other files, isn't it? Maybe for now I just change it tologger
, since we also don't have an instance where we pass it around that might lead to confusion. We can change it later if we decide to.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have it consistent, no matter what the final name. So we can leave it as logger now and do a single change in all the repo, changing all loggers to _LOGGER. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's what I would have suggested as well. I will change it to
logger
.