Add support for Homekit battery service #14288

schmittx · 2018-05-04T17:41:51Z

Description:

Add battery service for entities with battery_level attribute.

Related issue (if applicable): N/A

Pull request in home-assistant.github.io with documentation (if applicable): N/A

Example entry for `configuration.yaml` (if applicable):

N/A

Checklist:

The code change is tested and works locally.
Local tests pass with tox. Your PR cannot be merged unless tests pass

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated in home-assistant.github.io

If the code does not interact with devices:

Tests have been added to verify that the new code works.

schmittx · 2018-05-04T20:13:42Z

@cdce8p I'm not sure how to handle ChargingState characteristic, there's no standard Home Assistant constant attribute (yet). My image is that the constants below would eventually be moved to Home Assistant global constants and other components would be updated to use them.

Any thoughts?

# #### BATTERY SERVICE ####
ATTR_CHARGING_STATE = 'charging_state'
ATTR_CHARGING_STATE_CHARGING = 'charging'
ATTR_CHARGING_STATE_NOT_CHARGING = 'not_charging'
ATTR_CHARGING_STATE_NOT_SUPPORTED = 'not_supported'

cdce8p

Which entities currently support a Charging State? The rule is that a new feature should be used at least once or it can't be added.

It might be necessary to harmonize the attribute first. That would start with a new issue in https://github.com/home-assistant/architecture.

Nevertheless I added a few first comments.

cdce8p · 2018-05-04T20:58:27Z

homeassistant/components/homekit/__init__.py

@@ -149,7 +149,8 @@ def get_accessory(hass, state, aid, config):
        return None

    _LOGGER.debug('Add "%s" as "%s"', state.entity_id, a_type)
-    return TYPES[a_type](hass, state.name, state.entity_id, aid, config=config)
+    return TYPES[a_type](hass, state.name, state.entity_id, state.attributes,
+                         aid, config=config)


Don't pass state.attributes. Fetch the new state from hass instead with hass.states.get(entity_id) inside the HomeAccessory.

cdce8p · 2018-05-04T20:59:17Z

homeassistant/components/homekit/accessories.py

+            serv_battery = self.add_preload_service(SERV_BATTERY_SERVICE)
+            self.char_battery = serv_battery.configure_char(
+                CHAR_BATTERY_LEVEL,
+                value=0)


This can go on one line.

cdce8p · 2018-05-04T21:00:11Z

homeassistant/components/homekit/accessories.py

+                value=0)
+            self.char_charging = serv_battery.configure_char(
+                CHAR_CHARGING_STATE,
+                value=HASS_TO_HOMEKIT[STATE_UNKNOWN])


value=2 is enough. This should get overridden immediately after the driver is started.

cdce8p · 2018-05-04T21:02:54Z

homeassistant/components/homekit/accessories.py

