Define the turn on/off action for media player Kodi with scripts #8408
Conversation
| @@ -75,12 +86,17 @@ | |||
| vol.Optional(CONF_PORT, default=DEFAULT_PORT): cv.port, | |||
| vol.Optional(CONF_TCP_PORT, default=DEFAULT_TCP_PORT): cv.port, | |||
| vol.Optional(CONF_PROXY_SSL, default=DEFAULT_PROXY_SSL): cv.boolean, | |||
| vol.Optional(CONF_TURN_ON_ACTION, default=None): vol.In(TURN_ON_ACTION), | |||
There was a problem hiding this comment.
We should not try to be smart. We should just support the Script syntax. They can do WoL if they want, or call anything else. Use cv.SCRIPT_SCHEMA for validation.
Once validates as SCRIPT_SCHEMA, just make it a script like this: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/alexa.py#L125-L126 and call that during turn_on
There was a problem hiding this comment.
Ok, so I'll remove the wake_on_lan option of the turn_on_action list, and the related parameters (mac and wake_on_lan_broadcast). I did it with the spirit to simplify the original reason of #8240, which was to implement WOL to turn Kodi on, and do it like the existent list of posible actions to turn it off.
Do you want me to remove this list and validate directly turn_on_action as cv.SCRIPT_SCHEMA? I thought of that validation for the new params script_on and script_off (instead of cv.entity_id), but writing an entity name sounded cleaner for me than write the script syntax inside the Kodi media player yaml config.
And doing so, what happens with the new option script to turn Kodi off?
I could change the script_on and script_off parameters to cv.SCRIPT_SCHEMA and maintain the turn_on_action list.
Other reason to implement explicitly the WOL option was as proof of concept for the change of WOL dependent platforms, because I'm calling the wake_on_lan setup if needed but not present. This way, the user don't need to define the wake_on_lan: component if using WOL from any platform, so no future breaking changes in these. What do you think, @balloob?
There was a problem hiding this comment.
I've changed entity names for cv.SCRIPT_SCHEMA validation, but the symmetry lover that I am makes me keep the turn_on_action list, like the existent turn_off_action list, and ables to implement the script_off action as well, as one more option to the existent list, so no breaking change.
How do you see it now, @balloob?
There was a problem hiding this comment.
I agree with @balloob here. The big difference is that turn_off is running methods available from the Kodi API, whereas turn_on is just running actions that are otherwise available from hass.
I think the asymmetry is just a necessary side-effect of the fact that we have to try and turn on a Kodi instance we can't interact with directly. It's still better than the current universal media player workaround.
|
Ok @armills, @balloob, so, if I understand correctly, you prefer this: media_player:
- platform: kodi
host: 192.168.1.XX
turn_on_action:
- service: wake_on_lan.send_magic_packet
data:
mac: B8:27:EB:BD:A4:28
broadcast_address: 192.168.255.255
- service: persistent_notification.create
data:
message: 'Turn on Kodi with Wake On LAN'
turn_off_action: quitAnd no media_player:
- platform: kodi
host: 192.168.1.XX
turn_on_action: script
script_on:
- service: wake_on_lan.send_magic_packet
data:
mac: B8:27:EB:BD:A4:28
broadcast_address: 192.168.255.255
- service: persistent_notification.create
data:
message: 'Turn on Kodi with Wake On LAN'
turn_off_action: script
script_off:
- service: media_player.kodi_call_method
data:
entity_id: media_player.kodi
method: Addons.ExecuteAddon
addonid: script.json-cec
params:
command: standby
- service: homeassistant.turn_off
entity_id: group.restroom
- service: persistent_notification.create
data:
message: 'Turn off Kodi'Am I correct? I kinda like more the original proposal (with entity names or script_schema), but I'll do what you guys decide. |
|
Another thought: The |
|
So now that I'm looking at this more, a big reason the original What do you think about eliminating the selection list from |
|
@armills, you mean like this?: turn_off_action:
- service: media_player.kodi_call_method
data:
entity_id: media_player.kodi
method: System.HibernateI'll give it a try, but my first thought is that the user is going to see this as an ugly change, right? turn_off_action: hibernateI don't know... |
|
Yeah, that's how it would look with that proposal. It's certainly more verbose, but having the full flexibility of the script syntax there will be really powerful. I think if we didn't have any |
|
Too much verbose for my taste, but I'll do it if this is the preferred method. Where can I see a sample of the migration instructions for a breaking change like this, to document it properly? Because this will break almost all Kodi configs. |
|
There's a few ways to do it. The nicest way is to try and handle both configuration options temporarily so we can give users a deprecation warning if they're using the old setup, and eventually remove it. The breaking changes are usually documented in the release notes, and it could potentially be written up as it's own blog post as well. If you go through the history of https://home-assistant.io/blog/ you should be able to find some examples. It probably is also a good idea to recruit some second opinions from the dev chat, or wait to let @balloob weigh in. |
media_player:
- platform: kodi
host: 192.168.1.XX
turn_on_action:
- service: wake_on_lan.send_magic_packet
data:
mac: B8:27:EB:BD:A4:28
broadcast_address: 192.168.255.255
- service: persistent_notification.create
data:
message: 'Turn on Kodi with Wake On LAN'looks greate instead the origin from initial post. |
…version of old `turn_off_action` config and warning with instructions about deprecated mode
|
ok @armills, @balloob, @pvizeli , so I've changed that, removed the WOL option, and made I'm reading the old mode ( There is a little problem with the method that generates the equivalent script: it needs the entity name to make the script, and in setup it's not present (am I wrong?). I'm slugifying the Even so, this needs to be marked as breaking-change for the users to look at. |
|
Awesome work with the deprecation logic! As far as the entity_id problem, you could move the deprecation check into the |
|
Ok! Thanks, I didn't know about this |
|
@azogue you can pass the entity_id of calling device with variable to script and there you can use it with template like |
…tine, `async_on_quit` now is also a coroutine and triggers when turn_off script is executed
|
@pvizeli, sorry, I did the change that @armills suggested before you commented. And I didn't understand your message very well in the context of the script creation for the deprecated options... @armills, I moved the script creation and the deprecation check to BUT, when looking at the code, I remembered an issue the all-day running Kodi machines have with the HA implementation: we never see the So I made 2 commits, being the second (which I can reset for proposing other PR or simply forget about) an introduction of a internal flag to represent the media player state as a boolean, to make real (and more intuitive) the What do you think about? ** To use the Kodi media player as a switch using the turn on / off actions, a little fix in *** The WAF of this change is very high, in my case. One switch less, intuitive option to turn on/off the TV (the big standby button in the frontend), and direct Siri-HomeKit integration. |
|
It's not necessary. The history in the PR is nice to have, and we'll squash it with GitHub when merging into dev. |
|
ok, I'll edit the title and description anyway. |
azogue
changed the title
| def async_turn_on(self): | ||
| """Execute turn_on_action to turn on media player.""" | ||
| if self._turn_on_action is not None: | ||
| yield from self._turn_on_action.async_run() |
There was a problem hiding this comment.
add variable={"entity_id": self.entity_id} as param
There was a problem hiding this comment.
Ok, I got it, so, if changing the template in the warning to:
turn_off_action:
service: media_player.kodi_call_method
data_template:
entity_id: '{{ entity_id }}'
method: System.Suspendthere is no need for using the async_added_to_hass coroutine, isn't it? I'll move the script creation to the setup again
There was a problem hiding this comment.
Ups, no, I can't. Now I need the async_update_ha_state and async_on_quit entity methods to create the scripts... I'll simply change the template and move out the deprecation conversion
| """Execute turn_on_action to turn on media player.""" | ||
| if self._turn_on_action is not None: | ||
| yield from self._turn_on_action.async_run() | ||
| yield from self.async_update_ha_state(force_refresh=True) |
There was a problem hiding this comment.
There was a problem hiding this comment.
Ok, I'll do it as well with the async_on_quit call for the turn_off_action script, ok?
| self.hass, self._turn_on_action, "Kodi Turn ON action script") | ||
| if self._turn_off_action is not None: | ||
| # Deprecation check | ||
| if isinstance(self._turn_off_action, str): |
There was a problem hiding this comment.
I think we should make a hard breaking change. it is only a platform.
There was a problem hiding this comment.
Do you mean to disable Kodi for almost everyone with the update? I respectfully disagree. IMHO, the warning will be sufficiently explicit for the user to see it and will made the change easily by copy/pasting from the log, everything without the need to see a bad Kodi setup and restart after the obligated analysis. These are very precious minutes, for me at last.
I know the # of users is not in the equation here to decide this, but, off-topic-ing a little only because curiosity, do we now already some big numbers about the components and platforms penetration, from the stats collected by the updater component? I know it's so early for this, but I'm a very curious guy ;-)
There was a problem hiding this comment.
Agree with @azogue . Backwards compatibility wouldn't be a requirement to make this change, but if it's already written I'd prefer to keep it to make the transition smooth for everyone.
|
@pvizeli, about the internal bin state (already removed from this PR):
I don't own one of these, so maybe I don't understand the implementation, but, as I see, a differentiation is done between a I don't think it's the same problem, because Kodi running 24/7 attached to the TV doesn't know if the TV is turned on (can't pull that state from anywhere, as I know), so is always running and presenting the My idea was to simply mark a binary state to represent the state of turn_on/off actions, to set the 'off' state to perdure until the next turn_on, or if the Kodi state changes to play something. The implementation works for the web socket connection (which now perdures between HA on/off cycles by default), and for the polling mode of update, and because Kodi instances which really die on turn_off will appear like before, the change doesn't touch them. I'll present another PR with this |
There was a problem hiding this comment.
Got a chance to go over this in more detail. Great job handling all of the deprecation logic at setup time, so it doesn't spread through the rest of the code.
Just a few small requests here. It looks like some of the changes relating to the alternate state tracking are still here. Let's keep this PR limited to the action changes.
| @@ -249,9 +286,6 @@ def __init__(self, hass, name, host, port, tcp_port, encryption=False, | |||
| self._ws_server.Player.OnStop = self.async_on_stop | |||
| self._ws_server.Application.OnVolumeChanged = \ | |||
| self.async_on_volume_changed | |||
| self._ws_server.System.OnQuit = self.async_on_quit | |||
There was a problem hiding this comment.
Don't get rid of these handlers. They're still used to track the state of Kodi. This is probably part of the second PR.
| def async_on_quit(self, sender, data): | ||
| """Handle the muted volume.""" | ||
| @asyncio.coroutine | ||
| def async_on_quit(self): |
There was a problem hiding this comment.
Change this back to a callback. Docstring fix is a nice catch though. 😳
| turn_on_action = script.Script( | ||
| self.hass, turn_on_action, | ||
| "Kodi Turn ON action script", | ||
| partial(self.async_update_ha_state, force_refresh=True)) |
There was a problem hiding this comment.
Since async_update_ha_state is a coroutine, you shouldn't need to use partial here. You can just pass it as self.async_update_ha_state(True). It won't get executed until Script schedules the job.
| if turn_on_action is not None: | ||
| turn_on_action = script.Script( | ||
| self.hass, turn_on_action, | ||
| "Kodi Turn ON action script", |
There was a problem hiding this comment.
Do we want to incorporate the entity name here? Maybe even as simple as
"{} turn ON script".format(self.name)| if turn_off_action is not None: | ||
| turn_off_action = script.Script( | ||
| self.hass, _check_deprecated_turn_off(turn_off_action), | ||
| "Kodi Turn OFF action script", self.async_on_quit) |
There was a problem hiding this comment.
Don't include async_on_quit here. We want to issue the command and rely on Kodi to report the change instead of assuming it happens.
|
Seems travis doesn't want to run either. |
|
@pvizeli, Is there anything else you want me to change in this PR? |
|
I think @pvizeli's concerns have been addressed, but we do need to have Travis run before we can merge. You might need to open a new PR from this branch since it doesn't seem to be triggering here. |
|
Thanks! Looks like that one is working. |
ghost
added
Description:
Support for generic Home Assistant script sequences to be defined as
turn_on_actionandturn_off_actionparameters for the media player Kodi.Example entry for
configuration.yaml:For the old way of defining the
turn_off_action(quit,hibernate,suspend,reboot, andshutdown), a conversion to the script method is performed, and a deprecation warning is logged.Add turn_on and turn_off actions to the media player Kodi to be able to:- Wake on LAN a device withmedia_player/turn_on.- Run ascriptor apython_scriptwithmedia_player/turn_onormedia_player/turn_off.Pull request in home-assistant.github.io with documentation (if applicable): home-assistant/home-assistant.io#2951
Checklist:
If user exposed functionality or configuration variables are added/changed:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests passMore info
This PR is related to the previous work of @tadly in #8240, but with the new approach discussed there, with a new component
wake_on_lan(proposed in #8397) which is called from Kodi.Because it relies on thewake_on_lancomponent, which is not merged at this time, it can't work now (ImportErrors). If #8397 is not accepted, I will change this accordingly.The final approach has been to add the
turn_on_actionandturn_off_actionas generic script sequences, deprecating the old turn off options list.