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

embedded-io compatibility #653

Closed
wants to merge 4 commits into from
Closed

Conversation

bugadani
Copy link

Resolves #652

I'm opening this PR early because I have a few open questions:

  • embedded-io traits have an associated error type for IO errors. How should we incorporate this?
    • we can make EncodeError/DecodeError generic, and default to std::io::Error
    • we can introduce a custom io error type, map known std errors to it, and map embedded-io errors to EOF (ReadExact error) or Other.
  • decode_from_embedded_io_read is a bit verbose. I think I'd try to create a single encode/decode function and abstract the std/embedded options with a bincode-provided marker trait. The implementation would be more complex, but the API would be simpler. What is your preference here?

Right now we can't have both std and embedded-io features active, because of the IO error question. I intend this as a temporary restriction and plan on lifting it if the resolution of that question allows.

@VictorKoenders
Copy link
Contributor

Thanks for this!

embedded-io traits have an associated error type for IO errors.

I think make an EmbeddedIo { ... } variant, and put that behind the config flag. They're completely different errors from std's Io errors. In hindsight we maybe should've named the existing variant StdIo.

The error type is marked as #[non_exhaustive] so people cannot match on all variants, so I believe it is fine to add variants based on features being active

decode_from_embedded_io_read is a bit verbose.

What about bincode::embedded_io::decode_from_read or bincode::embedded_io::decode?

@VictorKoenders
Copy link
Contributor

Also please add a line to the features table in src/lib.rs

@bugadani

This comment was marked as outdated.

@bugadani
Copy link
Author

bugadani commented Jun 20, 2023

Current implementation ignores the actual embedded-io error for simplicity. Trying to make Decode/EncodeError generic is at this point not practical.

I have also fixed a few typos I found.

@bugadani bugadani marked this pull request as ready for review June 20, 2023 12:14
@stale
Copy link

stale bot commented Aug 21, 2023

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Aug 21, 2023
@bugadani bugadani closed this Aug 22, 2023
@bugadani bugadani deleted the embedded-io branch August 22, 2023 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature request: embedded-io compatibility
2 participants