Try all Samsung TV websocket ports by escoand · Pull Request #33001 · home-assistant/core

escoand · 2020-03-19T16:01:26Z

Breaking change

Proposed change

Currently the Samsung TV integration does only try to connect port 8001. This fix will also try 8002.

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

Example entry for `configuration.yaml`:

# Example configuration.yaml

Additional information

This PR fixes or closes issue: fixes Samsung TV not supported #32942
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

beloso · 2020-03-19T16:10:54Z

I don't mind testing this branch. I am using Hassio, how could I load up this on my end?

escoand · 2020-03-19T18:01:19Z

@beloso Download the complete samsungtv directory and put it in the config directory under config/custom/components/samsungtv/

beloso · 2020-03-19T19:00:32Z

I can confirm that it's working.

MartinHjelmare

Great!

tulindo · 2020-03-19T20:28:59Z

Well done @escoand!

beloso · 2020-03-19T21:14:32Z

I've downloaded and tested the reviewed version of the component. Deleted the old integration and added again, it worked again.

beloso · 2020-03-19T21:22:03Z

I guess I spoke too soon. With the revised version I am getting a loop of permission requests.
And the log gives me this error:

Update for media_player.samsungtv fails
Traceback (most recent call last):
  File "/usr/local/lib/python3.7/site-packages/websocket/_socket.py", line 81, in recv
    bytes_ = sock.recv(bufsize)
  File "/usr/local/lib/python3.7/ssl.py", line 1056, in recv
    return self.read(buflen)
  File "/usr/local/lib/python3.7/ssl.py", line 931, in read
    return self._sslobj.read(len)
socket.timeout: The read operation timed out

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 279, in async_update_ha_state
    await self.async_device_update()
  File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 476, in async_device_update
    await self.hass.async_add_executor_job(self.update)
  File "/usr/local/lib/python3.7/concurrent/futures/thread.py", line 57, in run
    result = self.fn(*self.args, **self.kwargs)
  File "/config/custom_components/samsungtv/media_player.py", line 127, in update
    self._state = STATE_ON if self._bridge.is_on() else STATE_OFF
  File "/config/custom_components/samsungtv/bridge.py", line 66, in is_on
    return self._get_remote() is not None
  File "/config/custom_components/samsungtv/bridge.py", line 256, in _get_remote
    self._remote.open()
  File "/usr/local/lib/python3.7/site-packages/samsungtvws/remote.py", line 149, in open
    response = self._process_api_response(self.connection.recv())
  File "/usr/local/lib/python3.7/site-packages/websocket/_core.py", line 310, in recv
    opcode, data = self.recv_data()
  File "/usr/local/lib/python3.7/site-packages/websocket/_core.py", line 327, in recv_data
    opcode, frame = self.recv_data_frame(control_frame)
  File "/usr/local/lib/python3.7/site-packages/websocket/_core.py", line 340, in recv_data_frame
    frame = self.recv_frame()
  File "/usr/local/lib/python3.7/site-packages/websocket/_core.py", line 374, in recv_frame
    return self.frame_buffer.recv_frame()
  File "/usr/local/lib/python3.7/site-packages/websocket/_abnf.py", line 361, in recv_frame
    self.recv_header()
  File "/usr/local/lib/python3.7/site-packages/websocket/_abnf.py", line 309, in recv_header
    header = self.recv_strict(2)
  File "/usr/local/lib/python3.7/site-packages/websocket/_abnf.py", line 396, in recv_strict
    bytes_ = self.recv(min(16384, shortage))
  File "/usr/local/lib/python3.7/site-packages/websocket/_core.py", line 449, in _recv
    return recv(self.sock, bufsize)
  File "/usr/local/lib/python3.7/site-packages/websocket/_socket.py", line 84, in recv
    raise WebSocketTimeoutException(message)
websocket._exceptions.WebSocketTimeoutException: The read operation timed out

escoand · 2020-03-19T21:34:11Z

@tulindo seems the config flow worked correctly but the token is getting lost or not valid any more. What do you think?

beloso · 2020-03-19T22:03:19Z

I cleaned up some leftover entity and I was able to configure it again. Now it's working fine.

escoand · 2020-03-19T22:08:05Z

@beloso Good for you, but hard to implement this workflow in the code. Could you be a bit more precisely? And maybe find a way to reproduce the problem and the solution?

tulindo · 2020-03-20T00:40:28Z

@beloso can you try to reproduce the error?
@escoand AFAIK token doesn't change... Websocket timeout is more related to the wrong port (i.e. he had config entity set to port 8001 instead 8002)

LeoCal · 2020-03-20T09:08:11Z

First of all, thanks for taking care of this fix in such a timely manner!
Just one more comment: I see that you referenced this defect (#33001) from the one I created (#33006). However, based on the traceback, you also need another fix if you want to duplicate 33006 to 33001. If you look at the backtrace I included in 33006, you need to add handling of the exception at line 251 (inside _get_remote function).

LeoCal · 2020-03-20T09:00:01Z

@@ -223,9 +224,13 @@ def try_connect(self):
                return RESULT_SUCCESS
            except WebSocketException:
                LOGGER.debug("Working but unsupported config: %s", config)


Minor, but I would suggest dropping a more meaningful message here, something like:
LOGGER.debug("Websocket connection attempt failed with the following config: %s", config)

Yes maybe it's a bit confusing but it describes exactly the problem: the connection is working but something went wrong, and if already the login fails it's apparently not like we support it.

escoand · 2020-03-20T09:41:16Z

@LeoCal With the referenced PR we now have the same situation in both issues. And yes, we need to handle this exception. But currently it looks like we have a problem with the token handling directly after config flow. After a restart it seems to work.

LeoCal · 2020-03-20T09:59:57Z

@LeoCal With the referenced PR we now have the same situation in both issues. And yes, we need to handle this exception. But currently it looks like we have a problem with the token handling directly after config flow. After a restart it seems to work.

Ok, thanks for the info!
But so, you're adding a second exception handling statement where I suggested, right? In the end, it never hurts handling one more possible exception in a certain code path.

beloso · 2020-03-20T18:38:20Z

So, my issue is when I try to add the integration manually. I go to the list and select Samsung TV. I type in the IP address, I type in a name. It displays the request on the TV, I accept. Then the device does not show on HA. Asks me to restart, I do, and then it still on show, and it caused that loop of notifications.

If I add it from the discovery component, it works as expected.

escoand · 2020-03-20T21:12:35Z

@beloso Thanks for the precise description. Could you please also test the configuration within the yaml files?

Can the other reproduce this as well?

LeoCal · 2020-03-21T08:26:04Z

@beloso Thanks for the precise description. Could you please also test the configuration within the yaml files?

Can the other reproduce this as well?

As mentioned in the issue I raised #33006, I also see a loop of notifications on my TV that I can stop only by killing home assistant instance. The config flow I followed is the one with the yaml file.

escoand · 2020-03-21T11:10:44Z

Yesterday I gazed on the code and found no reason for the difference between the two flows. I don't have a websocket TV, so I'm unable to test it.

beloso · 2020-03-21T11:13:15Z

I see no reason to merge this PR. It does fix the issue. I'll try to debug the other issue and keep you posted.

escoand · 2020-03-21T13:15:58Z

@MartinHjelmare Ready for merge from my side.

LeoCal · 2020-03-21T13:23:38Z

Agree, the second exception can be handled separately in the other issue I raised.

MartinHjelmare

Great! Just a clean up.

MartinHjelmare · 2020-03-21T15:50:48Z

Should this be tagged for a patch release?

escoand · 2020-03-21T17:21:10Z

From my point of view yes.

* Update bridge.py * add test * silence pylint * correct pylint * add some tests * Update test_media_player.py

Update bridge.py

ec031ea

homeassistant added the cla-signed label Mar 19, 2020

probot-home-assistant Bot added integration: samsungtv small-pr PRs with less than 30 lines. by-code-owner labels Mar 19, 2020

MartinHjelmare reviewed Mar 19, 2020

View reviewed changes

Comment thread homeassistant/components/samsungtv/bridge.py Outdated

add test

6d3af46

escoand mentioned this pull request Mar 19, 2020

Samsung TV component not working and logging traceback #33006

Closed

MartinHjelmare approved these changes Mar 19, 2020

View reviewed changes

MartinHjelmare reviewed Mar 19, 2020

View reviewed changes

Comment thread homeassistant/components/samsungtv/bridge.py

escoand added 2 commits March 19, 2020 21:35

silence pylint

4095469

correct pylint

f4a2c3a

LeoCal reviewed Mar 20, 2020

View reviewed changes

add some tests

657b558

MartinHjelmare approved these changes Mar 21, 2020

View reviewed changes

Comment thread tests/components/samsungtv/test_media_player.py Outdated

Update test_media_player.py

2bf9eac

MartinHjelmare merged commit 2bb2948 into home-assistant:dev Mar 21, 2020

MartinHjelmare added this to the 0.107.5 milestone Mar 21, 2020

escoand mentioned this pull request Mar 21, 2020

Support for older SamsungTV #33112

Closed

balloob added the cherry-picked label Mar 21, 2020

balloob pushed a commit that referenced this pull request Mar 21, 2020

Try all Samsung TV websocket ports (#33001)

de7e7e0

* Update bridge.py * add test * silence pylint * correct pylint * add some tests * Update test_media_player.py

balloob mentioned this pull request Mar 21, 2020

0.107.5 #33118

Merged

balloob added cherry-picked and removed cherry-picked labels Mar 21, 2020

balloob pushed a commit that referenced this pull request Mar 21, 2020

Try all Samsung TV websocket ports (#33001)

a129bc0

* Update bridge.py * add test * silence pylint * correct pylint * add some tests * Update test_media_player.py

escoand deleted the patch-2 branch March 22, 2020 12:27

lock Bot locked and limited conversation to collaborators Mar 22, 2020

Uh oh!

Conversation

escoand commented Mar 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Breaking change

Proposed change

Type of change

Example entry for configuration.yaml:

Additional information

Checklist

Uh oh!

beloso commented Mar 19, 2020

Uh oh!

Uh oh!

escoand commented Mar 19, 2020

Uh oh!

beloso commented Mar 19, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

tulindo commented Mar 19, 2020

Uh oh!

Uh oh!

beloso commented Mar 19, 2020

Uh oh!

beloso commented Mar 19, 2020

Uh oh!

escoand commented Mar 19, 2020

Uh oh!

beloso commented Mar 19, 2020

Uh oh!

escoand commented Mar 19, 2020

Uh oh!

tulindo commented Mar 20, 2020

Uh oh!

LeoCal commented Mar 20, 2020

Uh oh!

LeoCal Mar 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

escoand Mar 21, 2020

Choose a reason for hiding this comment

Uh oh!

escoand commented Mar 20, 2020

Uh oh!

LeoCal commented Mar 20, 2020

Uh oh!

beloso commented Mar 20, 2020

Uh oh!

escoand commented Mar 20, 2020

Uh oh!

LeoCal commented Mar 21, 2020

Uh oh!

escoand commented Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

beloso commented Mar 21, 2020

Uh oh!

escoand commented Mar 21, 2020

Uh oh!

LeoCal commented Mar 21, 2020

Uh oh!

MartinHjelmare left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

MartinHjelmare commented Mar 21, 2020

Uh oh!

escoand commented Mar 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

escoand commented Mar 19, 2020 •

edited

Loading

Example entry for `configuration.yaml`:

LeoCal Mar 20, 2020 •

edited

Loading

escoand commented Mar 21, 2020 •

edited

Loading