Merge branch 'fix/spi_slave_no_dma_rx_overwrite' into 'master'

fix(driver_spi): fixed slave no dma rx overwrite when trans_len below or over

Closes IDFGH-13574

See merge request espressif/esp-idf!33377

Author: wanckl

components/driver/test_apps/components/test_driver_utils/test_spi_utils.c

@@ -231,23 +231,19 @@ 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);
 
     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
 }

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);

components/hal/include/hal/spi_slave_hal.h

@@ -186,7 +186,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.
  *

components/hal/spi_slave_hal_iram.c

@@ -68,7 +68,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);
     }
 }