David-OConnor / stm32-hal

This library provides access to STM32 peripherals in Rust.
MIT License
148 stars 44 forks source link

SPI DMA for L4 #87

Open gworkman opened 1 year ago

gworkman commented 1 year ago

Hello again!

Back again with another question, but perhaps a possible bug I've found. After reading all of the examples, I can't figure out why DMA isn't working on my STM32L431 board for SPI reads (writes work okay). Some of the things that I double checked:

Also note that this happens for SPI1 and for SPI2.

I'm happy to poke around some registers, but not sure where to start. Help appreciated!

Minimal reproducible example:

//! This example shows a complete project, including file structure, and config
//! needed to flash using an ST-Link. The project structure is based on
//! [Knurling's app-template](https://github.com/knurling-rs/app-template).
//! This file demonstrates an overview of this library's features.

//! See the syntax example in the main STM32-HAL repo for a more detailed example.

#![no_main]
#![no_std]

use cortex_m::{
    self,
    delay::{self, Delay},
    interrupt::{free, Mutex},
    peripheral::NVIC,
};
use cortex_m_rt::entry;

// These lines are part of our setup for debug printing.
use defmt_rtt as _;
use panic_probe as _;

use defmt::println;

// Import parts of this library we use. You could use this style, or perhaps import
// less here.
use stm32_hal2::{
    self,
    clocks::Clocks,
    dma::{self, ChannelCfg, Dma, DmaChannel, DmaInterrupt},
    gpio::{OutputSpeed, OutputType, Pin, PinMode, Port},
    low_power, pac,
    pac::interrupt,
    spi::{Spi, SpiConfig},
};

#[entry]
fn main() -> ! {
    // Set up ARM Cortex-M peripherals. These are common to many MCUs, including all STM32 ones.
    let cp = cortex_m::Peripherals::take().unwrap();
    // Set up peripherals specific to the microcontroller you're using.
    let dp = pac::Peripherals::take().unwrap();

    // This line is required to prevent the debugger from disconnecting on entering WFI.
    // This appears to be a limitation of many STM32 families. Not required in production code,
    // and significantly increases power consumption in low-power modes.
    stm32_hal2::debug_workaround();

    // Create an initial clock configuration that uses the MCU's internal oscillator (HSI),
    // sets the MCU to its maximum system clock speed.
    let clock_cfg = Clocks::default();

    // Write the clock configuration to the MCU. If you wish, you can modify `clocks` above
    // in accordance with [its docs](https://docs.rs/stm32-hal2/0.2.0/stm32_hal2/clocks/index.html),
    // and the `clock_cfg` example.
    clock_cfg.setup().unwrap();

    println!("Start test");

    // setup SPI
    let mut spi2_miso = Pin::new(Port::B, 14, PinMode::Alt(5));
    let mut spi2_mosi = Pin::new(Port::B, 15, PinMode::Alt(5));
    let mut spi2_sck = Pin::new(Port::B, 13, PinMode::Alt(5));
    let mut spi2_cs = Pin::new(Port::B, 12, PinMode::Output);

    spi2_miso.output_speed(OutputSpeed::VeryHigh);
    spi2_mosi.output_speed(OutputSpeed::VeryHigh);
    spi2_sck.output_speed(OutputSpeed::VeryHigh);
    spi2_cs.output_speed(OutputSpeed::VeryHigh);

    spi2_miso.output_type(OutputType::PushPull);
    spi2_mosi.output_type(OutputType::PushPull);
    spi2_sck.output_type(OutputType::PushPull);
    spi2_cs.output_type(OutputType::PushPull);

    spi2_cs.set_high();

    let spi_cfg = SpiConfig {
        mode: stm32_hal2::spi::SpiMode::mode3(),
        ..Default::default()
    };
    let mut spi = Spi::new(dp.SPI2, spi_cfg, stm32_hal2::spi::BaudRate::Div256);

    // read some data synchronously
    let mut buf = [0u8; 4];
    spi2_cs.set_low();
    spi.write(&[0x81, 0, 0, 0, 0]).unwrap();
    spi2_cs.set_high();

    // read some data synchronously
    spi2_cs.set_low();
    spi.write(&[0x00]).unwrap();
    spi.transfer(&mut buf).unwrap();
    spi2_cs.set_high();

    println!("Buf: {:x}", buf);
    assert_eq!(buf, [0x34, 0x36, 0x37, 0x31]); // this is known device ID

    // now try to do the same thing but asynchronously
    let mut dma = Dma::new(dp.DMA1);

    // SPI2_RX is on DMA1_CH4
    dma.enable_interrupt(DmaChannel::C4, DmaInterrupt::TransferComplete);
    // SPI2_TX is on DMA1_CH5
    dma.enable_interrupt(DmaChannel::C5, DmaInterrupt::TransferComplete);

    unsafe {
        NVIC::unmask(pac::Interrupt::DMA1_CH4);
        NVIC::unmask(pac::Interrupt::DMA1_CH5);
    }

    let mut delay = Delay::new(cp.SYST, clock_cfg.sysclk());

    spi2_cs.set_low();
    unsafe {
        spi.write_dma(
            &[0x81, 0, 0, 0, 0],
            DmaChannel::C5,
            ChannelCfg::default(),
            &mut dma,
        );
    }

    // transfer should have completed by now. The interrupt handler
    // should have set the CS pin high and print to console
    delay.delay_ms(1000);

    spi2_cs.set_low();
    unsafe {
        spi.read_dma(
            &mut buf,
            DmaChannel::C5,
            ChannelCfg::default(),
            dma::DmaPeriph::Dma1,
        );
    }
    // transfer should have completed by now. The interrupt handler
    // should have set the CS pin high and print to console
    delay.delay_ms(1000);

    println!("Done");

    loop {
        low_power::sleep_now();
    }
}

