alexmohr / sonyapilib

Contains a python api to control sony devices.
MIT License
20 stars 11 forks source link

SonyDevice: Support 2010 Blu-ray players, e.g. BDP-S370 #58

Closed mtdcr closed 3 years ago

mtdcr commented 4 years ago

After your feedback in https://github.com/alexmohr/sonyapilib/issues/57, I decided to spend some more time with my old Blu-ray player. I analyzed another application to understand how Sony encodes IRCC commands. That allowed me to use IR key codes instead of fixed base64 encoded strings, and to prepare for other types of old devices. The IR key codes match the codes of my RMT-B107P remote control. There are three additional codes (Karaoke, Netflix and Mode3D), which my device doesn't know. But because they don't conflict and were present in tests/data/getRemoteCommandList.xml, I decided to include them. Maybe they become useful for another device. My device understands discrete PowerOn (46) and PowerOff (47), but I didn't include them, because using them over HTTP doesn't seem to make sense. If other devices are reachable during stand-by, these codes could be added later.

With this patch applied, I can finally control my BDP-S370. Though I haven't tried it together with your Home Assistant integration, yet.

alexmohr commented 4 years ago

Thanks! It looks great.

I don't think that this can break the home assistant integration, if you don't have an instance running you don't have to setup one only to test this. Of course it would be great if you wan't to test it.

The 3 tests that failed can easily be adjusted by mocking the new _use_builtin_command_list method and checking for the call count equal 1. The coveralls integration does not run until the tests passed. Coveralls enforces that new code is tested, so you will have to add a test to validate _use_builtin_command_list

alexmohr commented 4 years ago

Pylint warning sonyapilib/device.py:1:0: C0302: Too many lines in module (1023/1000) (too-many-lines) can be disabled. Refactoring and putting the default values in another file does not make much sense in my opinion.

mtdcr commented 4 years ago

I chose to fix the tests by setting the default api_version to 3, which restored the previously used code paths.

The remaining failure is yet another instance of pylint being annoying: sonyapilib/device.py:1:0: C0302: Too many lines in module (1023/1000) (too-many-lines)

mtdcr commented 4 years ago

Alright, coverage of course decreased. I'm not going to invest more time.

If you need further information to write additional test cases, please let me know.

mtdcr commented 4 years ago

In the meantime I've tested this PR with Home Assistant, and it works (hooray!), but I noticed that the library is quite noisy if the device is turned off during startup:

2020-09-20 23:58:44 ERROR (SyncWorker_35) [sonyapilib.device] HTTPError: HTTPConnectionPool(host='bdp-s370', port=52323): Max retries exceeded with url: /dmr.xml (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fdb23f92a90>: Failed to establish a new connection: [Errno 113] No route to host'))
2020-09-20 23:58:47 ERROR (SyncWorker_35) [sonyapilib.device] HTTPError: HTTPConnectionPool(host='bdp-s370', port=50001): Max retries exceeded with url: /Ircc.xml (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fdb22e791d0>: Failed to establish a new connection: [Errno 113] No route to host'))
2020-09-20 23:58:47 ERROR (SyncWorker_35) [sonyapilib.device] failed to get device information: HTTPConnectionPool(host='bdp-s370', port=50001): Max retries exceeded with url: /Ircc.xml (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fdb22e791d0>: Failed to establish a new connection: [Errno 113] No route to host'))
2020-09-20 23:58:50 ERROR (SyncWorker_35) [sonyapilib.device] HTTPError: HTTPConnectionPool(host='bdp-s370', port=50202): Max retries exceeded with url: /appslist (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7fdb2254bb50>: Failed to establish a new connection: [Errno 113] No route to host'))
2020-09-20 23:58:51 WARNING (MainThread) [homeassistant.helpers.entity] Update of media_player.sony_bdp_s370 is taking over 10 seconds
2020-09-20 23:58:51 WARNING (MainThread) [homeassistant.components.media_player] Updating sony media_player took longer than the scheduled update interval 0:00:10

These lines keep repeating every few seconds, so they really fill up the logs. I think the errors should be degraded to warnings, but maybe some kind of rate limiting could be applied, too. I don't think they are caused by my patch or the type of device (are newer devices always-on?), but who knows...

alexmohr commented 4 years ago

These logs are not caused by your patch. Some newer devices can be reached in standby to enable sending the power command instead of using wake on lan. Your suggestion with rate limiting and setting the error to warning seems like a good idea.

alexmohr commented 3 years ago

Test coverage is fixed in #60, therefore I'm closing this.