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

wip: update to embedded-io #39

Closed
wants to merge 2 commits into from
Closed

wip: update to embedded-io #39

wants to merge 2 commits into from

Conversation

lulf
Copy link
Member

@lulf lulf commented Aug 26, 2023

@rmja Currently awaiting rmja/buffered-io#1

@rmja rmja marked this pull request as ready for review August 26, 2023 20:53
@rmja
Copy link
Member

rmja commented Aug 26, 2023

I have released buffered-io v 0.2.1 that now works with embedded-io 0.5.
There is one thing that I am not certain about over there, and that is how to handle the new WriteAllError: https://github.com/rmja/buffered-io/blob/master/src/asynch/write.rs#L62
Any good ideas?

@lulf
Copy link
Member Author

lulf commented Aug 27, 2023

For embedded tls i went with propagating it https://github.com/drogue-iot/embedded-tls/blob/main/src/lib.rs#L138-L139

But for buffered.. maybe its ok with zero?

@rmja
Copy link
Member

rmja commented Aug 27, 2023

Year, in your case you already have a custom error type in which you can have a variant where the case can be assigned to. I do not have that in my case, as the error type is transparent to the buffering layer. I can see multiple places where this new error type can cause annoyances. I am really uncertain what the best way to handle this case is.

@rmja
Copy link
Member

rmja commented Aug 27, 2023

Right now I simply ignore the error and discard the currently buffered bytes which is clearly not correct.

@lulf
Copy link
Member Author

lulf commented Aug 27, 2023

Hmm, yeah, that is problematic. I guess there is no way around creating an error type specific to buffered-io then? Or maybe it would help if ErrorKind had a variant for WriteZero so that it WriteAllError could provide a kind() ?

@rmja
Copy link
Member

rmja commented Aug 29, 2023

I have now released buffered-io v. 0.3 which now fixes the outstanding issue where the WriteAllError::ZeroWrite error variant is ignored. It however adds a constraint on the error type to the below transport: https://github.com/rmja/buffered-io/blob/master/src/asynch/write.rs#L29-L30. This is really annoying, as it kind of cascades up to the consumer. I have tried to align reqwless to this change, and it seems to add a-lot of constraints all over the place. For tests that uses Vec as a transport, this also requires rust-embedded/embedded-hal#491.

I have looked at e.g. https://github.com/MabezDev/embedded-fatfs for comparison. It does something similar to circumvent this, but it is really annoying to work with.

@rmja
Copy link
Member

rmja commented Aug 31, 2023

I think that the WriteAllError/WriteFmtError error types will be removed again in the next release of embedded-io, at least that seemed to be the consensus at the last wg meeting. Lets wait for a final decision there and a possible release, before we proceed with this.

@lulf
Copy link
Member Author

lulf commented Aug 31, 2023

Agreed, thanks for following up on that!

@rmja
Copy link
Member

rmja commented Oct 5, 2023

buffered-io v 0.4 is now released implementing embedded-io 0.6, hence this should not block any further work here.

@lulf lulf closed this Oct 5, 2023
@lulf
Copy link
Member Author

lulf commented Oct 5, 2023

Continued in #41

@lulf lulf deleted the update-e-io branch October 5, 2023 12:54
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.

2 participants