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

chore: bump tokio to v1 #55

Merged
merged 1 commit into from
Mar 23, 2021
Merged

Conversation

oll3
Copy link
Contributor

@oll3 oll3 commented Mar 19, 2021

Bump tokio to version 1.x.
Use tokio AsyncFd since tokio no longer exposes mio internals.

Copy link
Member

@eldruin eldruin left a 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

@oll3
Copy link
Contributor Author

oll3 commented Mar 20, 2021

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 LineEventHandle::read_event to a regular std::io::read (eg let bytes_read = self.file.read(&mut data_as_buf)?;)?
I did it mainly to get a regular std::io::error instead of the unix specific one (which is expected to be returned by the closure passed to try_io()). Using the regular std::io::read also enforces the file descriptor to be mut.
Could probably be worked around either with another tokio specific read_event() or by mapping errors. Those solutions just seemed a bit more cumbersome than this.

Btw, I tested this locally and worked seemingly the same as before the tokio bump.

Copy link
Member

@eldruin eldruin left a 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 :)

Cargo.toml Outdated Show resolved Hide resolved
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)

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)?
} ...

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link

@ryankurte ryankurte left a 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?

Copy link
Member

@posborne posborne left a 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

@ryankurte
Copy link

^_^

bors r+

@bors
Copy link
Contributor

bors bot commented Mar 23, 2021

Build succeeded:

@bors bors bot merged commit 6d44537 into rust-embedded:master Mar 23, 2021
@eldruin eldruin mentioned this pull request May 18, 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.

4 participants