Implemented tplink_lte components and notify service via SMS

[andtos90]

Description:

A new component to handle TP-Link LTE routers. The only feature implemented is a notify service to send SMS trough the the router. I plan to add new features like router status and SMS inbox.

This component has a lot of similarities with the netgear_lte component and my code is heavily based on it. I think that the 2 components could be merged together as an abstract "LTE modem" handler. I'm going to find out where is the best place to talk about this proposal but for now I think that two separate components is the right solution.

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

Example entry for configuration.yaml (if applicable):

tplink_lte:
  - host: 192.168.1.1
    password: secret

notify:
  - platform: tplink_lte
    name: sms
    target: "+393405555555"

Checklist:

• The code change is tested and works locally.
• Local tests pass with tox. Your PR cannot be merged unless tests pass
  Tox with virtualenv seems to be broken in my local environment, I'll figure out a solution but for now I'm going to check the results on Travis

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

• Documentation added/updated in home-assistant.io

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.

[homeassistant]

Hi @andtos90,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

[houndci-bot]

'homeassistant.util.Throttle' imported but unused

[andtos90]

Docs updated

[houndci-bot]

blank line contains whitespace

[MartinHjelmare]

At least move it out of the loop. Usually we put imports in the beginning of the method.

[andtos90]

You're right, totally missed this

[andtos90]

@amelchio I'm not skilled at Python so maybe I miss something here. There's any reason why this import is placed inside the loop? (that's the same code as netgear_lte notify)

I would rather place the import between the line 51 and 52 or the begging of the method as suggested by @MartinHjelmare

[amelchio]

No reason, @MartinHjelmare is right.

[MartinHjelmare]

Please sort 🔡 within groups standard library, 3rd party and homeassistant imports.

[MartinHjelmare]

@amelchio is the author of the netgear lte component.

[andtos90]

@MartinHjelmare Travis completed successfully the job but it's still show as pending here

[MartinHjelmare]

I've restarted a short job to see if we can get a response from Travis upon completion.

[MartinHjelmare]

We should probably cancel the _setup_lte task, that we create on failure, upon home assistant stop event and catch CancelledError here and around modem.login. Otherwise there will be a stacktrace when closing home assistant if we're still in this delay loop.

[andtos90]

So, if I understand it correctly:

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

and

try:
    await modem.login(password=password)
except asyncio.CancelledError:
    return

should do the thing, right?

[MartinHjelmare]

We also need to store the task that is created on failure so we can register a listener to home assistant stop event that can call a function that calls task.cancel if the task isn't done.

[andtos90]

The updated code I would write is:

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

and

try:
    task = await modem.login(password=password)
    await task
except asyncio.CancelledError:
    return

and cancel the task inside the cleanup function

[MartinHjelmare]

I don't think that works. Coroutines don't return tasks by default. We have to use a function from the asyncio library to wrap a coroutine in a task.
https://docs.python.org/3/library/asyncio-task.html#asyncio.Task

I would just save the task that is returned here:
https://github.com/home-assistant/home-assistant/pull/17111/files#diff-5410e9b3cb190ba123d66d6583931ad4R93

[andtos90]

Oh yes you're right about coroutines but I'm not sure about storing only the task from here: https://github.com/home-assistant/home-assistant/pull/17111/files#diff-5410e9b3cb190ba123d66d6583931ad4R93

Doesn't mean that we'll miss the first iteration for each modem setup? I'm talking about the tasks created here:
https://github.com/home-assistant/home-assistant/pull/17111/files#diff-5410e9b3cb190ba123d66d6583931ad4R68

[MartinHjelmare]

We won't miss that since those tasks are already tracked by the home assistant core. There is also no delay for those tasks. So unless modem.login takes a very long time to return or error, we should be fine.

[andtos90]

I've nothing to add, it should be ok now

[MartinHjelmare]

Please don't squash commits after review has started. Squashing makes it hard to follow the changes. Now I have to read all the code again before I can approve.

We squash commits upon merge.

[MartinHjelmare]

Move default list to config schema.

[andtos90]

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

[MartinHjelmare]

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.

[MartinHjelmare]

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.

[amelchio]

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?

[amelchio]

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

[andtos90]

@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"

[amelchio]

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 👍

[andtos90]

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 😄

[amelchio]

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.

[MartinHjelmare]

This will error if task doesn't exist, eg if there was never a connection error. Also probably check that task is not done before cancelling it.

[MartinHjelmare]

Target is required in the config schema so use dict[key].

[MartinHjelmare]

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.

[MartinHjelmare]

Where is this used?

[MartinHjelmare]

Should we change to notify as @amelchio suggested?

[MartinHjelmare]

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

[MartinHjelmare]

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

[MartinHjelmare]

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.

[amelchio]

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.

[MartinHjelmare]

I like that!

[andtos90]

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

[amelchio]

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

[andtos90]

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?

[MartinHjelmare]

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

[MartinHjelmare]

We shouldn't warn the first time we login if we never failed to login. It's ok to log at info level the first time if we never failed.

[MartinHjelmare]

Looks good! Last fix and we can merge. Ok with you @amelchio?

[MartinHjelmare]

Define CONF_NOTIFY in this component. Config keys should be stored in constants called CONF_*.

[andtos90]

Do you mean just rename _NOTIFY_SCHEMA to CONF_NOTIFY or something else?

[amelchio]

Replace SERVICE_NOTIFY with a locally defined CONF_NOTIFY.

[amelchio]

👍

[MartinHjelmare]

Great! Can be merged when build passes.

[andtos90]

Thanks! 🎉