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

Show file tree
Hide file tree
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
8 changes: 4 additions & 4 deletions pyhap/accessories/SDS011.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,20 @@ def _set_services(self):
# PM2.5
air_quality_pm25 = loader.get_serv_loader().get("AirQualitySensor")
pm25_size = char_loader.get("AirParticulateSize")
pm25_size.set_value(0, should_notify=False)
pm25_size.value = 0
Copy link

Choose a reason for hiding this comment

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

I though that accessories should be calling set_value, and in the future value will be made read only with a @property getter.

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 didn't mentioned that above, but I realized that there are cases in which setting the value on its one does make sense. Testing as well as the one above. value = 0 is basically equivalent to set_value(0, should_notify=False) if no callback has been added and the char doesn't belong to any service / accessory yet.

Copy link

Choose a reason for hiding this comment

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

Yes, but doing this will ignore all the validation logic, which still needs to be run at some point. This could allow an invalid value to be added using __init__ and then sent to an iOS client during a get.

Better to just use set_value, and have the char figure out if sending a notification makes sense (if there is no broker, then just don't bother with the notification).

self.pm25_density = char_loader.get("AirParticulateDensity")
pm25_name = char_loader.get("Name")
pm25_name.set_value("PM2.5", should_notify=False)
pm25_name.value = "PM2.5"
self.pm25_quality = air_quality_pm25.get_characteristic("AirQuality")
air_quality_pm25.add_characteristic(pm25_name, pm25_size, self.pm25_density)

# PM10
air_quality_pm10 = loader.get_serv_loader().get("AirQualitySensor")
pm10_size = char_loader.get("AirParticulateSize")
pm10_size.set_value(1, should_notify=False)
pm10_size.value = 1
self.pm10_density = char_loader.get("AirParticulateDensity")
pm10_name = char_loader.get("Name")
pm10_name.set_value("PM10", should_notify=False)
pm10_name.value = "PM10"
self.pm10_quality = air_quality_pm10.get_characteristic("AirQuality")
air_quality_pm10.add_characteristic(pm10_name, pm10_size, self.pm10_density)

Expand Down
4 changes: 2 additions & 2 deletions pyhap/accessory.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,7 +390,7 @@ def stop(self):

# Broker

def publish(self, data, sender):
def publish(self, value, sender):
"""Append AID and IID of the sender and forward it to the broker.

Characteristics call this method to send updates.
Expand All @@ -409,7 +409,7 @@ def publish(self, data, sender):
acc_data = {
"aid": self.aid,
"iid": self.iid_manager.get_iid(sender),
"value": data["value"],
"value": value,
}
self.broker.publish(acc_data)

Expand Down
2 changes: 1 addition & 1 deletion pyhap/accessory_driver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,7 @@ def set_characteristics(self, chars_query, client_addr):
}
if "value" in cq:
# TODO: status needs to be based on success of set_value
char.set_value(cq["value"], should_notify=True)
char.update_value(cq["value"])
if "r" in cq:
response["value"] = char.value

Expand Down
168 changes: 62 additions & 106 deletions pyhap/characteristic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
A Characteristic is the smallest unit of the smart home, e.g.
a temperature measuring or a device status.
"""
import uuid


class HAP_FORMAT:
BOOL = 'bool'
Expand Down Expand Up @@ -72,10 +74,10 @@ class Characteristic:
like format, min and max values, valid values and others.
"""

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

def __init__(self, display_name, type_id, properties, value=None, broker=None):
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.
Expand All @@ -87,90 +89,50 @@ 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"]`
"""
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
self.broker = broker
self.broker = None
self.setter_callback = None
self.value = value or self._get_default_value()
self.value = self._get_default_value()

def __repr__(self):
"""Return the representation of the characteristic."""
return '<characteristic display_name={} value={} properties={}>' \
.format(self.display_name, self.value, self.properties)

def set_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.

I am still thinking about the should_notify and if there is a case for it. Image you want to set the value but not notify iOS clients - technically you can do it by setting the value property but then you don't have the to_valid_value transformation. What do you think, is it worth adding it?

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 though about this case as well. It's a trade of here. I think that e.g. value = 0 would be enough, since most of those cases are for static content which is hard coded. Therefore it would be quite easy to recognize a problem / bug and change it.

Copy link

Choose a reason for hiding this comment

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

Yeah adding a should_notify=True would probably be a good move.

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 guess it wont hurt anyone with should_notify=True

"""Called from user to indicate value change.

Will notify broker that publishes change to clients.
"""
value = self.to_valid_value(value)
self.value = value
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._validate_value(value)
value = HAP_FORMAT.DEFAULT[self.properties['Format']]
return self.to_valid_value(value)

def _validate_value(self, value):
"""Perform value validation depending on format."""
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
else:
return value
raise ValueError('%s is not a valid value'.format(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.

@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.

Copy link
Owner

Choose a reason for hiding this comment

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

Why do you think so?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 raise Error. It is only if you don't know how to properly handle it in your program. So you push the responsibility to the end user.

As an implementation for us: I thought about logger.error and return None. Adding an check if is not None to set_value, otherwise abort.

Copy link
Owner

@ikalchev ikalchev Apr 4, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How about logger.error, return value # as is. iOS will show no response or something if it gets an invalid value ( I think). Even more hints for the client to see the logs

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 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.

Copy link

Choose a reason for hiding this comment

The 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 client_set_value() and the appropriate status response sent back to iOS.

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 think we should had over the responsibility for that. Especially because we would need a try block for every set_value call.

For client_set_value(): By definition are all values stored in self.value are valid values for HomeKit. So HomeKit can't set an invalid value to HAP-python. There is just the case were it's an invalid value for the object / sensor you control, but I think this is for #75

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)
return max(self.properties.get('minValue', value), value)
value = str(value)[:256]
elif self.properties['Format'] == HAP_FORMAT.BOOL:
return bool(value)
elif self.properties['Format'] == HAP_FORMAT.ARRAY:
# TODO: Add validation
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

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 set_value(self, value, should_notify=True, should_callback=True):
"""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 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.
"""
self.value = self._validate_value(value)
if self.setter_callback and should_callback:
self.setter_callback(self.value)
if self.broker and should_notify:
self.notify()

def override_properties(self, properties=None, valid_values=None):
"""Override characteristic property values and valid values.

Expand All @@ -189,50 +151,44 @@ def override_properties(self, properties=None, valid_values=None):
self.properties['ValidValues'] = valid_values

def notify(self):
"""Notify clients about a value change. Sends the value.

.. note:: Non-blocking, i.e. does not wait for the update to be sent.
.. seealso:: accessory_driver.publish

:raise RuntimeError: When the broker is not set.
"""
if not self.broker:
raise RuntimeError('Notify failed, because broker is not set')

data = {
'type_id': self.type_id,
'value': self.value,
"""Notify clients about a value change. Sends the value."""
self.broker.publish(self.value, self)

def to_dict(self):
Copy link

Choose a reason for hiding this comment

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

Not sure about this. See comment in PR.

Copy link
Owner

@ikalchev ikalchev Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I would also prefer to_HAP for the same reasons to be honest. If to_dict is so needed, it can currently just return the result of to_HAP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

to_HAP exists to create a representation of the characteristic that the HAP protocol understands. The fact that it's a dict is just an implementation detail, and it shouldn't be relied on.

I disagree. It might just be an implementation detail, but that exactly makes it even better. to_HAP might not be clear for someone without a background in HomeKit who just started looking at HAP-python.

Changing it to to_dict give the impression that you could use that representation for something else

Why not? If someone whats to use that say for debugging, why should we limit that?

Changing it to to_dict give the impression [...] that it's always going to be a dict type. Neither of those things are true, in the future we may decide to introduce a HAPMessage type or something similar, which would be returned instead of a dict. to_dict ties our hands on that, conflating what the function does with what it returns.

I get that, but what prevents us to changing that back in the future?

I believe that to_dict would be a better representation of this method since this method name is widely used across many different python projects, including Home Assistant (for States and Events).
However if you two think otherwise, this shouldn't stand in the way of this PR.

Copy link
Owner

Choose a reason for hiding this comment

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

You can use to_HAP for debugging without it being to_dict. How about this:

def to_dict(self):
    return self.to_HAP()

In this way, we get best of both worlds. What is more, when/if to_dict changes because there are more stuff in the Char we want to dict, we can just add those in to_dict

Copy link

Choose a reason for hiding this comment

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

Still not convinced by this. to_HAP has a very explicit purpose, which is to serialise the object for the HAP proto.

I agree with @ikalchev, either to_dict should return the result of to_HAP, or perhaps better, to_HAP uses to_dict internally to build its serialisation.

Either way to_HAP shouldn't go anywhere as it's an important internal API. But I don't see any issues with added an additional to_dict method.

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 leave it at to_HAP.

def to_dict(self):
    return self.to_HAP()

That would add just another method to do the same think and would defeat the propose of this change (to reduce complexity). I didn't thought that would be so controversial 😅

Copy link

Choose a reason for hiding this comment

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

To nit pick, I think it actually makes more sense to do it the other way around. to_HAP calls to_dict and then trims the result.

Copy link
Owner

Choose a reason for hiding this comment

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

What one developer can do in one month, two developers can do in two months.

Copy link
Owner

@ikalchev ikalchev Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

to_HAP calls to_dict and then trims the result.

Probably better, yes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For a representation / debugging: __repr__ would be the way to go. I would stick with to_HAP for now then.

"""Create a dict repr of this object for json serialization."""
json_dict = {
'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'],
}
self.broker.publish(data, self)

def to_HAP(self, iid_manager):
"""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

: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"],
}
if self.properties['Format'] in HAP_FORMAT.NUMERIC:
json_dict.update({k: self.properties[k] for k in
self.properties.keys() & _HAP_NUMERIC_FIELDS})
elif self.properties['Format'] == HAP_FORMAT.STRING:
if len(self.value) > 64:
json_dict['maxLen'] = min(len(self.value), 256)
if HAP_PERMISSIONS.READ in self.properties['Permissions']:
json_dict['value'] = self.value

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()
return json_dict

if self.properties["Format"] == HAP_FORMAT.STRING:
if len(self.value) > 64:
value_info["maxLen"] = min(len(self.value), 256)
if HAP_PERMISSIONS.READ in self.properties["Permissions"]:
value_info["value"] = self.value
def update_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.

How about client_set_value to make it more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good 👍

Copy link

@thomaspurchas thomaspurchas Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Could I suggest client_change_value, I think it makes it clearer that it might fail. But I could just be bikeshedding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about client_update_value?

"""Called from broker for value change in Home app.

hap_rep.update(value_info)
return hap_rep
Change self.value to value and call callback.
The callback method must return a boolean that indicates if successful.
"""
self.value = value
Copy link

Choose a reason for hiding this comment

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

Value should only be set after the setter_callback is successful, otherwise the value won't represent what the device did.

self.notify()
if self.setter_callback:
return self.setter_callback(value)
return True
Copy link

Choose a reason for hiding this comment

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

Rather than returning True/False I think the callback should raise an exception on failure. The exception could be different depending on what went wrong.

We can then map the exceptions to the HAP protocol status values.

Using True/False feels a little bit like err values in C.

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 would think that True / False should be enough for our proposes, but you are right in that exceptions would allow for more options. @ikalchev What do you think?

Copy link

Choose a reason for hiding this comment

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

I would point out that True/False is not backwards compatible, and requires that users remember to return a True value on success (forgetting to return a value would result in None). This does not strike me as very pythonic.

Copy link
Owner

Choose a reason for hiding this comment

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

I just looked at Apple's spec - there are 11 error codes (which I need to add btw). If we use exceptions, we will force the Driver from understanding these exceptions and mapping them to error codes - in this case, why not set_value to return one of the error codes instead? What is more, returning an error code would allow "internal" errors to more "voicefully" propagate with an exception.

Copy link

@thomaspurchas thomaspurchas Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

There might be 11 error codes, but only some would be valid for a callback i.e. Request denied due to insufficient privileges. Unable to communicate with requested service, e.g. the power to the accessory was turned off. Resource is busy, try again. Accessory received an invalid value in a write request.

The rest are errors that should be managed by py-HAP. i.e. Operation timed out. could be thrown by setting a timeout on the callback (either via threads or asyncio).

Additionally exceptions can be nested so the following would work:

class AccessoryError(Exception):
  pass

class AccessoryBusyError(AccessoryError):
  pass

try:
  throw AccessoryBusyError
except AccessoryError as e:
  print(e)

Copy link
Owner

Choose a reason for hiding this comment

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

Sounds good :)

