Velleman / python-linkplay

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

Feature: TCPUART api #31

Closed daniel-simpson closed 3 months ago

daniel-simpson commented 3 months ago

Adds a helper to call the TCPUART API as per Arylic TCP documentation (as linked from Arylic documentation

Also adds two example calls to this API: one for manually setting a preset for a player (preset_play), and one to cycle through presets (preset_next).

Note: I've added some debug logging from the older linkplay HACS add on, let me know if you'd like me to remove this.

daniel-simpson commented 3 months ago

Note: previous TCPUART implementation from here: https://github.com/nagyrobi/home-assistant-custom-components-linkplay/blob/master/custom_components/linkplay/media_player.py#L445

dukeofphilberg commented 3 months ago

Thanks for the initial work!

My idea with the abstract LinkPlayEndpoint was so that there could be a LinkPlayTcpUartEndpoint implementation where the request and request_json could then do the stuff you are doing in call_tcpuart and call_tcpuart_json, reducing the complexity of the LinkPlayPlayer and LinkPlayDevice class. There will need to be a helper class that can translate the commands to either the httpapi or the tcpuart ones.

I don't think there would be a scenario where you're using both the httpapi and the tcpuart connection simultaniously. If there would be functionality missing in the httpapi that is present in tcpuart then it is worth considering supporting both at the same time, but I think the tcpuart is far less extensive?

daniel-simpson commented 3 months ago

Ahh nice one, probably should've reached out before starting work on this!

You're right about the TCPUART api being a lot less extensive, and a lot harder to work with, but it does seem to have some low-level functionality that I wasn't able to spot in the HTTP API (presets being the main one).

While I understand what you mean about using either the TCPUART API or the HTTP API, I've definitely found that (in some cases?) the TCPUART version provides data that the HTTP API doesn't. Would probably be worth ensuring something is playing before the second API call though.

Ideally I would love to have both implementations available, so long as there isn't significant overhead? (I'm thinking it could be a configuration setting during HA setup that drives whether to use the fallback endpoint for title data, or skip it by default).

Happy to modify my PR to inherit a new LinkPlayTcpUartEndpoint from LinkPlayEndpoint rather than adding to the (Http)Api implementation. Likely tomorrow / another day now.

daniel-simpson commented 3 months ago

Hmm, looking at how a bridge factory only takes a single endpoint, I guess I'm questioning the approach. I'm not sure we'd ever want to create a bridge using the TCPUART endpoint as the transport mechanism. I see that TCPUART API as something used in tandem for lower-level functionality, rather than replacing the standard API.

Would you be open to me creating a LinkPlayTcpUartEndpoint, and then having that as an optional extra argument to the bridge factory?

dukeofphilberg commented 3 months ago

Hmm, looking at how a bridge factory only takes a single endpoint, I guess I'm questioning the approach. I'm not sure we'd ever want to create a bridge using the TCPUART endpoint as the transport mechanism. I see that TCPUART API as something used in tandem for lower-level functionality, rather than replacing the standard API.

Would you be open to me creating a LinkPlayTcpUartEndpoint, and then having that as an optional extra argument to the bridge factory?

As there are things you can't do with the HTTPAPI but not with the TCPUART (and vice versa) it will create some overhead regardless keeping these two endpoints active. I would probably create a list of endpoints or a wrapper for both, and pass it to the bridge. When a request comes, a seperate component needs to decide which endpoint it'll take for which command. That's however quite the change to implement 😉

You can implement the LinkPlayTcpUartEndpoint in this PR and not use it until we figure out the design for knowing which endpoint needs the command

daniel-simpson commented 3 months ago

Note: with my latest push I've strippped this right back to just implementing a LinkPlayTcpUartEndpoint for now, happy to discuss how to use in tandem later.

Will separate out other minor changes into separate PR(s)

daniel-simpson commented 3 months ago

Thanks for the feedback! I'll switch it to draft while I work through your items

daniel-simpson commented 3 months ago

Have incorporated a bunch of your feedback and refactored TCPUART endpoint implementation to be truly async.

Tested a few bits locally and seems ok?

daniel-simpson commented 3 months ago

I think I'd addressed everything 🤞

Hopefully not a problem, but I "resolved" comments as I went, to try and keep on top of what was left.

Let me know if there's anything else!

dukeofphilberg commented 3 months ago

I think it looks pretty good now! Thank you very much for the contribution 🎉