5.0.0 proposal #34
Open
5.0.0 proposal #34
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change SerialPort to a struct, and move the implementation to a "sys" module which selects between different possible "real" implementations.
Add a public posix module containing a posix-specific SerialPortExt trait and the BreakDuration type.
Get the windows build working. Remove the windows-specific extensions module because windows doesn't have any methods not available on posix.
One of the CI toolchains uses an older version of rustc, which doesn't include as_deref, so we just do the same thing that as_deref does internally.
Implementing the extension in the posix extension mod ensures it will appear in documentation even if documentation is built on windows, since that mod is cfg(doc).
Current timeouts are based on [this comment from !78](https://gitlab.com/susurrus/serialport-rs/-/merge_requests/78#note_343695538), copying the windows timeout settings needed to get posix-like timeout behavior. That is, reads should return available data ASAP regardless of timeouts, but stop early when there is a timeout.
EventCache will hold the HANDLE for a read/write when not in use or will produce a new handle for each thread when multiple threads try to read/write at the same time. The cache holds a single value and will deallocate extra handles when there is already a handle stored. We have one cache for a read_handle and one cache for a write_handle. The expectation is that in the normal case, at most one thread will be reading/writing at a time, and so at most one handle will be created for each of read and write. But in abnormal cases, when multiple threads try to read/write at the same time, we auto-create and auto-close extra handles as needed. Fix the into_raw_handle/into_raw_fd to drop other fields that were previously leaked.
|
Thanks for the PR, sorry for the late response. Just to set your expectations, I likely won't be getting to this for awhile. I'm still working on getting all the details of the fork settled and would also like to release at least one more |
Open
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Removes the
SerialPorttrait, and instead creates astruct SerialPortwhich wraps and hides platform-specific serial port implementions (crate::sys::SerialPort). This removes the distinction between serial port trait objects and platform specific serial port structs, and prevents the need for usingBox<dyn SerialPort>to write platform-agnostic code, since theSerialPortstruct just compiles to the platform specific type. Platform specific behaviors are exposed as extension traits which only appear on the appropriate platform.This PR is mostly ready, but may still require some documentation updates. It also has not be tested thoroughly at this point, since I don't have access to a mac and haven't had time to do tests on windows or linux. If this proposal seems good, I can go ahead and test it a bit more thoroughly.