-
Notifications
You must be signed in to change notification settings - Fork 37
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
chore: bump tokio to v1 #55
Conversation
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.
Hello and thank you! This looks good to me but I do not know about tokio
so I would ask somebody else from @rust-embedded/embedded-linux for further review.
Could you also:
- Update MSRV in the README
- Add entry to changelog about update, noting what the changes to the interface are and that this is a breaking change
Will do! Just to elaborate a bit more on the change... I'm not sure if I have overlooked anything changing the unix specific read in Btw, I tested this locally and worked seemingly the same as before the tokio bump. |
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.
Looks good to me, thank you!
Let's wait for a review of a tokio
connoisseur from the team :)
Remove dependency of mio and use tokio AsyncFd instead. This since tokio no longer exposes mio internals. Also bump MSRV to 1.45, required due to tokio update.
let mut data: ffi::gpioevent_data = unsafe { mem::zeroed() }; | ||
let mut data_as_buf = unsafe { | ||
slice::from_raw_parts_mut( | ||
&mut data as *mut ffi::gpioevent_data as *mut u8, | ||
mem::size_of::<ffi::gpioevent_data>(), | ||
) | ||
}; | ||
let bytes_read = nix::unistd::read(self.file.as_raw_fd(), &mut data_as_buf)?; | ||
|
||
let bytes_read = self.file.read(&mut data_as_buf)?; | ||
if bytes_read != mem::size_of::<ffi::gpioevent_data>() { | ||
Ok(None) |
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.
again not really related to this PR (sorry, it's just caught my attention)... do you think there is any case in which a partial read should occur during operation?
i wonder whether this line is hiding dropping some data / a partial cause of #51, and whether this should instead be something like:
if bytes_read == 0 {
return Ok(None);
else if bytes_read < mem::sizeof::... {
// Error? Flush (drop events but recover)?
} ...
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.
Hm, possibly. Do you know if a gpio char device can return partial packets? Couldn't find anything about that.
I couldn't repeat that select!
error locally either, tested with master branch. Not running on a raspberry though.
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.
With the way that the file works in the kernel for a character device, I wouldn't ever expect a read call to return a partial structure. I'll have to consider the linked issue but I don't think that branch would ever be hit in practice.
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.
in that case all lgtm, thanks for your effort!
@posborne are you happy with this?
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.
@adamgreig Yep, LGTM
🤦 Meant @ryankurte
^_^ bors r+ |
Build succeeded: |
Bump tokio to version 1.x.
Use tokio AsyncFd since tokio no longer exposes mio internals.