Wake on LAN for UI-configured Samsung TVs

[OnFreund]

Proposed change

When configuring a Samsung TV through YAML, you can define an action to turn it on. This configuration is not available through the UI (for either manually initiated or through discovery).
Since I assume that in most cases the turn-on action would be wake on lan (and for more advanced cases one can use a universal media player), this PR adds the ability to set a MAC address through an options flow, which will be used to turn the TV on through wake on lan.

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

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

• Documentation added/updated for www.home-assistant.io

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

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

[probot-home-assistant]

Hey there @escoand, mind taking a look at this pull request as its been labeled with a integration (samsungtv) you are listed as a codeowner for? Thanks!

[OnFreund]

@escoand this PR is just a placeholder and has no real functionality yet. If you think this is the right approach, I'll go ahead and complete it.

[OnFreund]

@escoand let me know what you think. I'll happily complete this if you think this direction makes sense.

[frenck]

@OnFreund Can you put the PR into "Draft" in the mean time?

There is a "Convert to draft" in the reviewers section in the sidebar of this PR.

[escoand]

@OnFreund Interesting, never used such a options flow. How to use/test this?

[OnFreund]

Once I implement this, it will be available as a gear button on the integration page. The demo integration has a functioning example that doesn't require special hardware.

[escoand]

Nice, would love to see this. Best would be something like the automation editor UI (my TV is not able to wake-on-lan), but let's start with the MAC.

[OnFreund]

The UI in options flow is very limited (same as config flow). We could potentially have a field for service name, and a field for json arguments, but there will be no autocompletion, and high possibility of misconfiguration.

[OnFreund]

@escoand this is the implementation I had in mind. There's one weird test failure that I can't understand, and a small section I'm not sure how to test.

[escoand]

Is there a cv type for mac address validation? Your current implementation accepts everything if I'm right.

[OnFreund]

Unfortunately there isn't. According to home-assistant/architecture#222 there also might never be.

[escoand]

No validation?

[OnFreund]

The only risk here is a malformed MAC address, which is no different than a wrong MAC address. This is an advanced feature, and the input will most likely be pasted (unless obtained automatically). The wrong/malformed address will fail quickly and be easy to fix. Given all of the above, I don't think the validation overhead is worth it.
I've also quickly searched through the code base and couldn't find an integration that validates mac addresses.

[OnFreund]

@escoand I think the request review button isn't working for this PR.
Mind taking another look?

[escoand]

Have not tested it, but it looks good in theory.

[OnFreund]

Thanks. I tested it locally and it's working well.

I still don't understand that test failure, though. @MartinHjelmare any chance you can help with that?

[OnFreund]

thanks @MartinHjelmare your suggestion fixed it.

@escoand in order to simulate this and test this case locally, I added a config entry with an invalid hostname. The logs are now full of:

2020-04-17 23:38:11 ERROR (MainThread) [homeassistant.helpers.entity] Update for media_player.dummy_samsung_tv fails
Traceback (most recent call last):
  File "/Users/onfreund/dev/personal/home-assistant/lib/python3.7/site-packages/websocket/_http.py", line 143, in _get_addrinfo_list
    hostname, port, 0, 0, socket.SOL_TCP)
  File "/usr/local/Cellar/python/3.7.5/Frameworks/Python.framework/Versions/3.7/lib/python3.7/socket.py", line 748, in getaddrinfo
    for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Users/onfreund/dev/personal/home-assistant/homeassistant/helpers/entity.py", line 279, in async_update_ha_state
    await self.async_device_update()
  File "/Users/onfreund/dev/personal/home-assistant/homeassistant/helpers/entity.py", line 476, in async_device_update
    await self.hass.async_add_executor_job(self.update)
  File "/usr/local/Cellar/python/3.7.5/Frameworks/Python.framework/Versions/3.7/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/Users/onfreund/dev/personal/home-assistant/homeassistant/components/samsungtv/media_player.py", line 140, in update
    self._state = STATE_ON if self._bridge.is_on() else STATE_OFF
  File "/Users/onfreund/dev/personal/home-assistant/homeassistant/components/samsungtv/bridge.py", line 66, in is_on
    return self._get_remote() is not None
  File "/Users/onfreund/dev/personal/home-assistant/homeassistant/components/samsungtv/bridge.py", line 256, in _get_remote
    self._remote.open()
  File "/Users/onfreund/dev/personal/home-assistant/lib/python3.7/site-packages/samsungtvws/remote.py", line 146, in open
    sslopt=sslopt
  File "/Users/onfreund/dev/personal/home-assistant/lib/python3.7/site-packages/websocket/_core.py", line 514, in create_connection
    websock.connect(url, **options)
  File "/Users/onfreund/dev/personal/home-assistant/lib/python3.7/site-packages/websocket/_core.py", line 223, in connect
    options.pop('socket', None))
  File "/Users/onfreund/dev/personal/home-assistant/lib/python3.7/site-packages/websocket/_http.py", line 113, in connect
    hostname, port, is_secure, proxy)
  File "/Users/onfreund/dev/personal/home-assistant/lib/python3.7/site-packages/websocket/_http.py", line 154, in _get_addrinfo_list
    raise WebSocketAddressException(e)
websocket._exceptions.WebSocketAddressException: [Errno 8] nodename nor servname provided, or not known

Might be worth seeing if this could be more gracefully handled.

[escoand]

@OnFreund how did you add an invalid hostname? The config flow in the UI should fail. Have you edited the config entry?

[OnFreund]

Yes, I edited the config entry. It's an edge case, but it could happen, if for example, the IP changed.

[escoand]

I don't have statistics over wol-able TVs but from the issues I had seen there are quite much which aren't. And from my point of view I don't like the idea of automatically created automations. Let's discover the mac address and push it along. But AFAIK this is impossible in some situations (e.g. VLAN), so we will end up with an unreliable solution.

[elupus]

I think it would be better with a WOL switch entity, that will discover the Mac of a device on setup (config flow). That way the automation just need to trigger the switch.

[escoand]

@elupus And whats the difference between the switch and the de-facto switch when adding the trigger? I would say the second is even better because you don't have an additional switch entity which is actually NOT a switch but just a trigger.

[elupus]

Just the MAC address lookup. By a standard config entry, the MAC address can be deduced during config and stored. If unable to ARP find it, you can manually enter it.

WOL can work between VLAN if the broadcast domain of the subnet the other device is on is known.

However, if the MAC address is known by this integration by some other mean rather than ARP (sometimes exposed in metadata and/or SSDP discovery). Exposing hostname/ip as well as MAC in the event would allow hookup to a standard WOL service without a separate switch.

My point is really, i don't think this integration should try to deduce MAC, if the only reason is WOL. Separation of concerns.

[OnFreund]

I think that users expect the ability to turn the device on to be supported without too much configuration. WOL is a common way to achieve that (probably the common way, by far, and also the one that's currently detailed in the docs), so I would think we should lower the barrier to using it, which means automatically obtaining the mac address.
Separate VLAN is a less interesting use case - if you're sophisticated enough to have HA and your TV on separate VLANs, you're a power user, and you can figure it out. My concern is the layman user.

[escoand]

I was wondering how we could avoid all the "power button is not working" issues. What do you think of creating a default-deactivated automation and give a hint in the config flow that's maybe possible and the user has to try and activate/drop it?

The solution with the extra switch entity is the least preferred one.

[OnFreund]

I like this (are there precedents of config flows creating automations?) as it reduces the friction to creating a functioning turn-on implementation in the common case. However, If I understand correctly, you would still have a "power button is not working" situation, it would just be a bit more explicit.

I'm not sure we can have the cake and eat it too - the event based solution has a lot of pros, but has a clear unmitigated con in the form of the power button not working unless setup. If we go with it, we need to live with the consequences.

[escoand]

Yes, we could still have this issues, but when removing yaml some day, every user has the change to read this hint.

One other thing which comes to my mind: we have to setup wakeonlan as well.

[stale]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

[OnFreund]

Seems like we have one solution that is generic and configurable (device trigger), and one that has a better user experience (SUPPORT_TURN_ON implemented correctly, automatic mac detection).
Before I give up, I want to make another proposal, which I believe combines the best of both worlds:

• The options flow will ask for the entity id of a script
• The existence of that entity id will turn on SUPPORT_TURN_ON
• When turning on, that script will be called with a parameter - the entity id of the media player
• In order to simply the common case of wake on lan, the config flow will detect the mac address (the device needs to be on during the config flow anyway), and the media player entity will expose it as an attribute. This also means that a single script can easily support multiple TVs
• For already existing config entries, a single attempt to obtain the mac will be made when the device switches state to on. If it fails, no more attempts will be made

This approach is fully generic and configurable, while also implementing SUPPORT_TURN_ON correctly, and simplifies the common wake on lan case. The only down side, is that it's slightly more complex than a device trigger (need to define a script with a parameter, and make sure you spell its entity id correctly in the options flow).
@escoand what do you think?

[escoand]

Sorry for the delay, forgot to answer.

Your proposal is probably working, but as you already mentioned it's a bit a hacky for the user. Your only problem with the device trigger is the missing support flag, right? The UI is already able to list every use of an entity. Couldn't we use this discovery to add SUPPORT_TURN_ON?

[escoand]

Could we additionally init a new automation edit screen as done from the device page and add a wake-on-lan call?

[OnFreund]

Your proposal is probably working, but as you already mentioned it's a bit a hacky for the user

I wouldn't call it "hacky". Yes, the initial setup is a bit more complicated, but ongoing functionality is better, I believe, for two reasons:

1. A single script can support multiple TVs. You can achieve that with device triggers as well, but I believe it'll be more complicated.
2. As you mentioned - SUPPORT_TURN_ON is implemented correctly with the script case. To implement correctly with an automation, you first have to find a way to list all of the uses (which, as you mentioned, is possible, since the UI does it), but then you have to figure out the actual use and whether it's for turning on, which is probably the complicated part.

[OnFreund]

Anyway, I'm more than happy to tackle the "mac address as attribute" part. I'll defer to you on how to actually handle the turning on part.

[stale]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

[OnFreund]

@escoand let me know if you'd like me to tackle the "mac address as attribute" part

[escoand]

Yeah, do like you want. I haven't more or less to decide than any other. I've just added myself as codeowner to be informed not because I think the code is my own...

[OnFreund]

Yes, but eventually you'll be tagged for a review, so I want to make sure you're not categorically against it before I invest time :)

[MartinHjelmare]

I suggest making an issue in our architecture repo first.

[OnFreund]

Fair enough. I also have an idea how to make it more generic.
Will open an issue in the arch repo as soon as I can move forward with some of my other open PRs/Issues - trying to limit my WIP queue :)

[stale]

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

[OnFreund]

Fair enough. I also have an idea how to make it more generic.

The idea I had in mind is actually very similar to the new automation variables in 0.115.