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

Fix SpiDmaBus write impl #2843

Merged
merged 2 commits into from
Dec 19, 2024
Merged

Fix SpiDmaBus write impl #2843

merged 2 commits into from
Dec 19, 2024

Conversation

i404788
Copy link
Contributor

@i404788 i404788 commented Dec 19, 2024

The following code currently does not run:

  let spi_config_slow = esp_hal::spi::master::Config::default()
    .with_frequency((11).MHz())
    .with_mode(esp_hal::spi::SpiMode::Mode0);

  let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) =
    esp_hal::dma_buffers!(DMA_BUFFER_SIZE);

  defmt::info!(
    "RX: {:x} len {} ({} descripters)",
    rx_buffer.as_ptr(),
    rx_buffer.len(),
    rx_descriptors.len()
  );

  defmt::info!(
    "TX: {:x} len {} ({} descripters)",
    tx_buffer.as_ptr(),
    tx_buffer.len(),
    tx_descriptors.len()
  );

  let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
  let dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();

  // let (mut descriptors, mut rx_descriptors) = dma_descriptors!(32000);

  let mut spi = Spi::new(spi2, spi_config_slow)
    .unwrap()
    .with_sck(sclk)
    .with_mosi(mosi)
    .with_miso(miso)
    .with_dma(dma_channel)
    .with_buffers(dma_rx_buf, dma_tx_buf);

  spi.read(&mut [0u8]).unwrap();
  spi.write(&[0u8]).unwrap();

It will show:

INFO RX: 0x3fc8da28 len 4096 (2 descripters)
0x3fc8da28 - main::display::initialize_display::init_display::{{closure}}::BUFFER
    at ??:??
INFO TX: 0x3fc8ea40 len 4096 (2 descripters)
0x3fc8ea40 - main::display::initialize_display::init_display::{{closure}}::BUFFER
    at ??:??
DEBUG Enable Spi2 true
DEBUG Reset Spi2
DEBUG Enable Gdma true
DEBUG Reset Gdma
DEBUG Preparing RX transfer Preparation { start: 0x3fc8ea28, direction: In, accesses_psram: false, burst_transfer: BurstConfig { external_memory: Size16, internal_memory: Disabled }, check_owner: None }
0x3fc8ea28 - main::display::initialize_display::init_display::{{closure}}::BUFFER
    at ??:??
DEBUG Preparing RX transfer Preparation { start: 0x3fc90be0, direction: In, accesses_psram: false, burst_transfer: BurstConfig { external_memory: Size16, internal_memory: Disabled }, check_owner: Some(false) }
0x3fc90be0 - esp_hal::soc::MAPPED_PSRAM.1
    at ??:??


PanicInfo { message: called `Result::unwrap()` on an `Err` value: DmaError(DescriptorError), location: Location { file: "src/bin/display/initialize_display.rs", line: 74, col: 21 }, can_unwind: true, force_no_backtrace: false }

Description

Currently SpiDmaBus::write is broken due to the Rx being attempted instead of Tx.

Testing

See hil-test CI.

@bugadani
Copy link
Contributor

Good find. Can I ask you to add a test case to hil-test/tests/spi_full_duplex that would fail without this change?

@bugadani bugadani added the skip-changelog No changelog modification needed label Dec 19, 2024
Copy link
Collaborator

@Dominaezzz Dominaezzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops, my bad 😅. Thanks for the fix

@i404788
Copy link
Contributor Author

i404788 commented Dec 19, 2024

Can I ask you to add a test case to hil-test/tests/spi_full_duplex that would fail without this change?

Hil-test added 👍

Copy link
Contributor

@bugadani bugadani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@bugadani bugadani added this pull request to the merge queue Dec 19, 2024
Merged via the queue into esp-rs:main with commit f1c372f Dec 19, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip-changelog No changelog modification needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants