rust-embedded / embedded-hal

A Hardware Abstraction Layer (HAL) for embedded systems
Apache License 2.0
1.95k stars 197 forks source link

Updating write functions to return length of written bytes #502

Closed ryan-summers closed 11 months ago

ryan-summers commented 1 year ago

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.
MabezDev commented 1 year ago

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 (https://github.com/rust-lang/rust/issues/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 commented 1 year ago

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 (https://github.com/rust-lang/rust/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

MabezDev commented 1 year ago

I opened a PR here: https://github.com/rust-embedded/embedded-hal/pull/507

ryan-summers commented 11 months ago

Closing this as it was superseded by https://github.com/rust-embedded/embedded-hal/pull/507 (thanks @MabezDev! I got busier than expected)