nrf: New API supporting borrowed peripherals

[Dirbaio]

Yet another way... trying to workaround the type inference issues.

This adds the possibility of creating drivers with borrowed peripheral instances.

let p = pac::Peripherals::take().unwrap();
let mut irq = interrupt::take!(SPIM3);

let spim = spim::Spim::new(&mut p.spim3, &mut irq, &mut p.p0_29, &mut p.p0_28, &mut p.p0_30, config);
// use spim...
mem::drop(spim);

// Here we have access to p.spim3, irq, and the pins again!

It's cool:

• Type signature is Spim<'_, SPIM3> regardless whether it's owned or borrowed
• Spim always holds the zero-sized struct
• No weird unergonomic Borrow<'a, T> struct.

[richard-uk1]

I'm playing with this on my nrf-52832. You need the line

+                 $(#[$cfg])?

on line 10 of embassy-extras/src/macros.rs. I'm looking at ref 9b3243f

[richard-uk1]

Also, I might be wrong but I think you should use pac::Peripherals's taken value for embassy's Peripherals, because it is incorrect to use the raw peripheral and the embassy one at the same time. If the user wants to do something with the raw peripherals, they should use steal and acknowledge the unsafety.

You could do this by forwarding your take on to the pac's take.

[Dirbaio]

You need the line .. on line 10

Nice catch, added!

you should use pac::Peripherals's taken value for embassy's Peripherals

Yes, I'm aware of this but I'm not sure how to best solve it. Ideally I want to avoid the pac appearing in the public API at all, so we can bump the pac's major version (or switch to something completely different) without bumping embassy-nrf major version (ie the pac should just be an internal implementation detail on how we access registers).

One problem is that PACs intentionally forbid linking multiple major versions at once (precisely so you can't take() the peripherals multiple times), so even if the PAC doesn't appear in the public API bumping it is a breaking change...

The reasoning behind doing our own Peripherals is the PAC's Peripherals is autogenerated from SVDs, which is not ideal:

• Some peripherals are really just one. For example SPI0, SPIM0, SPIS0, TWI0, TWIM0, TWIS0 are really a single peripheral, you can only use one of them at a time.
• No structs for owning "parts" of a peripheral. I don't want to own P0, I want to own individual pins. Same for ADC, PWM, PPI channels.

[richard-uk1]

Maybe its better if I explain what I mean in code. I mean something like (in the Peripherals macro)

impl Peripherals {
    ///Returns all the peripherals *once*
    #[inline]
    pub fn take() -> Option<Self> {
        if pac::Peripherals::take().is_some() {
            Some(unsafe { <Self as ::embassy::util::Steal>::steal() })
        } else {
            None
        }
    }
}

Update: URGH, pac crates mark the peripherals as taken even if you steal them. This is disappointing! If that wasn't the case you could do what I suggest above.

[richard-uk1]

I've done some work on my local copy making nrf gpio match the PeripheralBorrow scheme, which you might as well copy/paste. Let me know if you want me to post it somewhere.

[Dirbaio]

@derekdreery Yes please, that'd be great! You can send a PR to this PR (target branch borrow-v3 instead of master)

[timokroeger]

RX abort is slow, previously we used embassy_extras::low_power_wait_until() to wait until it is over.

[timokroeger]

I had a look at the UARTE part, but I did not test on real hardware.
The new approach makes the logic much more apparent and the code size improvements are a big win in my eyes.

Still I have few nits for the RX part:

1. Previously the stop() method exposed a way to abort the reception and get to know the number of received bytes. When using select() we still can stop the transfer by dropping the future. But now there is no way to know how many bytes were received? Unfortunately UARTE does not provide the capability to implement the IdleUart trait.

2. I think there is a edge case for this specific reception scenario:

   • tasks_startrx
   • drop future, tasks_stoprx
   • spinning for endrx
   • drop uarte, disable uarte -> enters unspecified state (with high current consumption)
   • rxto outstanding (but not triggered because uarte is disabled)

   When canceling a reception we additionally need to wait for the RXTO event before disabling the peripheral.
   IIRC that’s how the nrfx libraries handle it and my current measurements showed agreement.
   When the reception ends by itself (DMA buffer full) RXTO will not happen and it is not required to wait before disabling the peripheral.

[Dirbaio]

Thanks for taking a look @timokroeger , very good points.

RX abort is slow, previously we used embassy_extras::low_power_wait_until() to wait until it is over

Yep, I'll bring that back. Requires some care with the new interrupt handling but I think it's doable.

Previously the stop() method exposed a way to abort the reception and get to know the number of received bytes

I removed it because IMO the stop() method on the returned future is not the way to go.

• it's extremely unconventional, therefore hard to discover.
• It makes it impossible to implement the drivers with async{ }, you have to manually implement a future.
• When Rust gets real async traits, the methods will be regular async fns with no way to attach the stop() method either.

I do agree it's a nice functionality to have. An alternative would be adding a .last_read_bytes() -> usize method to Uart which returns how many bytes were read by the last .read() call. Less nice, but plays nice with async{} and the future real async traits. We can make it an extension trait on uart::Read too which is nice.

When canceling a reception we additionally need to wait for the RXTO event before disabling the peripheral.

Oh, that's what RXTO is for 🤦 . I now understand why your impl was that complex, haha. Will fix.