@@ -93,9 +117,24 @@ def update_state_callback(self, entity_id=None, old_state=None,
    def update_state(self, new_state):
        """Method called on state change to update HomeKit value.

-        Overridden by accessory types.
+        Update battery service if available.


Instead of using the update_state method, why not add a new one update_battery_level and call it before update_state in update_state_callback? That way you don't need to add super calls to every accessory type.

cdce8p · 2018-05-05T02:52:16Z

homeassistant/components/homekit/accessories.py

+        """Update battery service if available."""
+        battery_level = convert_to_float(new_state.attributes
+                                         .get(ATTR_BATTERY_LEVEL))
+        if battery_level is not None:


You could reduce the indent level for the rest of the function by using

if battery_level is None: return if battery_level <=1: # an so on

Good catch, done.

cdce8p · 2018-05-05T02:54:28Z

homeassistant/components/homekit/accessories.py

+        battery_level = convert_to_float(new_state.attributes
+                                         .get(ATTR_BATTERY_LEVEL))
+        if battery_level is not None:
+            if battery_level <= 1:


Why is this necessary? The battery_level should be reported in the same range by all components. If not the component/platform needs to be fixed.

Valid point, I was trying to make it robust but I believe you're correct. Fixed.

cdce8p · 2018-05-05T02:56:20Z

homeassistant/components/homekit/const.py

+ATTR_CHARGING_STATE = 'charging_state'
+ATTR_CHARGING_STATE_CHARGING = 'charging'
+ATTR_CHARGING_STATE_NOT_CHARGING = 'not_charging'
+ATTR_CHARGING_STATE_NOT_SUPPORTED = 'not_supported'


Those should go inside homeassistant.const to be accessible for other platforms and components.

Removed charging state tracking.

schmittx · 2018-05-06T05:16:07Z

@cdce8p I agree that the charging state attribute architecture needs to be harmonized first and then affected components updated. So, I propose to set ChargingState characteristic to NotSupported as a fixed value for now. Once the charging state attribute is in place, then this can be updated to provide dynamic updates.

cdce8p · 2018-05-06T08:21:22Z

@schmittx I would advice you to propose the final solution directly otherwise I don't see much chance in getting it through once something is implemented.

schmittx · 2018-05-11T04:13:42Z

I added an attribute in homeassistant/const.py as discussed in home-assistant/architecture#25

cdce8p · 2018-05-12T14:36:29Z

tests/components/homekit/test_type_covers.py

@@ -31,7 +31,8 @@ def cls(request):
 async def test_garage_door_open_close(hass, cls):
    """Test if accessory and HA are updated accordingly."""
    entity_id = 'cover.garage_door'
-
+    hass.states.async_set(entity_id, None, {})
+    await hass.async_block_till_done()


If have extracted those changes to #14416, since this might take a bit until it's merged.

cdce8p

For the time being we should set this PR to WIP.

cdce8p · 2018-05-20T20:56:59Z

homeassistant/components/homekit/accessories.py

+    def update_battery_level(self, new_state):
+        """Update battery service if available."""
+        battery_level = convert_to_float(new_state.attributes
+                                         .get(ATTR_BATTERY_LEVEL))


battery_level = convert_to_float( new_state.attributes.get(ATTR_BATTERY_LEVEL))

schmittx · 2018-06-12T04:15:37Z

@cdce8p Sorry for the long delay on this. We finally got the dev docs updated to define standard attributes so this should be ready for final review(s). The sister PR to define ATTR_BATTERY_CHARGING in dev docs is here.

cdce8p · 2018-06-17T11:58:14Z

homeassistant/components/homekit/accessories.py

@@ -85,8 +100,32 @@ def update_state_callback(self, entity_id=None, old_state=None,
        _LOGGER.debug('New_state: %s', new_state)
        if new_state is None:
            return
+        self.hass.async_add_job(self.update_battery_level, new_state)


It doesn't make sense to always call self.update_batter_level only to exit moments later since it isn't support by that entity. Instead define a flag during __init__ and check if batter is supported before calling it.

cdce8p · 2018-06-17T12:00:08Z

homeassistant/components/homekit/accessories.py

+        self.char_low_battery.set_value(battery_level < 20)
+        _LOGGER.debug('%s: Updated battery level to %d', self.entity_id,
+                      battery_level)
+        self.hass.async_add_job(self.update_battery_charging, new_state)


This won't work!
update_bettery_level is a sync function and async_add_job must be called from an async context. If you still want to do it that way, use add_job instead. Even better would be to just combine the to methods.

cdce8p · 2018-06-17T12:02:59Z

homeassistant/components/homekit/accessories.py

+        """Update battery service if available."""
+        charging = new_state.attributes.get(ATTR_BATTERY_CHARGING)
+        if charging is None:
+            hk_charging = 2


If an entity is not chargeable the first time, it won't be any time later. Maybe another flag?

cdce8p · 2018-06-17T12:06:34Z

tests/components/homekit/test_accessories.py

+    await hass.async_block_till_done()
+
+    acc = HomeAccessory(hass, hk_driver, 'Battery Service',
+                        entity_id, 2, None)


It fits on one line. Same below

cdce8p · 2018-06-17T17:20:46Z

homeassistant/components/homekit/accessories.py

@@ -69,12 +69,15 @@ def __init__(self, hass, driver, name, entity_id, aid, config,
        self.entity_id = entity_id
        self.hass = hass
        self.debounce = {}
+        self._flag = {SERV_BATTERY_SERVICE: False,
+                      ATTR_BATTERY_CHARGING: True}


A dictionary for two values is probably a bit much.
What do you think of self._support_battery_level and self._support_charging?

cdce8p · 2018-06-17T17:22:46Z

homeassistant/components/homekit/accessories.py

        """Update battery service if available."""
        battery_level = convert_to_float(
            new_state.attributes.get(ATTR_BATTERY_LEVEL))
-        if battery_level is None:
-            return


battery_level could still be None, e.g. if conversion fails. Might be better to leave it in.

cdce8p · 2018-06-17T17:23:27Z

homeassistant/components/homekit/accessories.py

-        self.char_charging.set_value(hk_charging)
-        _LOGGER.debug('%s: Updated battery charging to %d', self.entity_id,
-                      hk_charging)
+        if self._flag[ATTR_BATTERY_CHARGING]:


Instead of indenting all, just exit with return if condition is false.

cdce8p · 2018-06-17T17:23:43Z

homeassistant/components/homekit/accessories.py

+            charging = new_state.attributes.get(ATTR_BATTERY_CHARGING)
+            if charging is None:
+                self._flag[ATTR_BATTERY_CHARGING] = False
+                hk_charging = 2


Same here: Exit with return.

cdce8p · 2018-06-17T17:25:12Z

homeassistant/components/homekit/accessories.py

+        """Add battery service if available"""
+        battery_level = self.hass.states.get(self.entity_id).attributes \
+            .get(ATTR_BATTERY_LEVEL)
+        if battery_level:


Also here: Exit with return.

cdce8p · 2018-06-17T18:30:03Z

I did some final touches. Mainly changing the chars to private. Although this doesn't effect anything, it might be warning not to use them. Will merge it, once tests pass.

schmittx requested a review from cdce8p as a code owner May 4, 2018 17:41

homeassistant added cla-signed integration: homekit labels May 4, 2018

schmittx closed this May 4, 2018

schmittx force-pushed the homekit-battery-service branch from b56628c to 7900ba3 Compare May 4, 2018 17:57

homeassistant added the cla-signed label May 4, 2018

schmittx reopened this May 4, 2018

homeassistant added the cla-signed label May 4, 2018

cdce8p self-assigned this May 4, 2018

cdce8p requested changes May 4, 2018

View reviewed changes

homeassistant added the cla-signed label May 5, 2018

cdce8p reviewed May 5, 2018

View reviewed changes

schmittx mentioned this pull request May 6, 2018

RFC: Battery Charging attribute home-assistant/architecture#25

Closed

schmittx force-pushed the homekit-battery-service branch from b7700f3 to a1caea0 Compare May 11, 2018 02:25

homeassistant added the cla-signed label May 11, 2018

schmittx requested a review from a team as a code owner May 11, 2018 04:11

homeassistant added the cla-signed label May 11, 2018

schmittx changed the title ~~[WIP] Homekit battery service~~ Homekit battery service May 11, 2018

schmittx force-pushed the homekit-battery-service branch from 7e8ddb1 to 40eb66a Compare May 12, 2018 04:30

homeassistant added the cla-signed label May 12, 2018

cdce8p reviewed May 12, 2018

View reviewed changes

schmittx force-pushed the homekit-battery-service branch from 6b3e6f1 to cd59a0d Compare May 13, 2018 14:49

homeassistant added the cla-signed label May 13, 2018

cdce8p requested changes May 20, 2018

View reviewed changes

schmittx changed the title ~~Homekit battery service~~ [WIP] Homekit battery service May 20, 2018

schmittx closed this Jun 12, 2018

schmittx force-pushed the homekit-battery-service branch from f463b6a to 576c806 Compare June 12, 2018 02:59

homeassistant added the cla-signed label Jun 12, 2018

Initial commit

730ddaf

schmittx reopened this Jun 12, 2018

homeassistant added the cla-signed label Jun 12, 2018

ghost added the in progress label Jun 12, 2018

schmittx mentioned this pull request Jun 12, 2018

Define battery charging standard attribute home-assistant/developers.home-assistant#25

Merged

schmittx changed the title ~~[WIP] Homekit battery service~~ Add support for Homekit battery service Jun 12, 2018

cdce8p requested changes Jun 17, 2018

View reviewed changes

Combine battery and charging update into single method

01de86a

homeassistant added the cla-signed label Jun 17, 2018

cdce8p reviewed Jun 17, 2018

View reviewed changes

schmittx and others added 2 commits June 17, 2018 13:38

Style update and change flag dict

03d1956

Final changes

f48d7ea

cdce8p approved these changes Jun 17, 2018

View reviewed changes

cdce8p merged commit 1533bc1 into home-assistant:dev Jun 17, 2018

ghost removed the in progress label Jun 17, 2018

schmittx deleted the homekit-battery-service branch July 5, 2018 15:36

balloob mentioned this pull request Jul 6, 2018

0.73 #15330

Merged

home-assistant locked and limited conversation to collaborators Dec 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Homekit battery service #14288

Add support for Homekit battery service #14288

schmittx commented May 4, 2018 •

edited

Loading

schmittx commented May 4, 2018

cdce8p left a comment

cdce8p May 4, 2018

cdce8p May 4, 2018

cdce8p May 4, 2018

cdce8p May 4, 2018

cdce8p May 5, 2018

schmittx May 6, 2018

cdce8p May 5, 2018

schmittx May 6, 2018

cdce8p May 5, 2018

schmittx May 6, 2018

schmittx commented May 6, 2018

cdce8p commented May 6, 2018

schmittx commented May 11, 2018

cdce8p May 12, 2018

cdce8p left a comment

cdce8p May 20, 2018

schmittx commented Jun 12, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p Jun 17, 2018

cdce8p commented Jun 17, 2018

Add support for Homekit battery service #14288

Add support for Homekit battery service #14288

Conversation

schmittx commented May 4, 2018 • edited Loading

Description:

Example entry for configuration.yaml (if applicable):

Checklist:

schmittx commented May 4, 2018

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schmittx commented May 6, 2018

cdce8p commented May 6, 2018

schmittx commented May 11, 2018

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

schmittx commented Jun 12, 2018

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

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

Choose a reason for hiding this comment

cdce8p commented Jun 17, 2018

schmittx commented May 4, 2018 •

edited

Loading

Example entry for `configuration.yaml` (if applicable):