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
13 changes: 12 additions & 1 deletion pyhap/characteristic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ def __repr__(self):
def set_value(self, value, should_notify=True):
"""Set the given raw value. It is checked if it is a valid value.

If not set_value will be aborted and an error message will be
displayed.

:param value: The value to assign as this Characteristic's value.
:type value: Depends on properties["Format"]

Expand All @@ -109,6 +112,8 @@ def set_value(self, value, should_notify=True):
"""
logger.debug('%s: Set value to %s', self.display_name, value)
value = self.to_valid_value(value)
if value is None:
return
self.value = value
if should_notify and self.broker:
self.notify()
Expand All @@ -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.',
Copy link

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

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.

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

Copy link
Contributor Author

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.

Copy link

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.

Copy link

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.

Copy link

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

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.

Copy link

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?

Copy link

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.

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

Copy link
Owner

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.

self.display_name, value)
return
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:
if not isinstance(value, (int, float)):
logger.error('%s: value=%s is not a numeric value.',
self.display_name, value)
return
value = min(self.properties.get('maxValue', value), value)
value = max(self.properties.get('minValue', value), value)
return value
Expand Down