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

Updating write functions to return length of written bytes #502

Closed
wants to merge 4 commits into from

Conversation

ryan-summers
Copy link
Contributor

@ryan-summers ryan-summers commented Sep 7, 2023

This PR updates the embedded_io::Write trait to return the number of bytes written for the write_fmt function. It seems the length was originally omitted out of simplicity/lack of demand.

However, during my usage, I noted that without this, using a byte slice with write!() doesn't provide a result to indicate where the written object ends:

let mut bytes = [0; 1024];
let _should_be_len = write!(&mut bytes[..], "{:?}", MyObjectThatImplsDebug).unwrap();

// At this point, there is no way to know how many bytes in `bytes` were written, so we don't know where the formatting ends.

@ryan-summers ryan-summers requested a review from a team as a code owner September 7, 2023 09:05
@MabezDev
Copy link
Member

MabezDev commented Sep 7, 2023

This breaks compatibility with std::io::Write. Whether that's a bad thing or not in this case is unclear because imo it was a mistake in the write_fmt signature to not return the number of bytes written (rust-lang/rust#63614).

An alternative to this, which I have used in the past before even embedded-io existed is to use a wrapper type which counts the number of bytes written to the inner buffer, which you can then retrieve after you're done formatting. Perhaps something like this could be added to this crate to avoid people rolling their own.

@ryan-summers
Copy link
Contributor Author

ryan-summers commented Sep 7, 2023

This breaks compatibility with std::io::Write. Whether that's a bad thing or not in this case is unclear because imo it was a mistake in the write_fmt signature to not return the number of bytes written (rust-lang/rust#63614).

I guess a valid question would be whether or not we're trying to be ane exact replacement. If so, then I agree that we should keep the same function signature.

An alternative to this, which I have used in the past before even embedded-io existed is to use a wrapper type which counts the number of bytes written to the inner buffer, which you can then retrieve after you're done formatting. Perhaps something like this could be added to this crate to avoid people rolling their own.

Yeah, which is essentially the embedded equivalent of std::io::Cursor. I'm not opposed to this approach.


Edit: To answer my own question, there's already some documented differences: https://github.com/rust-embedded/embedded-hal/tree/master/embedded-io#differences-with-stdio

@ryan-summers
Copy link
Contributor Author

ryan-summers commented Sep 7, 2023

Using the Cursor approach results in some pretty awful code. For some reason, the compiler is not able to determine the return arguments from the write!() macro, so you have to tell it the explicit error type.

Here's an example where a closure takes an error: impl Debug and a buf to format the error into. The closure returns a Result<usize, E>

let mut cursor = Cursor::new(buf);
write!(cursor, "{:?}", error)?;
Ok::<_, embedded_io::WriteFmtError<core::convert::Infallible>>(cursor.position())

@MabezDev
Copy link
Member

MabezDev commented Sep 7, 2023

I guess a valid question would be whether or not we're trying to be ane exact replacement. If so, then I agree that we should keep the same function signature.

Good question! I'm not sure of the long-term goal of embedded-io. I guess @Dirbaio has an idea.

Edit: To answer my own question, there's already some documented differences: https://github.com/rust-embedded/embedded-hal/tree/master/embedded-io#differences-with-stdio

I think the main difference here is that those are differences because of limitations in core, and where possible we add std impls so that std continues to work as expected. This would be a conscious decision to make a breaking change to improve the API.

@jannic
Copy link
Member

jannic commented Sep 7, 2023

The write_fmt function in std has the following description:

Glue for usage of the [write!](https://doc.rust-lang.org/std/macro.write.html) macro with implementors of this trait.

This method should generally not be invoked manually, but rather through the [write!](https://doc.rust-lang.org/std/macro.write.html) macro itself.

And the write! macro doesn't specify exactly what its return value would be:

The macro returns whatever the write_fmt method returns; commonly a [fmt::Result](https://doc.rust-lang.org/std/fmt/type.Result.html), or an [io::Result](https://doc.rust-lang.org/std/io/type.Result.html).

So code using the std version of this method probably doesn't rely on an exact type being returned anyway. Changing the embedded-io version to return the bytes written doesn't look like a bad idea to me.

@Dirbaio
Copy link
Member

Dirbaio commented Sep 7, 2023

The Write impl for &mut [u8] shortens the slice as you write, so you can calculate the amount of written data. This works for both std::io and embedded-io:

    let mut bytes = [0; 1024];

    let mut w = &mut bytes[..];
    let len = w.len();
    write!(w, "{:?}", 1234).unwrap();
    let n = len - w.len();

    println!("{}", n); // prints 4

Using the Cursor approach results in some pretty awful code. For some reason, the compiler is not able to determine the return arguments from the write!() macro, so you have to tell it the explicit error type.

How have you implemented Cursor? that looks like you haven't constrained the error type.


I'm not sure whether deviating from std::io is worth it on this. Getting the written length is already possible (with either the snippet above, or Cursor). I agree it's somewhat verbose, but so is std::io.

@ryan-summers
Copy link
Contributor Author

ryan-summers commented Sep 7, 2023

The Write impl for &mut [u8] shortens the slice as you write, so you can calculate the amount of written data.

Ah, this indeed solves my use-case quite well with a single extra function call, which is even easier than the Cursor-based approach. Thanks for the info!

How have you implemented Cursor? that looks like you haven't constrained the error type.

Apparently not because I got the exact same error when using your snippet as:

let start = buf.len();
write!(buf, "{}", MyType)?;
Ok(start - buf.len())

However, oddly enough doing the following fixed it. I have a suspicion its because Rustc may be being arbitrarily restrictive on inferring Closure return types:

let start = buf.len();
write!(buf, "{}", MyType).and_then(|_| Ok(start - buf.len())

For context, the exact code I'm referring to is here: https://github.com/quartiq/miniconf/pull/134/files#diff-8ce4ae904c14212d6bed3296e5ced8732145b011aa19e6cb2fa5c614698d6f4cR492-R495 - the DeferredPublication just takes an Fn(&mut [u8]) -> Result<usize, E>


In any case, this does indeed appear possible with the current API (albeit quite unintuitive to the user). I have no issue with closing this PR if we determine that we want to maintain exact API compatibility, although as @jannic pointed out, the return type isn't currently used anyways.

@eldruin
Copy link
Member

eldruin commented Sep 7, 2023

Could some of you document this problem and solution in the method or somewhere else in embedded-io?

@MabezDev
Copy link
Member

I opened a PR here: #507

@ryan-summers
Copy link
Contributor Author

Closing this as it was superseded by #507 (thanks @MabezDev! I got busier than expected)

@ryan-summers ryan-summers deleted the rs/write-len branch October 6, 2023 14:11
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.

5 participants