This PR adds cfg(feature = "...") gates for all the peripherals that are generated. It means that if people only want to use certain peripherals, that they don't need to compile all of them which can take a very long time.

This PR is not yet finished (I want to see CI though :wink: ) – ideally there should also be a Cargo.toml skeleton that's generated or even just a .txt file which includes all available feature flags for users to expose in the Cargo.toml.

Motivation

Today, in svd2rust and cortex-m APIs, peripherals are modeled as global singletons that require some synchronization, e.g. a critical section like interrupt::free, to be modified.

The consequences of this choice is that (a) driver APIs are not ergonomic as one would expect, see below:

use stm32f103xx::TIM6;

// Periodic timer
struct Timer<'a>(&'a TIM6);

impl<'a> Timer<'a> { .. }

fn main() {
    interrupt::free(|cs| {
        let tim6 = TIM6.borrow(cs);
        let timer = Timer(tim);
        timer.init(..);
        // ..
        timer.resume();
    });
}

interrupt!(TIM6, tim6);

fn tim6() {
    interrupt::free(|cs| {
        let tim6 = TIM6.borrow(cs);
        let timer = Timer(tim);
        timer.clear_update_flag();
        // ..
    });
}

Here the Timer abstraction has to be reconstructed in each execution context. One would prefer to instantiate Timer as a static variable and then use that from each execution context. However, that's not possible with this Timer struct because of the non-static lifetime of the inner field. (It is possible to remove the inner field from Timer at the expense of having a critical section per method call -- which has worse performance than the non-static lifetime approach).

Even worst is that (b) driver abstractions can be easily broken due to the global visibility property of peripherals. This means that there's no way to e.g. make sure that TIM6 is only used as a Timer in main and tim6. Nothing prevents you, or some other crate author, from silently using TIM6 in other execution context -- TIM6 doesn't even have to appear as a function argument because it's always in scope. This issue not only breaks abstractions; you can also have race conditions on TIM6 -- yes, even with interrupt::free you can have race conditions and that's totally memory safe per Rust definition -- if Timer is being used in other execution contexts and you don't know about it.

So, what can we do to fix these issues? We can remove the root of all these problems: global visibility.

This RFC is proposing to go from global singletons to scoped singletons. Instead of having peripheral singletons with global visibility you'll have to explicitly import a peripheral singleton into scope, into the current execution context. Because we are talking about singletons here you can only import singleton P into scope (execution context) S once. IOW, if you imported P into an execution context S then you can't import P into another scope S'.

This RFC not only addresses the problems (a) and (b) mentioned above; it also helps us tackle the problem of compile time checked configuration -- more about that later on.

Detailed design

Zero sized proxies that represent a peripheral register block, as shown below, will be added to cortex-m and svd2rust generated crates.

pub struct GPIOA { _marker: PhantomData<*const ()> }

// Peripherals are `Send` but *not* `Sync`
unsafe impl Send for GPIOA {}

impl Deref for GPIOA {
    type Target = gpioa::RegisterBlock;

    fn deref(&self) -> &gpio::RegisterBlock { /* .. */ }
}

These proxies will be impossible to directly instantiate. Instead there will be a guarded function that returns all the proxies only once.

pub struct Peripherals {
    pub GPIOA: GPIOA,
    pub GPIOB: GPIOB,
    // ..
}

impl Peripherals {
    pub fn all() -> Option<Self> {
        interrupt::free(|_| unsafe {
            static mut USED = false;

            if USED {
                None
            } else {
                USED = true;
                Some(Peripherals { .. })
            }
        })
    }
}

The user will be able to access the proxies like this:

fn main() {
    let p = Peripherals::all().unwrap(); // first call: OK
    //let p = Peripherals::all().unwrap(); // a second call would panic!

    p.GPIOA.bsrr.write(|w| /* .. */);
    p.GPIOB.bsrr.write(|w| /* .. */);
}

Thus the proxies are singletons: there can only exist a single instance of each of them during the whole lifetime of the program. The proxies are also scoped, in this case they are tied to main, so they are not visible to other execution contexts, unless they are explicitly moved into another execution context.

