Cannot work with DMA channels in a generic way

[ivmarkov]

trafficstars

Bug description

I'm trying not to hard-code peripherals and work with their all-generic versions, yet I fail when trying to swap-in a generic channel type for where a concrete DMA channel was passed previously.

Consider this snippet:

use esp_hal::dma;
use esp_hal::dma::*;
use esp_hal::dma_buffers;
use esp_hal::gpio;
use esp_hal::peripheral::Peripheral;
use esp_hal::peripherals;
use esp_hal::prelude::*;
use esp_hal::spi;
use esp_hal::Async;

fn new_foo<'d, T, const N: u8>(
    dma_channel: dma::ChannelCreator<N>, // PROBLEM: I want this to be generic
    spi: impl Peripheral<P = T> + 'd,
    sclk: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    sdo: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    sdi: impl Peripheral<P = impl gpio::InputPin> + 'd,
    cs: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    reset: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    ready: impl Peripheral<P = impl gpio::InputPin> + 'd,
)
where 
    T: spi::master::InstanceDma,
    dma::ChannelCreator<N>: esp_hal::dma::DmaChannelConvert<<T as esp_hal::dma::DmaEligible>::Dma>,
{
    let dma_channel = dma_channel.configure_for_async(false, DmaPriority::Priority0);

    let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000);
    let dma_rx_buf = dma::DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
    let dma_tx_buf = dma::DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();
            
    let spi_driver = spi::master::Spi::new(spi, 10.MHz(), spi::SpiMode::Mode0)
        .with_sck(sclk)
        .with_mosi(sdo)
        .with_miso(sdi)
        .with_dma(dma_channel)
        .with_buffers(dma_rx_buf, dma_tx_buf);
}

Multiple issues with it:

1. dma_channel is moved into the function, whereas I want to keep the flexibility that it is either moved or mutably borrowed (and can be reused when the spi_driver instance gets dropped)
2. Even this current ^^^ syntax does not work, because configure_for_async is defined (with a macro) for each concrete ChannelCreator<1, 2, 3, ...4> rather than for impl<const N: u8> ChannelCreator<N>
3. The above code needs this weird dma::ChannelCreator<N>: esp_hal::dma::DmaChannelConvert<<T as esp_hal::dma::DmaEligible>::Dma> to work. That is, given that the issue (2.) is solved

To Reproduce

Try to type-check the above code.

Expected behavior

Ideally, I should be able to type something like: dma_channel: impl Peripheral<P = impl SomethingSomethingChannelCreatorSomething> + 'd and be done with it.

Environment

I'm working with esp32s3, but I think the problem is generic. Latest released esp-hal as of today (0.21.1) but the issue persist with main.

[ivmarkov]

To add a bit more, to me (IMHO) this whole configure_for_async pattern also feels out of place in code that embraces the notion of a Peripheral, in that it takes a ChannelCreator (a - hm - "peripheral"?) - and then "transforms" it into something else (i.e. it takes self).

So in a way, the channels of the DMA peripherals do not look like peripherals, but like something else, in that you can "transform" them. But I don't want to transform them really. I just want to use them either with a move, or with a mutable borrow, like every other peripheral.

[Dominaezzz]

I think you should be calling configure/configure_for_async before new_foo, rather than inside it. Though I do see it becoming a trait in the future (with an associated type that impl's DmaChannel).

The hal should then support something along these lines.

fn new_foo<'d, T: spi::master::InstanceDma, const N: u8>(
    dma_channel: dma::Channel<T::Channel>, // T::Channel would be either Spi2Dma, Spi3Dma or AnyGdmaChannel.
    spi: impl Peripheral<P = T> + 'd,
    sclk: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    sdo: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    sdi: impl Peripheral<P = impl gpio::InputPin> + 'd,
    cs: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    reset: impl Peripheral<P = impl gpio::OutputPin> + 'd,
    ready: impl Peripheral<P = impl gpio::InputPin> + 'd,
) {

(If this suggestion looks familiar, it's because it was my initial DMA erasure proposal)

[ivmarkov]

I think you should be calling configure/configure_for_async before new_foo, rather than inside it. Though I do see it becoming a trait in the future (with an associated type that impl's DmaChannel).

As per my second comment, I'm, not sure this is solving anything, because configure_for_async takes self. What I mean is, imagine the following situation:

1. I want to take some channel, say dma.channel1
2. I want to use it in an SPI peripheral, in async mode (so far so hood)
3. At some point in time, I want to drop the SPI driver
4. After dropping the SPI driver, I want to (1) reuse dma.channel1 somewhere else, and (2) reuse it NOT in async mode

This ^^^ is not possible, unless DMA channels are treated as peripherals. I.e. I cannot use them as impl Peripheral<P = impl DmaChannel> currently, and thus I cannot mutably borrow them, and therefore I cannot re-use them somewhere else.

The hal should then support something along these lines.

fn new_foo<'d, T: spi::master::InstanceDma, const N: u8>(
    dma_channel: dma::Channel<T::Channel>, // T::Channel would be either Spi2Dma, Spi3Dma or AnyGdmaChannel.

^^^ This is still always moved in the SPI driver. The whole point is that IMO it should not work like that. This should be a peripheral, which can be either moved, or mutably borrowed, like everything else.

[Dominaezzz]

I suppose what I'm trying to say there is Channel should be the thing you borrow, not the creator. (You can't do this now but that's what it should be)

[ivmarkov]

I suppose what I'm trying to say there is Channel should be the thing you borrow, not the creator. (You can't do this now but that's what it should be)

I get it. But (sorry for persisting), this is still not the same as a Peripheral. With your suggestion:

• I have to decide if I would borrow, or move the channel when I am authoring my API (the new_foo function from above). The whole point of Peripheral is that it is not the API who decides, but the caller of the API. Which gives the whole flexibility...
• You are bypassing the fact that once I construct the Channel, I cannot go back to a creator. What if I want to construct a different Channel from the original creator, which is no longer "async"? This is currently impossible.

In the end, however we twist it, nether the "ChannelCreator" nor the "Channel" are peripherals, in the strict Peripheral sense. What are they I have no idea, but they don't fit.

[Dominaezzz]

(sorry for persisting)

Please do! We need to get this right.

• I have to decide if I would borrow, or move the channel

Yeah I was lazy with my reply. I implied in there that Channel would implement Peripheral.

Though, borrowing channels has awkward consequences for statics but we can deal with those later.

• What if I want to construct a different Channel from the original creator, which is no longer "async"? This is currently impossible.

This will be fixed by #2321 .

[ivmarkov]

• What if I want to construct a different Channel from the original creator, which is no longer "async"? This is currently impossible.

This will be fixed by #2321 .

So if I understand it right, (a) ChannelCreator will remain.... "something" (b) Channel will be a Peripheral, however you need to construct it (i.e. it will not "pre-exist", forever and ever); a bit like a driver, which is weird, but I guess with Io::new and Dma::new this can of worms is already opened... (c) Moreover, Channel will be a Peripheral with a typestate in that you can "into" it into something else, and then "from" it to something else?

[Dominaezzz]

Yes!

(I should note that this is just my idea. I haven't spoken to Daniel about what he plans to do.)

[ivmarkov]

Sounds complex, but I think that would work, at least for my case.

Also, the peripherals of the Esp are not simple either...