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

Add config entry for SimpliSafe #17148

Merged
merged 23 commits into from Oct 12, 2018

Conversation

@bachya
Contributor

bachya commented Oct 5, 2018

Description:

This PR adds a config entry for SimpliSafe (and in so doing, converts the integration to a component).

Breaking Changes:

  1. There is a new configuration format (outlined below).
  2. Instead of defaulting each system's name to "SimpliSafe", the address of the system is used.

Assistance Needed:

When the config entry is deleted, the alarm_control_panel does not get removed; instead, it stays behind until HASS is restarted. I'm not sure why and would love some extra eyes. CC @balloob

Related issue (if applicable): N/A

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#6543

Example entry for configuration.yaml (if applicable):

simplisafe:
  accounts:
    - username: user1@email.com
      password: password
    - username: user2@email.com
      password: password

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:

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.
if account[CONF_USERNAME] in configured_instances(hass):
continue
hass.async_add_job(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

Use hass.async_create_task.

_LOGGER.debug('Logging in with credentials')
except SimplipyError as err:
_LOGGER.error("There was an error during setup: %s", err)
return

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

Return False.

return await self._show_form()
if user_input[CONF_USERNAME] in configured_instances(self.hass):
return await self._show_form({'base': 'identifier_exists'})

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

Use CONF_USERNAME key instead of base to mark the relevant form entry in the GUI.

_LOGGER.info('Refresh token expired; using credentials')
simplisafe = await API.login_via_credentials(
username, password, websession)
else:

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

When could this happen?

This comment has been minimized.

@bachya

bachya Oct 5, 2018

Contributor

Great point: it can't. 😆

return self.async_create_entry(
title=user_input[CONF_USERNAME],
data=user_input,

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

I don't think we want to store both username and password in the config entry. We should keep the login process here in the config flow. The username and token is probably ok to store.

This comment has been minimized.

@balloob

balloob Oct 5, 2018

Member

Correct, we should never store password if we can have a token.

token_data['refresh_token'], websession)
_LOGGER.debug('Logging in with refresh token')
except SimplipyError:
_LOGGER.info('Refresh token expired; using credentials')

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

This should be caught in the config flow already.

@bachya

This comment has been minimized.

Contributor

bachya commented Oct 5, 2018

@MartinHjelmare Changes in place; additionally, I'd appreciate your thoughts on my original comment:

When the config entry is deleted, the alarm_control_panel does not get removed; instead, it stays behind until HASS is restarted. I'm not sure why and would love some extra eyes.

@bachya bachya referenced this pull request Oct 5, 2018

Merged

Add docs for upcoming SimpliSafe component #6543

2 of 2 tasks complete

@bachya bachya changed the title from WIP: Add config entry for SimpliSafe to Add config entry for SimpliSafe Oct 5, 2018

self._code = str(code) if code else None
self._name = name
self._code = code
self._dispatch_remove = None

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

This doesn't need to be an instance attribute. It can be a local variable in async_added_to_hass.

async def async_added_to_hass(self):
"""Register callbacks."""
self._dispatch_remove = async_dispatcher_connect(

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

Just save the remove function in a local variable.

hass.data[DOMAIN][DATA_CLIENT].pop(entry.entry_id)
remove_listener = hass.data[DOMAIN][DATA_LISTENER].pop(entry.entry_id)
remove_listener()

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

Forward entry unload to all platforms here. That will unload the alarm_control_panel platform.

See deconz component for example.

This comment has been minimized.

@bachya

bachya Oct 5, 2018

Contributor

I've added that code in my latest commit; the alarm control panel still doesn't get removed.

FYI: I've noticed that recently (in the last version or so), my OpenUV config entry has the same issue (and didn't previously).

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

Is it the entity that is still left?

This comment has been minimized.

@bachya

bachya Oct 5, 2018

Contributor

Correct: the config entry (thus, the component) goes away, but the alarm control panel entity remains.

This comment has been minimized.

@balloob

balloob Oct 10, 2018

Member

Any errors in the logs?

This comment has been minimized.

@bachya

bachya Oct 10, 2018

Contributor

Negative, unfortunately. A reboot of HASS clears away the alarm control panel, but even then, the logs don’t show anything. I’ll trace the unload calls and see if I can figure out where the break occurs.

This comment has been minimized.

@bachya

bachya Oct 10, 2018

Contributor

No break that I can see. The only strangeness (to me) are these logs (with some of my debugging statements in place):

1. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.core] Removing entity: alarm_control_panel.1234_main_street
2. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.core] Firing event
3. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.core] Bus:Handling <Event state_changed[L]: entity_id=alarm_control_panel.1234_main_street, old_state=<state alarm_control_panel.1234_main_street=disarmed; code_format=None, changed_by=None, alarm_active=False, temperature=65, friendly_name=1234 Main Street @ 2018-10-10T09:57:57.711751-06:00>, new_state=None>
4. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.components.websocket_api.http.connection.4559956960] Sending {'id': 2, 'type': 'event', 'event': {'event_type': 'state_changed', 'data': {'entity_id': 'alarm_control_panel.1234_main_street', 'old_state': <state alarm_control_panel.1234_main_street=disarmed; code_format=None, changed_by=None, alarm_active=False, temperature=65, friendly_name=1234 Main Street @ 2018-10-10T09:57:57.711751-06:00>, 'new_state': None}, 'origin': 'LOCAL', 'time_fired': datetime.datetime(2018, 10, 10, 15, 58, 14, 595742, tzinfo=<UTC>), 'context': {'id': '1fb3f29315b44ed68b00afd99e2b3b17', 'user_id': None}}}
5. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.config_entries] Unload result is True
6. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.config_entries] Unload result is True
7. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.config_entries] state adjusted
8. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.core] Bus:Handling <Event state_changed[L]: entity_id=alarm_control_panel.1234_main_street, old_state=None, new_state=<state alarm_control_panel.1234_main_street=disarmed; code_format=None, changed_by=None, alarm_active=False, temperature=65, friendly_name=1234 Main Street @ 2018-10-10T09:58:14.621691-06:00>>
9. 2018-10-10 09:58:14 DEBUG (MainThread) [homeassistant.components.websocket_api.http.connection.4559956960] Sending {'id': 2, 'type': 'event', 'event': {'event_type': 'state_changed', 'data': {'entity_id': 'alarm_control_panel.1234_main_street', 'old_state': None, 'new_state': <state alarm_control_panel.1234_main_street=disarmed; code_format=None, changed_by=None, alarm_active=False, temperature=65, friendly_name=1234 Main Street @ 2018-10-10T09:58:14.621691-06:00>}, 'origin': 'LOCAL', 'time_fired': datetime.datetime(2018, 10, 10, 15, 58, 14, 621708, tzinfo=<UTC>), 'context': {'id': '8442566714d3438fb7b00f58fc0a5739', 'user_id': None}}}

