enum based API (alternative write API)

[japaric]

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

[Emilgardis]

This is hugely appreciated, currently doing this manually by wrapping functionality inside std drivers.

[japaric]

@Emilgardis Do you prefer this write API over the one in #52?

[Emilgardis]

This one, not having to use every enum seems like a good idea, but it would actually be nice to be able to pass in these enums for functions.
Minimal example from my current code:

pub enum HclkSrc {
    Hxt = 0x00,
    Lxt = 0x01,
    Pll = 0x02,
    Lirc = 0x03,
    Hirc = 0x07,
}

pub fn set_hclk(src: HclkSrc, divider: u8) {
    let mut clk = peripherals::clk_mut();
    // Set hclk to HIRC (...)
    ...
    // switch to `src`
    clk.clksel0.modify(|_, w| w.hclksel(src as u8));
    ...
}

Using this pr the fn would be something like

pub fn set_hclk<F>(src: F, divider: u8) where F: Fn<(HclkselSrcW)> -> HclkselSrcW {
    let mut clk = peripherals::clk_mut();
    // Set hclk to HIRC (...)
    ...
    // switch to `src`
    clk.clksel0.modify(|_,w| src(w.hclksel()) ); // As I understand it is not possible in any other way with this PR
}

With #52 it would be more ergonomical

pub fn set_hclk<F>(src: F, divider: u8) where F: Fn<(HclkselSrc)> -> HclkselSrc {
    let mut clk = peripherals::clk_mut();
    // Set hclk to HIRC (...)
    ...
    // switch to `src`
    clk.clksel0.write(|w| w.hclksel.src(src));
}

If it would be possible having both API, that would be the best for me. Also providing functionality to set which API to use would work suboptimally (for me).

[japaric]

Based on the feedback here and elsewhere. I intent to generate an API that looks like this:

fn main() {
    // enums are scoped using modules
    use gpio::lckr::Lckk;
    //              ^~~~ (bit)field
    //        ^~~~ register
    //  ^~~~ peripheral

    // write: "method" API
    gpioa.lckr.write(|w| w.lckk().lock_key_active());
    // or gpioa.lckr.write(|w| w.lckk().lock_key_not_active());

    // write: enum API
    gpioa.lckr.write(|w| w.lckk_enum(Lckk::LockKeyActive));
    //or gpioa.lckr.write(|w| w.lckk_enum(Lckk::LockKeyNotActive));

    // write: bits API
    // (unsafe because you may write a RESERVED bit pattern into the bitfield)
    // (this could be smarter and only be unsafe if a RESERVED bit pattern exists)
    gpioa.lckr.write(|w| unsafe { w.lckk_bits(0b1) });
    // or gpioa.lckr.write(|w| unsafe { w.lckk_bits(0b0) });

    // read: "method" API
    // (no one has requested this but it makes the read / write API more symmetric)
    if gpioa.lckr().read().lckk().is_lock_key_active() {
        // something
    }

    // read: enum API
    if gpioa.lckr.read().lckk_enum() == Lckk::LockKeyActive {
        // something
    }

    // read: bits API
    if gpioa.lckr.read().lckk_bits() == 0b1 {
        // something
    }

    // derivedFrom can be used to use the same enum on different bitfields
    // in this case, both LCK0 and LCK1 are derivedFrom the same <enumeratedValues>
    let lck0: Lck = gpio.lckr.read().lck0();
    let lck1: Lck = gpio.lckr.read().lck1();

    // --------------------------------------------------------------------------

    // some SVD files, like the nrf51.svd file, have, for some fields, 
    // two sets of enumeratedValues, one for reads the other for writes.
    // In those cases, we also generate two different sets of enums

    use uart::intenset::{CtsR, CtsW};

    // one for reads
    let cts: CtsR = uart0.intenset.read().cts_enum();
    // enum CtsR { Disabled, Enabled }

    // and one for writes
    uart0.intenset.write(|w| w.intenset.write(|w| w.cts_enum(CtsW::Set)));
    // enum CtsW { Set }

    // this difference extends to the "method" API as well
    if uart0.intenset.read().cts().is_enabled() {
        // something
    }

    uart0.intenset.write(|w| w.cts().set());
}

I'll probably start working on this tomorrow so if anyone else has comments or criticism about this API let me know ASAP.

