New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use web sockets for Harmony HUB #19440

Merged
merged 12 commits into from Dec 19, 2018

Conversation

@ehendrix23
Copy link
Contributor

ehendrix23 commented Dec 18, 2018

Description:

Use web sockets for Harmony HUB instead of XMPP. This also includes changing it to async.

Note, I submitted request for pyharmony to be updated as well that would increase the version number. iandday/pyharmony#21

Related issue (if applicable): fixes #

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

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

  • New or updated dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
Updates to Harmony for web sockets
Updates to harmony to use web sockets with async
Show resolved Hide resolved homeassistant/components/remote/harmony.py
Show resolved Hide resolved homeassistant/components/remote/harmony.py Outdated
Show resolved Hide resolved homeassistant/components/remote/harmony.py Outdated
Show resolved Hide resolved homeassistant/components/remote/harmony.py Outdated
@bachya

This comment has been minimized.

Copy link
Contributor

bachya commented Dec 19, 2018

In anticipation (and appreciation) of this PR and considering that HASS releases will take an official break until the new year (unless @balloob decides otherwise), I want to remind everyone of the procedure for slipstreaming this fix – once completed – into an active HASS install. Quite simply:

  1. Create a new, nested directory in your config directory: /config/custom_components/remote
  2. Download the harmony.py referenced in this PR; you can find it by going to Files changed, clicking on View file next to homeassistant/components/remote/harmony.py, clicking Raw, then save that file into the directory structure created above.
  3. Restart HASS.

This custom component will now override the built-in one. You will get a warning in the logs, but for this scenario, that can be safely ignored.

Once this fix is included in an official release, remember to delete the /config/custom_components/remote directory.

Fix send_command
Continued improvements:
-) Fixed send_command
-) Get HUB configuration during update in case it was not retrieved earlier (i.e. HUB unavailable)
@airdrummingfool

This comment has been minimized.

Copy link

airdrummingfool commented Dec 19, 2018

In addition to following @bachya's instructions, I had to make a few more changes to bring this fix into my current HA setup.

  1. Switch the version of pyharmony that HomeAssistant uses to @ehendrix23's fixed version:
    pip install git+https://github.com/ehendrix23/pyharmony.git@websockets --upgrade
    (I run HA in Docker, so I had to get a shell in the HA Docker container, install git with apk add git, then run the above command.)

  2. Edit line 24 in /config/custom_components/remote/harmony.py to change the required version back to 1.0.20, since @ehendrix23's PR doesn't change the version number.

  3. Restart HASS.

Now, my (admittedly very basic) Harmony integration is working again. Thank you @ehendrix23 and @bachya !

Note: to undo this, just switch back to iandday's pyharmony: pip install pyharmony --upgrade (and delete the /config/custom_components/remote/ directory as in @bachya's instructions).

ehendrix23 added some commits Dec 19, 2018

Further improvements
Completely removed dependency on __main__ for pyharmony, instead everything is now done from the HarmonyClient class.
Writing out Harmony configuration file as a JSON file.
Using same functionality to determine if activity provided is an ID or name for device, allowing send_command to receive a device ID or device name.
Point requirements to updated pyharmony repo
Updated REQUIREMENTS to point to repository containing the updates for pyharmony.
lint
lint
@ehendrix23

This comment has been minimized.

Copy link
Contributor

ehendrix23 commented Dec 19, 2018

@bachya @airdrummingfool I just pushed some more updates with fixes (i.e. send_command was not working).
There are also updates to pyharmony, but I updated REQUIREMENTS so that it should (note keyword should as I've never done this) be downloading it from my repository now with the changes.

Please let us know as well if you encounter any issues.

ehendrix23 and others added some commits Dec 19, 2018

Small fix for device and activity ID
Small fix in checking if provided device or activity ID is valid.
@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 19, 2018

After the changes that @ehendrix23 made, the comments by @airdrummingfool no longer apply. Just follow the instructions by @bachya and you should be good.

@balloob balloob added this to the 0.84.4 milestone Dec 19, 2018

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 19, 2018

Forked your repo @ehendrix23 so I could bump the version, because the part after the # in requirements need to contain the same version as is in the zip file, to prevent that Home Assistant will install this version on every reboot. However, it should not be the same as the previous version because that will not trigger an update of the package.

I do think that your websockets branch should be released as a new package named aioharmony and just be an async version. it's hard to keep something both sync and async compatible, and I already see breakages with things like sending multiple commands.

@balloob

This comment has been minimized.

Copy link
Member

balloob commented Dec 19, 2018

So I am making an exception for the pyharmony dependency to be based on a GitHub repo until the next release.

@balloob balloob merged commit 23a5794 into home-assistant:dev Dec 19, 2018

3 of 4 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA

@wafflebot wafflebot bot removed the in progress label Dec 19, 2018

balloob added a commit that referenced this pull request Dec 19, 2018

Use web sockets for Harmony HUB (#19440)
* Updates to Harmony for web sockets

Updates to harmony to use web sockets with async

* Lint

* Small fixes

* Fix send_command

Continued improvements:
-) Fixed send_command
-) Get HUB configuration during update in case it was not retrieved earlier (i.e. HUB unavailable)

