Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Characteristic improvements #73

Merged
merged 11 commits into from
Apr 6, 2018
Merged

Characteristic improvements #73

Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
e555607
Merged set_value and get_hap_value
cdce8p Mar 28, 2018
78f6758
Additional changes
cdce8p Mar 31, 2018
2505401
Addressed comments
cdce8p Apr 2, 2018
b25f052
Added debug statements
cdce8p Apr 2, 2018
9c26ce3
Changed '_LOGGER' to 'logger'
cdce8p Apr 4, 2018
9269b42
Changed raise error to return None
cdce8p Apr 4, 2018
4bcf00c
Raising ValueErrors with custom msg
cdce8p Apr 5, 2018
2dafa0f
Bugfix, added additional logger.error statements
cdce8p Apr 5, 2018
79de11f
Rearanged functions
cdce8p Apr 5, 2018
1d94c34
Added error_msg parameter
cdce8p Apr 5, 2018
228d9f8
Added tests
cdce8p Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
120 changes: 63 additions & 57 deletions pyhap/characteristic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,21 +56,14 @@ 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"}
_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:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

object inheritance is no longer necessary

"""Represents a HAP characteristic, the smallest unit of the smart home.

A HAP characteristic is some measurement or state, like battery status or
Expand All @@ -79,6 +72,9 @@ class Characteristic(object):
like format, min and max values, valid values and others.
"""

__slots__ = ['display_name', 'type_id', 'properties', 'broker',
'setter_callback', 'value']