Zero cost

An unsafe, unguarded variant of Peripherals::all will also be provided:

static mut USED = false;

impl Peripherals {
    pub fn all() -> Option<Self> {
        interrupt::free(|_| unsafe { /* .. */ })
    }

    // NOTE for safety this function can only be called once and before `Peripherals::all` is called
    pub unsafe fn _all() -> Self {
        USED = true;
        Peripherals { .. }
    }
}

When only the unsafe variant is used the cost of having scoped singletons becomes zero:

fn main() {
    let p = unsafe { Peripherals::_all() };

    p.GPIOA.bssr.write(|w| /* .. */)
}

This program has the same cost as using a global, unsynchronized GPIOA register block (which is what you see in C HALs).

Sidestepping the proxy

Each peripheral will provide a static method, ptr, that returns a raw pointer into the register block:

impl GPIOA {
    pub fn ptr() -> *const gpioa::RegisterBlock {
        0x4001_0800 as *const _
    }
}

This is useful for implementing safe APIs that perform "unsynchronized" reads on registers that have no side effect when read:

impl DWT {
    /// Reads the cycle counter
    // NOTE this is a static method and doesn't require an instance of `DWT`
    pub fn cyccnt() -> u32  {
        unsafe { (*DWT::ptr()).cyccnt.read() }
    }
}

// time something
let before = DWT::cyccnt();
// ..
let after = DWT::cyccnt();
let elapsed = after.wrapping_sub(before);

Enabling new APIs

Scoped singletons effectively give move semantics to peripherals. This enables richer, type safer APIs than what can be expressed with the current peripheral API. Let's see some examples:

(you can find some initial experiments with these APIs in the singletons branch of the f3 repository)

Type state as a contract

Digital I/O pins can be configured as inputs, outputs or to some special functionality like serial, SPI or I2C. In some cases you want to configure a pin to operate in a certain mode for the duration of the whole program; that is you don't want the pin to be re-configured by mistake.

Type state is a good way to encode this property: a type is used to encode the state of an object. To transition the object from a state to another it needs to be moved so that the previous state can no longer be used.

Here's a tentative GPIO API that uses type state:

use blue_pill::GpioExt;

let p = Peripherals::all().unwrap();

// from `GpioExt`: `fn pins(self) -> gpioa::Pins`
// `Pins` is a struct that contains all the pins. Each pin is (also) a singleton (!).
let pins = p.GPIOA.pins();

// `fn as_output(self) -> Output<Self>`
// `as_output` configures a pin as an output pin
let pa0 = pins.PA0.as_output();

// `fn as_input(self) -> Input<Self>`
// `as_input` configures a pin as an input pin
let pa1 = pins.PA1.as_input();

// `Output::high(&mut self)`
// set PA0 high (to 3.3V)
pa0.high();

// `Input::is_high(&self) -> bool`
if pa1.is_high() {
    // ..
}

// this would cause a compile error because `PA0` is not in the `Input` state
// pa0.is_high();

// this would cause a compile error because `PA1` is not in the `Output` state
// pa1.high();

// this would cause a compile error because `GPIOA` was consumed by the `pins` call
// p.GPIOA.moder.modify(|_, w| /* configure PA0 as an input */)

Here the Input and Output newtypes are used to encode the type state. The most important part here is that GPIOA is consumed to generate the individual pins and thus it can't no longer be used to configure the pins -- which would break Input / Output contract of "this pin can only be used as an input for the rest of the program".

Compile time pin allocation

On modern microcontrollers a single pin can be configured to have one of many functionalities (Serial, PWM, I2C, etc.). This lets vendor pack more peripherals in a microcontroller without increasing the number of pins.

A problem that arises from this flexibility is that one might wrongly configure two, or more, peripherals to use the same pin. With move semantics you can construct an API that ensures that this can't happen:

let p = Peripherals::all().unwrap();
let pins = p.GPIOA.pins();

// use PA9 and PA10 for the USART1 serial port

