Ape / samsungctl

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

Add websocket support for 2016 TVs #19

Closed nunofgs closed 7 years ago

nunofgs commented 7 years ago

This PR adds support for 2016 Samsung TVs such as the popular KS8000 model. It works great with my TV but I don't know exactly which models it will support!

Note to reviewers: my native language is not Python, so I'd very much appreciate any tips or comments regarding my approach.

Closes #18.

Ape commented 7 years ago

Additionally, the README.md should be updated.

nunofgs commented 7 years ago

@kylerw since you demonstrated interest in the KS8000 support, can you give a hand in testing this?

Ape commented 7 years ago

Now that you have Remote class again, I was thinking if the implementations should inherit it (as they implement the same interface). Seems like close() implementation could be shared. The exceptions could also be on Remote.

This is just some thoughts. I don't have strong opinions on this.

Ape commented 7 years ago

Can you check that connecting to missing or otherwise invalid hosts will produce meaningful error messages? E.g. "Error: Connection refused" or "Error: Timed out!".

Ape commented 7 years ago

Since we already have References section in README.md perhaps you should add this line:

https://github.com/kyleaa/homebridge-samsungtv2016 ('websocket' method)

kylerw commented 7 years ago

This is awesome @nunofgs!!!

@Ape - I'll try fire it up tonight to test if you still need it.

I haven't dug into the changes, but does the update use the port in the config to pick identify websocket vs traditional automatically if the new parameter isn't identified (since the port was there before)? I believe this would make it easy for HomeAssistant and others to allow functionality with 2016+ models without much (if any) change required.

kylerw commented 7 years ago

Sorry guys, the week has gotten away from me. I'll work to test this this weekend and report back.

nunofgs commented 7 years ago

@Ape: Now that you have Remote class again, I was thinking if the implementations should inherit it (as they implement the same interface). Seems like close() implementation could be shared. The exceptions could also be on Remote.

Not sure if I understand what you mean here. If we make RemoteLegacy and RemoteWebsocket inherit from Remote, then we'd be forcing a BC since dependees would have to import the RemoteLegacy class explicitly.

Am I looking at this wrong?

@Ape: Can you check that connecting to missing or otherwise invalid hosts will produce meaningful error messages? E.g. "Error: Connection refused" or "Error: Timed out!".

Here are a couple of examples. The remaining errors are harder to simulate (unless we add unit tests) but they should be displayed in the same way.

❯ python3 -m samsungctl --host 192.168.1.92 --method websocket2 KEY_VOLDOWN
Error: Unknown method 'websocket2'

❯ python3 -m samsungctl --host 192.168.1.92 --method websocket KEY_VOLDOWN
Error: Connection refused

@Ape: Since we already have References section in README.md perhaps you should add this line:

Great idea! Done.

@kylerw: I haven't dug into the changes, but does the update use the port in the config to pick identify websocket vs traditional automatically if the new parameter isn't identified (since the port was there before)? I believe this would make it easy for HomeAssistant and others to allow functionality with 2016+ models without much (if any) change required.

I thought of this originally. It would indeed "automatically" add support for my Samsung KS8000 in Home Assistant without requiring them to do any code changes at all (which is pretty cool), but it felt a little strange to infer the method from the port.

If we go that route, then we'd have to assume that port 8001 is "websocket" and anything else is "legacy".

I'm fine with either implementation and will happily update the PR as needed. @Ape, any thoughts on this?

Mofef commented 7 years ago

using @nunofgs' code and --method websocket the program terminates without error, but the TV (UE40JU6500) does not react. With wireshark I found that the request

{"params": {"DataOfCmd": "KEY_MUTE", "Option": "false", "Cmd": "Click", "TypeOfRemote": "SendRemoteKey"}, "method": "ms.remote.control"}

is answered by the TV with

{"event":"ms.error","data":{"message":"unrecognized method value : ms.remote.control"}}

:/ Any ideas?

nunofgs commented 7 years ago

@Mofef: hmm interesting. I may have an idea. Can you install the Samsung app on your phone, pair it with your TV, and then try my PR again?

Mofef commented 7 years ago

@nunofgs I installed the Android version of it and paired it. but sadly the eror is stil the same. :/ Btw: Thanks for pointing out this app I didn't find the official Remote app from samsung so far. :)