def __init__(self, display_name, type_id, properties, value=None, broker=None):
"""Initialise with the given properties.

Expand All @@ -98,34 +94,66 @@ def __init__(self, display_name, type_id, properties, value=None, broker=None):
- 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
if 'Format' not in properties or 'Permissions' not in properties:
raise ValueError('Invalid 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.setter_callback = None
self.value = value or 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 _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._validate_value(value)

def _validate_value(self, value):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method actually transforms the value to a valid one, i.e. it is not validation only. Can we rename to to_valid_value or something like this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think this would be quite a good usecase for @property and @property.setter to do to_valid_value, since this should be executed every time we try to set a value.

Copy link

@thomaspurchas thomaspurchas Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know I suggested an @property.setter above. But thinking about it some more, I am a little concerned that using a @property.setter might result in confusing behaviour.

As a user of the API you would write a value to .value, and it could either raise an exception or when you read .value you wouldn't get the value you expected.

Just wondering if instead .value should be stored in ._value, and then expose a set_value() and update_value() methods which would do the validation. Also using a @property getter to allow reading .value but not writing to it.

That way, to a user it's clear that when you call set_value()/update_value() other clever things are gonna happen (including callbacks and notifications). So shouldn't be surprised if the value passed is changed, or a validation exception is raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After my comment I though about this again an I would agree in many cases @thomaspurchas:

  • a property.setter might indeed be confusing
  • ._value might be a good idea. We use the value variable only as cache for to_HAP if we need to send this to HK. I don't think the user should interact with value directly. _value should be enough, we don't need to explicitly constrain it.
  • @thomaspurchas I understand were your push for two separate methods is comming from. I personally don't like the way it't currently handled. Since should_notify is set to false when update came from HomeKit client, the callback needs to call set_value, or at least notify, so that all clients have the updated value. Additionally when you want to call set_value, I always need to pass in callback=false to prevent the callback. That is kind of redundant.

"""Perform value validation depending on format."""
if self.properties.get('ValidValues'):
if value not in self.properties['ValidValues'].values():
raise ValueError
else:
return value
elif self.properties['Format'] == HAP_FORMAT.STRING:
return str(value)[:256]
elif self.properties['Format'] == HAP_FORMAT.NUMERIC:
value = min(self.properties.get('maxValue', value), value)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not every char that is numeric has a max or min value. How about:
min and max -> min
min -> min
max -> max
no min and no max -> (probably handled by ValidValues, but for sake of completeness) 0

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually that's what I should have done here, or at least intended to do (if it doesn't work).
get('maxValue', value) has value as default. E.g. no maxValue:

  • get --> value
  • min(value, value) -> value

return max(self.properties.get('minValue', value), value)
elif self.properties['Format'] == HAP_FORMAT.BOOL:
return bool(value)
elif self.properties['Format'] == HAP_FORMAT.ARRAY:
# TODO: Add validation
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if we want to add validation for those cases as well

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't, it will become needlessly complex.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will delete those than

pass
elif self.properties['Format'] == HAP_FORMAT.DICTIONARY:
# TODO: Add validation
pass
elif self.properties['Format'] == HAP_FORMAT.DATA:
# TODO: Add validation
pass
elif self.properties['Format'] == HAP_FORMAT.TLV8:
# TODO: Add validation
pass

return value

def set_value(self, value, should_notify=True, should_callback=True):
"""Set the given raw value. It is checked if it is a valid value.
"""Set the given raw 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.
The notification is called _after_ the setter callback.
Notify will be performed if broker is set.
:type should_notify: bool

:param should_callback: Whether to invoke the callback, if such is set. This
Expand All @@ -137,13 +165,10 @@ def set_value(self, value, should_notify=True, should_callback=True):
: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
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:
self.value = self._validate_value(value)
if self.setter_callback and should_callback:
self.setter_callback(self.value)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the callback should just be a callback and only be called if the value is changed in HomeKit.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cdce8p could you point me to an example where setter_callback is used? It strikes me as odd that setter_callback is properly exposed or documented. Also I would be inclined to agree with you with regards to not call it.

Also set_value is an interesting name. In JS-HAP, there are two APIs set_value and update_value. set_value does not notify anything, just updates the objects internal state. Where as update_value will make sure that all the clients are notified of the change.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To expand a little more on the above. There doesn't currently appear to be away for an Accessory to indicate a value update success or failure.

@ikalchev do you have any docs that explaining how client value updates are processed and forwarded onto Accessorys? I would be interested in extending it to allow the Accessory objects to indicate failure, and have that sent back to the requesting client.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I may: You have a bulb, which eventually the user taps on in iOS. This sends a request to HAP-python, which triggers the On characteristic's set_value method with should_callback as True. The setter_callback allows users of HAP-python to plug in and actually turn the bulb on, whether it is by interacting with GPIO or whatever.

About the docs: I haven't documented it, sorry about that. Will update soon.

In HAP-python, update_value is the JS set_value, whereas internal state can be modified with self.value.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ikalchev ok, if I understand this right.

iOS clients cause set_value to be called with should_callback as True, and Accessory objects should call set_value with should_callback=False and should_notify=True?

If that's the case wouldn't providing separate APIs be a little cleaner, rather than using flags?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomaspurchas Client updates end up in AccessoryDriver.set_characteristics, which is where char.set_value is called. Currently, there is no (easy) way to indicate failure of this. I can propose two things:

  1. set_value returns True or False, which is checked in set_characteristics
  2. set_value throws something , which is catches in set_characteristics

In both cases the user should be able to plug into set_value. So, again, here are a few options:

  • Use the current setter_callback - not perfect because the self.value is set before this is called. We can of course change the flow.
  • Use some other mechanism

Personally, I think changing the flow in set_value as

...
success = self.callback(new_value)
if not success:
    return False
# continue as before

will be flexible enough, but some input will mean a lot. Thoughts?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accessory objects should call set_value with should_callback=False and should_notify=True?

It really depends on your use case to be honest. I had discussion with people for this before, but we couldn't figure a better way for it. For example, if your accessory wants to turn on the light, it can just call set_value(new_value) to whatever is necessary (with should_callback=True, should_notify=True, both are the default) and then leave it to the setter_callback to actually turn the bulb on. On the other hand, if turning the lights CAN ALSO happen by some external action that you detect after it's done, then the above approach breaks down, because you have to have more complicated control logic.

The problem is how to handle both cases above well enough. So far I have found these flags acceptable in the cases I've seen, but then what have I seen? If you think the API is missing stuff or if you have a suggestion, I would be more than happy to discuss it and make changes.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will try to come up with a better way for this once more, but a use case/workflow to think about would be nice.

Copy link

@thomaspurchas thomaspurchas Mar 31, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach I would use is have set_value replaced with update_value, which should be called when an iOS client wants the value to change. Then the callback is responsible for trying to make that happen, and raises and exception if it can't.

For the Accessory object, I think that a @property setter could be used on the value attribute, that triggers the notification process.

I think it's very odd for people to be using this API to have the callback triggered. Presumably anything that wants to manipulate the Characteristic will do it via the Accessory object. In which case the Accessory object would be responsible for calling the callback, or only setting value once the callback has completed successfully.

That way there is a clear separation of responsibilities, Characteristics only deal with what needs to be presented to iOS. Marshalling value sets to iOS clients as notifications, and marshalling requests from iOS to the Accessory. The Accessory is then responsible for making sure that the value stores in a Characteristic matches the real world device, regardless how that value was set.

This would also closely match how JS-HAP works as well, and prevents py-HAP from being tangled into peoples accessory code.

Happy to put together some cases/workflows, and also a PR to make that happen. Thoughts from yourself and @cdce8p welcomed!

if self.broker and should_notify:
self.notify()

def override_properties(self, properties=None, valid_values=None):
Expand All @@ -163,43 +188,25 @@ 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_driver.publish

:raise NotConfiguredError: When the broker is not set.
:raise RuntimeError: When the broker is not set.
"""
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.")
if not self.broker:
raise RuntimeError('Notify failed, because broker is not set')

data = {
"type_id": self.type_id,
"value": self.get_hap_value(),
'type_id': self.type_id,
'value': self.value,
}
self.broker.publish(data, self)

def to_HAP(self, iid_manager):
"""Create a HAP representation of this Characteristic.

.. note:: Uses the `get_hap_value`, i.e. sends the HAP value.
"""Create a HAP representation of this Characteristic. Sends the value.

:param iid_manager: IID manager to query for this object's IID.
:type iid_manager: IIDManager
Expand All @@ -221,12 +228,11 @@ def to_HAP(self, iid_manager):
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 len(self.value) > 64:
value_info["maxLen"] = min(len(self.value), 256)
if HAP_PERMISSIONS.READ in self.properties["Permissions"]:
value_info["value"] = val
value_info["value"] = self.value

hap_rep.update(value_info)
return hap_rep