// NOTE consumes `pa9`
let tx = pa9.as_usart1_tx();

// NOTE consumes `pa10`
let rx = pa10.as_usart1_rx();

// `Serial::new` in turn consumes `tx` and `rx`
let serial = Serial::new(p.USART1, (tx, rx), 115_200.bps());

// this would error because `pa9` was consumed above
//let ch2 = pa9.as_tim1_ch2();
//let pwm = Pwm::new(p.TIM1, ch2, 10.khz());

Here we have high level abstractions like Serial consume the pins they are going to use. This way once one such abstraction is created no other abstraction can't use any of the pins the first one is using.

Non overlapping register access

The "split" technique used for GPIO pins can also be used to split a peripherals in "parts" that (a) modify different registers and (b) modify non overlapping parts of a same register. This comes in handy with peripherals like the DMA which usually interacts with several other peripherals.

Vendors usually design their DMA peripherals so that even if the settings related to different channels are stored in a single register that register can be modified atomically using e.g. "clear flag" bits (no RMW operation required to clear a flag). If the vendor doesn't provide such mechanism bit banding can probably be used in its place, if the device has support for it.

RTFM protects peripherals at the register block level. Without move semantics, to clear "transfer complete" interrupt flags from two interrupts handlers running at different priorities you would need a lock in the lowest priority handler:

// priority = 2
fn dma1_channel1(r: DMA1_CHANNEL1::Resources) {
    r.DMA1.lock(|dma1| {
        // clear the transfer complete flag for this channel
        dma1.ifcr.write(|w| w.ctcif1().set_bit());
        // ..
    });
}

// priority = 3
fn dma1_channel2(r: DMA1_CHANNEL2::Resources) {
    let dma1 = r.DMA1;
    // clear the transfer complete flag for this channel
    dma1.ifcr.write(|w| w.ctcif2().set_bit())
    // ..
}

But with move semantics you can split the DMA in channels and assign exclusive access to each channel to each interrupt handler:

fn init(p: init::Peripherals) -> init::LateResourceValues {
    let channels = p.DMA1.split();

    // ..

    init::LateResourceValues { CH1: channels.1, CH2: channels.2 }
}

// priority = 2
// resources = [CH1]
fn dma1_channel1(r: DMA1_CHANNEL1::Resources) {
    // clear the transfer complete flag for this channel
    r.CH1.clear_transfer_complete_flag();
}

// priority = 3
// resources = [CH2]
fn dma1_channel2(r: DMA1_CHANNEL2::Resources) {
    // clear the transfer complete flag for this channel
    r.CH2.clear_transfer_complete_flag();
}

Thus no locking is needed. Each channel will operate on a non-overlapping portion of the shared IFCR register.

Configuration freezing

In some cases you want to configure the core and peripherals clocks to operate at certain frequencies during initialization and then make sure that these frequencies are not changed later at runtime.

Again, move semantics can help here by "discarding" the peripheral in charge of clock configuration once the clock has been configured:

use blue_pill::RccExt;

let p = Peripherals::all().unwrap();

// .. use `p.RCC`, or a higher level API based on it, to configure the clocks ..

// from `RccExt`: `fn clocks(self) -> Clocks`
// `clocks` contains information about the operating frequency of the peripheral buses
let clocks = p.RCC.freeze();

// To compute USART1 internal prescalers and achieve the desired baud rate,
// information about the current clock configuration is required.
// `clocks` provides that information
let serial = Serial::new(p.USART1, (tx, rx), &clocks, 115_200.bps());

Here, once the clock is configured, its configuration gets frozen by consuming / discarding the RCC peripheral. With this ... move the clock frequencies can no longer be changed. Freezing RCC returns a Clocks struct that contains the frequency of every peripheral bus. This information is required to properly configure the operating frequency of each peripheral so it's passed to peripherals' init functions.

Drawbacks

Regression when not using RTFM

And you still want to use interrupts.