Mofef commented 7 years ago

@nunofgs do you know how this app actually works? i wasn't able to trace any packages from it via wireshark oO. I also made sure to block the infrared port on my phone, so it can't use it.

The only thing i was able to capture which might help is that my TV (192.168.178.76) is sometimes multicasting(?) to 224.0.0.7 via UDP: {"type":"alive","ttl":8000,"sid":"uuid:dacc4663-1a1e-4c15-a1e7-xxxxxxxxxa2","data":{"v1":{"uri":"http://192.168.178.76:8001/ms/1.0/"},"v2":{"uri":"http://192.168.178.76:8001/api/v2/"}}}

nunofgs commented 7 years ago

I admit I haven't sniffed the traffic to/from the app. I simply ported the code in https://github.com/kyleaa/homebridge-samsungtv2016.

Hmm, I'm stumped. I'll have to whip out wireshark as well and try to find the cause.

By the way, this is working flawlessly on my KS8000 TV. Here's a quick video:

img_6193

(I swear I'm not using the remote in the background haha 😄 )

@Mofef what TV model do you have?

Mofef commented 7 years ago

@nunofgs it's a UE40JU6550

wow only now i notice that it's not from 2016... sorry about that. without -method websocket it says connection refused.

this is what nmap finds as open ports by the way. only from this i was guessing that i probably has this websocket api. But maybe a different version...? PORT STATE SERVICE 7676/tcp open imqbrokerd 8000/tcp open http-alt 8001/tcp open vcom-tunnel 8080/tcp open http-proxy 9999/tcp open abyss

kylerw commented 7 years ago

Finally got around to testing on my KS8000 using the command line and it is working like a charm. Although the Power command is KEY_POWER which seems to differ from the legacy API - maybe something to convert if the legacy command is used (for backwards compatibility)?.

kylerw commented 7 years ago

Another thought I had, would it make sense to add the wake on lan ability like (https://github.com/kyleaa/homebridge-samsungtv2016) implemented to allow turning the TV on?

Ape commented 7 years ago

I think we shouldn't modify the key codes. But, we could add new alias key codes that are translated properly to each device. E.g. power => KEY_POWER or KEY_POWEROFF. This would be a separate pull request.

I think the wake on lan capability is a bit out of scope for samsungctl. There are already utilities and libraries that do that just fine. It's not specific to Samsung TVs.

Ape commented 7 years ago

I think this pull request is soon ready to be merged. I'll make a new release, maybe 1.0.0, after a brief testing period after the merge.

But I'd like to have completely updated README.md before I'll merge this. See https://github.com/Ape/samsungctl/pull/19#issuecomment-260258791.

Mofef commented 7 years ago

Btw in the meanwhile I found that I can start the authentication process by sending "pin4" via POST to http://192.168.178.76:8080/ws/apps/CloudPINPage then the tv shows a 4 digit screen. Anyone who has a similarly behaving TV? Further experiences?

nunofgs commented 7 years ago

@Ape But I'd like to have completely updated README.md before I'll merge this. See #19 (comment).

Oops, totally lost track of this. I just updated the PR with the changes you requested.

Also, I'm starting to wonder if the method parameter is really the way to go here. It requires every dependee to introduce these changes.

WDYT of implementing the change that @kylerw mentioned here?

kylerw commented 7 years ago

Hey @nunofgs and @Ape any update on this PR? Would love to get the changes merged into HomeAssistant!

Ape commented 7 years ago

I'm sorry for the delay. I have been busy lately. Thanks for pushing me to finally reviewing and testing this PR.

reedlaw commented 7 years ago

I get Error: Connection refused. I have a Samsung KUF30E (should be the same as KU6300). Anyone get this model to work?

AceoStar commented 7 years ago

@kylerw I'll help test once you merge into Home assistant. I'm testing a 2016 8000 that currently just shows the remote in hass. :D Looking forward to it!

Ape commented 7 years ago

@kylerw I released 0.6.0 with this feature.

onemico commented 7 years ago

@Mofef did you ever get to the bottom of your issue? I am having very similar issues on my TV, which appears to have web socket ports open but is a 2015 TV as far as I am aware. Have paired with Samsung remote (my MyTifi) without any issues, but don't get offered a PIN for this.