* Further improvements

Completely removed dependency on __main__ for pyharmony, instead everything is now done from the HarmonyClient class.
Writing out Harmony configuration file as a JSON file.
Using same functionality to determine if activity provided is an ID or name for device, allowing send_command to receive a device ID or device name.

* Point requirements to updated pyharmony repo

Updated REQUIREMENTS to point to repository containing the updates for pyharmony.

* lint

lint

* Small fix for device and activity ID

Small fix in checking if provided device or activity ID is valid.

* Pin package version

* No I/O in event loop

* Point at HA fork with correct version bump

* Fix req

@balloob balloob referenced this pull request Dec 19, 2018

Merged

0.84.4 #19459

REQUIREMENTS = ['pyharmony==1.0.20']
# REQUIREMENTS = ['pyharmony==1.0.22']
REQUIREMENTS = [
'https://github.com/home-assistant//pyharmony/archive/'

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 19, 2018

Member

Should there be double slash after home-assistant?

This comment has been minimized.

@balloob

balloob Dec 19, 2018

Member

There shouldn't be, but I tested it locally with pip3 install https://github.com/home-assistant//pyharmony/archive/4b27f8a35ea61123ef531ad078a4357cc26b00db.zip#pyharmony==1.0.21b0 and it seems to work fine.

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 19, 2018

Member

Ok. Then this and other comments can be fixed later.

This comment has been minimized.

@balloob

balloob Dec 19, 2018

Member

Yay, I was not ready to wait another CI run. I've been cutting releases all day.

This comment has been minimized.

@ehendrix23

ehendrix23 Dec 19, 2018

Contributor

Hey Martin, yep. Will start working on a new fork already to include your comments and hopefully will then also be able to go back to pyrequirements. :-)

@@ -142,23 +151,30 @@ def __init__(self, name, host, port, activity, out_path, delay_secs):
self._state = None
self._current_activity = None
self._default_activity = activity
self._client = pyharmony.get_client(host, port, self.new_activity)
# self._client = pyharmony.get_client(host, port, self.new_activity)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 19, 2018

Member

Stale code.

This comment has been minimized.

@ehendrix23

ehendrix23 Dec 19, 2018

Contributor

I just commented that out for now. Plan is to update pyharmony to be able to use callback again so that HASS does not need to poll every 5 seconds for a change. Next update should be able to include this. :-)

self._delay_secs = delay_secs
_LOGGER.debug("HarmonyRemote device init completed for: %s", name)

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 19, 2018

Member

We don't want logging side effect in init method.


self.hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, shutdown)

_LOGGER.debug("Connecting.")

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 19, 2018

Member

Remove period in end of logging message.

This comment has been minimized.

@ehendrix23

ehendrix23 Dec 19, 2018

Contributor

Me and my periods. :-) Guess they really drilled it into me when I was in school to end a sentence with a period. :-)

@@ -180,52 +196,101 @@ def is_on(self):
"""Return False if PowerOff is the current activity, otherwise True."""
return self._current_activity not in [None, 'PowerOff']

async def async_update(self):
"""Retrieve current activity from Hub."""
_LOGGER.debug("Updating Harmony.")

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 19, 2018

Member

See above.

_LOGGER.debug("%s activity reported as: %s", self._name, activity_name)
self._current_activity = activity_name
self._state = bool(self._current_activity != 'PowerOff')
return

This comment has been minimized.

@MartinHjelmare

MartinHjelmare Dec 19, 2018

Member

Not needed return.

@ehendrix23 ehendrix23 deleted the ehendrix23:update-harmony-for-websockets branch Dec 19, 2018

mxworm added a commit to mxworm/home-assistant that referenced this pull request Dec 21, 2018