From what I'm reading:

  • Line 1: homeassistant.core.async_remove is called with the correct entity ID.
  • Lines 2-4: The state_changed event is fired (with an old_state of whatever the alarm is currently set at and a new_state of None).
  • Lines 5-6: component.async_unload_entry is called twice (with the result being True both times).
  • Line 7: hass.config_entries.state is set to not_loaded.
  • Line 8: Another state_changed event is fired (with an old_state of None and a new_state of whatever the alarm is currently set at).

I'm not the expert on this subsystem, but it seems like there might be two areas of strangeness:

  1. Should component.async_unload_entry be called twice?
  2. Why does the second state change occur at the end? Does a non-None state somehow keep the entity around?

Apologies I'm not more famliar. Happy to help debugging if you can direct me.

This comment has been minimized.

@balloob

balloob Oct 11, 2018

Member

Check which component is used for generating the unload result.

This comment has been minimized.

@bachya

bachya Oct 11, 2018

Contributor
2018-10-11 09:08:58 DEBUG (MainThread) [homeassistant.config_entries] Unload called from <module 'homeassistant.components.simplisafe' from '/Users/abach/Git/home-assistant/homeassistant/components/simplisafe/__init__.py'>
2018-10-11 09:08:58 DEBUG (MainThread) [homeassistant.config_entries] Unload called from <module 'homeassistant.components.alarm_control_panel' from '/Users/abach/Git/home-assistant/homeassistant/components/alarm_control_panel/__init__.py'>

This comment has been minimized.

@bachya

bachya Oct 12, 2018

Contributor

FYI, tracking this issue separately: #17370.

Show resolved Hide resolved homeassistant/components/simplisafe/__init__.py Outdated
"""Register callbacks."""
listener = async_dispatcher_connect(
self.hass, TOPIC_UPDATE, self._update_data)
self.async_on_remove(listener)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Oct 5, 2018

Member

Looks like we should define async_will_remove_from_hass instead and call the function to remove the listener from that.

This comment has been minimized.

@bachya

bachya Oct 9, 2018

Contributor

I gave this a shot:

async def async_added_to_hass(self):
    """Register callbacks."""
    @callback
    def update(self):
        """Update the state."""
        self.async_schedule_update_ha_state(True)

    self._async_unsub_dispatcher_connect = async_dispatcher_connect(
        self.hass, TOPIC_UPDATE, update)

async def async_will_remove_from_hass(self) -> None:
    """Disconnect dispatcher listener when removed."""
    if self._async_unsub_dispatcher_connect:
        self._async_unsub_dispatcher_connect()
        self._async_unsub_dispatcher_connect = None

...but that yielded a crazy exception:

