embassy-rs / embassy

Modern embedded framework, using Rust and async.
https://embassy.dev
Apache License 2.0
5.66k stars 790 forks source link

STM32H7 SpiDeviceWithConfig does not properly apply the config #2259

Open SteveNguyen opened 12 months ago

SteveNguyen commented 12 months ago

I am trying to use SpiDeviceWithConfig https://docs.embassy.dev/embassy-embedded-hal/git/default/shared_bus/asynch/spi/struct.SpiDeviceWithConfig.html in order to share a spi bus between different device drivers with different spi config (on a STM32H7). I am using a shared_bus as explained here: https://docs.embassy.dev/embassy-embedded-hal/git/default/shared_bus/blocking/spi/index.html and it seems to work ok. My problem is that it seems that the Config is not correctly applied for each of my device. After a bit of debugging I can confirm that the correct set_config() is called: https://github.com/embassy-rs/embassy/blob/83bed7bca476a71da6cfbdb074fdd59862929750/embassy-stm32/src/spi/mod.rs#L325 with the correct config but if I check the actual config of the spi after this call, it is stuck at the spi init state. After a bit of tinkering, it happens that if I do in set_config() as it is done in new_inner(): https://github.com/embassy-rs/embassy/blob/83bed7bca476a71da6cfbdb074fdd59862929750/embassy-stm32/src/spi/mod.rs#L213 with the T::enable_and_reset() and setting the registers, everything seems to work as intended, i.e. the config is correctly set for each of my device at each spi call.

To clarify a little bit my code looks something like that:

` let mut spi_config = spi::Config::default(); spi_config.frequency = embassy_stm32::time::Hertz(1_000_000); spi_config.bit_order = spi::BitOrder::MsbFirst; spi_config.mode = spi::MODE_1;

let spi = spi::Spi::new(
    peri,
    sck,
    mosi,
    miso,
    NoDma,
    NoDma,
    spi_config,
);
let spi_bus: Mutex<NoopRawMutex, _> = Mutex::new(RefCell::new(spi));

let mut spi_device1_config = spi::Config::default();
spi_device1_config.frequency = embassy_stm32::time::Hertz(1_000_000);
spi_device1_config.mode = spi::MODE_3;
spi_device1_config.bit_order = spi::BitOrder::MsbFirst;

let mut spi_device2_config = spi::Config::default();
spi_device2_config.mode = spi::MODE_3;
spi_device2_config.frequency = embassy_stm32::time::Hertz(1_000_000);
spi_device2_config.bit_order = spi::BitOrder::MsbFirst;

let spi_device1 = SpiDeviceWithConfig::new(
    &spi_bus,
    Output::new(device1_cs, Level::High, Speed::Medium),
    spi_device1_config,
);

let spi_device2 = SpiDeviceWithConfig::new(
    &spi_bus,
    Output::new(device2_cs, Level::High, Speed::Medium),
    spi_device2_config,
);

loop{
    spi_device1
        .transfer_in_place(&mut transfer_data)
        .map_err(|e| {
            error!("!!! Error SPI {:?}!!!", e);
            embassy_stm32::spi::Error::Framing
        })?;
    Timer::after(Duration::from_millis(1)).await;
    spi_device2
        .transfer_in_place(&mut transfer_data)
        .map_err(|e| {
            error!("!!! Error SPI {:?}!!!", e);
            embassy_stm32::spi::Error::Framing
        })?;

    Timer::after(Duration::from_millis(1)).await;
}

`

By adding some debug I clearly see the set_config() being called for each transfer but the bus config keeps stuck at the spi_config state.

Did I miss something?

Dirbaio commented 11 months ago

Nice find! :eyes:

it could be taht SPI needs to be disabled (SPE=false) before changing the config? This might be why an RCC reset fixes it, because it disables it. We should avoid a full RCC reset, it's too heavyweight.

If you figure it out, could you send a PR fixing this?

SteveNguyen commented 11 months ago

Well, looks like you are right :) disabling/enabling the SPI at each reconfigure seems to do the trick. I inserted a:

T::REGS.cr1().modify(|w| {
  w.set_spe(false);
});

...

T::REGS.cr1().modify(|w| {
  w.set_spe(true);
});

In this block: https://github.com/embassy-rs/embassy/blob/83bed7bca476a71da6cfbdb074fdd59862929750/embassy-stm32/src/spi/mod.rs#L344C10-L344C10 which corresponds to my chip. I can't say for the other cfg but it is probably the same deal?