#[interrupt]
fn DMA1_CH4() {
    dma::clear_interrupt(
        dma::DmaPeriph::Dma1,
        DmaChannel::C4,
        DmaInterrupt::TransferComplete,
    );

    let mut spi2_cs = Pin::new(Port::B, 12, PinMode::Output);
    spi2_cs.set_high();

    println!("CH4 interrupt called")
}

#[interrupt]
fn DMA1_CH5() {
    dma::clear_interrupt(
        dma::DmaPeriph::Dma1,
        DmaChannel::C5,
        DmaInterrupt::TransferComplete,
    );

    let mut spi2_cs = Pin::new(Port::B, 12, PinMode::Output);
    spi2_cs.set_high();

    println!("CH5 interrupt called")
}

// same panicking *behavior* as `panic-probe` but doesn't print a panic message
// this prevents the panic message being printed *twice* when `defmt::panic` is invoked
#[defmt::panic_handler]
fn panic() -> ! {
    cortex_m::asm::udf()
}

The console output:

Start test
Buf: [34, 36, 37, 31]
CH5 interrupt called
Done
David-OConnor commented 1 year ago

Could be a set up/tear down thing. I think transfer_dma is what you want. STM32 is picky about how you handle DMA readings on SPI. Here are some quick+dirty CPs you can modify A/R. I think the examples folder has some relevant ones too.

Start the transfer like this:

    .cs_imu.set_low();

    unsafe {
        spi.transfer_dma(
            &WRITE_BUF,
            &mut IMU_READINGS,
            IMU_TX_CH,
            IMU_RX_CH,
            ChannelCfg::default(),
            ChannelCfg::default(),
            DmaPeriph::Dma1,
        );
    }

On TC interrupt, close it out like this:

         dma::clear_interrupt(
            setup::IMU_DMA_PERIPH,
            setup::IMU_RX_CH,
            DmaInterrupt::TransferComplete,
        );
        cx.local.cs_imu.set_high();

        cx.shared.spi1.lock(|spi1| {
            // Note that this step is mandatory, per STM32 RM.
            spi1.stop_dma(
                setup::IMU_TX_CH,
                Some(setup::IMU_RX_CH),
                setup::IMU_DMA_PERIPH,
            );
        });
gworkman commented 1 year ago

Oh interesting - I hadn't seen the need to call stop_dma before. This works to fix my consecutive writes (which I didn't even realize were having a DMA issue until you pointed this example out), but it still doesn't solve the reads. Here's what I got from my actual codebase:

In the setup:

let mut dma = Dma::new(dp.DMA1);
dma.enable_interrupt(DmaChannel::C2, DmaInterrupt::TransferComplete);
dma.enable_interrupt(DmaChannel::C5, DmaInterrupt::TransferComplete);

unsafe {
        NVIC::unmask(Interrupt::TIM2);
        NVIC::unmask(Interrupt::DMA1_CH2);
        NVIC::unmask(Interrupt::DMA1_CH5);
    }

In the timer interrupt (which is working fine):

self.spi.transfer_dma(
                &[0; 8], // I don't really need this but don't know if possible to read without it
                &mut BUFFER, // also of size 8
                DmaChannel::C3,
                DmaChannel::C2,
                ChannelCfg::default(),
                ChannelCfg::default(),
                DmaPeriph::Dma1,
            )

Also tried:

self.spi.read_dma(
                &mut BUFFER,
                DmaChannel::C2,
                ChannelCfg::default(),
                DmaPeriph::Dma1,
            )

And then this is never calling my DMA1_CH2 interrupt. I have no idea why though

gworkman commented 1 year ago

Oh and I also added the spi.stop_dma call in the interrupt handler, but that function is not even called so something isn't right to begin with...

David-OConnor commented 1 year ago

I suspect the issue is with the write buffer. When transferring, the first Val is generally the device's first or only reg to read; the rest are padded to the read len. I think this might depend on the device you're reading from. So, the transfer you posted is telling the device to read from reg addr 0.

