-
Notifications
You must be signed in to change notification settings - Fork 23
Use futures_io::{AsyncRead, AsyncWrite} for L2Cap Streams
#38
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
Conversation
|
That looks pretty good to me. @lulf can you review how the proposed L2CAP API would work for your use-case? |
|
I'll give this a spin and a review, thanks @abezukor. |
| let stream = pin::pin!(&mut self.stream); | ||
| let ret = stream.poll_write(cx, buf); | ||
| ret | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we still expose the non-blocking try_write or is it available via some traits?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been using it as writer.write_all(&buf).await, from future-lite's pub trait AsyncWriteExt: AsyncWrite.
I think it does repeated poll_write(&whatevers_left_buf) until it returns Ok((0)) (written bytes) or Err.
Worked no prob for writing 100kb bytes buf
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but I need to be able to write to the output buffer in a non-async context, without blocking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use writer.write(...).now_or_never(). Or (equivalently) you could call writer.poll_write(Context::from_waker(Waker::noop()). They're not the most ergonomic options, but we could provide a try_write impl based on them if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, thanks. now_or_never() was what I was looking for. It's not that easy to discover, but works for me.
lulf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so I confirm this works well on both android and ios for us
|
Thanks! @abezukor I think this should be ok to merge one the CI issues are resolved. I'd like to keep L2CAP unstable until we have the Linux implementation, just to be sure there's nothing we need to change. |
Thanks. I am on vacation without a laptop this week, but I should be able to fix the CI next week. I can also follow up with a Linux implementation. |
|
+1, kudos for the PR! I needed l2cap on android and linux, so been playing with it, adding linux l2cap via AsyncRead AsyncWrite traits as here (been of great help as reference). Could submit a PR once this gets merged, if that's of use? (also added l2cap listener for android and linux, advertising, and extended scan fn with sercive-uuid filter) |
d5a6919 to
2655b5f
Compare
As discussed in my previous PR (#18) and (#33), it is a better interface to implement
AsyncReadandAsyncWritefrom futures-io, rather than use custom functions. This PR switches to that interface.Open Questions
Should we remove the unstable feature? Are there any more changes expected for L2Cap?
Todos