Add steal() for each peripheral.

[JalonWong]

This is more convenient than the Peripherals::steal()

[thejpster]

Is it sufficient to jus tcall the existing FOO::ptr() method to get a Foo::RegisterBlock instead?

I'm not sure I'd want to make it easier to steal peripherals, because a stolen peripheral bypasses the ownership system (p.FOO is supposed to be a singleton and indicate exclusive access to the Foo peripheral).

[JalonWong]

@thejpster
The FOO::ptr() is not sufficient. For instance, I have five UARTs, but only two different uart::RegisterBlock types. This makes it challenging for me to treat each UART differently. Moreover, using UART types also enhances code readability.

Indeed, a stolen peripheral bypasses the ownership system. So I implemented the steal() as an unsafe method. And there are indeed practical needs for it, such as when I want to split a UART into four instances: a sender, a receiver and two interrupt handlers. I found it difficult to achieve this without the steal() method.

I believe that anyone who uses unsafe methods should take responsibility for their choices. And it can be challenging without these unsafe methods when working on low-level MCU development.

[thejpster]

But you don't have a RegisterBlock, you have a *mut RegisterBlock - the value of the pointer indicates which of the multiple UART peripherals you are currently dealing with. I seem to recall that only thing the singleton objects really do is hide this from you by automatically conjuring up a pointer whenever it is required, and implementing trait Deref so accesses to it are transparent.

If a HAL wishes to unsafely access the peripherals without using the PAC's sense of ownership, it can do that by creating its own types which are Copy or Clone. How it chooses to ensure that a read-modify-write cannot happen concurrently from multiple threads (or interrupts) is then a problem for the HAL author to solve.

But I'm still not sure the PAC needs to make it easier for HAL authors to do this, because it's outside the normal pattern of operation.

[adamgreig]

We discussed this PR a bit more in today's meeting, I think it's fair to say there is interest in merging it but we're not sure yet, and in particular wonder if the new function would need to set the global flag that Peripherals::steal() sets. Hopefully not, but we'll try and find out.

[JalonWong]

We discussed this PR a bit more in today's meeting, I think it's fair to say there is interest in merging it but we're not sure yet, and in particular wonder if the new function would need to set the global flag that Peripherals::steal() sets. Hopefully not, but we'll try and find out.

No need to set the global flag, because before calling the steal() of each peripheral, Peripherals::take() or Peripherals::steal() must be called first.

[reitermarkus]

No need to set the global flag, because before calling the steal() of each peripheral, Peripherals::take() or Peripherals::steal() must be called first.

Given the peripherals are already stolen/taken, maybe this should be named something like clone_unchecked.

[JalonWong]

Given the peripherals are already stolen/taken, maybe this should be named something like clone_unchecked.

@reitermarkus
This does make more sense.

But you don't have a RegisterBlock, you have a *mut RegisterBlock - the value of the pointer indicates which of the multiple UART peripherals you are currently dealing with. I seem to recall that only thing the singleton objects really do is hide this from you by automatically conjuring up a pointer whenever it is required, and implementing trait Deref so accesses to it are transparent.

@thejpster
Implementing traits and writing generics for RegisterBlock always feels a bit weird to me.
So currently, I'm doing it like this:

pub trait UartReg {
    fn ptr() -> *mut Self;
    // or
    unsafe fn mut_ref() -> &'static mut Self;
    // or
    unsafe fn steal_mut(&self) -> &'static mut Self;
}

impl UartReg for USART1 { 
    fn ptr() -> *mut Self {
        (1_usize as *mut Self)
    }
    //...
}

impl UartReg for UART4 { //... }

fn foo<U: UartReg>() {
    let u = unsafe { &(*U::ptr()) };
}

They are very useful. However, they are also weird because the pointer points to an invalid address.

So I think adding an unsafe clone method to each peripheral would be a more elegant solution.

[adamgreig]

Thanks for the PR! We discussed it again in today's meeting and we think it might be best if these new methods were associated functions (i.e., like fn steal() -> Self without requiring &self).

They'd still be unsafe and do the same thing, but you don't need to already have the peripheral before you can make a new copy. With an associated steal() there's also no need for a clone_unchecked(), since you can just steal() directly. We would separately make it so that Peripherals::steal() does not set the global flag, so that doesn't need to be a problem for this PR.

What do you think?

[JalonWong]

@adamgreig

Thanks for the PR! We discussed it again in today's meeting and we think it might be best if these new methods were associated functions (i.e., like fn steal() -> Self without requiring &self).

It is sufficient for me.
But this approach makes it easier to steal peripheral instances than my current approach. If you think it's appropriate, I have no objections.

[adamgreig]

I think this is ready to merge, @rust-embedded/tools any last thoughts?

[thejpster]

Won't clippy complain if an unsafe fn doesn't have a # Safety in the docs?

[burrbull]

@adamgreig Maybe you fix docs yourself?

[burrbull]

cc @thejpster

[thejpster]

Sorry I'm on vacation at the moment.