Merge branch 'dev' into current
* dev: (75 commits)
  Add native ESPHome Home Assistant state feature (home-assistant#19429)
  Add new sensor platform to expose Islamic prayer times (home-assistant#19444)
  Add timeout to RainMachine login (home-assistant#19476)
  Reorder FLOW entries in config_entries.py (home-assistant#19475)
  Update pynetgear to 0.5.2 (home-assistant#19490)
  Rename ocr.png to ssocr-(entity_name).png to allow multiple instances (home-assistant#18634)
  Add Mythic Beasts DNSAPI Component (home-assistant#18333)
  Adds battery_percent which had been introduced with pyatmo 1.4 and resolves unknown var warning. (home-assistant#19309)
  Bump Pybotvac To Support D7 On Latest Firmware (home-assistant#19463)
  Add device registry to MQTT alarm control panel (home-assistant#19331)
  Add device registry to MQTT lock (home-assistant#19333)
  Bump pyharmony (home-assistant#19460)
  Add testcase test_entity_id_update
  Add device registry to MQTT climate
  Updated frontend to 20181219.0
  Update translations
  ZHA entity ZCL reporting configuration (home-assistant#19177)
  Use web sockets for Harmony HUB (home-assistant#19440)
  Correct calls to subscription.async_unsubscribe_topics (home-assistant#19414)
  Various updates (home-assistant#19449)
  ...
@steve28

This comment has been minimized.

Copy link

steve28 commented Dec 21, 2018

I'm getting errors in the log every time I use this send a command to harmony in 0.84.5. Here's the sequence that occurs immediately upon sending the command:

2018-12-21 12:29:15 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:29:19 WARNING (MainThread) [homeassistant.helpers.entity] Update of remote.living_room is taking over 10 seconds
2018-12-21 12:29:20 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:29:26 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:29:32 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:29:38 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:29:44 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:29:50 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:29:56 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:30:02 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:30:08 WARNING (MainThread) [homeassistant.components.remote] Updating harmony remote took longer than the scheduled update interval 0:00:05
2018-12-21 12:30:09 ERROR (MainThread) [homeassistant.helpers.entity] Update for remote.living_room fails
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 528, in transfer_data
    msg = yield from self.read_message()
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 580, in read_message
    frame = yield from self.read_data_frame(max_size=self.max_size)
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 645, in read_data_frame
    frame = yield from self.read_frame(max_size)
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 710, in read_frame
    extensions=self.extensions,
  File "/usr/local/lib/python3.6/site-packages/websockets/framing.py", line 100, in read
    data = yield from reader(2)
  File "/usr/local/lib/python3.6/asyncio/streams.py", line 672, in readexactly
    raise IncompleteReadError(incomplete, n)
asyncio.streams.IncompleteReadError: 0 bytes read on a total of 2 expected bytes

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/src/app/homeassistant/helpers/entity.py", line 221, in async_update_ha_state
    await self.async_device_update()
  File "/usr/src/app/homeassistant/helpers/entity.py", line 347, in async_device_update
    await self.async_update()
  File "/usr/src/app/homeassistant/components/remote/harmony.py", line 205, in async_update
    activity_id = await self._client.get_current_activity()
  File "/usr/local/lib/python3.6/site-packages/pyharmony/client.py", line 234, in get_current_activity
    'vnd.logitech.harmony/vnd.logitech.harmony.engine'
  File "/usr/local/lib/python3.6/site-packages/pyharmony/client.py", line 184, in _send_request
    return await self._wait_response(msgid)
  File "/usr/local/lib/python3.6/site-packages/pyharmony/client.py", line 190, in _wait_response
    response_json = await self._websocket.recv()
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 350, in recv
    yield from self.ensure_open()
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 501, in ensure_open
    self.close_code, self.close_reason) from self.transfer_data_exc
websockets.exceptions.ConnectionClosed: WebSocket connection is closed: code = 1006 (connection closed abnormally [internal]), no reason
2018-12-21 12:30:09 ERROR (MainThread) [homeassistant.core] Error doing job: Task exception was never retrieved
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 528, in transfer_data
    msg = yield from self.read_message()
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 580, in read_message
    frame = yield from self.read_data_frame(max_size=self.max_size)
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 645, in read_data_frame
    frame = yield from self.read_frame(max_size)
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 710, in read_frame
    extensions=self.extensions,
  File "/usr/local/lib/python3.6/site-packages/websockets/framing.py", line 100, in read
    data = yield from reader(2)
  File "/usr/local/lib/python3.6/asyncio/streams.py", line 672, in readexactly
    raise IncompleteReadError(incomplete, n)
asyncio.streams.IncompleteReadError: 0 bytes read on a total of 2 expected bytes

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "/usr/src/app/homeassistant/helpers/service.py", line 277, in _handle_service_platform_call
    await getattr(entity, func)(**data)
  File "/usr/src/app/homeassistant/components/remote/harmony.py", line 240, in async_turn_on
    await self._client.start_activity(activity_id)
  File "/usr/local/lib/python3.6/site-packages/pyharmony/client.py", line 289, in start_activity
    response = await self._wait_response(msgid)
  File "/usr/local/lib/python3.6/site-packages/pyharmony/client.py", line 190, in _wait_response
    response_json = await self._websocket.recv()
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 350, in recv
    yield from self.ensure_open()
  File "/usr/local/lib/python3.6/site-packages/websockets/protocol.py", line 501, in ensure_open
    self.close_code, self.close_reason) from self.transfer_data_exc
websockets.exceptions.ConnectionClosed: WebSocket connection is closed: code = 1006 (connection closed abnormally [internal]), no reason
@ehendrix23

This comment has been minimized.

Copy link
Contributor

ehendrix23 commented Dec 21, 2018

Is it causing actual issues right now or more annoyance but workable?
Work is being done on a more robust solution that should resolve this.

@steve28

This comment has been minimized.

Copy link

steve28 commented Dec 21, 2018

annoyance - will wait until future solution. thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Dec 22, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.