[embassy-rp] Remove instance from rp spi and i2c

[marcemmers]

A while back I made this (#4155) change for the uart peripherals. The spi and i2c peripherals could still use this same change, so here is it.

The only thing I had to remove that somebody recently added (#4405) was the Debug derive on the I2c type. I don't really understand the use-case of having it so I removed it since there is no easy way of keeping it. If there is a use-case for it I can try to find a way to manually implement it to get similar output.

[ivmarkov]

Hm, is removing the 'd lifetime OK?

Wouldn't that mean that the peripherals will not be borrowed for the lifetime of the driver but rather - immediately available for reuse which is problematic?

[marcemmers]

I did think about that and I don't think its really an issue.

All functions creating an instance take the peripherals by move instead of borrowing them, so the borrow checker should take care of it anyway.

If you would reborrow the peripherals it's still possible, but that would be the case with the lifetime still there too.

[ivmarkov]

I did think about that and I don't think its really an issue.

All functions creating an instance take the peripherals by move instead of borrowing them, so the borrow checker should take care of it anyway.

If you would reborrow the peripherals it's still possible, but that would be the case with the lifetime still there too.

I don't think that's correct. Perhaps you could try it by yourself? Reborrow a periph, push it down the driver's new function and then observe how the original periph is available for reuse right after the new function had been called and well before the driver is dropped.

IMO you should keep the 'd lifetime into the driver as it used to be.

[marcemmers]

Seems that you are right. I did test it but only after my fix and of course then it was possible. I will add back the lifetime, thanks!

Learn something everyday with the borrow checker 😄

[ivmarkov]

Nit: shouldn't this be PhantomData<&'d mut ()>?

[marcemmers]

This goes a little bit over my head on what the reason would be for this.
I did find this: https://doc.rust-lang.org/nomicon/phantom-data.html, but this is talking about PhantomData with a T in there. Since T is () here I expect it does not really matter that much as we care mostly about the lifetime which is handled the same.

But I would rather get an answer from somebody who knows this better than me.

[ivmarkov]

Since T is ()

The "phantom" data you are modeling is NOT () but a reference (to ()). To be more precise, a mutable (= exclusive) reference to the data.

And () is used simply because it does not really matter what exactly that reference points to, but rather that it is a reference, with a certain lifetime.

And the reason I say you need to faithfully model a &'d mut rather than &'d is because there are differences between (exclusive) mutable references, and shared (immutable) references. Whether these differences are material for the concrete case of Peri<'d> I'm not sure (hence the "nit"), but it is till a good idea I think to try to faithfully model the reference. And for the case of Peri::borrow() it is a mutable (exclusive) reference in that you can't borrow the same peripheral twice and pass the two borrows to two separate drivers.

Then - and if nothing else - the original reference in the PhantomData before your changes was &'d mut () so I guess if it wasn't broken we should not change it. :)

[ivmarkov]

Oh yeah, if it is not clear, "pointers" in Rust (i.e. references) are also "data".

[ivmarkov]

Here's the definition of Peri. Since you are "erasing" the Peri type (as it does not matter) all that's left is the reference to the erased T peripheral, which reference - inside Peri - is &'d mut.

[marcemmers]

Ah I see my link was not complete, I was referencing this table at the bottom of the page:
https://doc.rust-lang.org/nomicon/phantom-data.html#table-of-phantomdata-patterns

Looking at that table it seems that mut or not is only influencing the variance of T and if the type will be Send/Sync. I am probably oversimplifying it though.

I agree with you that changing it back to mut is better, even if it is because it was that way like you said. Just also trying to learn a little bit ;)

Thanks for the explanations so far!

[ivmarkov]

@marcemmers In case you are wondering what is going on - I don't have commit privileges for embassy-rp. You need to ping some of the maintainers for a more thoughtful code review and merge approval.

Probably (@lulf ?)

[lulf]

The change looks good to me, but perhaps you can create an explicit impl Debug that instead of removing it? Even though the instance is erased, it could still print the mode.

[marcemmers]

The change looks good to me, but perhaps you can create an explicit impl Debug that instead of removing it? Even though the instance is erased, it could still print the mode.

Sure I can add something, could be something like this:

impl<M: Mode> core::fmt::Debug for I2c<'_, M> {
    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        let instance = match self.info.regs {
            pac::I2C0 => "I2C0",
            pac::I2C1 => "I2C1",
            _ => "N/A",
        };
        f.debug_struct("I2c")
            .field("Instance", &instance)
            .field("phantom", &self.phantom)
            .finish_non_exhaustive()
    }
}

But I kind of have the feeling that it is a bit over-engineered, especially as most other peripherals don't derive Debug either. So I am still wondering if it is useful to keep it. I saw another I2c PR for the raspberry crate also removing it.

[ivmarkov]

The change looks good to me, but perhaps you can create an explicit impl Debug that instead of removing it? Even though the instance is erased, it could still print the mode.

Sure I can add something, could be something like this:

impl<M: Mode> core::fmt::Debug for I2c<'_, M> {
    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
        let instance = match self.info.regs {
            pac::I2C0 => "I2C0",
            pac::I2C1 => "I2C1",
            _ => "N/A",
        };
        f.debug_struct("I2c")
            .field("Instance", &instance)
            .field("phantom", &self.phantom)
            .finish_non_exhaustive()
    }
}

But I kind of have the feeling that it is a bit over-engineered, especially as most other peripherals don't derive Debug either. So I am still wondering if it is useful to keep it. I saw another I2c PR for the raspberry crate also removing it.

I think Ulf is asking you to just print the M so to say, while you are going a bit out of your way to print what was just lost/erased - the instance.