From 987f00bb1dece3857504416bdf6722bbd30f9774 Mon Sep 17 00:00:00 2001 From: liebman Date: Mon, 23 Sep 2024 03:27:43 -0700 Subject: [PATCH] PARL_IO: fix for garbage output at the start of some TX operations (#2211) * per TRM the TX clock should only be re-enabled after tx_start * CHANGELOG * added tests to check the for the correct number of clocks during valid * parl_io: fix test for esp32h2 * tests: parl_io: h2 PCNT does not like 20MHz --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/parl_io.rs | 6 +- hil-test/Cargo.toml | 8 ++ hil-test/tests/parl_io_tx.rs | 198 ++++++++++++++++++++++++++++ hil-test/tests/parl_io_tx_async.rs | 200 +++++++++++++++++++++++++++++ 5 files changed, 411 insertions(+), 2 deletions(-) create mode 100644 hil-test/tests/parl_io_tx.rs create mode 100644 hil-test/tests/parl_io_tx_async.rs diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 2219acd6d..8fec8ff52 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -68,6 +68,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - SPI: Fixed an issue where `wait` has returned before the DMA has finished writing the memory (#2179) - SPI: Fixed an issue where repeated calls to `dma_transfer` may end up looping indefinitely (#2179) - SPI: Fixed an issue that prevented correctly reading the first byte in a transaction (#2179) +- PARL_IO: Fixed an issue that caused garbage to be output at the start of some requests (#2211) ### Removed diff --git a/esp-hal/src/parl_io.rs b/esp-hal/src/parl_io.rs index 0be79817c..9fffe6d60 100644 --- a/esp-hal/src/parl_io.rs +++ b/esp-hal/src/parl_io.rs @@ -1452,8 +1452,6 @@ where let pcr = unsafe { &*crate::peripherals::PCR::PTR }; pcr.parl_clk_tx_conf() .modify(|_, w| w.parl_tx_rst_en().set_bit()); - pcr.parl_clk_tx_conf() - .modify(|_, w| w.parl_tx_rst_en().clear_bit()); Instance::clear_tx_interrupts(); Instance::set_tx_bytes(len as u16); @@ -1474,6 +1472,10 @@ where } Instance::set_tx_start(true); + + pcr.parl_clk_tx_conf() + .modify(|_, w| w.parl_tx_rst_en().clear_bit()); + Ok(()) } } diff --git a/hil-test/Cargo.toml b/hil-test/Cargo.toml index c774a6f23..f7f693d22 100644 --- a/hil-test/Cargo.toml +++ b/hil-test/Cargo.toml @@ -91,6 +91,14 @@ harness = false name = "systimer" harness = false +[[test]] +name = "parl_io_tx" +harness = false + +[[test]] +name = "parl_io_tx_async" +harness = false + [[test]] name = "pcnt" harness = false diff --git a/hil-test/tests/parl_io_tx.rs b/hil-test/tests/parl_io_tx.rs new file mode 100644 index 000000000..a4efbb3c1 --- /dev/null +++ b/hil-test/tests/parl_io_tx.rs @@ -0,0 +1,198 @@ +//! PARL_IO TX test + +//% CHIPS: esp32c6 esp32h2 +#![no_std] +#![no_main] + +#[cfg(esp32c6)] +use esp_hal::parl_io::{TxPinConfigWithValidPin, TxSixteenBits}; +use esp_hal::{ + dma::{ChannelCreator, Dma, DmaPriority}, + gpio::{interconnect::InputSignal, AnyPin, Io, NoPin}, + parl_io::{ + BitPackOrder, + ClkOutPin, + ParlIoTxOnly, + SampleEdge, + TxEightBits, + TxPinConfigIncludingValidPin, + }, + pcnt::{ + channel::{CtrlMode, EdgeMode}, + unit::Unit, + Pcnt, + }, + peripherals::PARL_IO, + prelude::*, +}; +use hil_test as _; + +struct Context { + parl_io: PARL_IO, + dma_channel: ChannelCreator<0>, + clock: AnyPin, + valid: AnyPin, + clock_loopback: InputSignal, + valid_loopback: InputSignal, + pcnt_unit: Unit<'static, 0>, +} + +#[cfg(test)] +#[embedded_test::tests] +mod tests { + // defmt::* is load-bearing, it ensures that the assert in dma_buffers! is not + // using defmt's non-const assert. Doing so would result in a compile error. + #[allow(unused_imports)] + use defmt::{assert_eq, *}; + + use super::*; + + #[init] + fn init() -> Context { + let peripherals = esp_hal::init(esp_hal::Config::default()); + + let io = Io::new(peripherals.GPIO, peripherals.IO_MUX); + let (clock, _) = hil_test::common_test_pins!(io); + let valid = io.pins.gpio0.degrade(); + let clock_loopback = clock.peripheral_input(); + let valid_loopback = valid.peripheral_input(); + let clock = clock.degrade(); + let pcnt = Pcnt::new(peripherals.PCNT); + let pcnt_unit = pcnt.unit0; + let dma = Dma::new(peripherals.DMA); + let dma_channel = dma.channel0; + + let parl_io = peripherals.PARL_IO; + + Context { + parl_io, + dma_channel, + clock, + valid, + clock_loopback, + valid_loopback, + pcnt_unit, + } + } + + #[cfg(esp32c6)] + #[test] + #[timeout(3)] + fn test_parl_io_tx_16bit_valid_clock_count(ctx: Context) { + const BUFFER_SIZE: usize = 64; + let tx_buffer = [0u16; BUFFER_SIZE]; + let (_, tx_descriptors) = esp_hal::dma_descriptors!(0, 2 * BUFFER_SIZE); + + let pins = TxSixteenBits::new( + NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, + NoPin, NoPin, NoPin, ctx.valid, + ); + let mut pins = TxPinConfigIncludingValidPin::new(pins); + let mut clock_pin = ClkOutPin::new(ctx.clock); + + let pio = ParlIoTxOnly::new( + ctx.parl_io, + ctx.dma_channel.configure(false, DmaPriority::Priority0), + tx_descriptors, + 10.MHz(), + ) + .unwrap(); + + let mut pio = pio + .tx + .with_config( + &mut pins, + &mut clock_pin, + 0, + SampleEdge::Invert, + BitPackOrder::Msb, + ) + .unwrap(); // TODO: handle error + + // use a PCNT unit to count the negitive clock edges only when valid is high + let clock_unit = ctx.pcnt_unit; + clock_unit.channel0.set_edge_signal(ctx.clock_loopback); + clock_unit.channel0.set_ctrl_signal(ctx.valid_loopback); + clock_unit + .channel0 + .set_input_mode(EdgeMode::Increment, EdgeMode::Hold); + clock_unit + .channel0 + .set_ctrl_mode(CtrlMode::Disable, CtrlMode::Keep); + + for _ in 0..100 { + clock_unit.clear(); + let xfer = pio.write_dma(&tx_buffer).unwrap(); + xfer.wait().unwrap(); + info!("clock count: {}", clock_unit.get_value()); + assert_eq!(clock_unit.get_value(), BUFFER_SIZE as _); + } + } + + #[test] + #[timeout(3)] + fn test_parl_io_tx_8bit_valid_clock_count(ctx: Context) { + const BUFFER_SIZE: usize = 64; + let tx_buffer = [0u8; BUFFER_SIZE]; + let (_, tx_descriptors) = esp_hal::dma_descriptors!(0, 2 * BUFFER_SIZE); + + let pins = TxEightBits::new( + NoPin, + NoPin, + NoPin, + NoPin, + NoPin, + NoPin, + NoPin, + #[cfg(esp32h2)] + ctx.valid, + #[cfg(esp32c6)] + NoPin, + ); + + #[cfg(esp32h2)] + let mut pins = TxPinConfigIncludingValidPin::new(pins); + #[cfg(esp32c6)] + let mut pins = TxPinConfigWithValidPin::new(pins, ctx.valid); + + let mut clock_pin = ClkOutPin::new(ctx.clock); + + let pio = ParlIoTxOnly::new( + ctx.parl_io, + ctx.dma_channel.configure(false, DmaPriority::Priority0), + tx_descriptors, + 10.MHz(), + ) + .unwrap(); + + let mut pio = pio + .tx + .with_config( + &mut pins, + &mut clock_pin, + 0, + SampleEdge::Invert, + BitPackOrder::Msb, + ) + .unwrap(); // TODO: handle error + + // use a PCNT unit to count the negitive clock edges only when valid is high + let clock_unit = ctx.pcnt_unit; + clock_unit.channel0.set_edge_signal(ctx.clock_loopback); + clock_unit.channel0.set_ctrl_signal(ctx.valid_loopback); + clock_unit + .channel0 + .set_input_mode(EdgeMode::Increment, EdgeMode::Hold); + clock_unit + .channel0 + .set_ctrl_mode(CtrlMode::Disable, CtrlMode::Keep); + + for _ in 0..100 { + clock_unit.clear(); + let xfer = pio.write_dma(&tx_buffer).unwrap(); + xfer.wait().unwrap(); + info!("clock count: {}", clock_unit.get_value()); + assert_eq!(clock_unit.get_value(), BUFFER_SIZE as _); + } + } +} diff --git a/hil-test/tests/parl_io_tx_async.rs b/hil-test/tests/parl_io_tx_async.rs new file mode 100644 index 000000000..22d9b989b --- /dev/null +++ b/hil-test/tests/parl_io_tx_async.rs @@ -0,0 +1,200 @@ +//! PARL_IO TX async test + +//% CHIPS: esp32c6 esp32h2 +//% FEATURES: generic-queue + +#![no_std] +#![no_main] + +#[cfg(esp32c6)] +use esp_hal::parl_io::{TxPinConfigWithValidPin, TxSixteenBits}; +use esp_hal::{ + dma::{ChannelCreator, Dma, DmaPriority}, + gpio::{interconnect::InputSignal, AnyPin, Io, NoPin}, + parl_io::{ + BitPackOrder, + ClkOutPin, + ParlIoTxOnly, + SampleEdge, + TxEightBits, + TxPinConfigIncludingValidPin, + }, + pcnt::{ + channel::{CtrlMode, EdgeMode}, + unit::Unit, + Pcnt, + }, + peripherals::PARL_IO, + prelude::*, +}; +use hil_test as _; + +struct Context { + parl_io: PARL_IO, + dma_channel: ChannelCreator<0>, + clock: AnyPin, + valid: AnyPin, + clock_loopback: InputSignal, + valid_loopback: InputSignal, + pcnt_unit: Unit<'static, 0>, +} + +#[cfg(test)] +#[embedded_test::tests(executor = esp_hal_embassy::Executor::new())] +mod tests { + // defmt::* is load-bearing, it ensures that the assert in dma_buffers! is not + // using defmt's non-const assert. Doing so would result in a compile error. + #[allow(unused_imports)] + use defmt::{assert_eq, *}; + + use super::*; + + #[init] + async fn init() -> Context { + let peripherals = esp_hal::init(esp_hal::Config::default()); + + let io = Io::new(peripherals.GPIO, peripherals.IO_MUX); + let (clock, _) = hil_test::common_test_pins!(io); + let valid = io.pins.gpio0.degrade(); + let clock_loopback = clock.peripheral_input(); + let valid_loopback = valid.peripheral_input(); + let clock = clock.degrade(); + let pcnt = Pcnt::new(peripherals.PCNT); + let pcnt_unit = pcnt.unit0; + let dma = Dma::new(peripherals.DMA); + let dma_channel = dma.channel0; + + let parl_io = peripherals.PARL_IO; + + Context { + parl_io, + dma_channel, + clock, + valid, + clock_loopback, + valid_loopback, + pcnt_unit, + } + } + + #[cfg(esp32c6)] + #[test] + #[timeout(3)] + async fn test_parl_io_tx_async_16bit_valid_clock_count(ctx: Context) { + const BUFFER_SIZE: usize = 64; + let tx_buffer = [0u16; BUFFER_SIZE]; + let (_, tx_descriptors) = esp_hal::dma_descriptors!(0, 2 * BUFFER_SIZE); + + let pins = TxSixteenBits::new( + NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, NoPin, + NoPin, NoPin, NoPin, ctx.valid, + ); + let mut pins = TxPinConfigIncludingValidPin::new(pins); + let mut clock_pin = ClkOutPin::new(ctx.clock); + + let pio = ParlIoTxOnly::new( + ctx.parl_io, + ctx.dma_channel + .configure_for_async(false, DmaPriority::Priority0), + tx_descriptors, + 10.MHz(), + ) + .unwrap(); + + let mut pio = pio + .tx + .with_config( + &mut pins, + &mut clock_pin, + 0, + SampleEdge::Invert, + BitPackOrder::Msb, + ) + .unwrap(); + + // use a PCNT unit to count the negitive clock edges only when valid is high + let clock_unit = ctx.pcnt_unit; + clock_unit.channel0.set_edge_signal(ctx.clock_loopback); + clock_unit.channel0.set_ctrl_signal(ctx.valid_loopback); + clock_unit + .channel0 + .set_input_mode(EdgeMode::Increment, EdgeMode::Hold); + clock_unit + .channel0 + .set_ctrl_mode(CtrlMode::Disable, CtrlMode::Keep); + + for _ in 0..100 { + clock_unit.clear(); + pio.write_dma_async(&tx_buffer).await.unwrap(); + info!("clock count: {}", clock_unit.get_value()); + assert_eq!(clock_unit.get_value(), BUFFER_SIZE as _); + } + } + + #[test] + #[timeout(3)] + async fn test_parl_io_tx_async_8bit_valid_clock_count(ctx: Context) { + const BUFFER_SIZE: usize = 64; + let tx_buffer = [0u8; BUFFER_SIZE]; + let (_, tx_descriptors) = esp_hal::dma_descriptors!(0, 2 * BUFFER_SIZE); + + let pins = TxEightBits::new( + NoPin, + NoPin, + NoPin, + NoPin, + NoPin, + NoPin, + NoPin, + #[cfg(esp32h2)] + ctx.valid, + #[cfg(esp32c6)] + NoPin, + ); + + #[cfg(esp32h2)] + let mut pins = TxPinConfigIncludingValidPin::new(pins); + #[cfg(esp32c6)] + let mut pins = TxPinConfigWithValidPin::new(pins, ctx.valid); + + let mut clock_pin = ClkOutPin::new(ctx.clock); + + let pio = ParlIoTxOnly::new( + ctx.parl_io, + ctx.dma_channel + .configure_for_async(false, DmaPriority::Priority0), + tx_descriptors, + 10.MHz(), + ) + .unwrap(); + + let mut pio = pio + .tx + .with_config( + &mut pins, + &mut clock_pin, + 0, + SampleEdge::Invert, + BitPackOrder::Msb, + ) + .unwrap(); + + // use a PCNT unit to count the negitive clock edges only when valid is high + let clock_unit = ctx.pcnt_unit; + clock_unit.channel0.set_edge_signal(ctx.clock_loopback); + clock_unit.channel0.set_ctrl_signal(ctx.valid_loopback); + clock_unit + .channel0 + .set_input_mode(EdgeMode::Increment, EdgeMode::Hold); + clock_unit + .channel0 + .set_ctrl_mode(CtrlMode::Disable, CtrlMode::Keep); + + for _ in 0..100 { + clock_unit.clear(); + pio.write_dma_async(&tx_buffer).await.unwrap(); + info!("clock count: {}", clock_unit.get_value()); + assert_eq!(clock_unit.get_value(), BUFFER_SIZE as _); + } + } +}