Proposal: Generate proxy structs for registers

hannobraun commented 6 years ago

Introduction

svd2rust generates two structs to represent a peripheral:

The RegisterBlock, that resides at the memory location where the registers are mapped.
A proxy struct that deref's to the RegisterBlock.

This is useful, because it means that the proxy struct can be moved and owned, allowing us to build safe abstractions on top of it.

I suggest that much in the same way, it would be useful to generate proxy structs for single registers, allowing us to move and own those registers.

TL;DR

Assume a peripheral named PERIPHERAL with a register named REG. Generate the current register struct (peripheral::REG, the one with the VolatileCell) as peripheral::reg::Register instead. Generate a proxy struct that re-uses the old name (peripheral::REG) that basically looks like the peripheral proxy struct (PERIPHERAL), but dereferences to peripheral::reg::Register.

Motivation

Suppose we have a peripheral that can be logically split into multiple parts, each of which has full control over a set of registers. We might want to write an API that looks something like this:

struct PERIPHERAL {
    part_a: PartA,
    part_b: PartB,
}

struct PartA { ... }

impl PartA {
    // methods that access register set A
}

struct PartB { ... }

impl PartB {
    // methods that access register set B
}

Right now there are several ways to implement such an API, each of which has drawbacks:

Have PERIPHERAL borrow the peripheral struct. Give PartA and PartB references to their respective registers. This works fine, but it means we need lifetime parameters everywhere (no big deal), and we can't easily put Peripheral into a static.
Don't give PartA and PartB anything, but have them access the registers in an unsafe way. This allows us to write a nice API, but under the hood it's harder to get right.
Give the peripheral struct to a third struct, and pass that into PartA and PartB whereever they need to access registers. This is the way to go if there actually needs to be some synchronization when accessing registers, but if the register two sets are truly independent, this is unnecessary, cumbersome, and limits the API user's designs (because that third struct needs to be available whereever PartA and PartB live).

If we had proxy structs for registers, in the same way we have proxy structs for peripherals, we could just give those to PartA and PartB, allowing us to provide a nice API that doesn't require too much unsafe to implement. It could look something like this:

struct PERIPHERAL {
    part_a: PartA,
    part_b: PartB,
}