RTFM supports moving runtime initialized resources into tasks (interrupts) at zero, or very little, cost but if you are not using RTFM then a static variable is required to share a peripheral between e.g. main and an interrupt handler. Putting a peripheral singleton in a static variable means making it globally visible which means you have global singletons again, and all their disadvantages, but with worse performance (because an Option and a RefCell are needed, see below) than what you get with today's API.

With this RFC:

// you want to share `GPIOA` between `main` and `exti0`
// but this makes it global so any other execution context can also use it
static GPIOA: Mutex<RefCell<Option<GPIOA>>> = Mutex::new(RefCell::new(None));

fn main() {
    // initialize `GPIOA`
    let p = Peripherals::all().unwrap();
    interrupt::free(move |cs| {
        *GPIOA.borrow(cs).borrow_mut() = Some(p.GPIOA);
    });

    loop {
        interrupt::free(|cs| {
            let gpioa = GPIOA.borrow(cs).borrow().as_ref().unwrap();

            // ..
        });

        // ..
    }
}

interrupt!(EXTI0, exti0);

fn exit0() {
    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs).borrow().as_ref().unwrap();

        // ..
    });
}

With today's API:

// you don't have the RefCell + Option overhead but you still have global visibility

fn main() {
    loop {
        interrupt::free(|cs| {
            let gpioa = GPIOA.borrow(cs);

            // ..
        });

        // ..
    }
}

interrupt!(EXTI0, exti0);

fn exit0() {
    interrupt::free(|cs| {
        let gpioa = GPIOA.borrow(cs);

        // ..
    });
}

Compare this to RTFM + this RFC:

app! {
    resources: {
         static GPIOA: GPIOA;
    },

    idle: {
        resources: [GPIOA],
    },

    tasks: {
        EXTI0: {
            path: exti0,
            resources: [GPIOA],
        },
    },
}

fn init(p: init::Peripherals) -> init::LateResourceValues {
    init::LateResourceValues {
        GPIOA: p.GPIOA,
    }
}

fn idle(t: &mut Threshold, r: idle::Resources) -> ! {
    loop {
        r.GPIOA.claim(threshold, |gpioa| {
            // do stuff with `r.GPIOA`
        });

        // ..
    }
}

fn exti0(r: EXTI0::Resources) {
     // do stuff with `r.GPIOA`
}

No Mutex, no RefCell, no Option and no global visibility. Plus only idle needs a lock to access GPIOA. Without this RFC, even with RTFM, it's possible to access GPIOA from an execution context that's not idle or exti0 due to this issue / bug / hole: japaric/cortex-m-rtfm#13.

It breaks the world

Breaking changes in cortex-m, svd2rust and cortex-m-rtfm are required to implement this.

Unresolved questions

• Should we have finer grained access to peripherals as in a GPIOA::take() that returns Option<GPIOA> (in addition to Peripherals::all() -- one would invalidate the other)?

• The unsafe variant, Peripherals::_all, needs a better name.

Implementation bits

Note that the implementation is a bit crude at this point. Expect bugs. Still, I'm posting now to get feedback and to allow others to experiment.

• The f3 repo contains a high level API, and examples that use it, based on this RFC.
• japaric/cortex-m#65
• japaric/svd2rust#158
• japaric/cortex-m-rtfm#50

cc @pftbest @thejpster @therealprof @hannobraun @nagisa @kjetilkjeka

EDIT: I realized that it may not be obvious how the RFC solves problem (a) so here's an example:

Let's say that you want to use a serial port in idle and in some interrupt handler. You know that idle only needs to use the transmitter half the serial port and the interrupt handler only needs to use the receiver part.

app! {
    resources: {
        static TX: Tx;
        static RX: Rx;
    },

    idle: {
        resources: [TX],
    },

    tasks: {
        EXTI0: {
            path: exti0,
            resources: [RX],
        },
    },
}

