fix(spi_nand_flash): Fix review comments

Author: RathiSonika

spi_nand_flash/CMakeLists.txt

@@ -15,7 +15,8 @@ endif()
 
 if(${target} STREQUAL "linux")
     list(APPEND srcs "src/nand_impl_linux.c"
-                     "src/nand_linux_mmap_emul.c")
+                     "src/nand_linux_mmap_emul.c"
+                     "src/spi_nand_flash_test_helpers.c")
 else()
     list(APPEND srcs "src/devices/nand_winbond.c"
                      "src/devices/nand_gigadevice.c"
@@ -25,6 +26,7 @@ else()
                      "src/devices/nand_xtx.c"
                      "src/nand_impl.c"
                      "src/nand_diag_api.c"
+                     "src/spi_nand_flash_test_helpers.c"
                      "src/spi_nand_oper.c")
 
     set(priv_reqs esp_mm)

spi_nand_flash/README.md

@@ -46,7 +46,8 @@ graph TD
 ```
 
 **Key Benefits:**
-- **Backward Compatible**: Existing code works unchanged
+- **Backward Compatible**: Existing code works unchanged; sector-named APIs are retained as aliases of the page API
+- **Page terminology**: Public API uses *page* (read_page, write_page, get_page_count, get_page_size) to align with NAND flash; sector names remain for compatibility
 - **Modular Design**: Clear separation between wear-leveling and flash management
 - **Enhanced Features**: Direct access to flash and wear-leveling layers
 

spi_nand_flash/host_test/README.md

@@ -48,9 +48,13 @@ spi_nand_flash_config_t nand_flash_config = {&cfg, 0, SPI_NAND_IO_MODE_SIO, 0};
 spi_nand_flash_device_t *handle;
 spi_nand_flash_init_device(&nand_flash_config, &handle);
 
-// Use direct NAND operations
-spi_nand_flash_read_sector(handle, buffer, sector_id);
-spi_nand_flash_write_sector(handle, buffer, sector_id);
+// Use direct NAND operations (page API preferred; sector API is also supported)
+uint32_t page_size;
+spi_nand_flash_get_page_size(handle, &page_size);
+uint8_t *buffer = malloc(page_size);
+uint32_t page_id = 0;
+spi_nand_flash_read_page(handle, buffer, page_id);
+spi_nand_flash_write_page(handle, buffer, page_id);
 
 // Cleanup
 spi_nand_flash_deinit_device(handle);
@@ -69,7 +73,8 @@ esp_blockdev_handle_t nand_bdl;
 nand_flash_get_blockdev(&nand_flash_config, &device_handle, &nand_bdl);
 
 // Use block device operations
-nand_bdl->ops->read(nand_bdl, buffer, sector_size, offset, size);
+uint32_t page_size = nand_bdl->geometry.read_size;  // BDL geometry uses page size
+nand_bdl->ops->read(nand_bdl, buffer, page_size, offset, size);
 nand_bdl->ops->write(nand_bdl, buffer, offset, size);
 
 // Cleanup

spi_nand_flash/host_test/main/test_nand_flash.cpp

@@ -8,57 +8,34 @@
 #include <string.h>
 
 #include "spi_nand_flash.h"
+#include "spi_nand_flash_test_helpers.h"
 #include "nand_linux_mmap_emul.h"
 #include "nand_private/nand_impl_wrap.h"
 
 #include <catch2/catch_test_macros.hpp>
 
-#define PATTERN_SEED    0x12345678
-
-static void fill_buffer(uint32_t seed, uint8_t *dst, size_t count)
-{
-    srand(seed);
-    for (size_t i = 0; i < count; ++i) {
-        uint32_t val = rand();
-        memcpy(dst + i * sizeof(uint32_t), &val, sizeof(val));
-    }
-}
-
-static void check_buffer(uint32_t seed, const uint8_t *src, size_t count)
-{
-    srand(seed);
-    for (size_t i = 0; i < count; ++i) {
-        uint32_t val;
-        memcpy(&val, src + i * sizeof(uint32_t), sizeof(val));
-        if (!(rand() == val)) {
-            printf("Val not equal\n");
-        }
-    }
-}
-
 TEST_CASE("verify mark_bad_block works", "[spi_nand_flash]")
 {
     nand_file_mmap_emul_config_t conf = {"", 50 * 1024 * 1024, true};
     spi_nand_flash_config_t nand_flash_config = {&conf, 0, SPI_NAND_IO_MODE_SIO, 0};
     spi_nand_flash_device_t *device_handle;
     REQUIRE(spi_nand_flash_init_device(&nand_flash_config, &device_handle) == ESP_OK);
 
-    uint32_t sector_num, sector_size;
-    REQUIRE(spi_nand_flash_get_capacity(device_handle, &sector_num) == 0);
-    REQUIRE(spi_nand_flash_get_sector_size(device_handle, &sector_size) == 0);
+    uint32_t block_num;
+    REQUIRE(spi_nand_flash_get_block_num(device_handle, &block_num) == 0);
 
     uint32_t test_block = 15;
-    if (test_block < sector_num) {
-        bool is_bad_status = false;
-        // Verify if test_block is not bad block
-        REQUIRE(nand_wrap_is_bad(device_handle, test_block, &is_bad_status) == 0);
-        REQUIRE(is_bad_status == false);
-        // mark test_block as a bad block
-        REQUIRE(nand_wrap_mark_bad(device_handle, test_block) == 0);
-        // Verify if test_block is marked as bad block
-        REQUIRE(nand_wrap_is_bad(device_handle, test_block, &is_bad_status) == 0);
-        REQUIRE(is_bad_status == true);
-    }
+    REQUIRE((test_block < block_num) == true);
+
+    bool is_bad_status = false;
+    // Verify if test_block is not bad block
+    REQUIRE(nand_wrap_is_bad(device_handle, test_block, &is_bad_status) == 0);
+    REQUIRE(is_bad_status == false);
+    // mark test_block as a bad block
+    REQUIRE(nand_wrap_mark_bad(device_handle, test_block) == 0);
+    // Verify if test_block is marked as bad block
+    REQUIRE(nand_wrap_is_bad(device_handle, test_block, &is_bad_status) == 0);
+    REQUIRE(is_bad_status == true);
 
     spi_nand_flash_deinit_device(device_handle);
 }
@@ -80,30 +57,32 @@ TEST_CASE("verify nand_prog, nand_read, nand_copy, nand_is_free works", "[spi_na
     uint8_t *temp_buf = (uint8_t *)malloc(sector_size);
     REQUIRE(temp_buf != NULL);
 
-    fill_buffer(PATTERN_SEED, pattern_buf, sector_size / sizeof(uint32_t));
+    spi_nand_flash_fill_buffer(pattern_buf, sector_size / sizeof(uint32_t));
 
     bool is_page_free = true;
     uint32_t test_block = 20;
     uint32_t test_page = test_block * (block_size / sector_size); //(block_num * pages_per_block)
     uint32_t dst_page = test_page + 1;
-    if (test_page < sector_num) {
-        // Verify if test_page is free
-        REQUIRE(nand_wrap_is_free(device_handle, test_page, &is_page_free) == 0);
-        REQUIRE(is_page_free == true);
-        // Write/program test_page
-        REQUIRE(nand_wrap_prog(device_handle, test_page, pattern_buf) == 0);
-        // Verify if test_page is used/programmed
-        REQUIRE(nand_wrap_is_free(device_handle, test_page, &is_page_free) == 0);
-        REQUIRE(is_page_free == false);
-
-        REQUIRE(nand_wrap_read(device_handle, test_page, 0, sector_size, temp_buf) == 0);
-        check_buffer(PATTERN_SEED, temp_buf, sector_size / sizeof(uint32_t));
-
-        REQUIRE(nand_wrap_copy(device_handle, test_page, dst_page) == 0);
-
-        REQUIRE(nand_wrap_read(device_handle, dst_page, 0, sector_size, temp_buf) == 0);
-        check_buffer(PATTERN_SEED, temp_buf, sector_size / sizeof(uint32_t));
-    }
+
+    REQUIRE((test_page < sector_num) == true);
+
+    // Verify if test_page is free
+    REQUIRE(nand_wrap_is_free(device_handle, test_page, &is_page_free) == 0);
+    REQUIRE(is_page_free == true);
+    // Write/program test_page
+    REQUIRE(nand_wrap_prog(device_handle, test_page, pattern_buf) == 0);
+    // Verify if test_page is used/programmed
+    REQUIRE(nand_wrap_is_free(device_handle, test_page, &is_page_free) == 0);
+    REQUIRE(is_page_free == false);
+
+    REQUIRE(nand_wrap_read(device_handle, test_page, 0, sector_size, temp_buf) == 0);
+    REQUIRE(spi_nand_flash_check_buffer(temp_buf, sector_size / sizeof(uint32_t)) == 0);
+
+    REQUIRE(nand_wrap_copy(device_handle, test_page, dst_page) == 0);
+
+    REQUIRE(nand_wrap_read(device_handle, dst_page, 0, sector_size, temp_buf) == 0);
+    REQUIRE(spi_nand_flash_check_buffer(temp_buf, sector_size / sizeof(uint32_t)) == 0);
+
     free(pattern_buf);
     free(temp_buf);
     spi_nand_flash_deinit_device(device_handle);

spi_nand_flash/host_test/main/test_nand_flash_bdl.cpp

@@ -8,35 +8,13 @@
 #include <string.h>
 
 #include "spi_nand_flash.h"
+#include "spi_nand_flash_test_helpers.h"
 #include "nand_linux_mmap_emul.h"
 #include "esp_blockdev.h"
 #include "esp_nand_blockdev.h"
 
 #include <catch2/catch_test_macros.hpp>
 
-#define PATTERN_SEED    0x12345678
-
-static void fill_buffer(uint32_t seed, uint8_t *dst, size_t count)
-{
-    srand(seed);
-    for (size_t i = 0; i < count; ++i) {
-        uint32_t val = rand();
-        memcpy(dst + i * sizeof(uint32_t), &val, sizeof(val));
-    }
-}
-
-static void check_buffer(uint32_t seed, const uint8_t *src, size_t count)
-{
-    srand(seed);
-    for (size_t i = 0; i < count; ++i) {
-        uint32_t val;
-        memcpy(&val, src + i * sizeof(uint32_t), sizeof(val));
-        if (!(rand() == val)) {
-            printf("Val not equal\n");
-        }
-    }
-}
-
 TEST_CASE("verify mark_bad_block works with bdl interface", "[spi_nand_flash][bdl]")
 {
     nand_file_mmap_emul_config_t conf = {"", 50 * 1024 * 1024, true};
@@ -48,19 +26,18 @@ TEST_CASE("verify mark_bad_block works with bdl interface", "[spi_nand_flash][bd
     uint32_t block_num = nand_bdl->geometry.disk_size / block_size;
 
     uint32_t test_block = 15;
-    if (test_block < block_num) {
-        REQUIRE(nand_bdl->ops->erase(nand_bdl, test_block * block_size, block_size) == 0);
-        // Verify if test_block is not bad block
-        esp_blockdev_cmd_arg_is_bad_block_t bad_block_status = {test_block, false};
-        REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_BAD_BLOCK, &bad_block_status) == 0);
-        REQUIRE(bad_block_status.status == false);
-        // mark test_block as a bad block
-        uint32_t block = test_block;
-        REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_MARK_BAD_BLOCK, &block) == 0);
-        // Verify if test_block is marked as bad block
-        REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_BAD_BLOCK, &bad_block_status) == 0);
-        REQUIRE(bad_block_status.status == true);
-    }
+    REQUIRE((test_block < block_num) == true);
+    REQUIRE(nand_bdl->ops->erase(nand_bdl, test_block * block_size, block_size) == 0);
+    // Verify if test_block is not bad block
+    esp_blockdev_cmd_arg_is_bad_block_t bad_block_status = {test_block, false};
+    REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_BAD_BLOCK, &bad_block_status) == 0);
+    REQUIRE(bad_block_status.status == false);
+    // mark test_block as a bad block
+    uint32_t block = test_block;
+    REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_MARK_BAD_BLOCK, &block) == 0);
+    // Verify if test_block is marked as bad block
+    REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_BAD_BLOCK, &bad_block_status) == 0);
+    REQUIRE(bad_block_status.status == true);
 
     nand_bdl->ops->release(nand_bdl);
 }
@@ -81,24 +58,25 @@ TEST_CASE("verify nand_prog, nand_read, nand_copy, nand_is_free works with bdl i
     uint8_t *temp_buf = (uint8_t *)malloc(sector_size);
     REQUIRE(temp_buf != NULL);
 
-    fill_buffer(PATTERN_SEED, pattern_buf, sector_size / sizeof(uint32_t));
+    spi_nand_flash_fill_buffer(pattern_buf, sector_size / sizeof(uint32_t));
 
     uint32_t test_block = 20;
     uint32_t test_page = test_block * (block_size / sector_size); //(block_num * pages_per_block)
-    if (test_page < sector_num) {
-        // Verify if test_page is free
-        esp_blockdev_cmd_arg_is_free_page_t page_free_status = {test_page, true};
-        REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_FREE_PAGE, &page_free_status) == 0);
-        REQUIRE(page_free_status.status == true);
-        // Write/program test_page
-        REQUIRE(nand_bdl->ops->write(nand_bdl, pattern_buf, test_page * sector_size, sector_size) == 0);
-        // Verify if test_page is used/programmed
-        REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_FREE_PAGE, &page_free_status) == 0);
-        REQUIRE(page_free_status.status == false);
-
-        REQUIRE(nand_bdl->ops->read(nand_bdl, temp_buf, sector_size, test_page * sector_size, sector_size) == 0);
-        check_buffer(PATTERN_SEED, temp_buf, sector_size / sizeof(uint32_t));
-    }
+
+    REQUIRE((test_page < sector_num) == true);
+    // Verify if test_page is free
+    esp_blockdev_cmd_arg_is_free_page_t page_free_status = {test_page, true};
+    REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_FREE_PAGE, &page_free_status) == 0);
+    REQUIRE(page_free_status.status == true);
+    // Write/program test_page
+    REQUIRE(nand_bdl->ops->write(nand_bdl, pattern_buf, test_page * sector_size, sector_size) == 0);
+    // Verify if test_page is used/programmed
+    REQUIRE(nand_bdl->ops->ioctl(nand_bdl, ESP_BLOCKDEV_CMD_IS_FREE_PAGE, &page_free_status) == 0);
+    REQUIRE(page_free_status.status == false);
+
+    REQUIRE(nand_bdl->ops->read(nand_bdl, temp_buf, sector_size, test_page * sector_size, sector_size) == 0);
+    REQUIRE(spi_nand_flash_check_buffer(temp_buf, sector_size / sizeof(uint32_t)) == 0);
+
     free(pattern_buf);
     free(temp_buf);
     nand_bdl->ops->release(nand_bdl);

spi_nand_flash/include/esp_nand_blockdev.h

@@ -173,7 +173,8 @@ extern "C" {
  */
 typedef struct {
     uint32_t num;               /*!< IN:  Block number or page number */
-    bool status;                /*!< OUT: Bad block status or page free status (true/false) */
+    bool status;                /*!< OUT: Bad block status (True: bad block or False: good block)
+                                  or Page free status (True: free page or False: occupied page)*/
 } esp_blockdev_cmd_arg_status_t;
 
 typedef esp_blockdev_cmd_arg_status_t esp_blockdev_cmd_arg_is_bad_block_t;