Guidelines for HAL impl

[jamesmunns]

Here are some notes I took, if this looks good to folks, it might be worth adding to our docs somewhere, or upstream to the HAL book. (edit: and also fixing all of our HAL impls to be consistent with this)

Notes for HAL implementation

When implementing a HAL peripheral for the nrf-hal-common library, please do the following:

Implement the HAL struct as Generic over an Instance trait

Do this, so your code can be generic. Prefer this INSTEAD of using macros for implementation, to keep the code easy to read.

pub struct Peripheral<T>
where
    T: Instance,
{
    periph: T
}

Use a Sealed Trait to prevent external impls

Do this, so people can't build unsound code on our abstractions

// NOT PUB!
mod sealed {
    // Pub, but don't re-export!
    pub trait Sealed {}
    impl Sealed for super::PERIPHERAL0 {}
    impl Sealed for super::PERIPHERAL1 {}

    #[cfg(not(feature = "cheap_chip"))]
    impl Sealed for super::PERIPHERAL2 {}
}

Implement the Instance Trait like this

We want to bound the trait with our sealed trait, to prevent further impls. We also want to enumerate the correct interrupt associated with this peripheral, so that external libraries can trigger the right interrupt when necessary.

// Is Pub!
pub trait Instance: sealed::Sealed {
    const INTERRUPT: Interrupt;
}

impl Instance for pac::PERIPHERAL0 {
    const INTERRUPT: Interrupt = Interrupt::PERIPHERAL0;
}
impl Instance for pac::PERIPHERAL1 {
    const INTERRUPT: Interrupt = Interrupt::PERIPHERAL1;
}

#[cfg(not(feature = "cheap_chip"))]
impl Instance for pac::PERIPHERAL2 {
    const INTERRUPT: Interrupt = Interrupt::PERIPHERAL2;
}

[jamesmunns]

CC @hannobraun, and maybe @therealprof or @adamgreig (for upstream relevance)?

[timokroeger]

I think https://github.com/nrf-rs/nrf-hal/issues/8 is related

[therealprof]

Looks great!

Can we get a little more details on what an Instance is supposed to constitute? Is this a HAL impl wide common trait? Or is it a trait per peripheral kind? Except for the interrupt, what is supposed to be part of such a trait?

[hannobraun]

@therealprof

Can we get a little more details on what an Instance is supposed to constitute? Is this a HAL impl wide common trait? Or is it a trait per peripheral kind? Except for the interrupt, what is supposed to be part of such a trait?

I can't speak for James' intent, but I've pioneered the use of the Instance trait in this HAL and I've been using this style over in LPC8xx HAL for a long time now, so here's an explanation from my perspective:

• There is one Instance trait for every peripheral that has multiple instances. So there would be usart::Instance (implemented for USART0, USART1, ...), i2c::Instance (implemented for I2C0, I2C1, ...), spi::Instance (implemented for SPI0, SPI1, ...), etc.
• Peripherals that don't have multiple instances like that don't need an Instance trait.
• The point of it is to provide a clean separation between macro-generated instance-specific code and hand-written generic code. This is the best pattern I've found to achieve this.
• We add to that trait whatever is needed to implement a generic API on top of it. There's nothing dogmatic about it, from my perspective. It wholly depends on the hardware, the PAC, and the features provided by the HAL API. Here's what that looks like for LPC8xx HAL's USART, for example: https://github.com/lpc-rs/lpc8xx-hal/blob/da1ea3fe42efca2f2ca5a5abe3dbccea2126a00e/src/usart/instances.rs#L11

[therealprof]

• Peripherals that don't have multiple instances like that don't need an Instance trait.

But if the intent is to make additional information (like the associated Interrupt) accessible, wouldn't it make sense to still have an Instance trait?

[hannobraun]

@timokroeger

I think #8 is related

Yes, I agree. I'd say #7 is also related. When I added the Instance stuff I meant to build up towards an LPC8xx HAL-like Peripherals trait, but ran out of time (the work happened at Oxidize 2019's impl days and I haven't found time to pick it up again, so far).

[hannobraun]

@therealprof

But if the intent is to make additional information (like the associated Interrupt) accessible, wouldn't it make sense to still have an Instance trait?

That was never my intent, but that doesn't mean we couldn't do it that way. I introduced Instance to get away from the huge macros that generate basically all of a peripheral's API.

Maybe it's worth using Instance everywhere, which would have the benefit of consistency. But there are other ways to handle that specific problem:

• Make additional information for singleton peripherals available without a trait, for example using a peripheral::INTERRUPT constant.
• Make it unnecessary for the user the access the interrupt directly by providing convenience APIs. For example, we have USART::enable_in_nvic in LPC8xx HAL, but I'm not sure if that's the best way to handle that.