fn init(p: init::Peripherals) -> init::LateResourceValues {
    // consumes `GPIOA`
    let pins = p.GPIOA.pins();

    // consumes `PA9`
    let pa9 = pins.PA9.as_usart1_tx();

    // consumes `PA10`
    let pa10 = pins.PA10.as_usart1_rx();

    // consumes `USART1`, `pa9` and `pa10`
    let serial: Serial = Serial::new(p.USART1, (pa9, pa10), 115_200.bps());

    // consumes `serial`
    let (tx, rx): (Tx, Rx) = serial.split();

    // initializes the resources `TX` and `RX`
    init::LateResourceValues {
        TX: tx,
        RX: rx,
    }
}

fn idle(_t: &mut Threshold, r: idle::Resources) -> ! {
    let tx: &mut Tx = r.TX;

    loop {
        // do stuff with `tx`
    }
}

fn exti0(_t: &mut Threshold, r: EXTI0::Resources) {
    let rx: &mut Rx = r.RX;

    // do stuff with `rx`
}

With all the moves in init you are sure that:

• No task or crate (dependency) can reconfigure the pins through GPIOA, because it (GPIOA) was created (cf. the init::Peripherals argument of init) and consumed in init.

• No task or crate (dependency) can reconfigure or drive the pins PA9 and PA10, because both pins were created and consumed in init.

• No task or crate (dependency) can do serial I/O through USART1 because it (USART1) was created and consumed in init.

• No task or crate (dependency) other than idle can do serial writes because only idle has access to Tx.

• No task or crate (dependency) other than exti0 can do serial reads because only exti0 has access to Rx.

Without this RFC you can't have any of these guarantees because the svd2rust API lets you use GPIOA, USART1 in any execution context so even a function foo with signature fn() (note: no arguments) can reconfigure the pins or start a serial operation.