Copy link

Choose a reason for hiding this comment

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

@ikalchev still doesn't feel pythonic. We would then need a bunch of static vars somewhere to match the docs, or rely on users parsing the docs.

Not sure how best to move this convo into another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@thomaspurchas maybe continue here: #75

Copy link
Owner

Choose a reason for hiding this comment

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

Let's think on that, it's not such a big deal, whatever we chose.

BTW there is a discord chat, but I haven't mentioned that here because it would make things more hard to follow.

Copy link

Choose a reason for hiding this comment

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

I've copied a summary of this thread into there.


@classmethod
def from_dict(cls, name, json_dict):
Copy link

Choose a reason for hiding this comment

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

When do you imagine this will be used?

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 plan to use it for the Char_Loader class, but it might make more sense if added when there is a use for it. I will remove it for now.

"""Convert json dictionary to characteristic instance."""
return cls(name, type_id=uuid.UUID(json_dict.pop('UUID')),
properties=json_dict)
4 changes: 2 additions & 2 deletions pyhap/service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,11 +60,11 @@ def to_HAP(self, iid_manager=None):
:rtype: dict.
"""
assert iid_manager is not None
characteristics = [c.to_HAP(iid_manager) for c in self.characteristics]
chars = [c.to_dict() for c in self.characteristics]

hap_rep = {
"iid": iid_manager.get_iid(self),
"type": str(self.type_id).upper(),
"characteristics": characteristics,
"characteristics": chars,
}
return hap_rep