diff --git a/components/spi_flash/esp_flash_api.c b/components/spi_flash/esp_flash_api.c index ff9c49e4d..7073ab62e 100644 --- a/components/spi_flash/esp_flash_api.c +++ b/components/spi_flash/esp_flash_api.c @@ -497,12 +497,28 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add bool direct_read = chip->host->supports_direct_read(chip->host, buffer); uint8_t* temp_buffer = NULL; + //each time, we at most read this length + //after that, we release the lock to allow some other operations + size_t read_chunk_size = MIN(MAX_READ_CHUNK, length); + if (!direct_read) { - uint32_t length_to_allocate = MAX(MAX_READ_CHUNK, length); - length_to_allocate = (length_to_allocate+3)&(~3); - temp_buffer = heap_caps_malloc(length_to_allocate, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); - ESP_LOGV(TAG, "allocate temp buffer: %p", temp_buffer); - if (temp_buffer == NULL) return ESP_ERR_NO_MEM; + /* Allocate temporary internal buffer to use for the actual read. If the preferred size + doesn't fit in free internal memory, allocate the largest available free block. + + (May need to shrink read_chunk_size and retry due to race conditions with other tasks + also allocating from the heap.) + */ + unsigned retries = 5; + while(temp_buffer == NULL && retries--) { + read_chunk_size = MIN(read_chunk_size, heap_caps_get_largest_free_block(MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT)); + read_chunk_size = (read_chunk_size + 3) & ~3; + temp_buffer = heap_caps_malloc(read_chunk_size, MALLOC_CAP_INTERNAL | MALLOC_CAP_8BIT); + } + ESP_LOGV(TAG, "allocate temp buffer: %p (%d)", temp_buffer, read_chunk_size); + + if (temp_buffer == NULL) { + return ESP_ERR_NO_MEM; + } } esp_err_t err = ESP_OK; @@ -514,9 +530,9 @@ esp_err_t IRAM_ATTR esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t add } //if required (dma buffer allocated), read to the buffer instead of the original buffer uint8_t* buffer_to_read = (temp_buffer)? temp_buffer : buffer; - //each time, we at most read this length - //after that, we release the lock to allow some other operations - uint32_t length_to_read = MIN(MAX_READ_CHUNK, length); + + // Length we will read this iteration is either the chunk size or the remaining length, whichever is smaller + size_t length_to_read = MIN(read_chunk_size, length); if (err == ESP_OK) { err = chip->chip_drv->read(chip, buffer_to_read, address, length_to_read); @@ -682,27 +698,32 @@ esp_err_t esp_flash_app_disable_protect(bool disable) #ifndef CONFIG_SPI_FLASH_USE_LEGACY_IMPL +/* Translate any ESP_ERR_FLASH_xxx error code (new API) to a generic ESP_ERR_xyz error code + */ static IRAM_ATTR esp_err_t spi_flash_translate_rc(esp_err_t err) { switch (err) { case ESP_OK: - return ESP_OK; case ESP_ERR_INVALID_ARG: - return ESP_ERR_INVALID_ARG; + case ESP_ERR_NO_MEM: + return err; + case ESP_ERR_FLASH_NOT_INITIALISED: case ESP_ERR_FLASH_PROTECTED: return ESP_ERR_INVALID_STATE; + case ESP_ERR_NOT_FOUND: case ESP_ERR_FLASH_UNSUPPORTED_HOST: case ESP_ERR_FLASH_UNSUPPORTED_CHIP: return ESP_ERR_NOT_SUPPORTED; + case ESP_ERR_FLASH_NO_RESPONSE: return ESP_ERR_INVALID_RESPONSE; + default: - ESP_EARLY_LOGE(TAG, "unexpected spi flash error code: %x", err); + ESP_EARLY_LOGE(TAG, "unexpected spi flash error code: 0x%x", err); abort(); } - return ESP_OK; } esp_err_t IRAM_ATTR spi_flash_erase_range(uint32_t start_addr, uint32_t size) diff --git a/components/spi_flash/include/esp_flash.h b/components/spi_flash/include/esp_flash.h index afce78c5e..ec196deff 100644 --- a/components/spi_flash/include/esp_flash.h +++ b/components/spi_flash/include/esp_flash.h @@ -230,8 +230,7 @@ esp_err_t esp_flash_set_protected_region(esp_flash_t *chip, const esp_flash_regi * * @return * - ESP_OK: success - * - ESP_ERR_NO_MEM: the buffer is not valid, however failed to malloc on - * the heap. + * - ESP_ERR_NO_MEM: Buffer is in external PSRAM which cannot be concurrently accessed, and a temporary internal buffer could not be allocated. * - or a flash error code if operation failed. */ esp_err_t esp_flash_read(esp_flash_t *chip, void *buffer, uint32_t address, uint32_t length); diff --git a/components/spi_flash/test/test_esp_flash.c b/components/spi_flash/test/test_esp_flash.c index d0ec5ca31..d646ad292 100644 --- a/components/spi_flash/test/test_esp_flash.c +++ b/components/spi_flash/test/test_esp_flash.c @@ -68,17 +68,25 @@ static uint8_t sector_buf[4096]; #define VSPI_PIN_NUM_CS FSPI_PIN_NUM_CS #endif -#define ALL_TEST_NUM (sizeof(config_list)/sizeof(flashtest_config_t)) +#define TEST_CONFIG_NUM (sizeof(config_list)/sizeof(flashtest_config_t)) + typedef void (*flash_test_func_t)(esp_flash_t* chip); +/* Use FLASH_TEST_CASE for SPI flash tests that only use the main SPI flash chip +*/ #define FLASH_TEST_CASE(STR, FUNC_TO_RUN) \ - TEST_CASE(STR, "[esp_flash]") {flash_test_func(FUNC_TO_RUN, 1);} + TEST_CASE(STR, "[esp_flash]") {flash_test_func(FUNC_TO_RUN, 1 /* first index reserved for main flash */ );} + +/* Use FLASH_TEST_CASE_3 for tests which also run on external flash, which sits in the place of PSRAM + (these tests are incompatible with PSRAM) + + These tests run for all the flash chip configs shown in config_list, below (internal and external). + */ #if defined(CONFIG_SPIRAM_SUPPORT) || TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2BETA) -// These tests needs external flash, right on the place of psram #define FLASH_TEST_CASE_3(STR, FUNCT_TO_RUN) #else #define FLASH_TEST_CASE_3(STR, FUNC_TO_RUN) \ - TEST_CASE(STR", 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH]") {flash_test_func(FUNC_TO_RUN, ALL_TEST_NUM);} + TEST_CASE(STR", 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH]") {flash_test_func(FUNC_TO_RUN, TEST_CONFIG_NUM);} #endif //currently all the configs are the same with esp_flash_spi_device_config_t, no more information required @@ -271,12 +279,14 @@ void teardown_test_chip(esp_flash_t* chip, spi_host_device_t host) static void flash_test_func(flash_test_func_t func, int test_num) { for (int i = 0; i < test_num; i++) { + ESP_LOGI(TAG, "Testing config %d/%d", i, test_num); flashtest_config_t* config = &config_list[i]; esp_flash_t* chip; setup_new_chip(config, &chip); (*func)(chip); teardown_test_chip(chip, config->host_id); } + ESP_LOGI(TAG, "Completed %d configs", test_num); } /* ---------- Test code start ------------*/ @@ -615,7 +625,7 @@ TEST_CASE("SPI flash test reading with all speed/mode permutations", "[esp_flash #if !TEMPORARY_DISABLED_FOR_TARGETS(ESP32S2BETA) TEST_CASE("SPI flash test reading with all speed/mode permutations, 3 chips", "[esp_flash][test_env=UT_T1_ESP_FLASH]") { - for (int i = 0; i < ALL_TEST_NUM; i++) { + for (int i = 0; i < TEST_CONFIG_NUM; i++) { test_permutations(&config_list[i]); } } @@ -685,4 +695,73 @@ static void test_write_large_buffer(esp_flash_t *chip, const uint8_t *source, si write_large_buffer(chip, part, source, length); read_and_check(chip, part, source, length); -} \ No newline at end of file +} + +#ifdef CONFIG_SPIRAM_USE_MALLOC + +/* Utility: Read into a small internal RAM buffer using esp_flash_read() and compare what + we read with 'buffer' */ +static void s_test_compare_flash_contents_small_reads(esp_flash_t *chip, const uint8_t *buffer, size_t offs, size_t len) +{ + const size_t INTERNAL_BUF_SZ = 1024; // Should fit in internal RAM + uint8_t *ibuf = heap_caps_malloc(INTERNAL_BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_INTERNAL); + TEST_ASSERT_NOT_NULL(ibuf); + + for (int i = 0; i < len; i += INTERNAL_BUF_SZ) { + size_t to_read = MIN(INTERNAL_BUF_SZ, len - i); + ESP_ERROR_CHECK( esp_flash_read(chip, ibuf, offs + i, to_read) ); + TEST_ASSERT_EQUAL_HEX8_ARRAY(buffer + i, ibuf, to_read); + } + + free(ibuf); +} + +static void test_flash_read_large_psram_buffer(esp_flash_t *chip) +{ + const size_t BUF_SZ = 256 * 1024; // Too large for internal RAM + const size_t TEST_OFFS = 0x1000; // Can be any offset, really + + uint8_t *buf = heap_caps_malloc(BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_SPIRAM); + TEST_ASSERT_NOT_NULL(buf); + + ESP_ERROR_CHECK( esp_flash_read(chip, buf, TEST_OFFS, BUF_SZ) ); + + // Read back the same into smaller internal memory buffer and check it all matches + s_test_compare_flash_contents_small_reads(chip, buf, TEST_OFFS, BUF_SZ); + + free(buf); +} + +FLASH_TEST_CASE("esp_flash_read large PSRAM buffer", test_flash_read_large_psram_buffer); + + +/* similar to above test, but perform it under memory pressure */ +static void test_flash_read_large_psram_buffer_low_internal_mem(esp_flash_t *chip) +{ + const size_t BUF_SZ = 256 * 1024; // Too large for internal RAM + const size_t REMAINING_INTERNAL = 1024; // Exhaust internal memory until maximum free block is less than this + const size_t TEST_OFFS = 0x8000; + + /* Exhaust the available free internal memory */ + test_utils_exhaust_memory_rec erec = test_utils_exhaust_memory(MALLOC_CAP_INTERNAL|MALLOC_CAP_8BIT, REMAINING_INTERNAL); + + uint8_t *buf = heap_caps_malloc(BUF_SZ, MALLOC_CAP_8BIT|MALLOC_CAP_SPIRAM); + TEST_ASSERT_NOT_NULL(buf); + + /* Calling esp_flash_read() here will need to allocate a small internal buffer, + so check it works. */ + ESP_ERROR_CHECK( esp_flash_read(chip, buf, TEST_OFFS, BUF_SZ) ); + + test_utils_free_exhausted_memory(erec); + + // Read back the same into smaller internal memory buffer and check it all matches + s_test_compare_flash_contents_small_reads(chip, buf, TEST_OFFS, BUF_SZ); + + free(buf); +} + +FLASH_TEST_CASE("esp_flash_read large PSRAM buffer low memory", test_flash_read_large_psram_buffer_low_internal_mem); + + + +#endif diff --git a/tools/unit-test-app/components/test_utils/include/test_utils.h b/tools/unit-test-app/components/test_utils/include/test_utils.h index d01e45cdc..efd880f21 100644 --- a/tools/unit-test-app/components/test_utils/include/test_utils.h +++ b/tools/unit-test-app/components/test_utils/include/test_utils.h @@ -257,3 +257,31 @@ esp_err_t test_utils_set_leak_level(size_t leak_level, esp_type_leak_t type, esp * return Leak level */ size_t test_utils_get_leak_level(esp_type_leak_t type, esp_comp_leak_t component); + + + +typedef struct test_utils_exhaust_memory_record_s *test_utils_exhaust_memory_rec; + +/** + * Limit the largest free block of memory with a particular capability set to + * 'limit' bytes (meaning an allocation of 'limit' should succeed at least once, + * but any allocation of more bytes will fail.) + * + * Returns a record pointer which needs to be passed back in to test_utils_free_exhausted_memory + * before the test completes, to avoid a major memory leak. + * + * @param caps Capabilities of memory to exhause + * @param limit The size to limit largest free block to + * @return Record pointer to pass to test_utils_free_exhausted_memory() once done + */ +test_utils_exhaust_memory_rec test_utils_exhaust_memory(uint32_t caps, size_t limit); + + +/** + * Call to free memory which was taken up by test_utils_exhaust_memory() call + * + * @param rec Result previously returned from test_utils_exhaust_memory() + */ +void test_utils_free_exhausted_memory(test_utils_exhaust_memory_rec rec); + + diff --git a/tools/unit-test-app/components/test_utils/test_utils.c b/tools/unit-test-app/components/test_utils/test_utils.c index 3624f1cfd..7745f19c0 100644 --- a/tools/unit-test-app/components/test_utils/test_utils.c +++ b/tools/unit-test-app/components/test_utils/test_utils.c @@ -141,3 +141,41 @@ size_t test_utils_get_leak_level(esp_type_leak_t type_of_leak, esp_comp_leak_t c } return leak_level; } + + +#define EXHAUST_MEMORY_ENTRIES 100 + +struct test_utils_exhaust_memory_record_s { + int *entries[EXHAUST_MEMORY_ENTRIES]; +}; + +test_utils_exhaust_memory_rec test_utils_exhaust_memory(uint32_t caps, size_t limit) +{ + int idx = 0; + test_utils_exhaust_memory_rec rec = calloc(1, sizeof(struct test_utils_exhaust_memory_record_s)); + TEST_ASSERT_NOT_NULL_MESSAGE(rec, "test_utils_exhaust_memory: not enough free memory to allocate record structure!"); + + while (idx < EXHAUST_MEMORY_ENTRIES) { + size_t free_caps = heap_caps_get_largest_free_block(caps); + if (free_caps <= limit) { + return rec; // done! + } + rec->entries[idx] = heap_caps_malloc(free_caps - limit, caps); + TEST_ASSERT_NOT_NULL_MESSAGE(rec->entries[idx], + "test_utils_exhaust_memory: something went wrong while freeing up memory, is another task using heap?"); + heap_caps_check_integrity_all(true); + idx++; + } + + TEST_FAIL_MESSAGE("test_utils_exhaust_memory: The heap with the requested caps is too fragmented, increase EXHAUST_MEMORY_ENTRIES or defrag the heap!"); + abort(); +} + +void test_utils_free_exhausted_memory(test_utils_exhaust_memory_rec rec) +{ + for (int i = 0; i < EXHAUST_MEMORY_ENTRIES; i++) { + free(rec->entries[i]); + } + free(rec); +} +