cc @Emilgardis @brandonedens

[Emilgardis]

Sounds good to me

[japaric]

I realized yesterday that all the read variants can be merged into one method:

// returns enum Lckk { LockKeyNotActive, LockKeyActive }
if gpioa.lckr.read().lckk() == Lckk::LockKeyNotActive { .. }

// the "method" API is implemented as ... methods on that enum
if gpioa.lckr.read().lckk().is_lock_key_not_active() { .. }

// the bits API is also implemented as a method on that enum
if gpio.lckr.read().lckk().bits() == 0b1 { .. }

So I'll go ahead with that version.

[japaric]

OK. I have now implemented the stuff I mentioned before except for putting the enums in modules. After implementing that, I'm going to review that documentation is generated where appropriate and then I'll merge this.

[homunkulus]

☔ The latest upstream changes (presumably a799550) made this pull request unmergeable. Please resolve the merge conflicts.

[japaric]

Finally, I've refactored the code and moved everything into modules, which
makes the enum / struct names less awful. In the process, I've tweaked the API
yet again! Here's what it looks like today:

fn read(gpioa: &Gpio) {
    // enum PinR { Low, High }
    use gpio::in_::PinR;

    // whole register access
    {
        // bits API
        if gpioa.in_.read().bits() & (1 << 1) == 1 {
            ..
        }
    }
    
    // bitfield access
    {
        // bits API
        if gpioa.in_.read().pin1().bits() == 1 {
            ..
        }

        // enum API
        if gpioa.in_.read().pin1() == PinR::High {
            ..
        }

        // method API
        if gpioa.in_.read().pin1().is_high() {
            ..
        }
    }
}

fn write(gpioa: &mut Gpio) {
    // enum PinW { Low, High }
    use gpio::out::PinW;

    // whole register access
    {
        // bits API
        gpio.out.write(|w| unsafe { w.bits(0xdeadbeef) });
    }
    
    // bitfield access
    {
        // bits API
        // (NOTE not unsafe because this bitfield doesn't have reserved bit
        // patterns)
        gpio.out.write(|w| w.pin1().bits(1));

        // enum API
        gpio.out.write(|w| w.pin1().variant(PinW::High));

        // method API
        gpio.out.write(|w| w.pin1().high());
    }
}

What's left now is rebasing and reviewing the generated docs.

[japaric]

OK, this should be ready to land as it now actually geneates docs and the crate level docs have been updated.

I have one more question left though. Before this PR the method to write to a 1-bit field accepted a boolean (gpioa.out.write(|w| w.pin0(true))); similarly the read method of 1-bit fields returned a boolean (if gpioa.out.read().pin0()). With this PR, I have not re-implemented that special case so the read and write methods take an u8 as argument instead of a boolean (gpioa.out.write(|w| w.pin0().bits(1)) and if gpioa.out.read().pin0().bits() == 1). This makes the read case less ergonomic but makes everything more uniform as there is no special case (both read and write always use integers). I'd like to know: if anyone would like to have the boolean based APIs back for 1-bit fields.

[japaric]

@homunkulus try

[homunkulus]

⌛ Trying commit acd121d with merge acd121d...

[homunkulus]

💔 Test failed - status-travis

[japaric]

I'm gonna go ahead and land this. We can add a "boolean API" afterwards in a backward compatible way. Something like this:

if register.read().field().bits() == 1 { .. }
if register.read().field().is_set() { .. }
if register.read().field().is_clear() { .. }

register.write(|w| w.field().bits(1));
register.write(|w| w.field().set());
register.write(|w| w.field().clear());

@homunkulus r+

[homunkulus]

📌 Commit fc8f6d7 has been approved by japaric

[homunkulus]

⌛ Testing commit fc8f6d7 with merge fc8f6d7...

[homunkulus]

💔 Test failed - status-travis

[japaric]

@homunkulus retry

network error

[homunkulus]

⌛ Testing commit fc8f6d7 with merge fc8f6d7...

[japaric]

@homunkulus r+

[homunkulus]

📌 Commit 102cd10 has been approved by japaric

[homunkulus]

⌛ Testing commit 102cd10 with merge 102cd10...

[homunkulus]

☀️ Test successful - status-appveyor, status-travis
Approved by: japaric
Pushing 102cd10 to master...