Added getter_callback to Characteristic #90

jslay88 · 2018-04-17T03:25:28Z

cdce8p · 2018-04-17T07:14:06Z

@jslay88 Can you rebase your branch into to current dev branch? That should solve the merge conflicts.

cdce8p · 2018-04-17T07:43:46Z

As far as I understand it, the getter_callback should be used to pull updates from a sensor within client_update_value. Wouldn't it then be enough to make value optional and add

if self.getter_callback:
    value = self.getter_callback()

jslay88 · 2018-04-20T18:58:42Z

Yeah, you are probably right. This is what I get for trying to put something in late at night waiting on things to finish. Give me a day, I want to rework this now that I am looking at it again.

jslay88 · 2018-04-22T04:37:27Z

Should we change the setter_callback to not use value as well? Seeing as how it is implied through char.value property anyway?

codecov-io · 2018-04-22T06:55:38Z

Codecov Report

Merging #90 into dev will decrease coverage by <.01%.
The diff coverage is 57.14%.

@@            Coverage Diff             @@
##              dev      #90      +/-   ##
==========================================
- Coverage   50.94%   50.93%   -0.01%     
==========================================
  Files          14       14              
  Lines        1329     1337       +8     
  Branches      138      140       +2     
==========================================
+ Hits          677      681       +4     
- Misses        641      643       +2     
- Partials       11       13       +2

Impacted Files	Coverage Δ
pyhap/accessory_driver.py	`56.27% <0%> (ø)`	⬆️
pyhap/service.py	`95% <0%> (-5%)`	⬇️
pyhap/characteristic.py	`98.19% <80%> (-1.81%)`	⬇️

cdce8p

I had it a bit backwards. The value property you added should be the right way to go. Can you add the getter_callback to service.configure_char?
Left some additional comments as well.

cdce8p · 2018-04-22T13:13:10Z

pyhap/characteristic.py

+        :return: value
+        """
+        if not getattr(self, '_value', None):
+            self._value = self._get_default_value()


Why not leave that in __init__ with self._value = self.get_default_value()? Currently you do the check every time char.value is called, also it's only None for the first time.

cdce8p · 2018-04-22T13:15:29Z

pyhap/characteristic.py

+        if not getattr(self, '_value', None):
+            self._value = self._get_default_value()
+        if self.getter_callback:
+            self._value = self.to_valid_value(self.getter_callback())


Maybe change that to self._value = self.to_valid_value(value=self.getter_callback()).
By adding value= it might be a bit clearer what self.getter_callback() will return.

cdce8p · 2018-04-22T13:17:41Z

pyhap/characteristic.py

        except ValueError:
-            self.value = self._get_default_value()
+            self.set_value(self._get_default_value(), should_notify=False)


override_properties shouldn't call set_value. It would only trigger unnecessary calls, like the log output.
Just use:

try: self._value = self.to_valid_value(self.value) except ValueError: self._value = self._get_default_value()

cdce8p · 2018-04-22T13:19:07Z

pyhap/characteristic.py

+                     self.display_name, value)
+        # Set new value before anything else, set_value calls notify
+        self.set_value(value)
+        # Call setter_callback


The same here. It would be quite confusion if a client_update_value call cause a set_value log output as well.
Instead use:

self._value = value self.notify()

cdce8p · 2018-04-22T14:28:30Z

tests/test_characteristic.py

-        char.value = 'aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeee' \
-            'ffffffffffgggggggggg'
+        char.set_value('aaaaaaaaaabbbbbbbbbbccccccccccddddddddddeeeeeeeeee'
+                       'ffffffffffgggggggggg')


Personally I would prefer char._value = instead. That would make it a bit clearer what is being tested and you don't have to be that careful what is being mocked when.

ikalchev · 2018-04-22T20:04:52Z

I think a more straightforward solution would be to make a new interface for getting a value from iOS clients. The calls that get the char value to give to iOS is in AccessoryDriver.get_characteristics - currently it does only char.value. We can define a new method for Characteristic that gets the iOS value. Actually this was the idea of the recently-removed get_value. We can get it back and use that to do

def get_value(self):
    if self.getter_callback:
        return self.getter_callback()
    else:
        return value

However, we also need to use this when getting the value in the to_HAP.

What are your thoughts?

PS sorry for the late review!

jslay88 · 2018-04-23T04:14:32Z

Well, that's kind of the intent behind making value a property. That way, it is intuitive for get just by accessing char.value. Maybe I am over/under thinking it though. Using char.value we wouldn't have to go and update a bunch of code outside of characteristic.py, and not breaking everyone's source again going back to get_value

ikalchev

The thing is that you now need an odd-looking _value and, thus, you have two sources of truth about a chars value. The only place you would need to touch code outside of the char file is one call in the driver (mentioned before). Furthermore, IMO properties are something that can be retrieved fairly quickly - the getter might read from a sensor which is slow and could be unexpected by the user.

ikalchev · 2018-04-23T05:19:19Z

Another point: havingget_value and value makes it intuitive which is getting a fresh value and which gets the cached one (compare with value and _value)

cdce8p · 2018-04-23T21:28:51Z

The breaking change with using the property decorator wouldn't be that big of a problem, since the user interacts with set_value and not value.
I didn't though of the time aspect though and that makes sense.

What I don't quite get is, why get_value should be used in to_HAP. The way I image it at the moment is the following:

The driver methods to get the value of a char calls get_value
get_value updates the value internally for the char, it can return the new value
The driver checks if the value has changed. If so it either retrieves the new or uses the returned one to send an update to HomeKit.
In both cases char.value is equal to the current value.
to_HAP isn't called.

An additional argument for not including the get_value call in to_HAP is that separating those two things makes the conversion to async easier later.

ikalchev · 2018-04-25T05:53:59Z

I didn’t quite get point 3 - the driver does not check the value - it just returns it.

My argument about using get value in to HAP is that othwewise get_accessories and get_characteristics will have different semantics (one uses to hap, the other .value)

cdce8p · 2018-04-25T19:37:44Z

I didn’t quite get point 3 - the driver does not check the value - it just returns it.

I could have been a bit clearer. My idea for get_value was to implement a method on the service level to update the value regularly, something like the @Accessory.run_at_interval method at the moment. In that case the driver would only need to send and update to HomeKit if the value has changed between the last two calls.

My argument about using get value in to HAP is that othwewise get_accessories and get_characteristics will have different semantics (one uses to hap, the other .value)

I see. My idea was separation of get_value and to_HAP. While I'm thinking about it, do we really need to call get_value in either of those methods? For get_accessories sending the default values should be enough and get_characteristics could just get the current char value (without updating it before). That would play quite nicely with my idea above, to call get_value regularly for the accessory and then to push changes.

ikalchev · 2018-04-26T05:31:44Z

Basically I see two use cases for how the value is retrieved - you retrieve a cached value or get it anew, where the latter is achieved with setting the getter_callback. Your point about regularly refreshing the value falls into the first case and can be achieved using the already established approach with the Accessory.run method.

Having said that, I think it is flexible enough to have a get_value method (because the property does not make it obvious that there could be delays) and an optional getter_callback which is called within it. In this way, we leave it to the user to choose the drawback most suitable for her -(1) increased latency in to_HAP because of the getter_callback, but minimum interaction with the "device" or (2) fast to_HAP, because of no getter_callback, but increased overhead of regularly updating the value.

From the set of sensors I have, there are some which have limited lifetime and these should be "read from" as rarely as possible. Thus, I have a 20/30 min run_at_interval for them currently and would pretty much like a getter_callback. However, for others I have a 1/2 min interval which works fine as it is.

Thoughts?

cdce8p · 2018-04-26T20:54:25Z

Please correct me if I haven't understand something properly.

What it comes done to, as I think of it, is to use get_value only inside the Accessory.run method. Basically adding a helper method to make calling a function to retrieve a value easier. I don't see any real value in a callback in another situation.

In that case it might make sense to expand upon the current idea of using just:

def get_value():
    if self.getter_callback:
        self._value = self.to_valid_value(value=self.getter_callback())
    return self._value

and incorporate the notify call if the new value is different. Something like:

def get_value():
    if not self.getter_callback:
        raise ValueError('No getter_callback defined')
    value = self.to_valid_value(value=self.getter_callback())
    if value != self.value:
        self.value = value
        self.notify()

Maybe a renaming to getter_helper or something else would work as well.

jslay88 · 2018-04-28T06:48:17Z

What if we were to have a bool on the char to define the state's requirements.
There are some cases where a char should use the stored state, and there are some where it should ALWAYS pull a new state, even if it is the same state as stored. Then, we can have, instead of getter_callback, update_value_callback, or something else.

Either that or we leave the Accessory.run method in and those that want to use a polling period can use that, people that want to have a fresh value every time, can define getter_callback.

Just throwing more ideas out there. I really have no weight in which method we go with.

ikalchev · 2018-04-29T17:15:39Z

@cdce8p I think getting a value should not trigger a notify, because the "gotten" value will be deliver to iOS anyway. Getter callback is intuitive for me - method to be called when a get happens.

@jslay88 I think the getter should decide on that internally, i.e. no need to expose the bool in the char.

Can we agree on:

def get_value():
    if self.getter_callback:
        self.value = self.to_valid_value(value=self.getter_callback())
    return self.value

with no _value and calling this in to_HAP and the driver methods? Do we have any pain points and disagreements left?

cdce8p · 2018-04-30T10:02:44Z

We can do that, although small changes for the async conversion might be necessary later.

ikalchev · 2018-04-30T18:03:07Z

@jslay88 Are you fine with the above?

jslay88 · 2018-04-30T18:05:43Z

Sure thing. Will make the changes here shortly.

…

On Mon, Apr 30, 2018 at 12:03 Ivan Kalchev ***@***.***> wrote: @jslay88 <https://github.com/jslay88> Are you fine with the above? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#90 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF36UdZz40d_S-1azX0BUxMXomb2wfWmks5tt1HcgaJpZM4TXk6o> .

cdce8p

Added a few comments. There are some changes from #99 that need to be done.

cdce8p · 2018-04-30T23:47:13Z

pyhap/characteristic.py

        """
-        logger.debug('client_update_value: %s to %s',
-                      self.display_name, value)
+        logger.debug('%s: Client update value to %s',


I changed the log statement in one of the last merged PR #99

cdce8p · 2018-04-30T23:47:54Z

pyhap/characteristic.py

+        logger.debug('%s: Client update value to %s',
+                     self.display_name, value)
+        # Set new value before anything else, then notify client
+


This and the comment below shouldn't be necessary.

cdce8p · 2018-04-30T23:56:15Z

tests/test_characteristic.py

@@ -202,3 +202,162 @@ def test_from_dict():
    assert char.display_name == 'Test Char'
    assert char.type_id == uuid
    assert char.properties == {'Format': 'int', 'Permissions': 'read'}
+
+class TestCharacteristic(unittest.TestCase):


All tests in the TestCharacteristic class got changed in #99 as well. They are now outside, so you should be able to remove the class.

ikalchev

Don't forget that AccessoryDriver.get_characteristics now needs to call get_value instead. Also, get_value should be called in to_HAP as well.

ikalchev · 2018-05-01T06:39:15Z

pyhap/characteristic.py

@@ -114,6 +115,11 @@ def _get_default_value(self):
            value = HAP_FORMAT_DEFAULTS[self.properties[PROP_FORMAT]]
            return self.to_valid_value(value)

+    def get_value(self):


Documentation stating that this calls the getter if any to update the .value

ikalchev · 2018-05-07T16:59:31Z

@jslay88 any issues with this?

jslay88 · 2018-05-07T17:23:50Z

No, I just haven't had the chance to really get time to make the changes. Will try and see if I can get around to it tonight. Sorry for the delay.

…

On Mon, May 7, 2018 at 10:59 AM Ivan Kalchev ***@***.***> wrote: @jslay88 <https://github.com/jslay88> any issues with this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#90 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AF36UfW7CUjjrWYX6hgehCDXy7zd7mR3ks5twH10gaJpZM4TXk6o> .

ikalchev · 2018-05-07T17:24:33Z

Cool, no rush, just checking

jslay88 · 2018-05-07T17:37:48Z

I blasted away at it, I think I got everything requested between @ikalchev and @cdce8p.

Please double check, as I was quickly rushing through this.

ikalchev · 2018-05-07T17:57:06Z

pyhap/characteristic.py

@@ -221,10 +236,10 @@ def to_HAP(self):
            hap_rep.update({k: self.properties[k] for k in
                            self.properties.keys() & PROP_NUMERIC})
        elif self.properties[PROP_FORMAT] == HAP_FORMAT_STRING:
-            if len(self.value) > 64:
-                hap_rep[HAP_REPR_MAX_LEN] = min(len(self.value), 256)
+            if len(self.get_value()) > 64:


Maybe do value = self.get_value() at the beginning of the method, otherwise you could potentially call the callback two times in this if

Yup, thanks for checking. Pushed changes.

cdce8p

Just two small comments (see below).
Can you update the changelog as well?

cdce8p · 2018-05-07T22:06:33Z

pyhap/characteristic.py

        self.value = value
        self.notify()
+        # Call setter_callback


Can you please revert the changes to client_update_value? I don't think they are necessary for get_value.

cdce8p · 2018-05-07T22:07:35Z

pyhap/characteristic.py

@@ -217,14 +232,16 @@ def to_HAP(self):
            HAP_REPR_FORMAT: self.properties[PROP_FORMAT],
        }

+        value = self.get_value()
+


A small one: value is used in the block below, so why not delete the empty line?

cdce8p · 2018-05-08T10:24:15Z

@jslay88 Take a look at the current diff. Especially the debug log statement for client_update_value should stay.

jslay88 · 2018-05-08T21:34:23Z

So do you want the logger in client_update_value or not? You requested to have it removed, and now are wanting it back?

This and the comment below shouldn't be necessary.

cdce8p · 2018-05-08T21:43:07Z

I see. The comment earlier was meant for # Set new value before anything else, then notify client and # Call setter_callback, not the logger statement. I should have been more precise, sorry about that.

jslay88 · 2018-05-08T21:48:53Z

Done

cdce8p · 2018-05-08T21:57:55Z

Thanks! The code looks good IMO.
If you could do the last finishing touch and add this change to CHANGELOG.md that would be really great. You might need to rebase to avoid merge conflicts, before you do that.

jslay88 · 2018-05-08T22:06:31Z

Done

ikalchev · 2018-05-09T13:32:59Z

Great work, thanks for addressing all comments!

jslay88 changed the base branch from master to dev April 17, 2018 03:25

Added getter_callback to Characteristic

9d9a240

Rebased to Dev, changed getter_callback handling.

4148873

jslay88 added 2 commits April 22, 2018 01:34

Added docstrings

6c46754

Fixed typo in docstring

e9b16f7

cdce8p reviewed Apr 22, 2018

View reviewed changes

Made requested changes

cece383

ikalchev reviewed Apr 23, 2018

View reviewed changes

jslay and others added 2 commits April 30, 2018 13:44

Made agreed upon changes.

9750db2

Merge branch 'dev' into getter_callback

7851399

cdce8p reviewed Apr 30, 2018

View reviewed changes

ikalchev requested changes May 1, 2018

View reviewed changes

Quick run at requested changes.

781aad9

ikalchev requested changes May 7, 2018

View reviewed changes

Made requested changes.

49a4e07

cdce8p reviewed May 7, 2018

View reviewed changes

Made requested changes

9bb6cf4

jslay88 added 2 commits May 8, 2018 15:45

Made requested changes

c1c4ade

4e7ae11

jslay88 added 2 commits May 8, 2018 16:05

Added PR #90 to CHANGELOG

9b2875d

Merge branch 'dev' into getter_callback

d2c628b

Fixed misplacement in CHANGELOG

a9d4c4e

ikalchev approved these changes May 9, 2018

View reviewed changes

ikalchev merged commit c434ea9 into ikalchev:dev May 9, 2018

Added getter_callback to Characteristic #90

Added getter_callback to Characteristic #90

Conversation

jslay88 commented Apr 17, 2018

cdce8p commented Apr 17, 2018

cdce8p commented Apr 17, 2018

jslay88 commented Apr 20, 2018

jslay88 commented Apr 22, 2018

codecov-io commented Apr 22, 2018 • edited Loading

Codecov Report

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p Apr 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikalchev commented Apr 22, 2018

jslay88 commented Apr 23, 2018 • edited Loading

ikalchev left a comment

Choose a reason for hiding this comment

ikalchev commented Apr 23, 2018

cdce8p commented Apr 23, 2018

ikalchev commented Apr 25, 2018

cdce8p commented Apr 25, 2018

ikalchev commented Apr 26, 2018

cdce8p commented Apr 26, 2018

jslay88 commented Apr 28, 2018 • edited Loading

ikalchev commented Apr 29, 2018

cdce8p commented Apr 30, 2018

ikalchev commented Apr 30, 2018

jslay88 commented Apr 30, 2018 via email

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikalchev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikalchev commented May 7, 2018

jslay88 commented May 7, 2018 via email

ikalchev commented May 7, 2018

jslay88 commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdce8p commented May 8, 2018

jslay88 commented May 8, 2018

cdce8p commented May 8, 2018

jslay88 commented May 8, 2018

cdce8p commented May 8, 2018

jslay88 commented May 8, 2018

ikalchev commented May 9, 2018

codecov-io commented Apr 22, 2018 •

edited

Loading

cdce8p Apr 22, 2018 •

edited

Loading

jslay88 commented Apr 23, 2018 •

edited

Loading

jslay88 commented Apr 28, 2018 •

edited

Loading