gworkman commented 1 year ago

Yes so in this case, I don't care about the SPI transfer data. I'm actually dealing with an SSI encoder, and given a clock it will output valid data on the SPI_MISO bus. It might be a factor in all of this, but I actually don't even configure the MISO pin, because I don't use it (and writing data to it might affect other things).

I guess I should try configuring for RX only mode, but it was working without DMA so I didn't do that yet

gworkman commented 1 year ago

would it be possible to do something like this?

self.spi.transfer_dma(
                &[], 
                &mut BUFFER, // size 8
                DmaChannel::C3,
                DmaChannel::C2,
                ChannelCfg::default(),
                ChannelCfg::default(),
                DmaPeriph::Dma1,
            )

I think I tried this but it didn't work. Your examples have one byte in the write buffer?

David-OConnor commented 1 year ago

Gotcha! I do a fair bit of SPI DMA (On G4 and H7), but the construct for the devices I use is, pass the first register to read, then the device fills the buf, incrementing the register. It does sound like for this you need read and not transfer. (And when you call stop, pass None as the second argument)

I'll let you know if I have any ideas. So, I mislead you on transfer; I think read is what you want.

gworkman commented 1 year ago

Thanks for confirmation on that. Is there a set of registers you recommend poking around in?

And I noticed there is a dma.cfg_channel which is public, should I be looking into using that? It wasn't in the examples I was looking at iirc

David-OConnor commented 1 year ago

Status register; check what flags are set.

gworkman commented 1 year ago

Here's another mind-boggling one: I set up the DMA SPI write such that it will trigger when complete to unset the chip select.

#[interrupt]
fn DMA1_CH5() {
    dma::clear_interrupt(
        DmaPeriph::Dma1,
        DmaChannel::C5,
        DmaInterrupt::TransferComplete,
    );

    defmt::println!("DMA1_CH5"); // this one here

    free(|cs| {
        access_global!(DRIVER, driver, cs);
        driver.dma_tx_complete();
    });
}

But, when I take the println call out of the interrupt handler it no longer performs the write to the device (which is verified externally)

David-OConnor commented 1 year ago

cfg_channel is handled by write_dma, read_dma etc; you don't need to set it.

Just remembered something that is definitely relevant here: I think your buffers are dropping before the reads/writes complete. The easiest way around this is using static buffers. (But any Rusty tricks you have to keep them alive will work). This is why I marked the dma fns as unsafe.

gworkman commented 1 year ago

The main buffer I'm trying to read into is already static -

pub static mut BUFFER: [u8;8] = [0; 8];

self.spi.read_dma(
                &mut BUFFER,
                DmaChannel::C2,
                ChannelCfg::default(),
                DmaPeriph::Dma1,
            )

I will double check the other buffers and see what they look like. Thanks!

gworkman commented 1 year ago

So I did some poking around the last couple days on this issue. One thing I notice off the bat is that I have an overrun flag set after I do the spi write with DMA. Since I'm using the spi.write_dma and spi.read_dma instead of spi.transfer_dma, that is why this is happening, right? From looking at the reference manual I'm not sure how much of an issue it is.

Also I was trying to retrace the code and compare to the reference manual steps. I see that the stop_dma function deviates from the manual procedure a little bit because the call to self.disable is commented out. Not sure what effect this has.

I published the sample code I was playing with from above in a separate repo, so you can take a look and evaluate the same things I'm looking at:

https://github.com/gworkman/reproduce-spi-dma-error

I printed all of the register status, they are below. I realize you might not have access to L4 hardware so I appreciate that you've been very willing to help me out thus far :)

Start test
Buf: [34, 36, 37, 31]
BEFORE WRITE
SPI2.cr1 1101111111
SPI2.cr2 1011100000000
SPI2.crcpr 111
SPI2.dr 11011000110100
SPI2.rxcrcr 0
SPI2.sr 10
SPI2.txcrcr 0

CH5 interrupt called

BEFORE READ
SPI2.cr1 1101111111
SPI2.cr2 1011100000000
SPI2.crcpr 111
SPI2.dr 0
SPI2.rxcrcr 0
SPI2.sr 10001000011
SPI2.txcrcr 0

AFTER READ
SPI2.cr1 1101111111
SPI2.cr2 1011100000001
SPI2.crcpr 111
SPI2.dr 0
SPI2.rxcrcr 0
SPI2.sr 1000000011
SPI2.txcrcr 0
Done
gworkman commented 9 months ago

Hi @David-OConnor, I wanted to ping you and see if there is anything I can do to further debug this issue. After putting it down for a while, I'm looking to pick back up our rust-based firmware, as it is much more maintainable and enjoyable to write then the C-based alternative :)

Any suggestions for debugging next steps? The repo in the above comment is still a valid reproduction of the issue