I want to get a complete working build for ATSAMD21 (https://github.com/rust-lang-nursery/embedded-wg/issues/61) so I'm taking a stab at that.

• Introduced a FieldRegion helper to reason about overlapping regions
• If overlaps are detected, a union container is emitted
• If the only item in a RegisterBlock is a union, that RegisterBlock is emitted as a union
• Otherwise: we generate a name for the union field by either taking the shortest common prefix of the union's alternates or the shortest register name (depending on type name conflicts). If that doesn't work just pick an artificial name like u1.
• If the starting address offset of elements in a union are not all the same, we don't have a way to emit padding for them today. We will emit a warning (and bad code) in that case (example below). The one example of this I see in ATSAMD21 is due to missing derivedFrom support for registers; we're currently generating bad code for these anyway. I have resolved in another branch that I'll turn into a PR once this one is landed.

WARNING: field Some(Ident("pmux1_1")) has different offset 177 than its union container 176
WARNING: field Some(Ident("pmux1_2")) has different offset 178 than its union container 176

Examples:

#[doc = "Real-Time Counter"]
pub mod rtc {
    #[doc = r" Register block"]
    #[repr(C)]
    pub union RegisterBlock {
        #[doc = "0x00 - Clock/Calendar with Alarm"]
        pub mode2: MODE2,
        #[doc = "0x00 - 16-bit Counter with Two 16-bit Compares"]
        pub mode1: MODE1,
        #[doc = "0x00 - 32-bit Counter with Single 32-bit Compare"]
        pub mode0: MODE0,
    }

    #[doc = r" Register block"]
    #[repr(C)]
    pub struct USART {
        #[doc = "0x00 - USART Control A"]
        pub ctrla: self::usart::CTRLA,
        #[doc = "0x04 - USART Control B"]
        pub ctrlb: self::usart::CTRLB,
        _reserved2: [u8; 4usize],
        #[doc = "USART Baud Rate"]
        pub baud: baud_union,
      ...
    }
    #[doc = "USART Baud Rate"]
    #[repr(C)]
    pub union baud_union {
        #[doc = "0x0c - USART Baud Rate"]
        pub baud_usartfp_mode: self::usart::BAUD_USARTFP_MODE,
        #[doc = "0x0c - USART Baud Rate"]
        pub baud_fracfp_mode: self::usart::BAUD_FRACFP_MODE,
        #[doc = "0x0c - USART Baud Rate"]
        pub baud_frac_mode: self::usart::BAUD_FRAC_MODE,
        #[doc = "0x0c - USART Baud Rate"]
        pub baud: self::usart::BAUD,
    }

Refs: https://github.com/japaric/svd2rust/issues/191 https://github.com/japaric/svd2rust/issues/16

Hi! :wave: Thanks for this awesome project!

I've run into a build error generating bindings for the psoc6-pac. Here's a example of one of the many build failures:

error: structure field `data_list_sent_update__status` should have a snake case name
   --> src/ble.rs:187:9
    |
187 |     pub data_list_sent_update__status: self::blell::DATA_LIST_SENT_UPDATE__STATUS,
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: convert the identifier to snake case: `data_list_sent_update_status`

This issue is caused by #![deny(warnings)] and should be fixed by adding the #![allow(non_snake_case)] exception. There may be alternative fixes, I'm open to suggestions!

Solution 2 for #660 which pre-parses ERC type names and provides expansion information to the render functions. Works great with a few SVDs from the Renesas RA DFP which tend to use disjoint arrays quite frequently.

The only dependency on bare-metal was the implementation of the Nr trait which has been removed from version 1.0.0 (rust-embedded/bare-metal#32).

Closes #453

Signed-off-by: Daniel Egger [email protected]

Add cluster support to svd2rust. I am calling it initial, because I have only tested it against the nRF52 svd file.

Note, this is based on https://github.com/japaric/svd2rust/pull/180, so I would prefer that PR be merged first. If that PR is rejected, I can instead rebase these changes on japaric/master (just let me know).

Edit: All changes relevant to clusters are squashed into the last commit of this PR, so if you want to see the relevant changes, check out https://github.com/japaric/svd2rust/pull/182/commits/6de6de760c535e6f60eed4acb41e30db1458f33c.

This is based on the work of @brandonedens earlier https://github.com/japaric/svd2rust/pull/149, and I believe would close https://github.com/japaric/svd2rust/issues/107 and related issues.

CC @japaric @emilgardis @therealprof

Edit 2: Here are some handy references:

• SVD snippet with clusters
• code generated before
• code generated after
• diff of code generated

This PR is like #52 but with an alternative write API.

The generated write API looks like this:

#[derive(Clone, Copy)]
#[repr(C)]
pub struct DirW {
    bits: u32,
}

pub struct DirWPin0<'a> {
    register: &'a mut DirW,
}

impl<'a> DirWPin0<'a> {
    pub fn input(self) -> &'a mut DirW {
        const MASK: u32 = 1;
        const OFFSET: u8 = 0u8;
        self.register.bits &= !((MASK as u32) << OFFSET);
        self.register.bits |= 0u32 << OFFSET;
        self.register
    }

    pub fn output(self) -> &'a mut DirW {
        const MASK: u32 = 1;
        const OFFSET: u8 = 0u8;
        self.register.bits &= !((MASK as u32) << OFFSET);
        self.register.bits |= 1u32 << OFFSET;
        self.register
    }
}

impl DirW {
    #[doc = "Bit 0 - Pin 0."]
    pub fn pin0(&mut self) -> DirWPin0 {
        DirWPin0 { register: self }
    }
    
    ..
}

Instead of exposing a method that takes an enum to modify a bitfield of a register (register.bitfield(Enum::VariantA)), like in #52, this API exposes a method that returns a proxy struct whose methods let you pick a value for the bitfield (register.bitfield().variant_a()) and this method returns back the register so you don't lose the chain-ability: register.bitfield1().variant_a().bitfield2().variant_b().

Here's the blinky example from #52 ported to this alternative write API.

cc @brandonedens

This PR brings MSP430 PAC generation up to parity with Cortex M PAC generation. Among other things, a device.x file is now generated for each MSP430 PAC, and the interrupt! macro has been replaced with an attribute macro from the not-yet-published msp430-rt v0.2.0.

The remaining changes are docs and cleanups of the generated code; a number of features have become stable. Only abi_msp430_interrupt is not stable yet, and I have a workaround for this that I've not written yet.

Switching to an interrupt attribute macro is a breaking change, so I incremented the "major" version number.