impl PERIPHERAL {
    fn new(peripheral: raw::PERIPHERAL) -> Self {
        PERIPHERAL {
            part_a: PartA {
                reg_a1: peripheral.reg_a1,
                reg_a2: peripheral.reg_a2,
            },
            part_b: PartB {
                reg_b1: peripheral.reg_b1,
                reg_b2: peripheral.reg_b2,
            },
    }
}

struct PartA {
    reg_a1: REG_A1,
    reg_a2: REG_A2,
}

impl PartA {
    // methods that access register set A
}

struct PartB {
    reg_b1: REG_B1,
    reg_b2: REG_B2,
}

impl PartB {
    // methods that access register set B
}

Design

Suppose we have a peripheral called PERIPHERAL with a register called REG. Currently, svd2rust would generate the following:

The proxy struct PERIPHERAL.
The register block peripheral::RegisterBlock.
The struct peripheral::REG (containing the VolatileCell).
The module peripheral::reg, containing all the other code related to that register.

I propose making the following changes:

Generate the currently existing register struct (with the VolatileCell) as peripheral::reg::Register instead.
Generate the new proxy struct peripheral::REG, that basically looks like PERIPHERAL, except it deref's to peripheral::reg::Register.
Remove PERIPHERAL's PhantomData field. Instead add the proxy structs for all registers there.

Partial Implementation

I have a partial implementation of this proposal: branch, diff

This branch implements my suggestions 1. and 2., but not 3. I ran into some trouble adding the register proxy fields to the peripheral proxy struct, and got the distinct feeling that the code needs some thourough refactoring to do this right. I don't have time for this right now, so I opted to post this proposal first, to see if we can reach agreement first.

I created experimental branches of lpc82x and lpc82x-hal to test this partial implementation.

lpc82x: branch, diff
lpc82x-hal (less relevant, feel free to skip): branch, diff

Open Questions

If this were implemented, do we still need RegisterBlock? It seems to me that it becomes redundant.

Feedback

Please leave your feedback to let me know what you think. If we can agree that this change would be worthwile, I'll try to allocate the time to implement it.

hannobraun commented 6 years ago

I've talked to @japaric and @pftbest about this proposal at RustFest.

@japaric was concerned about the safety of this change. Making changes to one register can have consequences in another (for example writing data into one register can reset error flags in another). If register proxy structs can be moved into a completely different context, this action-at-a-distance could be even more confusing, but I can't come up with a reason why it would actually be unsafe in a memory safety sense. @japaric, it would be great if you could elaborate, if you get the chance.

@pftbest noted that peripherals on MSP430 aren't located in a continuous memory regions, like they are on Cortex-M. If I understood correctly, he believes that this change would be beneficial in that scenario.

hannobraun commented 6 years ago

As there has been very little interest in this proposal so far, I've created a workaround in the lpc82x-hal crate. I've added a module with infrastructure for proxying registers. It's being used for the SYSCON API (1, 2).

If anyone's interested, I'd appreciate a review of my unsafe use. I think it's fine, but who knows. Unless someone convinces me that this is a stupid idea, I plan to publish this workaround as a standalone crate.

jamesmunns commented 6 years ago

So, I hadn't seen or comprehended this proposal before, however I ran into exactly this issue today, trying to refactor a peripheral that has a huge number of ~~peripherals~~ registers (integrated ethernet component, many registers for things like PHY and MAC control, as well as DMA control registers, all under the same peripheral).

I understand what @japaric is saying, sometimes it doesn't make sense to subdivide registers, as one could drastically alter the behaviors of others, though that's just as true for peripherals that control clocks, power gating, etc. in our current layout.

It would be nice to have the ability to destructure items, when necessary, and might reduce some of the macro magic required to actually do this, for example with GPIOs that have many sub-items. I feel like this would be particularly well suited to the nRF style of peripherals which are well isolated, and rarely rely on outside registers for configuration, control, and enable/disable actions. It could allow code to get messy, however svd2rust's output is mainly meant for HAL developers IMO, who are tasked with building safe abstractions over raw Peripheral Access Crates, as opposed to end developers.

As of now, I haven't taken a look in to the implementation details of @hannobraun's proposal, but in an abstract sense, I think it would make sense to draw the "atomic" level at individual registers, rather than at the peripheral level.

jamesmunns commented 6 years ago

@hannobraun additionally, for your first solution, where we pass references to everything, I was able to do something like this:

struct PeriphSubset {
    abc: &'static ABC,
    def: &'static DEF,
}

impl Constrain for PERIPHERAL {
    fn constrain(self) -> PeriphSubset {
        drop(self);
        let pref: &'static RegisterBlock = unsafe { &*PERIPHERAL::ptr() };
        PeriphSubset {
            abc: &pref.abc,
            def: &pref.def
        }
    }
}

However for this solution, it is important to remember that size_of::<&'static ABC>() is non-zero, so this solution incurs additional cost of storing pointers, rather than having the pointers as a constant value stored only in TEXT (and inlined whenever the register needs to be dereferenced).

hannobraun commented 6 years ago

@jamesmunns Cool, I didn't know that worked. I thought you needed transmute to get a 'static reference in a situation like that.

jamesmunns commented 6 years ago

Actually, I'd maybe suggest that the atomic layer could be one layer lower: at the individual register field level. For example, if you had a 32 bit register that acts as a direction register for each of 32 GPIOs, and you could "split" that to give to each GPIO, that would allow for even less macro magic.

this nrf-rs function is a particularly good example of needed atomicity at the bitfield level, while this other nrf-rs function is a particularly good example of needed atomicity at the register level.

jamesmunns commented 6 years ago

@hannobraun basically we're casting a *const here to a reference with undefined lifetime. By making the return type &'static, we're basically just stating the lifetime. In practice, this is no different than the transmute, with just as much unsafe :)

hannobraun commented 6 years ago

@jamesmunns

Actually, I'd maybe suggest that the atomic layer could be one layer lower: at the individual register field level. For example, if you had a 32 bit register that acts as a direction register for each of 32 GPIOs, and you could "split" that to give to each GPIO, that would allow for even less macro magic.

I agree that this would useful in many cases. I think structuring GPIO registers like this is pretty common, at least from what I've seen so far.

Question is, how can svd2rust tell that it's okay to split a register like that? Because in the general case. you often need a full read-modify-write (which isn't atomic without a critical section) to write to a field. Are registers whose fields can be accessed independently discernible from the SVD file somehow?

