Skip to content

Methods to read into a buffer should take an uninitialized buffer #288

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
lachlansneff opened this issue Jun 22, 2021 · 5 comments
Closed

Comments

@lachlansneff
Copy link
Contributor

Methods like blocking::i2c::Read::read should take &mut [MaybeUninit<u8>] instead of &mut [u8] to allow for uninitialized buffers to be used.

For example:

pub trait Read<A: AddressMode = SevenBitAddress> {
    type Error;
    fn read(&mut self, address: A, buffer: &mut [u8]) -> Result<(), Self::Error>;
}

should become

pub trait Read<A: AddressMode = SevenBitAddress> {
    type Error;
    fn read<'a>(&mut self, address: A, buffer: &'a mut [MaybeUninit<u8>]) -> Result<&'a [u8], Self::Error>;
}

Since slices of maybe uninit can only be converted to slices of normal types through unsafe, the methods should return the initiated slice.

Issues raised in #286 apply, namely that the returned slice may refer to a subslice of the input buffer. If that's a safety issue, the traits should be made unsafe to enforce that invariant.

@ryankurte
Copy link
Contributor

Methods like blocking::i2c::Read::read should take &mut [MaybeUninit] instead of &mut [u8] to allow for uninitialized buffers to be used.

i am not sure i understand the benefits of this approach? it would seem to push a bunch of use of unsafe onto hal consumers, practically you can initialise an array once and reuse it anyway, and it appears you could achieve the equivalent by transmuting a MaybeUninit yourself if you did need to repeatedly use a new uninitialized array and were convinced in your application that it was okay to do so.

@eldruin
Copy link
Member

eldruin commented Jun 24, 2021

I agree with @ryankurte here. MaybeUninit would make very few uses more explicit and there is already a way to do the same manually which would keep the unsafe portion well confined. Pushing unsafe everywhere to the user would be very cumbersome and mostly unnecessary.

@lachlansneff
Copy link
Contributor Author

@ryankurte I might be misunderstanding you, but you cannot transmute an uninit array to an initialized array. It will result in undefined behavior and the compiler will do weird things.

The point of returning the initialized array would mean that users wouldn't have to use unsafe.

@burrbull
Copy link
Member

The point of returning the initialized array would mean that users wouldn't have to use unsafe.

Absence of unsafe in user code does not outweigh the complexity of your solution.
In usual I'd prefer just use initialized array.
If I have MaybeUninit::uninit(), I can just use as_mut_ptr() with unsafe. No problem with this.

But most importantly I can reuse one array for multiple write. How I can do this if fn read require [MaybeUninit]?

Just create other ReadUninit trait if you need it so much. Don't break existent.

@Rahix
Copy link

Rahix commented Jun 24, 2021

I do not think the embedded-hal traits are the right place for this kind of API. The traits are supposed to fit the common usecase as best as possible to allow for code reuse across the ecosystem. Using uninitialized arrays is a specialized optimization and not something most users of the traits will care about.

Thus, I believe, the right place for such an API would be the HAL crates themselves. I would assume that anyone who is optimizing to this extend is most likely not using generic drivers anyway - instead they would want to interact with the hardware as directly as possible, to have full control over the performance of the code.

If it does turn out to be a common requirement, I think introducing a separate, special ReadUninit trait as @burrbull suggested, is a much better alternative.

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

No branches or pull requests

5 participants