Velleman / python-linkplay

LinkPlay library for Python
MIT License
9 stars 4 forks source link

Add support for players using `getStatusEx` / `getPlayerStatusEx` endpoints and mTLS #18

Closed daniel-simpson closed 3 months ago

daniel-simpson commented 3 months ago

There are a few spots in the README's linked documentation (https://www.wiimhome.com/pdf/HTTP%20API%20for%20WiiM%20Mini.pdf) that mention players that use mTLS and slightly different endpoints (getPlayerStatusEx instead of getPlayerStatus, getStatusEx instead of getStatus).

Would love to see this type of player supported. I'm well aware that without a device to test with, this will be difficult, and so I'm happy to put in some work to get it there if you're open to it in this library?

And thanks for the great work on this clean library implementation


For context

All of my linkplay devices are in this category: 3x Edifier M50A and one Wiim Pro)

I started work on this in the now defunct linkplay custom HA component library here: https://github.com/nagyrobi/home-assistant-custom-components-linkplay/pull/147 This linked PR is a very rough first draft, would obviously want to make sure it's up to scratch before bringing into this library.

dukeofphilberg commented 3 months ago

As far as the testing on the devices that I did, getPlayerStatus and getPlayerStatusEx resulted in the same reponse. I think the parser just reads getPlayerStatus and ignores everything afterwards. Adding mTLS support is currently not high on the priority list, but will be noted.

daniel-simpson commented 3 months ago

The players I have definitely don't respond on the getStatus and getPlayerStatus endpoints (Both edifier M50A and WiiM pro).

I've got a PR incoming that would add this, but it's pretty early on. Happy to add in any feedback.

dukeofphilberg commented 3 months ago

As for the mTLS support, the library is purposely designed to allow the ClientSession to be set from outside the library. In terms of using it with HomeAssistant I'm wondering if we should add the certificate in HomeAssistant, with a helper function to do it from the library, rather than having it set by default?

daniel-simpson commented 3 months ago

I'll have a look. I did a small local POC and I tried setting it via the TCPConnector but wasn't having much luck.

I'm unsure of where to add the cert either. My original PR had the ability to reference it via file path (as the SSL library doesn't seem to be able to feed that certificate in as a stream, only from a file path 😬) which means it'll actually need to be a file on each user's HAOS or in their docker container.

For context, my Wiim player doesn't need the MTLS cert, but still works with it set. Not sure if you'd be happy with it shipped by default for all players though.

Ideally it'd be really nice if HA didn't need to know about the cert, and it was just an implementation detail handled by the library, or something only specified/provided to players that need it.

If you have any strong opinions one way or another let me know. I'll try attacking this over the next few days.

daniel-simpson commented 3 months ago

For anyone watching along, an mTLS POC is available as #27

dukeofphilberg commented 3 months ago

27 adds mTLS so we can close this. Thanks @daniel-simpson for implementing this!