pylgtv icon indicating copy to clipboard operation
pylgtv copied to clipboard

Add support for subscribed state updates and input commands

Open bendavid opened this issue 5 years ago • 9 comments

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 https://github.com/bendavid/home-assistant/commit/2640a0debca892659bfcc423fe0dea9d51059fc3

bendavid avatar Oct 27 '19 22:10 bendavid

(Just noticed https://github.com/TheRealLink/pylgtv/pull/18 now. I think this includes all the functionality added there.)

bendavid avatar Oct 27 '19 22:10 bendavid

(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.

poroping avatar Nov 01 '19 16:11 poroping

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 avatar Nov 13 '19 13:11 MadmanMonty

@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

MadmanMonty avatar Nov 16 '19 12:11 MadmanMonty

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)

bendavid avatar Nov 16 '19 12:11 bendavid

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?

MadmanMonty avatar Nov 16 '19 16:11 MadmanMonty

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.

bendavid avatar Nov 16 '19 16:11 bendavid

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.

MadmanMonty avatar Nov 16 '19 16:11 MadmanMonty

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 https://github.com/home-assistant/architecture/issues/299#issuecomment-546714758

bendavid avatar Nov 16 '19 17:11 bendavid