Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SPI DMA for L4 #87

Open
gworkman opened this issue Jul 3, 2023 · 14 comments
Open

SPI DMA for L4 #87

gworkman opened this issue Jul 3, 2023 · 14 comments

Comments

@gworkman
Copy link
Contributor

gworkman commented Jul 3, 2023

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:

  • For STM32L4, the DMA channels are fixed. I double checked, for SPI1_RX, the channel is DMA1_CH2, and for SPI2_RX, the channel is DMA1_CH4.
  • The DMA interrupts are enabled, and unmasked in the NVIC
  • A DMA request is sent by calling spi.read_dma

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
Copy link
Owner

David-OConnor commented Jul 3, 2023

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
Copy link
Contributor Author

gworkman commented Jul 3, 2023

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
Copy link
Contributor Author

gworkman commented Jul 3, 2023

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
Copy link
Owner

David-OConnor commented Jul 3, 2023

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
Copy link
Contributor Author

gworkman commented Jul 3, 2023

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
Copy link
Contributor Author

gworkman commented Jul 3, 2023

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
Copy link
Owner

David-OConnor commented Jul 3, 2023

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
Copy link
Contributor Author

gworkman commented Jul 3, 2023

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
Copy link
Owner

David-OConnor commented Jul 3, 2023

Status register; check what flags are set.

@gworkman
Copy link
Contributor Author

gworkman commented Jul 3, 2023

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
Copy link
Owner

David-OConnor commented Jul 3, 2023

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
Copy link
Contributor Author

gworkman commented Jul 3, 2023

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
Copy link
Contributor Author

gworkman commented Jul 6, 2023

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants