Skip to content

Service improvements#84

Merged
ikalchev merged 4 commits intoikalchev:devfrom
cdce8p:service
Apr 15, 2018
Merged

Service improvements#84
ikalchev merged 4 commits intoikalchev:devfrom
cdce8p:service

Conversation

@cdce8p
Copy link
Copy Markdown
Contributor

@cdce8p cdce8p commented Apr 14, 2018

Small improvements to the service class (see commit descriptions):

  • Cleanup service class (add_characteristic, to_HAP)
  • Removed iid_manager from to_HAP

RFC: I would suggest to add a helper method setup_char. This would simplify the process of configuring a characteristic:

# Instead of
my_char = service.get_characteristic('MY_CHAR')
my_char.set_value(1, should_notify=False)  # To set a default value
my_char.override_properties(properties={'minValue': -2, 'maxValue': 5})
my_char.setter_callback = set_my_char

# Do this
my_char = service.setup_char(
    'MY_CHAR', properties={'minValue': -2, 'maxValue': 5},
    value=1, callback=set_my_char)

@ikalchev
Copy link
Copy Markdown
Owner

Change looks good. The RFC also looks good, but I would like to detach the callback from this method, because the callbacks are, most of the time, attached to the accessory, whereas you might want to configure the char in the loader or somewhere else.

For them, I was thinking of something like:

class MyAcc(AsyncAcc):
    @getter_for("TemperatureSensor.CurrentTemperature")
    def get_temp(self):
        pass

    @setter_for("TemperatureSensor.CurrentTemperature")
    def set_temp(self, value):
        pass

PS Do you think configure_char would be better?

cdce8p added 2 commits April 15, 2018 10:27
* Cleanup service (add_characteristic, to_HAP)
* Removed iid_manager from to_HAP
@cdce8p
Copy link
Copy Markdown
Contributor Author

cdce8p commented Apr 15, 2018

I changed the method name to configure_char.

Regarding the callback. Personally I would like to leave it in there. All parameter are optional, so if you decide to assign the callback later nothing prevents you from doing that. At Home Assistant I do the char configuration / setup in the accessory __init__ method, so it fits perfectly there.

Regarding the getter and setter methods: I thought about using them for the characteristic class refactoring, but decided against it. They limit what we can do and don't really allow us to separate the two setter cases that we have. IMO the current solution with set_value and client_update_value is quite fitting.

@ikalchev
Copy link
Copy Markdown
Owner

I agree. I think I didn't explain myself properly. My point is that in most cases you have

class ...
    def _set_services(self):
        ...
        char.setter_callback = self.turn_bulb

    def turn_bulb(self, value):
        pass

You can avoid the assignment and do it more explicit like:

class ...

    @setter_for("TemperatureSensor.CurrentTemperature")
    def turn_bulb(self, value):
        pass

Anyway, this can go in parallel with this change. Let's keep both ways for configuring the setters.

Copy link
Copy Markdown
Owner

@ikalchev ikalchev left a comment

Choose a reason for hiding this comment

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

Final change

pyhap/service.py Outdated
def to_HAP(self, iid_manager=None):
"""Create a HAP representation of this Service.
def configure_char(char_name, properties=None, valid_values=None,
value=None, callback=None):
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

can we change it to setter_callback, as we expect to have a getter_callback as well?

@ikalchev ikalchev merged commit 5d82211 into ikalchev:dev Apr 15, 2018
@cdce8p cdce8p deleted the service branch April 15, 2018 09:18
@cdce8p cdce8p mentioned this pull request Apr 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants