search

Ape / samsungctl

Remote control Samsung televisions via a TCP/IP connection
MIT License
711 stars 191 forks source link

Error: Broken pipe #61

Closed Vulpecula-nl closed 5 years ago

Vulpecula-nl commented 6 years ago

When I start the interactive mode and when I choose to show the menu, it wil show it on de screen of my tv. But after about 30 seconds and entering a new command it's not responding anymore and the console gets closed with the message Error: Broken pipe

I can start the interactive mode again and the first few commands are working correctly but after 30 seconds I get the same error again.

System: Raspberry Pi 3 Tv: Samsung UE55KS7000 samsungctl 0.7.0

[Edit] I notice I have the same issue as #16. After a while I have to allow again before I can connect

ejel commented 6 years ago

I'm seeing this issue as well in Home Assistant, with websocket method. I'm getting BrokenPipe error when issuing new command after a brief idle period.

I'm curious if this issue has anything to do with websocket-client implementation, in that the usage pattern used by samsungctl is designed for short-live/one off send-receive but not for long-live connection? The reason I'm asking is that on websocket-client's README it shows a different usage pattern for long-live connection using callbacks.

ejel commented 6 years ago

It looks like the problem is with websocket-client in that websocket created with websocket-client.create_connection() does not seem to work reliably with long-live connection. After leaving the connection idle for a brief period of time (I can consistently reproduce with a wait time between 30 seconds to 1 minute on KS8000), there will be no response to the next issued command, and the following command will cause broken pipe error.

If I switched the implementation to WebSocketApp which according to the doc is meant to be used with long-live connection, the problem goes away. I tested this with samsungctl's interactive mode as well as with Home Assistant's samsungtv component.

The implication of using WebSocketApp is that it requires spawning up a separate thread just for itself. Another possible option, although with more ramification, is switching to a different websocket library such as websockets which provide simplified APIs based on asyncio.

@Ape I'm happy to help get this issue fixed but would love to hear what's your thought is on the direction that you prefer.

Vulpecula-nl commented 6 years ago

I have changed the port number from 55000 to 8001 and that did solve it for me. Because of your rely @ejel I enabled it again and found that the port number needs to be changed.

Ape commented 6 years ago

@ejel I would be happy to accept a pull request with a new websocket implementation if it fixes these issues. Either WebSocketApp or websockets would be good. I might prefer websockets.

ejel commented 6 years ago

@Vulpecula-nl Can you share your full command that makes it work? If it's a websocket Samsung TV it should only work with port 8001 and not at all with port 55000 as far as I understand.

ejel commented 6 years ago

@Ape I looked into websockets and played around with it today. Although the library is great its APIs are asyncio/coroutine-based. This means to use it samsungctl will need to be asyncio-based as well. I think it might make sense to use it when samsungctl wants to expose async API but at the moment it might not be feasible as it impacts not only websocket implementation but legacy implementation as well.

Because of this, I think it is more reasonable to pursue WebSocketApp approach and keeping the API as-is.

Vulpecula-nl commented 6 years ago

@ejel What do you want to know? I'm using Domoticz and what I did there was changing the port number from 55000 to 8001.

ejel commented 6 years ago

@Ape Turns out that there is no need to switch to WebSocketApps. The current WebSocket class is capable of long-live connection as long as there is a designated thread to keep the underlying socket alive. I posted a PR #88 with the change.

A caveat with this change is that a thread is created even for one-off / short-lived connection. If this is not desirable, one solution is to add a new config option to specify if the connection is intended to be long-lived, and only create a thread in such case. Let me know what you think.

ejel commented 6 years ago

@Vulpecula-nl You mentioned in the original post that you reproduced the issue using the interactive mode. Can you share your full command that reproduces the issue and one that fixes the issue?