Traceback (most recent call last):
  File "/Users/abach/.local/share/virtualenvs/home-assistant-E4Cz_ciq/lib/python3.7/site-packages/aiohttp/web_protocol.py", line 390, in start
    resp = await self._request_handler(request)
  File "/Users/abach/.local/share/virtualenvs/home-assistant-E4Cz_ciq/lib/python3.7/site-packages/aiohttp/web_app.py", line 366, in _handle
    resp = await handler(request)
  File "/Users/abach/.local/share/virtualenvs/home-assistant-E4Cz_ciq/lib/python3.7/site-packages/aiohttp/web_middlewares.py", line 106, in impl
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/static.py", line 66, in staticresource_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/real_ip.py", line 34, in real_ip_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/ban.py", line 66, in ban_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/auth.py", line 68, in auth_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/real_ip.py", line 34, in real_ip_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/ban.py", line 66, in ban_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/auth.py", line 68, in auth_middleware
    return await handler(request)
  File "/Users/abach/Git/home-assistant/homeassistant/components/http/view.py", line 113, in handle
    result = await result
  File "/Users/abach/Git/home-assistant/homeassistant/components/config/config_entries.py", line 69, in delete
    result = await hass.config_entries.async_remove(entry_id)
  File "/Users/abach/Git/home-assistant/homeassistant/config_entries.py", line 392, in async_remove
    entity_registry.async_clear_config_entry(entry_id)
  File "/Users/abach/Git/home-assistant/homeassistant/helpers/entity_registry.py", line 254, in async_clear_config_entry
    self._async_update_entity(entity_id, config_entry_id=None)
  File "/Users/abach/Git/home-assistant/homeassistant/helpers/entity_registry.py", line 196, in _async_update_entity
    new.update_listeners.remove(ref)
ValueError: list.remove(x): x not in list

This comment has been minimized.

@bachya

bachya Oct 10, 2018

Contributor

Got it!

from .const import (
DATA_CLIENT, DATA_FILE_SCAFFOLD, DATA_LISTENER, DOMAIN, TOPIC_UPDATE)
CONF_TOKEN_FILE = 'token_file'

This comment has been minimized.

@balloob

balloob Oct 8, 2018

Member

Not used.

async def async_setup_entry(hass, config_entry):
"""Set up OpenUV as config entry."""

This comment has been minimized.

@balloob

balloob Oct 8, 2018

Member

stale comment

return await self._show_form({'base': 'invalid_credentials'})
config_data = {'refresh_token': simplisafe.refresh_token}
await self.hass.async_add_executor_job(

This comment has been minimized.

@balloob

balloob Oct 8, 2018

Member

No. This should be stored in the config entry.

This comment has been minimized.

@bachya

bachya Oct 8, 2018

Contributor

I'll need to save new refresh tokens periodically – I'm fine with saving that in the config entry, but I thought I technically wasn't allowed to modify a config entry?

This comment has been minimized.

@bachya

bachya Oct 8, 2018

Contributor

Just pushed 2cd6bf9; let me know if that's in line with what you're thinking.

This comment has been minimized.

@balloob

balloob Oct 9, 2018

Member

hass.config_entries.async_update_entry 👍

@balloob

This comment has been minimized.

Member

balloob commented Oct 9, 2018

Few small comments, almost there 🎉

@bachya

This comment has been minimized.

Contributor

bachya commented Oct 9, 2018

Thanks, @balloob. Addressed your latest comments. Two additional things I'd love your thoughts on:

1. I attempted to implement @MartinHjelmare's comment, but as you can see, I get a wild exception – thoughts? I got it.
2. I'm still having the issue described here (and cannot figure out why 😠).

@callback
def _async_save_refresh_token(hass, config_entry, token):
config_entry.data[CONF_TOKEN] = token

This comment has been minimized.

@balloob

balloob Oct 12, 2018

Member

So this is not allowed. You need to create a new dictionary, than pass the updated dictionary to async_update_entry.

bachya added some commits Oct 12, 2018

if 403 in str(err):
_LOGGER.error('Invalid credentials provided')
return False

This comment has been minimized.

@houndci-bot

houndci-bot Oct 12, 2018

blank line contains whitespace

hass.config_entries.async_update_entry(
config_entry, data=config_entry.data)
config_entry,
data={

This comment has been minimized.

@balloob

balloob Oct 12, 2018

Member

This also works and is less likely to break in the future: {**config_entry.data, CONF_TOKEN: token}

@balloob balloob merged commit 401e22f into home-assistant:dev Oct 12, 2018

5 checks passed

Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 93.524%
Details

@wafflebot wafflebot bot removed the in progress label Oct 12, 2018

@balloob

This comment has been minimized.

Member

balloob commented Oct 12, 2018

🎉

@bachya bachya deleted the bachya:ss-config-entry branch Oct 12, 2018

@balloob balloob referenced this pull request Oct 26, 2018

Merged

0.81 #17809

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment