Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
3 changes: 3 additions & 0 deletions .coveragerc
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,9 @@ omit =
homeassistant/components/toon.py
homeassistant/components/*/toon.py

homeassistant/components/tplink_lte.py
homeassistant/components/*/tplink_lte.py

homeassistant/components/tradfri.py
homeassistant/components/*/tradfri.py

Expand Down
50 changes: 50 additions & 0 deletions homeassistant/components/notify/tplink_lte.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
"""TP-Link LTE platform for notify component.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/notify.tplink_lte/
"""

import logging

import attr

from homeassistant.components.notify import (
ATTR_TARGET, BaseNotificationService)

from ..tplink_lte import DATA_KEY

DEPENDENCIES = ['tplink_lte']

_LOGGER = logging.getLogger(__name__)


async def async_get_service(hass, config, discovery_info=None):
"""Get the notification service."""
if discovery_info is None:
return
return TplinkNotifyService(hass, discovery_info)


@attr.s
class TplinkNotifyService(BaseNotificationService):
"""Implementation of a notification service."""

hass = attr.ib()
config = attr.ib()

async def async_send_message(self, message="", **kwargs):
"""Send a message to a user."""
import tp_connected
modem_data = self.hass.data[DATA_KEY].get_modem_data(self.config)
if not modem_data:
_LOGGER.error("No modem available")
return

phone = self.config[ATTR_TARGET]
targets = kwargs.get(ATTR_TARGET, phone)
if targets and message:
for target in targets:
try:
await modem_data.modem.sms(target, message)
except tp_connected.Error:
_LOGGER.error("Unable to send to %s", target)
127 changes: 127 additions & 0 deletions homeassistant/components/tplink_lte.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
"""
Support for TP-Link LTE modems.

For more details about this component, please refer to the documentation at
https://home-assistant.io/components/tplink_lte/
"""
import asyncio
import logging

import aiohttp
import attr
import voluptuous as vol

from homeassistant.components.notify import ATTR_TARGET
from homeassistant.const import (
CONF_DEVICES, CONF_DISCOVERY, CONF_HOST, CONF_NAME, CONF_PASSWORD,
EVENT_HOMEASSISTANT_STOP)
from homeassistant.helpers import config_validation as cv, discovery
from homeassistant.helpers.aiohttp_client import async_create_clientsession

REQUIREMENTS = ['tp-connected==0.0.4']

_LOGGER = logging.getLogger(__name__)

DOMAIN = 'tplink_lte'
DATA_KEY = 'tplink_lte'

ATTR_TARGETS = "targets"

DEFAULT_DISCOVERY = True

_TARGET_SCHEMA = vol.All(vol.Schema({
vol.Optional(CONF_NAME): cv.string,
vol.Required(ATTR_TARGET): vol.All(cv.ensure_list, [cv.string]),
}))

CONFIG_SCHEMA = vol.Schema({
DOMAIN: vol.All(cv.ensure_list, [vol.Schema({
vol.Required(CONF_HOST): cv.string,
vol.Required(CONF_PASSWORD): cv.string,
vol.Optional(CONF_DISCOVERY, default=DEFAULT_DISCOVERY): cv.boolean,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where is this used?

vol.Required(CONF_DEVICES): {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we change to notify as @amelchio suggested?

vol.Optional(ATTR_TARGETS): vol.All(
cv.ensure_list, [_TARGET_SCHEMA]),
},
})])
}, extra=vol.ALLOW_EXTRA)


@attr.s
class ModemData:
"""Class for modem state."""

modem = attr.ib()


@attr.s
class LTEData:
"""Shared state."""

websession = attr.ib()
modem_data = attr.ib(init=False, factory=dict)

def get_modem_data(self, config):
"""Get the requested or the only modem_data value."""
if CONF_HOST in config:
return self.modem_data.get(config[CONF_HOST])
if len(self.modem_data) == 1:
return next(iter(self.modem_data.values()))

return None


async def async_setup(hass, config):
"""Set up TP-Link LTE component."""
if DATA_KEY not in hass.data:
websession = async_create_clientsession(
Comment thread
MartinHjelmare marked this conversation as resolved.
hass, cookie_jar=aiohttp.CookieJar(unsafe=True))
hass.data[DATA_KEY] = LTEData(websession)

tasks = [_setup_lte(hass, conf) for conf in config.get(DOMAIN, [])]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Move default list to config schema.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Could you please be more specific or give me some indication to other components that do this?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we can just use dict[key] here instead. The domain key will be required to set up the component so we shouldn't need any defaults here or in the schema. Sorry for my confusion.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was mistaken. We don't yet validate that the 'tplink_lte' key for the component is present in the config. Since that is required for a functioning integration, we should check that early and fail component setup if the config isn't ok.

Maybe it would make sense to move the whole config to the component and set up the notify platform via discovery instead. This will releave the user from having to juggle multiple config sections.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For the netgear_lte component I opted out of discovery because I saw no way to set things like notify name and sensor scan_interval. Was I mistaken?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think devices is odd here, isn't the host the device?

@andtos90 andtos90 Nov 2, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@amelchio you're right, the host is the physical device but I'm not sure about the right solution here, something like this is enough?

tplink_lte:
  - host: 192.168.5.1
    password: *********
    targets:
      - name: sms1
        target: "+3955555555"
      - name: sms2
        target: "+3955555555"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure myself. But once we decide, I want to do this for netgear_lte as well :)

I was thinking about using the component name of the platform that we setup ... so notify and at least netgear_lte will also need configuration for sensor...

netgear_lte:
  - host: 192.168.5.1
    password: *********
    notify:
      - name: sms1
        target: "+3955555555"
      - name: sms2
        target: "+3955555555"
    sensor:
      - usage
      - sms

This has some precedence, for example with the Cast component media_player configuration. But I am starting to wonder whether this is actually an architectural question. It would be really good to have guidelines for this quite common situation.

BTW @andtos90, it's awesome to see you keep positive during that rather tough review 👍

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I like this solution but should it be sensors instead of sensor? Just to know for future developments.

Your team is doing a great work and in the process I'm learning a lot, thanks for your work 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not sure and there are no rules :-/

We create sensor entities, that's why I picked singular. I agree that it looks a bit odd if one reads it just as yaml.

if tasks:
await asyncio.wait(tasks)

for conf in config.get(DOMAIN, []):

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We could store the domain config in a variable early since we use it above too.

for notify_conf in conf[CONF_DEVICES].get(ATTR_TARGETS):
discovery.load_platform(hass, 'notify', DOMAIN, notify_conf, conf)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Create tasks with hass.async_create_task and use discovery.async_load_platform.

hass.async_create_task(discovery.async_load_platform(
    hass, 'notify', DOMAIN, notify_conf, config))

Note that we need to send the original whole config as last argument to load platform.


return True


async def _setup_lte(hass, lte_config, delay=0):
"""Set up a TP-Link LTE modem."""
import tp_connected

try:
if delay:
await asyncio.sleep(delay)
except asyncio.CancelledError:
return

host = lte_config[CONF_HOST]
password = lte_config[CONF_PASSWORD]

websession = hass.data[DATA_KEY].websession
modem = tp_connected.Modem(hostname=host, websession=websession)

try:
await modem.login(password=password)
except asyncio.CancelledError:
return
except tp_connected.Error:
delay = max(15, min(2*delay, 300))
_LOGGER.warning("Retrying %s in %d seconds", host, delay)
task = hass.loop.create_task(_setup_lte(hass, lte_config, delay))
return

modem_data = ModemData(modem)
hass.data[DATA_KEY].modem_data[host] = modem_data

async def cleanup(event):
"""Clean up resources."""
if task is not None and not task.done():

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Init task to None in the beginning of _setup_lte. Otherwise there could be an error.

task.cancel()
await modem.logout()
Comment thread
MartinHjelmare marked this conversation as resolved.
Outdated

hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, cleanup)

@MartinHjelmare MartinHjelmare Nov 3, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we need to move this up after creating the retry setup task and before returning. Otherwise we will never register the listener until we've succeeded with setup.

We should also remove a potential existing listener, by calling the return value of the registration, each time before registering a new one.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI, I added the task cleanup to netgear_lte now: #18163

My proposal avoids removal of the existing listener by running a loop. I found that simpler to get working.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I like that!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@amelchio thanks for the hint, I reused your solution with the addition of two CancelledError exception handlers, hope that are useful there

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe catching CancelledError is only necessary if we want to do something, like release resources.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As @MartinHjelmare pointed out in a older commit with a CancelledError handler we avoid to print a not useful stack trace in some cases. Does homeassistant loop handle this for us?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry, I think @amelchio is correct. Eg:

In [22]: import asyncio

In [23]: async def running_task():
    ...:     while True:
    ...:         print('running')
    ...:         print('sleeping')
    ...:         await asyncio.sleep(5)
    ...:         

In [24]: def task_canceller(task):
    ...:     print('in task_canceller')
    ...:     task.cancel()
    ...:     print('canceled the task')
    ...:     
    ...:     

In [25]: async def main(loop):
    ...:     print('creating task')
    ...:     task = loop.create_task(running_task())
    ...:     loop.call_soon(task_canceller, task)
    ...:     print('sleeping 2 secs in main')
    ...:     await asyncio.sleep(2)
    ...:     print('finished main')
    ...:     
    ...:     

In [26]: loop = asyncio.new_event_loop()

In [27]: try:
    ...:     loop.run_until_complete(main(loop))
    ...: finally:
    ...:     loop.close()
    ...:     
creating task
sleeping 2 secs in main
running
sleeping
in task_canceller
canceled the task
finished main

3 changes: 3 additions & 0 deletions requirements_all.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1469,6 +1469,9 @@ toonlib==1.0.2
# homeassistant.components.alarm_control_panel.totalconnect
total_connect_client==0.18

# homeassistant.components.tplink_lte
tp-connected==0.0.4

# homeassistant.components.device_tracker.tplink
tplink==0.2.1

Expand Down