Skip to content
This repository has been archived by the owner on Dec 2, 2019. It is now read-only.

Add support for subscribed state updates and input commands #19

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

bendavid
Copy link

Add support for input commands (cursor actions and button presses), as well subscribed state updates to allow updating the state of the device without polling.

The implementation for the input commands is adapted from https://github.com/supersaiyanmode/PyWebOSTV

Supporting input commands and subscribed updates required migrating to a persistent connection model.

Usage of these new features in Home Assistant is implemented here
bendavid/home-assistant@2640a0d

…and migrate to purely async functions and persistant connections
@bendavid
Copy link
Author

(Just noticed #18 now. I think this includes all the functionality added there.)

@poroping
Copy link

poroping commented Nov 1, 2019

(Just noticed #18 now. I think this includes all the functionality added there.)

I think your way is probably neater, I just brute forced this on top.

@MadmanMonty
Copy link

Requires a version increment in setup.py post merge. I also suggest updating the documentations to raise awareness of the new functionality.

Excellant enhancement @bendavid

@MadmanMonty
Copy link

@bendavid my basic tests show your update breaks backwards compatibility.

In its simplest form, this test fails on your codebase due to the async changes:

from pylgtv import WebOsClient
import sys
web = WebOsClient('192.168.0.17')
web.volume_up()

returns:

RuntimeWarning: coroutine 'WebOsClient.volume_up' was never awaited
  web.volume_up()

Can I propose you raise two separate pull requests, one to add input command enhancements and one with any additional async proposed enhancements

@bendavid
Copy link
Author

The input command functionality doesn't make much sense without moving to persistent connections, and this is pretty intertwined with the async migration, so if I split it up in two like this, I would rather do it in the opposite order (one to add persistent connection handling and async migration, and a second to add input commands.)

For what concerns backwards compatibility, I intentionally dropped the synchronous wrapper functions to simplify things. I can bring them back if really desired (but it would be purely backwards compatible since once will still need to call connect manually)

If really really desired it would be possible to emulate the old behaviour by creating synchronous wrapper functions which call
connect
request
disconnect

(but I would prefer to avoid this, as handshaking on every command never made much sense anyways)

@MadmanMonty
Copy link

MadmanMonty commented Nov 16, 2019

Long term, I agree your move makes sense to async calls. However, doing it in an existing way breaks existing integrations and triggers what I would consider a major version change.

It would need to be called out in documentation they cannot take your update and gain the enhanced functionality without updating their integrations. - I think it might warrant documentation to support them in upgrading.

Your suggestion of synchronous wrappers would at least retain BC, with a documented recommendation to transition to the async, I agree perhaps very desirable with continuous connect/disconnect.

Alternative two release strategy, first sync with wrapper, which could be considered a minor release, with a second release that just has async, major version change, providing an upgrade path for existing integrations.

Thoughts?

@bendavid
Copy link
Author

So I already have changes ready to go for Home Assistant to move entirely to the async calls and persistent connections. I would agree this warrants a major version change, but I would be in favour of just moving forward.

If someone has a strong desire to use the new input command functionality without breaking compatibility then I can implement the wrappers though.

@MadmanMonty
Copy link

It appears we might both working on this library for the same reason. Do you have it working with home assistant?

I'm just reviewing your new media_player generic command function, do you have that working with this? - A pointer to a working example would be great ;-)

I took a slightly different angle, with an extended webostv having a new remote, leaving the existing media_player untouched.

@bendavid
Copy link
Author

Yes, working branch here:
https://github.com/bendavid/home-assistant/tree/home_theatre_control_0.100.2/homeassistant/components/webostv

I have a few more changes coming to more cleanly separate the notify and media_player platforms from the connection handling.

Currently I've added the generic command functionality within media player, but given Home Assistant developer's preferences not to do that, I was leaning towards moving them into a dedicated "webostv" domain (similar to how this is handled for the kodi integration)

For the remote platform, my objection/concern is the duplication of state and turnon/turnoff functionality.

There's some discussion of this here
home-assistant/architecture#299 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants