Skip to content

Replace fatfs::io traits with embedded-io traits #84

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

Closed
wants to merge 1 commit into from

Conversation

MabezDev
Copy link

@MabezDev MabezDev commented May 6, 2023

This slotted in fairly easily, due to how similar the two sets of traits were. The only real difference is how UnexpectedEof is handled. Previously this crate had methods on the IoError trait to create an UnexpectedEof error, in embedded-io the read_exact method actually returns an entirely different enum, one which handles the UnexpectedEof or other io errors.

I believe this is a good change, for a number of reasons. Firstly embedded-io is almost a drop-in solution, and by switching more of the ecosystem can use this crate. embedded-io is established in the community and growing in popularity. embedded-io has a set of async traits which may allow this crate to take advantage of async io in the future.

@MabezDev MabezDev force-pushed the feature/embedded-io-only branch from aedc2d3 to b84cfa3 Compare May 6, 2023 16:31
@MabezDev
Copy link
Author

MabezDev commented May 6, 2023

Hmm, I guess one small downside to this is that we need edition 2021, I'll bump that and the MSRV too. Actually, I think its just a CI error, thought I can't seem to figure out what's causing it?

@MabezDev MabezDev force-pushed the feature/embedded-io-only branch from ec39e4a to b84cfa3 Compare May 6, 2023 17:08
@MabezDev MabezDev force-pushed the feature/embedded-io-only branch from b84cfa3 to 19dfd73 Compare May 8, 2023 15:16
@MabezDev MabezDev marked this pull request as draft May 11, 2023 20:13
@MabezDev
Copy link
Author

Converting to draft as embedded-io needs to properly support WriteZero errors, relevant PR: embassy-rs/embedded-io#18

@Dirbaio
Copy link

Dirbaio commented May 12, 2023

the io traits (both std::io and embedded-io) represent "any byte stream". Perhaps a "block device" trait would make more sense here?

For example, the contract of the io traits doesn't specify all seeks/reads/writes have to be aligned to the block size, but for io implementations for block devices they have to be...

@MabezDev
Copy link
Author

the io traits (both std::io and embedded-io) represent "any byte stream". Perhaps a "block device" trait would make more sense here?

Good point! When I used version 0.3 of this crate with core_io, I ended up having to write a BufferredSdCard Wrapper which would ensure that any byte stream access went through as aligned block size reads and writes. I guess it is up to @rafalh whether it should be handled inside rust-fatfs as a block device or if the wrapper approach would be preferred.

@Dominaezzz
Copy link

Wrapper approach is probably better as I wouldn't want to have to conform to aligned reads for non block devices like EEPROMs.

@MabezDev
Copy link
Author

MabezDev commented Jul 12, 2023

FYI embedded-io has now been moved into rust-embedded/embedded-hal, where it will be used for the Serial traits and general I/O (rust-embedded/embedded-hal#466), further cementing the value of the crate.

@Dominaezzz
Copy link

0.5.0 was recently released so WriteZero is now available for use here

@MabezDev
Copy link
Author

MabezDev commented Sep 4, 2023

I won't be updating this PR any longer, as I have decided to hard fork to add async support here: https://github.com/MabezDev/embedded-fatfs. My fork uses the embedded-io traits, if you need blocking support I'd happily accept a PR which adds a blocking layer over the async API.

I hope this PR is still useful, anyone is welcome to salvage it to get e-io support in rust-fatfs.

@MabezDev MabezDev closed this Sep 4, 2023
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.

3 participants