jamesmunns commented 6 years ago

@hannobraun yeah, I agree it gets harder at the bitfield level. Maybe impl a function for each register, that wraps each subfield in a critical section/mutex for each subfield? Maybe with an API that prevents modification of the whole register?

I'd say if we enable this, it should be an explicit action, for example by default you just get the register as an individual struct, and that struct has two additional methods: one that takes ownership and returns each of the subfields with some kind of common MutexCell around each field, and an unsafe method that takes ownership and returns all of the subfields "raw"?

jamesmunns commented 6 years ago

CC @rust-embedded/tools - does anyone have strong feelings on this one way or the other?

Personally, I'd be interested in seeing at least the changes that @hannobraun originally proposed. From there, we can discuss the ability to split individual registers later.

hannobraun commented 6 years ago

@jamesmunns

@hannobraun yeah, I agree it gets harder at the bitfield level. Maybe impl a function for each register, that wraps each subfield in a critical section/mutex for each subfield? Maybe with an API that prevents modification of the whole register?

I'd say if we enable this, it should be an explicit action, for example by default you just get the register as an individual struct, and that struct has two additional methods: one that takes ownership and returns each of the subfields with some kind of common MutexCell around each field, and an unsafe method that takes ownership and returns all of the subfields "raw"?

I think that's an interesting idea. I believe registers should be an atomic unit, as you often don't want to split them and need to write many fields at once (say, for many configuration registers). But your idea can be implemented on top of that.

I think it makes sense to split your idea into two parts, that can be implemented and decided on separately:

Split the register into fields that will be accessed without synchronization. Maybe via an unsafe method (split_unchecked) on the register proxy object. This will definitely be useful, as I've seen this pattern used with GPIO peripherals basically everywhere I looked so far.
Split the register into fields that will be accessed via a MutexCell. That method would be safe (split_synchronized, maybe). I don't know how often that would be needed, but I've seen this pattern implemented manually a few times, at least. I've also seen it implemented wrongly, without the synchronization, so I believe such a method could prevent real bugs in the wild.

jamesmunns commented 6 years ago

@adamgreig suggested in IRC moving this code gen behind a feature flag, so that these register interfaces only show up if a user opts in to them.

japaric commented 5 years ago

Something I think it's missing from this proposal is how to deal with several instances of the same peripheral , say USART1 and USART2.

If I'm reading this correctly this proposes treating each register as its own singleton (ZST that implements Deref), but in that case you need USART1.dr and USART2.dr to have different types otherwise you would have two instances of the same singleton (they would both deref to the same address / reference). OTOH, if you go with different types for DR in this fashion:

pub struct USART1 {
    pub dr: usart1::DR,
    // ..
}

pub mod usart1 {
    pub struct DR { _not_send_or_sync: PhantomData<*const ()> }
}

pub struct USART2 {
    pub dr: usart2::DR,
    // ..
}

pub mod usart2 {
    pub struct DR { _not_send_or_sync: PhantomData<*const ()> }
}

You would lose the ability to write generic code. Today you can write Serial<USART> where USART: Deref<usart::RegisterBlock>, but that wouldn't be possible with the above approach.

I would go with something like this:

pub mod usart {
    /// All the USART registers; this is a zero-sized struct
    pub struct Registers<USART>
    where
        USART: crate::USART,
    {
        dr: DR<USART>,
        // ..
    }

    /// Data register
    pub struct DR<USART>
    where
        USART: crate::USART,
    {
        _not_send_or_sync: PhantomData<*const ()>,
        _usart: PhantomData<USART>,
    }

    impl<USART> DR<USART> {
        // the API you already know
        pub fn read(&self) -> .. { .. }
        pub fn modify(&self, ..) { .. }
        pub fn write(&self, ..) { .. }

        /* Private primitives used to implement the above API */
        fn ptr() -> usize {
            const OFFSET: usize = 0;

            USART::base_pointer() + OFFSET
        }

