spi_flash: change argument types

spi_flash_read and spi_flash_write currently have a limitation that source and destination must be word-aligned.
This can be fixed by adding code paths for various unaligned scenarios, but function signatures also need to be adjusted.
As a first step (since we are pre-1.0 and can still change function signatures) alignment checks are added, and pointer types are relaxed to uint8_t.
Later we will add handling of unaligned operations.
This change also introduces spi_flash_erase_range and spi_flash_get_chip_size functions.

We probably need something like spi_flash_chip_size_detect which will detect actual chip size.
This is to allow single application binary to be used on a variety of boards and modules.

components/nvs_flash/src/nvs_page.cpp

@@ -37,7 +37,7 @@ esp_err_t Page::load(uint32_t sectorNumber)
     mErasedEntryCount = 0;
 
     Header header;
-    auto rc = spi_flash_read(mBaseAddress, reinterpret_cast<uint32_t*>(&header), sizeof(header));
+    auto rc = spi_flash_read(mBaseAddress, reinterpret_cast<uint8_t*>(&header), sizeof(header));
     if (rc != ESP_OK) {
         mState = PageState::INVALID;
         return rc;
@@ -48,7 +48,7 @@ esp_err_t Page::load(uint32_t sectorNumber)
         // reading the whole page takes ~40 times less than erasing it
         uint32_t line[8];
         for (uint32_t i = 0; i < SPI_FLASH_SEC_SIZE; i += sizeof(line)) {
-            rc = spi_flash_read(mBaseAddress + i, line, sizeof(line));
+            rc = spi_flash_read(mBaseAddress + i, reinterpret_cast<uint8_t*>(line), sizeof(line));
             if (rc != ESP_OK) {
                 mState = PageState::INVALID;
                 return rc;
@@ -86,7 +86,7 @@ esp_err_t Page::load(uint32_t sectorNumber)
 
 esp_err_t Page::writeEntry(const Item& item)
 {
-    auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), reinterpret_cast<const uint32_t*>(&item), sizeof(item));
+    auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), reinterpret_cast<const uint8_t*>(&item), sizeof(item));
     if (rc != ESP_OK) {
         mState = PageState::INVALID;
         return rc;
@@ -114,7 +114,7 @@ esp_err_t Page::writeEntryData(const uint8_t* data, size_t size)
     assert(mFirstUsedEntry != INVALID_ENTRY);
     const uint16_t count = size / ENTRY_SIZE;
     
-    auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), reinterpret_cast<const uint32_t*>(data), static_cast<uint32_t>(size));
+    auto rc = spi_flash_write(getEntryAddress(mNextFreeEntry), data, size);
     if (rc != ESP_OK) {
         mState = PageState::INVALID;
         return rc;
@@ -396,8 +396,8 @@ esp_err_t Page::mLoadEntryTable()
     if (mState == PageState::ACTIVE ||
             mState == PageState::FULL ||
             mState == PageState::FREEING) {
-        auto rc = spi_flash_read(mBaseAddress + ENTRY_TABLE_OFFSET, mEntryTable.data(),
+        auto rc = spi_flash_read(mBaseAddress + ENTRY_TABLE_OFFSET, reinterpret_cast<uint8_t*>(mEntryTable.data()),
-                                 static_cast<uint32_t>(mEntryTable.byteSize()));
+                                 mEntryTable.byteSize());
         if (rc != ESP_OK) {
             mState = PageState::INVALID;
             return rc;
@@ -435,7 +435,7 @@ esp_err_t Page::mLoadEntryTable()
         while (mNextFreeEntry < ENTRY_COUNT) {
             uint32_t entryAddress = getEntryAddress(mNextFreeEntry);
             uint32_t header;
-            auto rc = spi_flash_read(entryAddress, &header, sizeof(header));
+            auto rc = spi_flash_read(entryAddress, reinterpret_cast<uint8_t*>(&header), sizeof(header));
             if (rc != ESP_OK) {
                 mState = PageState::INVALID;
                 return rc;
@@ -559,7 +559,7 @@ esp_err_t Page::initialize()
     header.mSeqNumber = mSeqNumber;
     header.mCrc32 = header.calculateCrc32();
 
-    auto rc = spi_flash_write(mBaseAddress, reinterpret_cast<uint32_t*>(&header), sizeof(header));
+    auto rc = spi_flash_write(mBaseAddress, reinterpret_cast<const uint8_t*>(&header), sizeof(header));
     if (rc != ESP_OK) {
         mState = PageState::INVALID;
         return rc;
@@ -577,7 +577,8 @@ esp_err_t Page::alterEntryState(size_t index, EntryState state)
     mEntryTable.set(index, state);
     size_t wordToWrite = mEntryTable.getWordIndex(index);
     uint32_t word = mEntryTable.data()[wordToWrite];
-    auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast<uint32_t>(wordToWrite) * 4, &word, 4);
+    auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast<uint32_t>(wordToWrite) * 4,
+            reinterpret_cast<uint8_t*>(&word), sizeof(word));
     if (rc != ESP_OK) {
         mState = PageState::INVALID;
         return rc;
@@ -600,7 +601,8 @@ esp_err_t Page::alterEntryRangeState(size_t begin, size_t end, EntryState state)
         }
         if (nextWordIndex != wordIndex) {
             uint32_t word = mEntryTable.data()[wordIndex];
-            auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast<uint32_t>(wordIndex) * 4, &word, 4);
+            auto rc = spi_flash_write(mBaseAddress + ENTRY_TABLE_OFFSET + static_cast<uint32_t>(wordIndex) * 4,
+                    reinterpret_cast<const uint8_t*>(&word), 4);
             if (rc != ESP_OK) {
                 return rc;
             }
@@ -612,7 +614,8 @@ esp_err_t Page::alterEntryRangeState(size_t begin, size_t end, EntryState state)
 
 esp_err_t Page::alterPageState(PageState state)
 {
-    auto rc = spi_flash_write(mBaseAddress, reinterpret_cast<uint32_t*>(&state), sizeof(state));
+    uint32_t state_val = static_cast<uint32_t>(state);
+    auto rc = spi_flash_write(mBaseAddress, reinterpret_cast<const uint8_t*>(&state_val), sizeof(state));
     if (rc != ESP_OK) {
         mState = PageState::INVALID;
         return rc;
@@ -623,7 +626,7 @@ esp_err_t Page::alterPageState(PageState state)
 
 esp_err_t Page::readEntry(size_t index, Item& dst) const
 {
-    auto rc = spi_flash_read(getEntryAddress(index), reinterpret_cast<uint32_t*>(&dst), sizeof(dst));
+    auto rc = spi_flash_read(getEntryAddress(index), reinterpret_cast<uint8_t*>(&dst), sizeof(dst));
     if (rc != ESP_OK) {
         return rc;
     }

components/nvs_flash/test/spi_flash_emulation.cpp

@@ -22,7 +22,7 @@ void spi_flash_emulator_set(SpiFlashEmulator* e)
     s_emulator = e;
 }
 
-esp_err_t spi_flash_erase_sector(uint16_t sec)
+esp_err_t spi_flash_erase_sector(size_t sec)
 {
     if (!s_emulator) {
         return ESP_ERR_FLASH_OP_TIMEOUT;
@@ -35,26 +35,26 @@ esp_err_t spi_flash_erase_sector(uint16_t sec)
     return ESP_OK;
 }
 
-esp_err_t spi_flash_write(uint32_t des_addr, const uint32_t *src_addr, uint32_t size)
+esp_err_t spi_flash_write(size_t des_addr, const uint8_t *src_addr, size_t size)
 {
     if (!s_emulator) {
         return ESP_ERR_FLASH_OP_TIMEOUT;
     }
 
-    if (!s_emulator->write(des_addr, src_addr, size)) {
+    if (!s_emulator->write(des_addr, reinterpret_cast<const uint32_t*>(src_addr), size)) {
         return ESP_ERR_FLASH_OP_FAIL;
     }
 
     return ESP_OK;
 }
 
-esp_err_t spi_flash_read(uint32_t src_addr, uint32_t *des_addr, uint32_t size)
+esp_err_t spi_flash_read(size_t src_addr, uint8_t *des_addr, size_t size)
 {
     if (!s_emulator) {
         return ESP_ERR_FLASH_OP_TIMEOUT;
     }
 
-    if (!s_emulator->read(des_addr, src_addr, size)) {
+    if (!s_emulator->read(reinterpret_cast<uint32_t*>(des_addr), src_addr, size)) {
         return ESP_ERR_FLASH_OP_FAIL;
     }
 

components/nvs_flash/test/spi_flash_emulation.h

@@ -44,7 +44,7 @@ public:
         spi_flash_emulator_set(nullptr);
     }
 
-    bool read(uint32_t* dest, uint32_t srcAddr, size_t size) const
+    bool read(uint32_t* dest, size_t srcAddr, size_t size) const
     {
         if (srcAddr % 4 != 0 ||
                 size % 4 != 0 ||
@@ -60,7 +60,7 @@ public:
         return true;
     }
 
-    bool write(uint32_t dstAddr, const uint32_t* src, size_t size)
+    bool write(size_t dstAddr, const uint32_t* src, size_t size)
     {
         uint32_t sectorNumber = dstAddr/SPI_FLASH_SEC_SIZE;
         if (sectorNumber < mLowerSectorBound || sectorNumber >= mUpperSectorBound) {
@@ -96,7 +96,7 @@ public:
         return true;
     }
 
-    bool erase(uint32_t sectorNumber)
+    bool erase(size_t sectorNumber)
     {
         size_t offset = sectorNumber * SPI_FLASH_SEC_SIZE / 4;
         if (offset > mData.size()) {

components/nvs_flash/test/test_spi_flash_emulation.cpp

@@ -30,7 +30,7 @@ TEST_CASE("flash starts with all bytes == 0xff", "[spi_flash_emu]")
     uint8_t sector[SPI_FLASH_SEC_SIZE];
 
     for (int i = 0; i < 4; ++i) {
-        CHECK(spi_flash_read(0, reinterpret_cast<uint32_t*>(sector), sizeof(sector)) == ESP_OK);
+        CHECK(spi_flash_read(0, sector, sizeof(sector)) == ESP_OK);
         for (auto v: sector) {
             CHECK(v == 0xff);
         }
@@ -42,9 +42,9 @@ TEST_CASE("invalid writes are checked", "[spi_flash_emu]")
     SpiFlashEmulator emu(1);
 
     uint32_t val = 0;
-    CHECK(spi_flash_write(0, &val, 4) == ESP_OK);
+    CHECK(spi_flash_write(0, reinterpret_cast<const uint8_t*>(&val), 4) == ESP_OK);
     val = 1;
-    CHECK(spi_flash_write(0, &val, 4) == ESP_ERR_FLASH_OP_FAIL);
+    CHECK(spi_flash_write(0, reinterpret_cast<const uint8_t*>(&val), 4) == ESP_ERR_FLASH_OP_FAIL);
 }
 
 
@@ -53,11 +53,11 @@ TEST_CASE("out of bounds writes fail", "[spi_flash_emu]")
     SpiFlashEmulator emu(4);
     uint32_t vals[8];
     std::fill_n(vals, 8, 0);
-    CHECK(spi_flash_write(0, vals, sizeof(vals)) == ESP_OK);
+    CHECK(spi_flash_write(0, reinterpret_cast<const uint8_t*>(vals), sizeof(vals)) == ESP_OK);
 
-    CHECK(spi_flash_write(4*4096 - sizeof(vals), vals, sizeof(vals)) == ESP_OK);
+    CHECK(spi_flash_write(4*4096 - sizeof(vals), reinterpret_cast<const uint8_t*>(vals), sizeof(vals)) == ESP_OK);
 
-    CHECK(spi_flash_write(4*4096 - sizeof(vals) + 4, vals, sizeof(vals)) == ESP_ERR_FLASH_OP_FAIL);
+    CHECK(spi_flash_write(4*4096 - sizeof(vals) + 4, reinterpret_cast<const uint8_t*>(vals), sizeof(vals)) == ESP_ERR_FLASH_OP_FAIL);
 }
 
 
@@ -65,9 +65,9 @@ TEST_CASE("after erase the sector is set to 0xff", "[spi_flash_emu]")
 {
     SpiFlashEmulator emu(4);
     uint32_t val1 = 0xab00cd12;
-    CHECK(spi_flash_write(0, &val1, sizeof(val1)) == ESP_OK);
+    CHECK(spi_flash_write(0, reinterpret_cast<const uint8_t*>(&val1), sizeof(val1)) == ESP_OK);
     uint32_t val2 = 0x5678efab;
-    CHECK(spi_flash_write(4096 - 4, &val2, sizeof(val2)) == ESP_OK);
+    CHECK(spi_flash_write(4096 - 4, reinterpret_cast<const uint8_t*>(&val2), sizeof(val2)) == ESP_OK);
 
     CHECK(emu.words()[0] == val1);
     CHECK(range_empty_n(emu.words() + 1, 4096 / 4 - 2));
@@ -83,7 +83,7 @@ TEST_CASE("after erase the sector is set to 0xff", "[spi_flash_emu]")
 TEST_CASE("read/write/erase operation times are calculated correctly", "[spi_flash_emu]")
 {
     SpiFlashEmulator emu(1);
-    uint32_t data[128];
+    uint8_t data[512];
     spi_flash_read(0, data, 4);
     CHECK(emu.getTotalTime() == 7);
     CHECK(emu.getReadOps() == 1);
@@ -141,7 +141,7 @@ TEST_CASE("read/write/erase operation times are calculated correctly", "[spi_fla
     CHECK(emu.getTotalTime() == 37142);
 }
 
-TEST_CASE("data is randomized predicatbly", "[spi_flash_emu]")
+TEST_CASE("data is randomized predictably", "[spi_flash_emu]")
 {
     SpiFlashEmulator emu1(3);
     emu1.randomize(0x12345678);

components/spi_flash/flash_ops.c

@@ -64,6 +64,11 @@ void spi_flash_init()
 #endif
 }
 
+size_t spi_flash_get_chip_size()
+{
+    return g_rom_flashchip.chip_size;
+}
+
 SpiFlashOpResult IRAM_ATTR spi_flash_unlock()
 {
     static bool unlocked = false;
@@ -77,28 +82,74 @@ SpiFlashOpResult IRAM_ATTR spi_flash_unlock()
     return SPI_FLASH_RESULT_OK;
 }
 
-esp_err_t IRAM_ATTR spi_flash_erase_sector(uint16_t sec)
+esp_err_t IRAM_ATTR spi_flash_erase_sector(size_t sec)
 {
+    return spi_flash_erase_range(sec * SPI_FLASH_SEC_SIZE, SPI_FLASH_SEC_SIZE);
+}
+
+esp_err_t IRAM_ATTR spi_flash_erase_range(uint32_t start_addr, uint32_t size)
+{
+    if (start_addr % SPI_FLASH_SEC_SIZE != 0) {
+        return ESP_ERR_INVALID_ARG;
+    }
+    if (size % SPI_FLASH_SEC_SIZE != 0) {
+        return ESP_ERR_INVALID_SIZE;
+    }
+    if (size + start_addr > spi_flash_get_chip_size()) {
+        return ESP_ERR_INVALID_SIZE;
+    }
+    size_t start = start_addr / SPI_FLASH_SEC_SIZE;
+    size_t end = start + size / SPI_FLASH_SEC_SIZE;
+    const size_t sectors_per_block = 16;
     COUNTER_START();
     spi_flash_disable_interrupts_caches_and_other_cpu();
     SpiFlashOpResult rc;
     rc = spi_flash_unlock();
     if (rc == SPI_FLASH_RESULT_OK) {
-        rc = SPIEraseSector(sec);
+        for (size_t sector = start; sector != end && rc == SPI_FLASH_RESULT_OK; ) {
+            if (sector % sectors_per_block == 0 && end - sector > sectors_per_block) {
+                rc = SPIEraseBlock(sector / sectors_per_block);
+                sector += sectors_per_block;
+                COUNTER_ADD_BYTES(erase, sectors_per_block * SPI_FLASH_SEC_SIZE);
+            }
+            else {
+                rc = SPIEraseSector(sector);
+                ++sector;
+                COUNTER_ADD_BYTES(erase, SPI_FLASH_SEC_SIZE);
+            }
+        }
     }
     spi_flash_enable_interrupts_caches_and_other_cpu();
     COUNTER_STOP(erase);
     return spi_flash_translate_rc(rc);
 }
 
-esp_err_t IRAM_ATTR spi_flash_write(uint32_t dest_addr, const uint32_t *src, uint32_t size)
+esp_err_t IRAM_ATTR spi_flash_write(size_t dest_addr, const uint8_t *src, size_t size)
 {
+    // TODO: replace this check with code which deals with unaligned sources
+    if (((ptrdiff_t) src) % 4 != 0) {
+        return ESP_ERR_INVALID_ARG;
+    }
+    // Destination alignment is also checked in ROM code, but we can give
+    // better error code here
+    // TODO: add handling of unaligned destinations
+    if (dest_addr % 4 != 0) {
+        return ESP_ERR_INVALID_ARG;
+    }
+    if (size % 4 != 0) {
+        return ESP_ERR_INVALID_SIZE;
+    }
+    // Out of bound writes are checked in ROM code, but we can give better
+    // error code here
+    if (dest_addr + size > g_rom_flashchip.chip_size) {
+        return ESP_ERR_INVALID_SIZE;
+    }
     COUNTER_START();
     spi_flash_disable_interrupts_caches_and_other_cpu();
     SpiFlashOpResult rc;
     rc = spi_flash_unlock();
     if (rc == SPI_FLASH_RESULT_OK) {
-        rc = SPIWrite(dest_addr, src, (int32_t) size);
+        rc = SPIWrite((uint32_t) dest_addr, (const uint32_t*) src, (int32_t) size);
         COUNTER_ADD_BYTES(write, size);
     }
     spi_flash_enable_interrupts_caches_and_other_cpu();
@@ -106,11 +157,29 @@ esp_err_t IRAM_ATTR spi_flash_write(uint32_t dest_addr, const uint32_t *src, uin
     return spi_flash_translate_rc(rc);
 }
 
-esp_err_t IRAM_ATTR spi_flash_read(uint32_t src_addr, uint32_t *dest, uint32_t size)
+esp_err_t IRAM_ATTR spi_flash_read(size_t src_addr, uint8_t *dest, size_t size)
 {
+    // TODO: replace this check with code which deals with unaligned destinations
+    if (((ptrdiff_t) dest) % 4 != 0) {
+        return ESP_ERR_INVALID_ARG;
+    }
+    // Source alignment is also checked in ROM code, but we can give
+    // better error code here
+    // TODO: add handling of unaligned destinations
+    if (src_addr % 4 != 0) {
+        return ESP_ERR_INVALID_ARG;
+    }
+    if (size % 4 != 0) {
+        return ESP_ERR_INVALID_SIZE;
+    }
+    // Out of bound reads are checked in ROM code, but we can give better
+    // error code here
+    if (src_addr + size > g_rom_flashchip.chip_size) {
+        return ESP_ERR_INVALID_SIZE;
+    }
     COUNTER_START();
     spi_flash_disable_interrupts_caches_and_other_cpu();
-    SpiFlashOpResult rc = SPIRead(src_addr, dest, (int32_t) size);
+    SpiFlashOpResult rc = SPIRead((uint32_t) src_addr, (uint32_t*) dest, (int32_t) size);
     COUNTER_ADD_BYTES(read, size);
     spi_flash_enable_interrupts_caches_and_other_cpu();
     COUNTER_STOP(read);

components/spi_flash/include/esp_spi_flash.h

@@ -38,37 +38,63 @@ extern "C" {
  */
 void spi_flash_init();
 
+/**
+ * @brief  Get flash chip size, as set in binary image header
+ *
+ * @note This value does not necessarily match real flash size.
+ *
+ * @return size of flash chip, in bytes
+ */
+size_t spi_flash_get_chip_size();
+
 /**
  * @brief  Erase the Flash sector.
  *
- * @param  uint16 sec : Sector number, the count starts at sector 0, 4KB per sector.
+ * @param  sector  Sector number, the count starts at sector 0, 4KB per sector.
  *
  * @return esp_err_t
  */
-esp_err_t spi_flash_erase_sector(uint16_t sec);
+esp_err_t spi_flash_erase_sector(size_t sector);
+
+/**
+ * @brief  Erase a range of flash sectors
+ *
+ * @param  uint32_t start_address : Address where erase operation has to start.
+ *                                  Must be 4kB-aligned
+ * @param  uint32_t size : Size of erased range, in bytes. Must be divisible by 4kB.
+ *
+ * @return esp_err_t
+ */
+esp_err_t spi_flash_erase_range(size_t start_addr, size_t size);
+
 
 /**
  * @brief  Write data to Flash.
  *
- * @param  uint32 des_addr  : destination address in Flash.
+ * @note Both des_addr and src_addr have to be 4-byte aligned.
- * @param  uint32 *src_addr : source address of the data.
+ *       This is a temporary limitation which will be removed.
- * @param  uint32 size      : length of data
+ *
+ * @param  des_addr  destination address in Flash
+ * @param  src_addr  source address of the data
+ * @param  size  length of data, in bytes
  *
  * @return esp_err_t
  */
-esp_err_t spi_flash_write(uint32_t des_addr, const uint32_t *src_addr, uint32_t size);
+esp_err_t spi_flash_write(size_t des_addr, const uint8_t *src_addr, size_t size);
 
 /**
  * @brief  Read data from Flash.
  *
- * @param  uint32 src_addr  : source address of the data in Flash.
+ * @note Both des_addr and src_addr have to be 4-byte aligned.
- * @param  uint32 *des_addr : destination address.
+ *       This is a temporary limitation which will be removed.
- * @param  uint32 size      : length of data
+ *
+ * @param  src_addr  source address of the data in Flash.
+ * @param  des_addr  destination address
+ * @param  size  length of data
  *
  * @return esp_err_t
  */
-esp_err_t spi_flash_read(uint32_t src_addr, uint32_t *des_addr, uint32_t size);
+esp_err_t spi_flash_read(size_t src_addr, uint8_t *des_addr, size_t size);
-
 
 /**
  * @brief Enumeration which specifies memory space requested in an mmap call
@@ -135,7 +161,7 @@ void spi_flash_mmap_dump();
 typedef struct {
     uint32_t count;     // number of times operation was executed
     uint32_t time;      // total time taken, in microseconds
-    uint32_t bytes;     // total number of bytes, for read and write operations
+    uint32_t bytes;     // total number of bytes
 } spi_flash_counter_t;
 
 typedef struct {