espressif / esp-idf

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

Add spi_bus_add_device when dev_config.queue_size < 1 Check (IDFGH-4945) #6736

Open ryan4volts opened 3 years ago

ryan4volts commented 3 years ago

After a chunk of time racking my head around what occurred when doing some code optimisations, I realised this parameter isn't checked in the spi_bus_add_device function. Can we add it in with ESP_ERR_INVALID_ARG something like below

esp_err_t spi_bus_add_device(spi_host_device_t host, const spi_device_interface_config_t *dev_config, spi_device_handle_t *handle)
{
    int freecs;
    int duty_cycle;

    SPI_CHECK(host>=SPI_HOST && host<=VSPI_HOST, "invalid host", ESP_ERR_INVALID_ARG);
    SPI_CHECK(spihost[host]!=NULL, "host not initialized", ESP_ERR_INVALID_STATE);
    SPI_CHECK(dev_config->spics_io_num < 0 || GPIO_IS_VALID_OUTPUT_GPIO(dev_config->spics_io_num), "spics pin invalid", ESP_ERR_INVALID_ARG);
    SPI_CHECK(dev_config->clock_speed_hz > 0, "invalid sclk speed", ESP_ERR_INVALID_ARG);
    SPI_CHECK(dev_config->queue_size > 0, "queue size must be > 0", ESP_ERR_INVALID_ARG);
KaeLL commented 3 years ago

@ryan4volts I don't see any reason why queue_size shouldn't automatically be changed to 1 if it's passed as 0, kind of like spi_bus_config_t's max_transfer_sz, especially considering it's only useful for interrupt transactions.

ryan4volts commented 3 years ago

Either if it forces the user to make a code change, or it makes the change itself is a good solution.