blocking/spi: Don't return the same buffer back.

[Dirbaio]

This is a rather minor improvement to the blocking SPI Transfer trait, but that IMO is still worth it.

Old: fn transfer<'w>(&mut self, words: &'w mut [W]) -> Result<&'w [W], Self::Error>
New: fn transfer(&mut self, words: &mut [W]) -> Result<(), Self::Error>

It is redundant (and confusing IMO) to return back the same buffer the user has passed in. It allows for 2 ways of writing the same thing: use the returned buffer, or ignore it and use the buffer that was passed in. It's not obvious which is better, and from the docs it's not obvious that both are really the same either.

Also, with the old signature, it is possible for implementors to return a subslice of the buffer instead of the whole buffer. This would be arguably wrong and would break user code that assumes the entire buffer is returned. Not re-returning the buffer makes this mistake impossible.

[rust-highfive]

r? @ryankurte

(rust-highfive has picked a reviewer for you, use r? to override)

[lachlansneff]

One thing that I just thought of here. If the read buffers get switched over to MaybeUninit, then returning the buffer as a slice of words lets people use the api without unsafe. There is the issue of returning a subslice though.

[Dirbaio]

True!

The propsed change is what I feel makes most sense given the current convention of not using MaybeUninit or similar.

Maybe you could open an issue on "add a way to read into uninitialized buffers" and we can discuss there :)

[David-OConnor]

Agreed. I noticed this recently as well, and was surprised. Eg this instead:

    /// Read multiple bytes to a buffer, blocking.
   fn transfer<'w>(&mut self, words: &'w mut [W]) -> Result<(), Error> {
        for word in words.iter_mut() {
            nb::block!(self.write_one(word.clone()))?;
            *word = nb::block!(self.read())?;
        }

        Ok(())
    }

[Dirbaio]

ping @eldruin @ryankurte @therealprof

I believe this should be fairly uncontroversial. The question of "what about MaybeUninit" is better debated in its own issue, and the verdict from that applied to all traits. I don't think it justifies keeping the returned slice here "just in case": no other trait does this, and we can always add it back if we do MaybeUninit.

[eldruin]

Looks good to me, thanks!

[therealprof]

@japaric Since you added the API way back when, did you have any intentions with regards to this particular interface? Any objections to removing it?

[ryankurte]

i guess without the returned subslice (or an n) you can't easily signal partial-success (eg, tried to transfer four bytes, only achieved three), but, it'd probably be less surprising to signal that as an error (especially with the new error types) than to require people to -also- check n on success. pending any thoughts by @japaric, sgtm.

[therealprof]

As discussed last week we're going to merge this pending objections. @Dirbaio Would you mind fixing the conflicts?

[Dirbaio]

@therealprof rebased

[eldruin]

uh, CI is broken due to the stm32f1 dev dependency. Updating it does not help: #304

[eldruin]

Alright, #305 should fix CI, then we can merge this.

[therealprof]

bors try

[bors]

try

Build succeeded:

• fmt
• ci-linux (stable, thumbv7m-none-eabi)
• ci-linux (stable, thumbv6m-none-eabi)
• ci-linux (stable, x86_64-unknown-linux-gnu)
• ci-linux (1.35.0, x86_64-unknown-linux-gnu)
• ci-linux-test (stable)
• ci-linux-test (1.36.0, x86_64-unknown-linux-gnu)

[therealprof]

LGTM, thanks!

bors r+

[bors]

Build succeeded:

• ci-linux (1.35.0, x86_64-unknown-linux-gnu)
• ci-linux (stable, thumbv6m-none-eabi)
• ci-linux (stable, thumbv7m-none-eabi)
• ci-linux (stable, x86_64-unknown-linux-gnu)
• ci-linux-test (1.36.0, x86_64-unknown-linux-gnu)
• ci-linux-test (stable)
• fmt