This PR extends https://github.com/rust-embedded/svd2rust/pull/193 (edit: and https://github.com/rust-embedded/svd2rust/pull/190), and also brings support for the changes made in svd-parser, primarily understanding the RegisterCluster type.

Make the Interrupt enum descriminant match the interrupt nr directly. This allows simplifying Interrupt::nr from being a complicated match to a simple cast to an u8.

According to #370, this should lead to a code reduction of as much as 304 bytes.

Fixes #211 Fixes #370

Hi,

I am currently writing a HAL for some ATtiny 1-series AVRs. These newer AVRs have a few registers that are deemed critical and are protected against erroneous writes. These registers are mostly clock configuration and flash-writing related.

To write to these registers, you need to write a special unlocking register with a magic value after which you have four instructions to write into the protected registers. After either the fourth instruction or the write to the register you need to unlock it again.

Doing this manually by issuing two consecutive writes using the generated pac isn't always working, because the compiler emits more than four instruction between the unlock and the actual register write. Sometimes it works, but as soon as the writes become more complex and you are maybe using struct variants it stops working.

This means you essentially have to perform the unlock and the write using inline assembly. There is no way around this to make this work reliably, I think. So I started hacking an extension trait in my HAL to add another method to a writable register that would perform the unlock and write the register by using an inline assembly block. The idea is to use the same Writer to turn all fields into bits and then instead of using the VolatileCell to write them to register, just pass them to an asm!-block. Right now the extension trait would be valid for all Writable registers. This needs to be restricted further to only applicable registers with a marker trait or something in the future, but I wanted to get it working first.

There is a problem though: The API that is generated from svd2rust is "too safe". I cannot instantiate a Writer and I also cannot access the bits field, because it's private. Even moving this code to the device crate that is generated by svd2rust doesn't help, because _reg in the writer is a private field.

This is what I came up with so far:

use core::marker::PhantomData;
use avr_device::generic::{Reg, W, Writable, Resettable};

pub trait ProtectedWritable<REG>
where
    REG: Writable
{
    fn write_protected<F>(&self, f: F)
    where
       F: FnOnce(&mut REG::Writer) -> &mut W<REG>;
}

impl<REG> ProtectedWritable<REG> for Reg<REG>
where
    REG: Writable + Resettable
{
    fn write_protected<F>(&self, f: F)
    where
        F: FnOnce(&mut REG::Writer) -> &mut W<REG>
    {
        unsafe {
            let ptr = self.as_ptr();
            let val = f(&mut REG::Writer::from(W {
               bits: REG::RESET_VALUE & !REG::ONE_TO_MODIFY_FIELDS_BITMAP
                   | REG::ZERO_TO_MODIFY_FIELDS_BITMAP,
               _reg: PhantomData,
            })).bits;

            let ccp_ptr = unsafe { &(*CPU::ptr()).ccp.as_ptr() };
            let unlock_magic = 0xD8u8;

            // insert asm! block for unlock and write here...
            // *ccp_ptr = unlock_magic
            // *ptr = val
        }

    }
}

The CPU.ccp register is the magic unlock register.

Any idea how I could implement something like this? My last resort is to patch svd2rust itself to generate this code somehow, but I wanted to avoid this until now.

edit & update: After trying a lot more things, it boils down to the fact that this doesn't seem to be implementable outside of svd2rust, but then again, this feels like the right place to implement this.

I found https://github.com/rust-embedded/svd2rust/issues/427 and https://github.com/rust-embedded/svd2rust/pull/520 which deal with similar issues that I have and I decided to try and apply the same approach that was done with the atomic operations on MSP430 for the locked AVR registers. Adding the write_protected function becomes trivial, because it's the same crate, even the same module and all the private field issues are just gone and I can use the code I already wrote.