        fn read_() -> u32 {
            unsafe {
                ptr::read_volatile(Self::ptr() as *const u32)
            }
        }

        fn write_(val: u32) {
            unsafe {
                ptr::write_volatile(Self::ptr() as *mut u32, val)
            }
        }
    }
}

/// A USART instance
pub unsafe trait USART {
    #[doc(hidden)]
    fn base_pointer() -> usize;
}

pub struct USART1;

impl USART for USART1 { .. }

pub struct USART2;

impl USART for USART2 { .. }

Then you would write a generic serial abstraction like this:

struct Serial<USART>
where:
   USART: pac::USART,
{
   dr: DR<USART>,
   sr: SR<USART>,
   // ..
}

impl<USART> Serial<USART>
where
    USART: pac::serial::USART,
{
    pub fn new(regs: pac::usart::Registers<USART>) -> Self { .. }
}

Bonus points: this lets us entirely side step the discussion about the correctness of VolatileCell.

https://github.com/rust-embedded/svd2rust/issues/213#issuecomment-396576642

I have updated my beliefs since then. I don't think being able to Send "coupled" registers (e.g. BSRR and ODR) to different execution contexts (running at different priorities) can result in UB given that we are using Atomic*-like shared (interior) mutability (i.e. read / write both use &- (shared) references). However, I still think that doing so is error-prone, see example below:

fn main() -> ! {
    let odr: ODR = ..;

    // set PA0 high
    odr.modify(|r| r | 1);
    //~^ if this gets preempted then the change in `SysTick` may get lost
}

#[exception]
fn SysTick() {
    let bssr: &BSRR = ..;

    // set PA1 high
    bsrr.write(1 << 1);
}

So I would propose something the following to prevent errors like that:

Each register singleton does not implement the Send trait. This is to prevent sending coupled registers to different execution contexts (see example above).
Each peripheral singleton does implement the Send trait. This is OK because you are sending all the potentially coupled registers (e.g. ODR and BSRR) together.

This means that to have to opt your abstractions into Send-ness:

unsafe impl<USART> Send for Serial<USART>
where
    USART: pac::serial::USART,
{}

There you are asserting that you have take care of coupled registers: either all the coupled registers are in the struct or you have dropped one of them, etc.

With register singletons you can one-up the level of abstraction craziness and split a register into bit fields (if bit banding is available)

// alternatively, you could store the index in the struct rather than in the type
pub struct ODRx<GPIO, const I: usize> {
    _gpio: PhantomData<GPIO>,
}

impl<GPIO> ODRx<GPIO, const I: usize>
where
   GPIO: pac::GPIO,
{
    pub fn set_low(&self) {
        unsafe {
            ptr::write_volatile(Self::ptr(), 0)
        }
    }

    fn ptr() -> usize {
         // compute bit band address here
    }
}

// you would still want to use a tuple here so you can *move out* each bit field
pub fn split<GPIO>(odr: ODR<GPIO>) -> (ODRx<GPIO, 0>, /* .. */,  ODRx<GPIO, 15>)
where
   GPIO: pac::GPIO,
{
    // ..
}

Not that this needs to be done in svd2rust, though.

hannobraun commented 5 years ago

Thanks for revisiting this proposal, @japaric!

Something I think it's missing from this proposal is how to deal with several instances of the same peripheral , say USART1 and USART2.

This is correct. I discovered this problem myself during my experimentation, but it seems that particular insight never made it back to this issue.

I fully agree with your analysis and proposed solution.

So I would propose something the following to prevent errors like that:

Each register singleton does not implement the Send trait. This is to prevent sending coupled registers to different execution contexts (see example above).

Each peripheral singleton does implement the Send trait. This is OK because you are sending all the potentially coupled registers (e.g. ODR and BSRR) together.

I fully agree with this, too.

bradleyharden commented 4 years ago

This is an old thread, but I just wanted to register my support for the proposal. I think registers are the most sensible choice for the "atomic" level. I haven't worked with embedded Rust for very long, but there have already been at least two instances where I wanted to split out a peripheral into individual registers or subsets of registers that provide a particular function. I'd really like to see this change happen.

rust-embedded / svd2rust