espressif / esp-idf

Espressif IoT Development Framework. Official development framework for Espressif SoCs.
Apache License 2.0
13.33k stars 7.2k forks source link

spi_bus_add_device hits FreeRTOS assert if queue size <= 0 (IDFGH-7932) #9450

Open allison-n-h opened 2 years ago

allison-n-h commented 2 years ago

https://github.com/espressif/esp-idf/blob/36f49f361c001b49c538364056bc5d2d04c6f321/components/driver/spi_master.c#L377

If a dev_config->queue_size is set to 0, we hit this assert: assert failed: xQueueGenericCreate queue.c:417 (uxQueueLength > ( UBaseType_t ) 0)

Arguably, spi_bus_add_device should return ESP_ERR_INVALID_ARG if any of the fields in spi_device_interface_config_t have invalid values. That, or it should be well documented in the docs or header file.

allison-n-h commented 2 years ago

We hit this with esp-idf version 4.4.1, but looks like it is the same in the latest master branch

KaeLL commented 2 years ago

Everyone just glosses over it. It's the default behavior since IDF 3, probably even before, though I wasn't around to confirm. As polling transactions don't use the transaction queue, this also touches on the issue of wasted memory and binary size due to the built in assumption that both types of transactions should be available when you use only one type.

sengulhamza commented 2 years ago

I agreed @KaeLL, maybe you can try to init SPI like this.

    // Initialize SPI bus
    spi_bus_config_t spi_bus_config = {
        .miso_io_num =  pin_miso,
        .mosi_io_num =  pin_mosi,
        .sclk_io_num =  pin_sclk,
        .quadwp_io_num = -1,
        .quadhd_io_num = -1,
        .max_transfer_sz = 0,
    };
    esp_err_t err = spi_bus_initialize(spi_host, &spi_bus_config, SPI_DMA_DISABLED);
    ESP_ERROR_CHECK(err);

    spi_device_interface_config_t spi_config = {
        .mode = 0,
        .clock_speed_hz = SPI_MASTER_FREQ_8M,
        .command_bits = 0,
        .address_bits = 8,
        .spics_io_num = -1,
        .queue_size = 1,
        .pre_cb = assert_nss, //or NULL
        .post_cb = deassert_nss, //or NULL
    };
    esp_err_t ret = spi_bus_add_device(spi_host, &spi_config, &spi_handle);
    ESP_ERROR_CHECK(ret);
    ESP_LOGI(TAG, "SPI initialized");
void spi_write(uint8_t cmd, const uint8_t *buf, size_t len)
{
    spi_transaction_t spi_transaction = {
        .addr = cmd,
        .length = 8 * len,
        .tx_buffer = buf,
    };
    esp_err_t err = spi_device_transmit(spi_handle, &spi_transaction);
    ESP_ERROR_CHECK(err);
}
void spi_read(uint8_t cmd, uint8_t *buf, size_t len)
{
    spi_transaction_t spi_transaction = {
        .addr = cmd,
        .length = 8 * len,
        .rxlength = 8 * len,
        .tx_buffer = buf,
        .rx_buffer = buf,
    };
    esp_err_t err = spi_device_transmit(spi_handle, &spi_transaction);
    ESP_ERROR_CHECK(err);
}

Also you can use polling SPI API function.

allison-n-h commented 2 years ago

It's easy enough to work around. This is a suggestion to return an error instead of hitting an assert if the queue size is zero. Or updating the documentation so it is clear to the developer that zero is an invalid value