Changes to the spi module

[bradleyharden]

@jbeaurivage, separate from the discussion in #614, I had another realization about the spi module.

Right now, I take the same approach as the uart module and define Spi as

pub struct Spi<C, A>
where
    C: ValidConfig,
    A: Capability,
{
    config: C,
    capability: A,
}

I used the Capability type parameter in a few cases when implementing the embedded_hal traits.

However, while trying to simplify and consolidate those implementations, I realized that the Capability was only ever used to modify the behavior of the implementation. It was never used to change the type signature or API in some way.

I realized that changing behavior could be equally-well accomplished by attaching an associated const to the Capability trait. In #613, I added a DynCapability enum,

pub enum DynCapability {
    Rx,
    Tx,
    Duplex,
}

and I altered the Capability trait to be:

pub trait Capability: {
    const DYN: DynCapability;
}

Then, within the embedded_hal implementations, I can switch based on the DynCapability const. This is just as good as using a type parameter, because all the functions are inlined, and constant propagation will ensure only one branch exists in the final code.

With this approach, I could remove the Capability type parameter from Spi. Thus, the new type becomes

pub struct Spi<P, M, Z>
where
    P: ValidPads,
    M: OpMode,
    Z: Size,
{
    // ...
}

and the implementations switch on Capability by doing something like this

impl<P, M, Z> Trait for Spi<P, M, Z>
where
    P: ValidPads,
    P::Capability: Transmit, // I can still limit `Trait` to `Transmit`-only
    M: OpMode,
    Z: Size,
{
    type Error = Error;

    #[inline]
    fn write(&mut self) {
        // But I don't need to have `Capability` as a separate type parameter,
        // because I can check it here
        if P::Capability::DYN == DynCapability::Duplex {
            duplex_impl(self)
        } else {
            tx_impl(self)
        }
    }
}

You can't do the same for the uart module, because you rely on the Capability type parameter to change the API of each Uart struct. But is it necessary for the i2c module? Could we make a similar change there, perhaps as part of a broader initiative to achieve uniformity, as discussed in #614?

[bradleyharden]

Oh, and one other topic. I've started to think we should make the read_data and write_data methods safe. Keeping them unsafe adds quite a bit of unsafe noise to the implementations of embedded_hal traits. Moreover, I don't know that you could get memory unsafety from it using embedded_hal alone. Worst case, I think you would just deadlock waiting on a DRE flag or something.

I think you would need to combine raw reads of the DATA register together with DMA or something to get memory corruption. But maybe that's still a good enough argument to keep it unsafe?

Edit: Actually, wouldn't DMA take ownership of the peripheral, so you could never call the read_data and write_data methods in that case?

[jbeaurivage]

That's a good idea, I think either approach works well enough. This one is nice because it removes a type parameter, although I don't actually mind having a capability type parameter, because it's super clear what the peripheral can do. I think the compiler error messages will also help? Maybe it would in this case too. That being said, I don't mind going with an approach or another, and if you choose to use this one, I can update I2C as well.

As far as marking read_data and write_data as safe: Technically the methods probably aren't memory unsafe, but I wouldn't want uninformed users using them instead of the embedded-hal implementations. Maybe we could:

• Mark them as safe but make them pub(crate)
• Mark them as safe but with a very visible disclaimer in the docs

[bradleyharden]

@jbeaurivage, I realized something. We could create a safe, pub(super) interface for internal use, and an unsafe pub interface for external use. I think I'll do that.

[bradleyharden]

@jbeaurivage, I updated #613 with two separate interfaces to the DATA register. Any chance you could test it out? Would it be easy for you? I might be able to test it myself here soon, but there are some other things in the way.

[jbeaurivage]

I'm a little short on time these days, but I might be able to test it over next weekend. I won't have access to my scope or LA so my debugging capabilities will be a little limited