There is still a catch: Not all registers are write protected and not all registers are unlocked in the same way. There needs to be some marker trait that can be implemented for RegisterSpecs which mark these registers as protected and also adds a generic constant which contains the magic unlock value. The question is, who should implement that? The right place is the PAC, but neither the AVR ATDF, nor the SVD that is generated from the ATDF contain the information which registers are protected. I could still patch the generated SVD where necessary to add this information in hindsight using the YAML patches, but after checking out the SVD spec, there is no concept of a locked register that needs to be unlocked before a write can occur. There are protected registers, but that is related to ARM's Trustzone and I don't want to abuse this feature to mark protected registers. That is a problem. I can't define the trait in the PAC and then implement the trait in the HAL for the appropriate register specs because of orphan rules. So, I guess I'll have to manually implement this marker trait for every register in every applicable controller in some additional file in the pac crate.

Any better ideas?

Also, now that I think of it, getting the pointer to the CCP register is going to be problematic as well. I can't take it for granted that it exists under the same name on all controllers and just statically use it like I do in my code snippet above. This would definitely needed to be passed to the protected_write function which would make it unsafe. Or I need to play with another trait that gets implemented only for the CCP register which defines it as a global unlock register for the configuration change protection on this controller which can then be passed into a protected_write function.

Previously enums _A used in multiple fields were re-exported under their other names. That was removed in https://github.com/rust-embedded/svd2rust/pull/627, and since then the enums are no longer exported under their other names.

When using the generated PAC, that leads to the manual task of finding the first name and adding a manual re-export like

pub use pac::device::peripheral::FIRST_NAME_A as OTHER_NAME_A;

so that the enum can be named logically with its correct name. This is still not ideal, since the enum OTHER_NAME_A is in the local module scope, not the scope pac::device::peripheral:: where it was previously.

I would propose adding these re-exports, since for PAC users it removes the manual task of identifying the first name for an enum.

#627 made code much clearer in most of cases, but looks like it breaks on some SVDs which have different fields with identical enum names. cc @luojia65

For example some NXP SVDs (see https://github.com/posborne/cmsis-svd/blob/master/data/NXP/LPC178x_7x.svd) have many enumeraredValues with name ENUM:

To be able to manipulation of peripheral registers through bit-banding, the values of the offset of the modifiable bit fields in the registers are required.

For example, there are functions for bit-banding here: https://docs.rs/stm32f1xx-hal/0.9.0/stm32f1xx_hal/bb/index.html

Implementation variant:

pub mod perih_name {
   ...
   pub mod reg_name {
      ...
      pub const field_name_OFFSET: u8 = ...;
      ...
   }
   ...
}

Example of an implementation variant (stm32f1):

pub mod rcc {
   ...
   pub mod apb2enr {
      ...
      pub const USART1EN_OFFSET: u8 = 14;
      ...
      impl<'a> USART1EN_W<'a> {
         ...
         pub fn bit(self, value: bool) -> &'a mut W {
            self.w.bits = (self.w.bits & !(0x01 << USART1EN_OFFSET)) | ((value as u32 & 0x01) << USART1EN_OFFSET);
            self.w
         }
      }
      ...
      impl R {
         ...
         pub fn usart1en(&self) -> USART1EN_R {
            USART1EN_R::new(((self.bits >> USART1EN_OFFSET) & 0x01) != 0)
         }
         ...
      }
      ...
   }
   ...
}

Usage example:

unsafe {
   use stm32f1xx_hal::{self as hal, pac};
   hal::bb::set(&(*pac::RCC::PTR).apb2enr, pac::rcc::apb2enr::USART1EN_OFFSET);
}

This flag allows implementing a marker trait for registers in specific memory addresses. These can then be used to implement methods on registers. One such example is providing atomic access to only some of the registers directly in the PAC. Currently this is done in HAL, but this loses the ability to use writer proxies. It is also finicky to use raw register addresses. Moreover this allows keeping atomic and non-atomic usage in HAL more similar.

For an example on how to use this, see my fork of rp2040-pac. The most interesting changes regarding the use reside in

• generic_extension.rs
• update.sh

Even though I have an example only for the RP2040, I believe that other architectures and devices could also benefit from this. For example on SMT32 MCUs this could be used to implement a nicer API for bit-banding.

Addresses #535