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

feat: allow blocking read #185

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ARizzo35
Copy link

@ARizzo35 ARizzo35 commented May 7, 2024

Fixes #12, similar to #50 but without the extra functions.

@ARizzo35
Copy link
Author

ARizzo35 commented May 7, 2024

cc @sirhcel

@sirhcel
Copy link
Contributor

sirhcel commented May 10, 2024

Thank you for the PR!

Fixes #12, similar to #50 but without the extra functions.

Yes, it does not need extra functions. But on the other hand switching from Duration to Option<Duration> breaks the current API and this change needs to wait for the next major release.

What about designating Duration::MAX as no timeout as proposed in #12 (comment) for the public interface meanwhile?

  • Documenting this quirk
  • Adding a no_timeout function to SerialPortBuilder and its implementation for making the intent explicit
  • Returning Duration::MAX in case no timeout is set

And with the next release switching switching from Duration to Option<Duration>.

@sirhcel
Copy link
Contributor

sirhcel commented May 10, 2024

What issue are you addressing with this PR in the first place @ARizzo35? I'm asking because the changes also include changing the default timeout used by SerialPortBuilder which might break the expectations of a lot of users.

I'm not trying to say that a default timeout of 0 ms is that what I would expect here. But that's the current default.

Prepared some changes for keeping the current API while allowing to make changing to no timeout more explicitly expressed in https://github.com/sirhcel/serialport-rs/commits/blocking-read/.

What's you opinion of timely providing a clean way of expressing no timeout and switching to a set_timeout(Option<Duration>) with the next major release @eldruin?

@ARizzo35
Copy link
Author

What issue are you addressing with this PR in the first place @ARizzo35? I'm asking because the changes also include changing the default timeout used by SerialPortBuilder which might break the expectations of a lot of users.

I'm not trying to say that a default timeout of 0 ms is that what I would expect here. But that's the current default.

Prepared some changes for keeping the current API while allowing to make changing to no timeout more explicitly expressed in https://github.com/sirhcel/serialport-rs/commits/blocking-read/.

What's you opinion of timely providing a clean way of expressing no timeout and switching to a set_timeout(Option<Duration>) with the next major release @eldruin?

I am trying to add support for blocking read in poll() / ppoll(). Setting a large duration with the current API achieves the same effect, but it is not the actual correct solution. It's also confusing to use the default timeout of 0 and then see your program using 100% CPU because it is busy looping on a non-blocking read somewhere. Any other Rust library implementation that impls Read will block (e.g. stdin) so things like for line in lines() from BufReader do not actually end up consuming a lot of CPU usage, however, when using that pattern with this serial port library, it causes very high CPU usage since the read does not actually block.

@sirhcel
Copy link
Contributor

sirhcel commented May 12, 2024

I am trying to add support for blocking read in poll() / ppoll().

Thank you for the context! So you are trying to read the available data after being notified? Are you switching the serial port to non-blocking for this task? If yes, using into_raw_fd?

Setting a large duration with the current API achieves the same effect, but it is not the actual correct solution.

Yes, the current default timeout is - lets say - somewhat special. I would have expected this to be blocking/no timeout on POSIX systems.

It's also confusing to use the default timeout of 0 and then see your program using 100% CPU because it is busy looping on a non-blocking read somewhere. Any other Rust library implementation that impls Read will block (e.g. stdin) so things like for line in lines() from BufReader do not actually end up consuming a lot of CPU usage, however, when using that pattern with this serial port library, it causes very high CPU usage since the read does not actually block.

Yes, this is another annoyance with the current default.

However, changing the default right now will likely break the somewhat special but expected behavior for other users. Therefor I would like to split things up: Allowing to specify no timeout now more explicitly in the short term. Switching the default behavior with the next major release and provide a cleaned up API for setting timeouts with this release.

@eldruin
Copy link
Contributor

eldruin commented May 22, 2024

Allowing to specify no timeout now more explicitly in the short term. Switching the default behavior with the next major release and provide a cleaned up API for setting timeouts with this release.

Sounds good to me!

@sirhcel sirhcel added this to the 5.0.0 milestone Jul 29, 2024
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.

read is always non-blocking even when no data is available to read
3 participants