-
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
Conversation
* Small improvements * Added helper method '_validate_value' * Added helper method '_get_default_value' * Raise ValueError if properties are invalid * Added __slots__
pyhap/characteristic.py
Outdated
"""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 comment
The reason will be displayed to describe this comment to others. Learn more.
object
inheritance is no longer necessary
pyhap/characteristic.py
Outdated
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) |
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 think the callback should just be a callback and only be called if the value is changed in HomeKit.
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.
@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.
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.
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 Accessory
s? I would be interested in extending it to allow the Accessory
objects to indicate failure, and have that sent back to the requesting client.
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.
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
.
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.
@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?
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.
@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:
set_value
returns True or False, which is checked inset_characteristics
set_value
throws something , which is catches inset_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?
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.
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.
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 will try to come up with a better way for this once more, but a use case/workflow to think about would be nice.
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.
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, Characteristic
s 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!
pyhap/characteristic.py
Outdated
elif self.properties['Format'] == HAP_FORMAT.BOOL: | ||
return bool(value) | ||
elif self.properties['Format'] == HAP_FORMAT.ARRAY: | ||
# TODO: Add validation |
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 don't know if we want to add validation for those cases as well
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 wouldn't, it will become needlessly complex.
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 will delete those than
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.
Good, same amount of work with fewer lines. I commented a few issues.
pyhap/characteristic.py
Outdated
value = HAP_FORMAT.DEFAULT[self.properties['Format']] | ||
return self._validate_value(value) | ||
|
||
def _validate_value(self, value): |
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.
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?
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.
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.
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 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.
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.
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.
pyhap/characteristic.py
Outdated
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) |
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.
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
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.
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
pyhap/characteristic.py
Outdated
elif self.properties['Format'] == HAP_FORMAT.BOOL: | ||
return bool(value) | ||
elif self.properties['Format'] == HAP_FORMAT.ARRAY: | ||
# TODO: Add validation |
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 wouldn't, it will become needlessly complex.
BTW why we want to rename to_HAP to to_dict? |
I think that it then might be more obvious what this method does just by looking at the method name. Also it would work quiet nicely with |
Unfortunately I won't have time tomorrow. I will resume working on this probably Monday. |
* New method: update_value (use for value updates from HAP-python) * Rename: to_HAP to to_dict * New method: from_dict (use during loading of chars)
Pushed an update. Sadly the
Still to do:
|
Re Changing it to |
pyhap/accessories/SDS011.py
Outdated
@@ -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 |
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 though that accessories should be calling set_value
, and in the future value
will be made read only with a @property
getter.
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 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.
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.
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).
pyhap/characteristic.py
Outdated
"""Notify clients about a value change. Sends the value.""" | ||
self.broker.publish(self.value, self) | ||
|
||
def to_dict(self): |
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.
Not sure about this. See comment in PR.
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 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
.
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.
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.
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.
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
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.
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.
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 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 😅
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.
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.
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.
What one developer can do in one month, two developers can do in two months.
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.
to_HAP calls to_dict and then trims the result.
Probably better, yes
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.
For a representation / debugging: __repr__
would be the way to go. I would stick with to_HAP
for now then.
pyhap/characteristic.py
Outdated
self.notify() | ||
if self.setter_callback: | ||
return self.setter_callback(value) | ||
return True |
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.
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.
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 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?
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 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.
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 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.
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.
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)
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.
Sounds good :)
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.
@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.
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.
@thomaspurchas maybe continue here: #75
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.
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.
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've copied a summary of this thread into there.
pyhap/characteristic.py
Outdated
Change self.value to value and call callback. | ||
The callback method must return a boolean that indicates if successful. | ||
""" | ||
self.value = value |
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.
Value should only be set after the setter_callback
is successful, otherwise the value
won't represent what the device did.
pyhap/characteristic.py
Outdated
return True | ||
|
||
@classmethod | ||
def from_dict(cls, name, json_dict): |
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.
When do you imagine this will be used?
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 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.
pyhap/characteristic.py
Outdated
|
||
:param value: The value to assign as this Characteristic's value. | ||
:type value: Depends on properties["Format"] | ||
def set_value(self, value): |
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 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?
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 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.
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.
Yeah adding a should_notify=True
would probably be a good move.
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 guess it wont hurt anyone with should_notify=True
pyhap/characteristic.py
Outdated
|
||
:param iid_manager: IID manager to query for this object's IID. | ||
:type iid_manager: IIDManager | ||
def update_value(self, value): |
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.
How about client_set_value
to make it more clear?
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.
Sounds good 👍
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.
Could I suggest client_change_value
, I think it makes it clearer that it might fail. But I could just be bikeshedding.
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.
What about client_update_value
?
* Fixed errors, small mistakes
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.
Just a minor nit about the logger, if you don't mind
pyhap/characteristic.py
Outdated
@@ -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__) |
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 to logger
, 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
.
pyhap/characteristic.py
Outdated
"""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 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.
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.
Why do you think so?
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.
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.
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.
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
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 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 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.
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 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
If you're ok with these, I would like to rearrange the functions before merging. |
pyhap/characteristic.py
Outdated
@@ -125,12 +130,18 @@ 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)) | |||
logger.error('%s: value=%s is an invalid value.', |
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.
Sorry, didn't get my comment in quick enough before.
It seems really strange to handle the error here. Surely it's better just to raise an Exception and have the caller handle it. Doing this means we now have special return values to indicate a failure (which is very unpythonic).
If people don't like the traceback, then they should be catching the ValueError
and logging it explicitly. (We could modify the examples to do this). py-HAP shouldn't be making that decision for them.
Addtionally if the ValueError
is caused by iOS, then we should be returning the correct status code to iOS to indicate an invalid value has been sent.
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 returning None
is that special. It's basically the check: Did to_valid_value
really return a valid value? We can of course implement the try catch in set_value
, but I don't not if that is worth it.
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.
return None
is also universal that the function aborted.
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 wouldn't catch in set_value
but in whatever called set_value
. It seems strange to rob the caller of the ability to handle the error, just because our one usage now dosen't require that ability, and tracebacks are ugly.
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.
Also returning None
hides the error from the caller. Making even less obvious to the developer using the API that something has gone wrong.
Submitting an invalid value the perfect time to fail fast and fail hard. Ensuring that anything that might rely on that valuing be set won't run unless the failure is explicitly handled.
Doing anything else if just going to quietly hide the error in impenetrable logs.
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 agree that a user shouldn’t get trace backs. But it’s on HA to catch Exceptions, it’s not for py-HAP to accept any old input and carry on, just so HA doesn’t have do proper error handling.
py-HAP is just an API, a HA user shouldn’t have to interact directly with it. The HA ui should be doing all the heavy lifting.
If people are using py-HAP directly then it’s on them to catch errors and handle them, and they should be devs and know what to do with them.
If people are just using the demo accessories then they should be updated to capture the exception and produce a meaningful error message.
In each these scenarios the output will be different, we shouldn't be picking one and forcing everyone to use it.
And specifically with HA, doing the error handling in HA, rather than in py-hap means we can produce much better error messages(including HA info). Helping us debug issues quicker.
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.
Can I also point out that your proposed error message is very unhelpful. It says there is an invalid value, be not what caused it. Which accessory tried to set the value, and onto what characteristic, and why is it invalid?
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.
One final point. If your frustrated by the amount of work needed to do the error handling. I’m happy to do that work, both here and in HA.
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 disagree. '%s: value=%s is an invalid value.', self.display_name, value
should cover almost all cases. It's true that you don't get the accessory, but you know it already since it hasen't been updated. You also get the distinction between valid values
and numerics
. There isn't really anything more I would add / need.
One final point. If your frustrated by the amount of work needed to do the error handling. I’m happy to do that work, both here and in HA.
I won't stop you from looking through the open issues and suggesting solutions.
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 think we are overthinking this a bit. It should be very rare to get invalid values:
- iOS cannot send an invalid value, because it picks values from the min/max/validValues fields that the characteristic defines. In the worst case, even if Apple change something, your accessory will not get added to the Home app in the first place (I tried)
- Accessories in HAP are developed from devs not end users. This is the target "audience" so to speak. Thus, we need the best solution for devs to be able to pinpoint the problem.
That being said, I think that the current ValueError is good enough. When the dev is coding a new Accessory, he/she will probably get these early in testing. Once they are gone, they should be gone for good.
Very well, my first suggestion is to leave this exception in.
For now, all I ask is you leave this commit out of this PR. Tomorrow I’ll go though and create PRs for better error handling in HA, and in the demo accessories in py-HAP.
Once I’ve done the PRs, if you still think dropping this exception is the best move, then we’ll do that move onto other issues.
…On 4 Apr 2018, 23:33 +0100, cdce8p ***@***.***>, wrote:
@cdce8p commented on this pull request.
In pyhap/characteristic.py:
> @@ -125,12 +130,18 @@ 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))
+ logger.error('%s: value=%s is an invalid value.',
I disagree. '%s: value=%s is an invalid value.', self.display_name, value should cover almost all cases. It's true that you don't get the accessory, but you know it already since it hasen't been updated. You also get the distinction between valid values and numerics. There isn't really anything more I would add / need.
> One final point. If your frustrated by the amount of work needed to do the error handling. I’m happy to do that work, both here and in HA.
I won't stop you from looking through the open issues and suggesting solutions.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
We don't get anywhere here. @ikalchev What do you think? |
I gave me some time to think about this and I might have been a bit short sided about it. Why not combine our ideas into an error with a custom message? That way we get a proper traceback and more information for debugging. I already pushed the commit for it so you can take a look. |
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.
This sounds good! I am ready to merge if you confirm there are no pending tasks for this PR.
I was testing the changes and found something interesting: At least in Home Assistant the Value Error will not be displayed by default until it is stopped. That might be true for other implementations as well. |
This is odd. Is that HA problem or by design? |
pyhap/characteristic.py
Outdated
"""Perform validation and conversion to valid value""" | ||
if self.properties.get('ValidValues'): | ||
if value not in self.properties['ValidValues'].values(): | ||
logger.error('%s: value={} is an invalid value.', |
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 don’t want to nit pick, but can we e = str.format(...); logger.error(e); raise ValueError(e)
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 named the string error_msg
if that's ok. New commit is online
I'm not sure. I did get the Value error once right after the call to set_value. But the was out of over ten tries. |
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.
Is it ready for merging?
Depends if I should update the tests. Feature wise it's ready. |
As long as they are passing, I think we are ok |
They are not currently, since there are breaking changes. |
Would it be OK if you try to fix them? Alternatively, If you are sick of this change I don't mind doing it |
I can do them. Just won't get to it until in a few hours, since I'm currently away from my computer. I should get them ready today. |
No hurry :) |
This will be an artefact of asyncio. The exception is being raised, and capture by a Relevant python docs here: https://docs.python.org/3/library/asyncio-dev.html#detect-exceptions-never-consumed Also worth looking at this: https://stackoverflow.com/questions/30361824/asynchronous-exception-handling-in-python |
* Added '/htmlcov' to .gitignore
I just finished the tests. Should be 100% coverage, although that doesn't mean much. |
This is a lot of great work! Cheers! |
* Remove breaking change description from README. * Characteristic improvements (#73) * Merged set_value and get_hap_value * Small improvements * Added helper method '_validate_value' * Added helper method '_get_default_value' * Raise ValueError if properties are invalid * Added __slots__ * Additional changes * New method: update_value (use for value updates from HAP-python) * Rename: to_HAP to to_dict * New method: from_dict (use during loading of chars) * Addressed comments * Fixed errors, small mistakes * Added debug statements * Changed '_LOGGER' to 'logger' * Changed raise error to return None * Raising ValueErrors with custom msg * Bugfix, added additional logger.error statements * Rearanged functions * Added error_msg parameter * Added tests * Added '/htmlcov' to .gitignore * Bump version number to 1.1.9
* Remove breaking change description from README. * Characteristic improvements (#73) * Merged set_value and get_hap_value * Small improvements * Added helper method '_validate_value' * Added helper method '_get_default_value' * Raise ValueError if properties are invalid * Added __slots__ * Additional changes * New method: update_value (use for value updates from HAP-python) * Rename: to_HAP to to_dict * New method: from_dict (use during loading of chars) * Addressed comments * Fixed errors, small mistakes * Added debug statements * Changed '_LOGGER' to 'logger' * Changed raise error to return None * Raising ValueErrors with custom msg * Bugfix, added additional logger.error statements * Rearanged functions * Added error_msg parameter * Added tests * Added '/htmlcov' to .gitignore * Bump version number to 1.1.9
I started working on
characteristic.py
and got some first results.Thats what I have planed:
set_value
andget_hap_value
to_HAP
This should lead to:
from_dict
--> laterfrom_dict
--> laterBreaking changes
set_value
doesn't takeshould_callback
anymore