From 5a2db45f40941b36715d5941d04457283991753b Mon Sep 17 00:00:00 2001 From: wanckl Date: Fri, 6 Sep 2024 21:18:39 +0800 Subject: [PATCH] fix(driver_spi): fixed slave no dma rx overwrite when trans_len below or over Closes https://github.com/espressif/esp-idf/issues/14462 --- .../test_driver_utils/test_spi_utils.c | 12 +- .../test_apps/slave/main/test_spi_slave.c | 151 +++++++++++------- components/hal/include/hal/spi_slave_hal.h | 4 +- components/hal/spi_slave_hal_iram.c | 2 +- 4 files changed, 100 insertions(+), 69 deletions(-) diff --git a/components/driver/test_apps/components/test_driver_utils/test_spi_utils.c b/components/driver/test_apps/components/test_driver_utils/test_spi_utils.c index 8b997e62df..124021167f 100644 --- a/components/driver/test_apps/components/test_driver_utils/test_spi_utils.c +++ b/components/driver/test_apps/components/test_driver_utils/test_spi_utils.c @@ -231,9 +231,9 @@ void spitest_gpio_input_sel(uint32_t gpio_num, int func, uint32_t signal_idx) esp_rom_gpio_connect_in_signal(gpio_num, signal_idx, 0); } -//Note this cs_num is the ID of the connected devices' ID, e.g. if 2 devices are connected to the bus, -//then the cs_num of the 1st and 2nd devices are 0 and 1 respectively. -void same_pin_func_sel(spi_bus_config_t bus, spi_device_interface_config_t dev, uint8_t cs_num) +//Note this cs_dev_id is the ID of the connected devices' ID, e.g. if 2 devices are connected to the bus, +//then the cs_dev_id of the 1st and 2nd devices are 0 and 1 respectively. +void same_pin_func_sel(spi_bus_config_t bus, spi_device_interface_config_t dev, uint8_t cs_dev_id) { spitest_gpio_output_sel(bus.mosi_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spid_out); spitest_gpio_input_sel(bus.mosi_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spid_in); @@ -241,13 +241,9 @@ void same_pin_func_sel(spi_bus_config_t bus, spi_device_interface_config_t dev, spitest_gpio_output_sel(bus.miso_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spiq_out); spitest_gpio_input_sel(bus.miso_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spiq_in); - spitest_gpio_output_sel(dev.spics_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spics_out[cs_num]); + spitest_gpio_output_sel(dev.spics_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spics_out[cs_dev_id]); spitest_gpio_input_sel(dev.spics_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spics_in); spitest_gpio_output_sel(bus.sclk_io_num, FUNC_GPIO, spi_periph_signal[TEST_SPI_HOST].spiclk_out); spitest_gpio_input_sel(bus.sclk_io_num, FUNC_GPIO, spi_periph_signal[TEST_SLAVE_HOST].spiclk_in); - -#if CONFIG_IDF_TARGET_ESP32S2 || CONFIG_IDF_TARGET_ESP32S3 - GPIO.func_in_sel_cfg[FSPIQ_IN_IDX].sig_in_sel = 1; -#endif } diff --git a/components/esp_driver_spi/test_apps/slave/main/test_spi_slave.c b/components/esp_driver_spi/test_apps/slave/main/test_spi_slave.c index d9e7c2f8d1..8a27902e71 100644 --- a/components/esp_driver_spi/test_apps/slave/main/test_spi_slave.c +++ b/components/esp_driver_spi/test_apps/slave/main/test_spi_slave.c @@ -36,22 +36,17 @@ static WORD_ALIGNED_ATTR uint8_t slave_rxbuf[320]; static const uint8_t master_send[] = { 0x93, 0x34, 0x56, 0x78, 0x9a, 0xbc, 0xde, 0xf0, 0xaa, 0xcc, 0xff, 0xee, 0x55, 0x77, 0x88, 0x43 }; static const uint8_t slave_send[] = { 0xaa, 0xdc, 0xba, 0x98, 0x76, 0x54, 0x32, 0x10, 0x13, 0x57, 0x9b, 0xdf, 0x24, 0x68, 0xac, 0xe0 }; -static inline void int_connect(uint32_t gpio, uint32_t sigo, uint32_t sigi) +static void custom_setup(void) { - esp_rom_gpio_connect_out_signal(gpio, sigo, false, false); - esp_rom_gpio_connect_in_signal(gpio, sigi, false); -} + //Initialize buffers + memset(master_txbuf, 0, sizeof(master_txbuf)); + memset(master_rxbuf, 0, sizeof(master_rxbuf)); + memset(slave_txbuf, 0, sizeof(slave_txbuf)); + memset(slave_rxbuf, 0, sizeof(slave_rxbuf)); -static void master_init(spi_device_handle_t *spi) -{ - esp_err_t ret; - spi_bus_config_t buscfg = { - .miso_io_num = PIN_NUM_MISO, - .mosi_io_num = PIN_NUM_MOSI, - .sclk_io_num = PIN_NUM_CLK, - .quadwp_io_num = UNCONNECTED_PIN, - .quadhd_io_num = -1 - }; + //Initialize SPI Master + spi_bus_config_t buscfg = SPI_BUS_TEST_DEFAULT_CONFIG(); + buscfg.flags |= SPICOMMON_BUSFLAG_GPIO_PINS; spi_device_interface_config_t devcfg = { .clock_speed_hz = 4 * 1000 * 1000, //currently only up to 4MHz for internal connect .mode = 0, //SPI mode 0 @@ -62,61 +57,28 @@ static void master_init(spi_device_handle_t *spi) .cs_ena_pretrans = 1, }; //Initialize the SPI bus - ret = spi_bus_initialize(TEST_SPI_HOST, &buscfg, SPI_DMA_CH_AUTO); - TEST_ASSERT(ret == ESP_OK); - //Attach the LCD to the SPI bus - ret = spi_bus_add_device(TEST_SPI_HOST, &devcfg, spi); - TEST_ASSERT(ret == ESP_OK); -} + TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &buscfg, SPI_DMA_CH_AUTO)); + //Attach the device to the SPI bus + TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg, &spi)); -static void slave_init(void) -{ - //Configuration for the SPI bus - spi_bus_config_t buscfg = { - .mosi_io_num = PIN_NUM_MOSI, - .miso_io_num = PIN_NUM_MISO, - .sclk_io_num = PIN_NUM_CLK - }; //Configuration for the SPI slave interface - spi_slave_interface_config_t slvcfg = { - .mode = 0, - .spics_io_num = PIN_NUM_CS, - .queue_size = 3, - .flags = 0, - }; + spi_slave_interface_config_t slvcfg = SPI_SLAVE_TEST_DEFAULT_CONFIG(); //Enable pull-ups on SPI lines so we don't detect rogue pulses when no master is connected. gpio_set_pull_mode(PIN_NUM_MOSI, GPIO_PULLUP_ONLY); gpio_set_pull_mode(PIN_NUM_CLK, GPIO_PULLUP_ONLY); gpio_set_pull_mode(PIN_NUM_CS, GPIO_PULLUP_ONLY); //Initialize SPI slave interface TEST_ESP_OK(spi_slave_initialize(TEST_SLAVE_HOST, &buscfg, &slvcfg, SPI_DMA_CH_AUTO)); -} - -static void custom_setup(void) -{ - //Initialize buffers - memset(master_txbuf, 0, sizeof(master_txbuf)); - memset(master_rxbuf, 0, sizeof(master_rxbuf)); - memset(slave_txbuf, 0, sizeof(slave_txbuf)); - memset(slave_rxbuf, 0, sizeof(slave_rxbuf)); - - //Initialize SPI Master - master_init(&spi); - //Initialize SPI Slave - slave_init(); //Do internal connections - int_connect(PIN_NUM_MOSI, spi_periph_signal[TEST_SPI_HOST].spid_out, spi_periph_signal[TEST_SLAVE_HOST].spiq_in); - int_connect(PIN_NUM_MISO, spi_periph_signal[TEST_SLAVE_HOST].spiq_out, spi_periph_signal[TEST_SPI_HOST].spid_in); - int_connect(PIN_NUM_CS, spi_periph_signal[TEST_SPI_HOST].spics_out[0], spi_periph_signal[TEST_SLAVE_HOST].spics_in); - int_connect(PIN_NUM_CLK, spi_periph_signal[TEST_SPI_HOST].spiclk_out, spi_periph_signal[TEST_SLAVE_HOST].spiclk_in); + same_pin_func_sel(buscfg, devcfg, 0); } static void custom_teardown(void) { - TEST_ASSERT(spi_slave_free(TEST_SLAVE_HOST) == ESP_OK); - TEST_ASSERT(spi_bus_remove_device(spi) == ESP_OK); - TEST_ASSERT(spi_bus_free(TEST_SPI_HOST) == ESP_OK); + TEST_ESP_OK(spi_slave_free(TEST_SLAVE_HOST)); + TEST_ESP_OK(spi_bus_remove_device(spi)); + TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST)); } TEST_CASE("test fullduplex slave with only RX direction", "[spi]") @@ -166,6 +128,79 @@ TEST_CASE("test fullduplex slave with only RX direction", "[spi]") ESP_LOGI(SLAVE_TAG, "test passed."); } +#define TEST_SLV_RX_BUF_LEN 15 +TEST_CASE("Test slave rx no_dma overwrite when length below/over config", "[spi]") +{ + spi_bus_config_t buscfg = SPI_BUS_TEST_DEFAULT_CONFIG(); + buscfg.flags |= SPICOMMON_BUSFLAG_GPIO_PINS; + TEST_ESP_OK(spi_bus_initialize(TEST_SPI_HOST, &buscfg, SPI_DMA_DISABLED)); + spi_device_handle_t spidev0; + spi_device_interface_config_t devcfg = SPI_DEVICE_TEST_DEFAULT_CONFIG(); + TEST_ESP_OK(spi_bus_add_device(TEST_SPI_HOST, &devcfg, &spidev0)); + + spi_slave_interface_config_t slvcfg = SPI_SLAVE_TEST_DEFAULT_CONFIG(); + TEST_ESP_OK(spi_slave_initialize(TEST_SLAVE_HOST, &buscfg, &slvcfg, SPI_DMA_DISABLED)); + + //initialize master and slave on the same pins break some of the output configs, fix them + same_pin_func_sel(buscfg, devcfg, 0); + + uint8_t master_tx[TEST_SLV_RX_BUF_LEN], slave_rx[TEST_SLV_RX_BUF_LEN]; + for (uint8_t i = 0; i < TEST_SLV_RX_BUF_LEN; i++) { + master_tx[i] = TEST_SLV_RX_BUF_LEN - i; + slave_rx[i] = 100; + } + + //------------------------------ trans_len < config_len ------------------------------ + printf("Testing trans_len < config_len:\n"); + spi_slave_transaction_t *slave_out, slave_tans = { + .length = 8 * TEST_SLV_RX_BUF_LEN, + .rx_buffer = slave_rx, + }; + TEST_ESP_OK(spi_slave_queue_trans(TEST_SLAVE_HOST, &slave_tans, portMAX_DELAY)); + + spi_transaction_t master_tans = { + .length = 8 * 7, + .tx_buffer = master_tx, + }; + spi_device_transmit(spidev0, &master_tans); + + TEST_ESP_OK(spi_slave_get_trans_result(TEST_SLAVE_HOST, &slave_out, portMAX_DELAY)); + + ESP_LOGI(SLAVE_TAG, "trans_len: %d, config_len %d", slave_tans.trans_len / 8, slave_tans.length / 8); + ESP_LOG_BUFFER_HEX("master tx", master_tans.tx_buffer, master_tans.length / 8); + ESP_LOG_BUFFER_HEX("slave rx", slave_tans.rx_buffer, TEST_SLV_RX_BUF_LEN); + + TEST_ASSERT_EQUAL(master_tans.length, slave_tans.trans_len); + for (uint8_t i = slave_tans.trans_len; i < slave_tans.length; i += 8) { + TEST_ASSERT_EQUAL(slave_rx[i / 8], 100); + } + + //------------------------------ trans_len > config_len ------------------------------ + printf("Testing trans_len > config_len:\n"); + slave_tans.length = 8 * 9; + TEST_ESP_OK(spi_slave_queue_trans(TEST_SLAVE_HOST, &slave_tans, portMAX_DELAY)); + + master_tans.length = 8 * 11, + spi_device_transmit(spidev0, &master_tans); + + TEST_ESP_OK(spi_slave_get_trans_result(TEST_SLAVE_HOST, &slave_out, portMAX_DELAY)); + + ESP_LOGI(SLAVE_TAG, "trans_len: %d, config_len %d", master_tans.length / 8, slave_tans.trans_len / 8); + ESP_LOG_BUFFER_HEX("master tx", master_tans.tx_buffer, master_tans.length / 8); + ESP_LOG_BUFFER_HEX("slave rx", slave_tans.rx_buffer, TEST_SLV_RX_BUF_LEN); + +#if !CONFIG_IDF_TARGET_ESP32 // esp32 already hardware limited trans_len <= config_len + TEST_ASSERT_EQUAL(master_tans.length, slave_tans.trans_len); +#endif + for (uint8_t i = slave_tans.length; i < TEST_SLV_RX_BUF_LEN * 8; i += 8) { + TEST_ASSERT_EQUAL(slave_rx[i / 8], 100); + } + + TEST_ESP_OK(spi_slave_free(TEST_SLAVE_HOST)); + TEST_ESP_OK(spi_bus_remove_device(spidev0)); + TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST)); +} + TEST_CASE("test fullduplex slave with only TX direction", "[spi]") { custom_setup(); @@ -336,8 +371,8 @@ static void unaligned_test_master(void) free(master_send_buf); free(master_recv_buf); free(slave_send_buf); - TEST_ASSERT(spi_bus_remove_device(spi) == ESP_OK); - TEST_ASSERT(spi_bus_free(TEST_SPI_HOST) == ESP_OK); + TEST_ESP_OK(spi_bus_remove_device(spi)); + TEST_ESP_OK(spi_bus_free(TEST_SPI_HOST)); } static void unaligned_test_slave(void) @@ -386,7 +421,7 @@ static void unaligned_test_slave(void) free(slave_send_buf); free(slave_recv_buf); free(master_send_buf); - TEST_ASSERT(spi_slave_free(TEST_SPI_HOST) == ESP_OK); + TEST_ESP_OK(spi_slave_free(TEST_SPI_HOST)); } TEST_CASE_MULTIPLE_DEVICES("SPI_Slave_Unaligned_Test", "[spi_ms][timeout=120]", unaligned_test_master, unaligned_test_slave); diff --git a/components/hal/include/hal/spi_slave_hal.h b/components/hal/include/hal/spi_slave_hal.h index e8807c84a2..24f89b8c72 100644 --- a/components/hal/include/hal/spi_slave_hal.h +++ b/components/hal/include/hal/spi_slave_hal.h @@ -140,7 +140,7 @@ void spi_slave_hal_user_start(const spi_slave_hal_context_t *hal); bool spi_slave_hal_usr_is_done(spi_slave_hal_context_t* hal); /** - * Post transaction operations, fetch data from the buffer and recored the length. + * Post transaction operations, fetch data from the buffer and recorded the length. * * @param hal Context of the HAL layer. */ @@ -150,7 +150,7 @@ void spi_slave_hal_store_result(spi_slave_hal_context_t *hal); * Get the length of last transaction, in bits. Should be called after ``spi_slave_hal_store_result``. * * Note that if last transaction is longer than configured before, the return - * value will be truncated to the configured length. + * value still the actual length. * * @param hal Context of the HAL layer. * diff --git a/components/hal/spi_slave_hal_iram.c b/components/hal/spi_slave_hal_iram.c index 0d0dcef6df..ff55e0c0b3 100644 --- a/components/hal/spi_slave_hal_iram.c +++ b/components/hal/spi_slave_hal_iram.c @@ -141,7 +141,7 @@ void spi_slave_hal_store_result(spi_slave_hal_context_t *hal) } if (!hal->use_dma && hal->rx_buffer) { //Copy result out - spi_ll_read_buffer(hal->hw, hal->rx_buffer, hal->bitlen); + spi_ll_read_buffer(hal->hw, hal->rx_buffer, (hal->rcv_bitlen > hal->bitlen) ? hal->bitlen : hal->rcv_bitlen); } }