-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
async support #23
Comments
Hey @fluffysquirrels , Thanks for filing the issue!
I'm definitely open to collaborating on this and believe that this would be a nice addition. Unfortunately, I don't have access to my usual development Chromecast device to test the changes at the moment (I happen to have |
This is no problem, sounds like a good plan. I've made some progress in a private branch getting an async client working. It's approximately the right design I think but still with rough edges. I have maybe 50 lines left to write before it can send the first request message and receive a response; the first I want to implement is getting the receiver status. Once that is working I'm planning to tidy it up slightly and push to my fork for you to look at. One problem currently is that my current design is a fairly large departure from the existing sync code. Some of that is my opinions on how to write the code, some of it is required due to switching to async. I don't know how you feel about that and how committed you are to the current API. A few options I see to resolve this:
|
If there is a need to change the API to improve it, go for it. The initial interface was written ~7-8 years ago when I was just learning Rust, and not much has changed in this interface since then. Options 2 and 3 sound like the best options to me. I don't have a preference for one over the other, but I believe there is still value in having sync APIs to make it much easier to get started. If we have to make breaking changes in the sync APIs along the way, it's fine too. We're at 0-major, so bumping the minor version because of breaking changes is reasonable. |
I've made some progress on this, and have the core features I need working on a branch. That includes a basic test CLI app that can:
These are enough features that I am confident the code design will work. I am ready to start polishing it up and refactoring to get ready for review. There is no need for you to look at the code yet, but I thought I'd give a status update. This is the diff between my Note that my |
Awesome, thanks for the update @fluffysquirrels! I've just merged #22 and also bumped dependencies' versions, so you might need to rebase your fork on |
I have a branch that adds async (draft PR #28), in a not too intrusive way (adds 93 lines net under src/). It's on top of my other two PRs (request ids and queue operations). https://github.com/g2p/rust-cast/commits/async I have not wrapped it back into a sync version, because it would require duplicating every function. Changing existing users to use async is mechanical: change callers to async fns and add .await everywhere the compiler asks. For me the point was to send commands and listen for status updates on the same connection. |
Sounds cool! I will take a look at your approach.
It does seem like your changes won't have the same API changes (or IMO improvements) that I wanted.
For example, in my branch all received messages are read and handled by a spawned Tokio task, including if the consumer is waiting for a specific message. I believe the sync code on `main` discarded messages that the consumer was not expecting, and with the same overall structure I would expect your async branch to do the same.
I haven't looked hard enough myself to decide what I will do with my own (atm private) project using my branch. And I don't have a suggestion yet for what we / `azasypkin` should do with this crate.
I hope to look more at your code today.
…On Thu, 7 Mar 2024, 14:02 Gabriel de Perthuis, ***@***.***> wrote:
I have a branch that adds async, in a not too intrusive way (adds about 93
lines under src/). It's on top of my other two PRs (request ids and queue
operations).
https://github.com/g2p/rust-cast/commits/async
I have not wrapped it back into a sync version, because it would require
duplicating every function. Changing existing users to use async is
mechanical: change callers to async fns and add .await everywhere the
compiler asks.
For me the point was to send commands and listen for status updates on the
same connection.
—
Reply to this email directly, view it on GitHub
<#23 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE3NEXEI7HXAYDHZVJ4K6TYXBXOLAVCNFSM6AAAAABCDCL4M2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOBTGU3DSOBRGM>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
The current API goes either through receive or through receive_find_map. receive_find_map callers do the request-response association, but a client that calls receive in parallel and discards some messages could starve a function waiting for a response. I think this was already the case if the thread-safe feature was used; one of the per-channel request-response function could be preempted between calling send and receive_find_map. It's worth adding something that maps from request-ids to waiting tasks. Maybe even make the general receive case return only broadcasts. I'll look at your approach. |
Indeed, I have that in Another 2 reasons I added the spawned
While I could put the logic and event loop task for these 2 in my own application, it made much more sense to me to do that once (in a crate) that others could re-use. For a one-off command to the Chromecast from a short-running CLI app, the current API works well enough. But in a longer-running app, you need to write quite a chunk in the app yourself. The style of the API on my branch was influenced by 2 JavaScript Chromecast libraries I used from another tool, which I found much easier to work with. |
Another point on API. In my own private application I want to discover and listen to all the Chromecasts on the LAN of the host device. To implement that I have a wrapper -- currently called
Then in my application, I send those status updates over WebSockets for a web UI display.
|
I have replaced receive_find_map with a new send_get_reply in my branch (g2p/rust-cast@queue-load...async,queue), now everything is plugged properly so that receive() only gets non-replies and wakes send_get_reply() futures, while send_get_reply() polls the stream for a matching requestId even if no receive() is running. The internals were refactored, (removing code around deserialisation, reworking MessageManager) but the external channel interface is basically the same, just async. Maybe there's a way to add some per-channel behaviour like keeping track of media statuses, which would go beyond the current request-reply interfaces. One thing that bugs me with the current request-reply pattern is that it's hard to track the latest media status (you have to track every response to a media channel method call, as well as messages from receive()), and you can't always get the latest receiver status (when calling set_volume(), the channel interface discards the reply to keep only a simplified volume struct, so you have to follow up with an extra get_status). It's motivation to add something that listens to every message on select channels. |
I do this in my branch. I broadcast on a channel a variety of status messages, including media and receiver status, for the reason you describe. I also updated the set volume method to return the receiver's status, not just the volume struct. @g2p, I feel like between my branch and yours we are duplicating exactly the same work between us. Is there an alternative? @azasypkin, these 2 branches and approaches are diverging and will not merge with each other. What do you want to do in this repo? |
Thanks for the ping @fluffysquirrels! Assuming our common goal here is to have an async API in addition to the sync one as soon as possible, would it be feasible for you and @g2p to collaborate on picking and implementing a single approach that sounds sensible for both of you? You both definitely use this library more than I do, and ideally it would make sense for you to shape it instead of me dictating the approach. With this in mind, I see three options:
I'd be happy to assist with additional review if needed or unblock us if we're stuck again for some reason. How does that sound @fluffysquirrels @g2p? |
Hey @fluffysquirrels, Assuming you haven't had any discussions with @g2p outside of this issue, and you still have the capacity and desire to push the async support over the finish line, let's proceed with your PR. @g2p, it would be great if you could still be the primary reviewer for @fluffysquirrels' async PR so that it also caters to your use case. Thanks. |
Hi folks, I just wanted to let you know that I've created a new separate crate which is async-only. I tried out the above linked WIP branches, but sadly the underlying TCP socket couldn't receive and send data simultaneously, which was needed for my use case (Shortwave). I tried to refactor the underlying mechanism without rewriting half of this crate, which sadly didn't work out. For the new crate I used a different API approach, which hopefully makes it a bit easier for implementing apps. For example, you don't need to take care of heartbeat messages anymore. |
Hi,
I am interested in an
async
module that communicates with my Chromecast as part of a web application.Would you open to collaborating on this? Alternatively I can make an
async
version and link back to this repo as the source of much of the work.The text was updated successfully, but these errors were encountered: