Skip to content
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

Tokio v1.0.0 support. #33

Closed
wants to merge 5 commits into from
Closed

Tokio v1.0.0 support. #33

wants to merge 5 commits into from

Conversation

mnbbrown
Copy link

@mnbbrown mnbbrown commented Dec 26, 2020

Opening a PR incase it's helpful. New to rust so not sure if it's the best implementation.

Draft because:

  • cleanup
  • windows support?

Addresses #31 and #32

@wess
Copy link

wess commented Jan 5, 2021

Are you looking for someone to help out with your PR? I would love to get upstream using tokio 1.

@ivomurrell
Copy link

I think with tokio's replacement of PollEvented with the Unix-specific AsyncFd there is no longer a clean way to integrate tokio and mio-serial on Windows. Perhaps Windows support should be dropped until a Windows equivalent of AsyncFd is implemented in tokio?

@jadamcrain
Copy link

I've talked with the Tokio folks on discord, and there is literally no way to hook an asynchronous serial port via a windows Handle in 1.0.

The only solution to provide Windows support in this version of Tokio would be to run the windows port on a standard thread, and then tie it to the async resource using something like a DuplexStream:

https://docs.rs/tokio/1.0.1/tokio/io/struct.DuplexStream.html

Long term, we need to look into how Tokio implements the Windows iodriver to see if the comm port handle could be plugin via a similar wrapper like AsyncFd.

@jadamcrain
Copy link

BTW, I've raised an issue on Tokio so that we can coordinate with them to see if there is a path forward:

tokio-rs/tokio#3396

ivomurrell added a commit to MoixaEnergy/tokio-modbus that referenced this pull request Jan 11, 2021
With the move to tokio 1.0, it looks likely that tokio-serial will drop
support for Windows until tokio adds a Windows equivalent of `AsyncFd`.
(See berkowski/tokio-serial#33)

Let's stop running CI on Windows in response. Not removing cfg platform
gates out of the code for now in case we are able to restore Windows as
target later on.
ivomurrell added a commit to MoixaEnergy/tokio-modbus that referenced this pull request Jan 11, 2021
With the move to tokio 1.0, it looks likely that tokio-serial will drop
support for Windows until tokio adds a Windows equivalent of `AsyncFd`.
(See berkowski/tokio-serial#33)

Let's stop running CI on Windows in response. Not removing cfg platform
gates out of the code for now in case we are able to restore Windows as
target later on.
return Ok(buf.advance(read));
}) {
Ok(result) => return Poll::Ready(result),
Err(_would_block) => return Poll::Pending,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks as the read() can return would block in which case you go pending without the async fd actually registered with the runtime; The original version with the loop is correct as read() returns would_block then the poll_read_ready() call should return Pending and everything should work nicely :).

fwiw i hit this problem in practise while playing with this branch and a fdti usb dongle in linux

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused about this: if the read returns EWOULDBLOCK doesn't that imply a bug somewhere else? We can't arrive at line 227 without being signaled by tokio that there are in fact some bytes to read. I haven't been using this branch but I have some code that is morally equiv that I have been testing with ftdi dongles (snap!) and I haven't seen this condition triggered (yet).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is possible to get a false positive from poll_read_ready and get the would block error, but the snippet is still incorrect — you have to loop around and call poll_read_ready again if that happens.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you are saying. Thanks for making that connection. I'd conflated the clearing of the tokio-side readiness state (which is handled by try_io) with the need to poll again to get registered. I also completely misread the sample code for AsyncFd. I guess I'd just gotten (un)lucky with my testing :)

Copy link

@mateuszkj mateuszkj Jun 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnbbrown Please look at PR: mnbbrown#1

We got bug with "hanging" usart and adding loops fixed the problem.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm I have been seeing this bug too (when working on tokio-modbus's update to tokio-1.0) and rfael's patch fixed it.

@sjoerdsimons
Copy link

As the discussion on the tokio side for windows support is still ongoing; Is there a chance this could move foward without windows support as a step for e.g. tokio-serial 5.0 ?

@berkowski
Copy link
Owner

I'll finally have time to work on this, but first up is getting berkowski/mio-serial#23 resolved.

@Darksonn
Copy link
Contributor

Darksonn commented May 5, 2021

Hey. I am currently looking into the Tokio situation on windows. Would named pipes alone resolve the windows support, or do you need some sort of AsyncHandle beyond that to support it?

@jadamcrain
Copy link

jadamcrain commented Jun 5, 2021

Any updates on this @berkowski or @Darksonn?

Getting serial working again with Tokio on Windows would be something my firm would be willing to financially sponsor.

If this would help move this along in any way, please reach out to me: adam@stepfunc.io

@Darksonn
Copy link
Contributor

Darksonn commented Jun 6, 2021

Named pipes are coming along nicely in tokio-rs/tokio#3760, but I haven't heard anything from @berkowski in several months, so it's unclear what is going to be happening on the tokio-serial side.

@david-mcgillicuddy-moixa

Named pipes just got released https://github.com/tokio-rs/tokio/releases/tag/tokio-1.7.0 🎊

@ColinFinck
Copy link
Collaborator

I've implemented the missing Windows part in https://github.com/enlyze/tokio-serial/tree/v4.4.0_serialstream_multiplatform
See also #39

This works well for me under Windows and Linux.
Requires pinning branches/revisions in Cargo.toml as long as they are not merged to master and released on crates.io. But other than that, I'm finally able to use serial ports with mio 0.7.x and tokio 1.x :)

@berkowski
Copy link
Owner

beta support for tokio 1.X available on crates.io with release 5.4.0-beta1.

@berkowski berkowski closed this Jul 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet