New flash writing method with offset/memory/size alignment handling (#7514)

* Do not write more data than requested on PUYA flashes

* Always align flash reads/writes to 4 bytes

* fixup! Always align flash reads/writes to 4 bytes

This commit simplifies the code a bit and fixes a bug that caused wrong number of bytes to be
written

* fixup! Always align flash reads/writes to 4 bytes

* fixup! Always align flash reads/writes to 4 bytes

* Check for result before additional read/write

* Add overloads for unaligned reads/writes

* fixup! Add overloads for unaligned reads/writes

* fixup! Add overloads for unaligned reads/writes

* fixup! Add overloads for unaligned reads/writes

* fixup! Add overloads for unaligned reads/writes

* fixup! Add overloads for unaligned reads/writes

* fixup! Add overloads for unaligned reads/writes

* fixup! Add overloads for unaligned reads/writes

* Add tests for flashRead/flashWrite

* fixup! Add overloads for unaligned reads/writes

* fixup! Add tests for flashRead/flashWrite

* fixup! Add tests for flashRead/flashWrite

* fixup! Add overloads for unaligned reads/writes

Author: drzony

cores/esp8266/Esp.cpp

@@ -42,11 +42,6 @@ extern struct rst_info resetInfo;
 #ifndef PUYA_SUPPORT
   #define PUYA_SUPPORT 1
 #endif
-#ifndef PUYA_BUFFER_SIZE
-  // Good alternative for buffer size is: SPI_FLASH_SEC_SIZE (= 4k)
-  // Always use a multiple of flash page size (256 bytes)
-  #define PUYA_BUFFER_SIZE 256
-#endif
 
 /**
  * User-defined Literals
@@ -668,11 +663,14 @@ static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, si
     if (data == nullptr) {
       return SPI_FLASH_RESULT_ERR;
     }
+    if (size % 4 != 0) {
+      return SPI_FLASH_RESULT_ERR;
+    }
     // PUYA flash chips need to read existing data, update in memory and write modified data again.
     static uint32_t *flash_write_puya_buf = nullptr;
 
     if (flash_write_puya_buf == nullptr) {
-        flash_write_puya_buf = (uint32_t*) malloc(PUYA_BUFFER_SIZE);
+        flash_write_puya_buf = (uint32_t*) malloc(FLASH_PAGE_SIZE);
         // No need to ever free this, since the flash chip will never change at runtime.
         if (flash_write_puya_buf == nullptr) {
             // Memory could not be allocated.
@@ -686,45 +684,261 @@ static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, si
     uint32_t pos = offset;
     while (bytesLeft > 0 && rc == SPI_FLASH_RESULT_OK) {
         size_t bytesNow = bytesLeft;
-        if (bytesNow > PUYA_BUFFER_SIZE) {
-            bytesNow = PUYA_BUFFER_SIZE;
-            bytesLeft -= PUYA_BUFFER_SIZE;
+        if (bytesNow > FLASH_PAGE_SIZE) {
+            bytesNow = FLASH_PAGE_SIZE;
+            bytesLeft -= FLASH_PAGE_SIZE;
         } else {
             bytesLeft = 0;
         }
-        size_t bytesAligned = (bytesNow + 3) & ~3;
-        rc = spi_flash_read(pos, flash_write_puya_buf, bytesAligned);
+        rc = spi_flash_read(pos, flash_write_puya_buf, bytesNow);
         if (rc != SPI_FLASH_RESULT_OK) {
             return rc;
         }
-        for (size_t i = 0; i < bytesAligned / 4; ++i) {
+        for (size_t i = 0; i < bytesNow / 4; ++i) {
             flash_write_puya_buf[i] &= *ptr;
             ++ptr;
         }
-        rc = spi_flash_write(pos, flash_write_puya_buf, bytesAligned);
+        rc = spi_flash_write(pos, flash_write_puya_buf, bytesNow);
         pos += bytesNow;
     }
     return rc;
 }
 #endif
 
-bool EspClass::flashWrite(uint32_t offset, uint32_t *data, size_t size) {
+bool EspClass::flashReplaceBlock(uint32_t address, const uint8_t *value, uint32_t byteCount) {
+    uint32_t alignedAddress = (address & ~3);
+    uint32_t alignmentOffset = address - alignedAddress;
+
+    if (alignedAddress != ((address + byteCount - 1) & ~3)) {
+        // Only one 4 byte block is supported
+        return false;
+    }
+#if PUYA_SUPPORT
+    if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) {
+        uint8_t tempData[4] __attribute__((aligned(4)));
+        if (spi_flash_read(alignedAddress, (uint32_t *)tempData, 4) != SPI_FLASH_RESULT_OK) {
+            return false;
+        }
+        for (size_t i = 0; i < byteCount; i++) {
+            tempData[i + alignmentOffset] &= value[i];
+        }
+        if (spi_flash_write(alignedAddress, (uint32_t *)tempData, 4) != SPI_FLASH_RESULT_OK) {
+            return false;
+        }
+    }
+    else
+#endif // PUYA_SUPPORT
+    {
+        uint32_t tempData;
+        if (spi_flash_read(alignedAddress, &tempData, 4) != SPI_FLASH_RESULT_OK) {
+            return false;
+        }
+        memcpy((uint8_t *)&tempData + alignmentOffset, value, byteCount);
+        if (spi_flash_write(alignedAddress, &tempData, 4) != SPI_FLASH_RESULT_OK) {
+            return false;
+        }
+    }
+    return true;
+}
+
+size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data, size_t size) {
+    size_t sizeLeft = (size & ~3);
+    size_t currentOffset = 0;
+    // Memory is unaligned, so we need to copy it to an aligned buffer
+    uint32_t alignedData[FLASH_PAGE_SIZE / sizeof(uint32_t)] __attribute__((aligned(4)));
+    // Handle page boundary
+    bool pageBreak = ((address % 4) != 0) && ((address / FLASH_PAGE_SIZE) != ((address + sizeLeft - 1) / FLASH_PAGE_SIZE));
+
+    if (pageBreak) {
+        size_t byteCount = 4 - (address % 4);
+
+        if (!flashReplaceBlock(address, data, byteCount)) {
+            return 0;
+        }
+        // We will now have aligned address, so we can cross page boundaries
+        currentOffset += byteCount;
+        // Realign size to 4
+        sizeLeft = (size - byteCount) & ~3;
+    }
+
+    while (sizeLeft) {
+        size_t willCopy = std::min(sizeLeft, sizeof(alignedData));
+        memcpy(alignedData, data + currentOffset, willCopy);
+        // We now have address, data and size aligned to 4 bytes, so we can use aligned write
+        if (!flashWrite(address + currentOffset, alignedData, willCopy))
+        {
+            return 0;
+        }
+        sizeLeft -= willCopy;
+        currentOffset += willCopy;
+    }
+
+    return currentOffset;
+}
+
+bool EspClass::flashWritePageBreak(uint32_t address, const uint8_t *data, size_t size) {
+    if (size > 4) {
+        return false;
+    }
+    size_t pageLeft = FLASH_PAGE_SIZE - (address % FLASH_PAGE_SIZE);
+    size_t offset = 0;
+    size_t sizeLeft = size;
+    if (pageLeft > 3) {
+        return false;
+    }
+
+    if (!flashReplaceBlock(address, data, pageLeft)) {
+        return false;
+    }
+    offset += pageLeft;
+    sizeLeft -= pageLeft;
+    // We replaced last 4-byte block of the page, now we write the remainder in next page
+    if (!flashReplaceBlock(address + offset, data + offset, sizeLeft)) {
+        return false;
+    }
+    return true;
+}
+
+bool EspClass::flashWrite(uint32_t address, const uint32_t *data, size_t size) {
     SpiFlashOpResult rc = SPI_FLASH_RESULT_OK;
+    bool pageBreak = ((address % 4) != 0 && (address / FLASH_PAGE_SIZE) != ((address + size - 1) / FLASH_PAGE_SIZE));
+
+    if ((uintptr_t)data % 4 != 0 || size % 4 != 0 || pageBreak) {
+        return false;
+    }
 #if PUYA_SUPPORT
     if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) {
-        rc = spi_flash_write_puya(offset, data, size);
+        rc = spi_flash_write_puya(address, const_cast<uint32_t *>(data), size);
     }
     else
-#endif
+#endif // PUYA_SUPPORT
     {
-        rc = spi_flash_write(offset, data, size);
+        rc = spi_flash_write(address, const_cast<uint32_t *>(data), size);
     }
     return rc == SPI_FLASH_RESULT_OK;
 }
 
-bool EspClass::flashRead(uint32_t offset, uint32_t *data, size_t size) {
-    auto rc = spi_flash_read(offset, (uint32_t*) data, size);
-    return rc == SPI_FLASH_RESULT_OK;
+bool EspClass::flashWrite(uint32_t address, const uint8_t *data, size_t size) {
+    if (size == 0) {
+        return true;
+    }
+
+    size_t sizeLeft = size & ~3;
+    size_t currentOffset = 0;
+
+    if (sizeLeft) {
+        if ((uintptr_t)data % 4 != 0) {
+            size_t written = flashWriteUnalignedMemory(address, data, size);
+            if (!written) {
+                return false;
+            }
+            currentOffset += written;
+            sizeLeft -= written;
+        } else {
+            bool pageBreak = ((address % 4) != 0 && (address / FLASH_PAGE_SIZE) != ((address + sizeLeft - 1) / FLASH_PAGE_SIZE));
+
+            if (pageBreak) {
+                while (sizeLeft) {
+                    // We cannot cross page boundary, but the write must be 4 byte aligned,
+                    // so this is the maximum amount we can write
+                    size_t pageBoundary = (FLASH_PAGE_SIZE - ((address + currentOffset) % FLASH_PAGE_SIZE)) & ~3;
+
+                    if (sizeLeft > pageBoundary) {
+                        // Aligned write up to page boundary
+                        if (!flashWrite(address + currentOffset, (uint32_t *)(data + currentOffset), pageBoundary)) {
+                            return false;
+                        }
+                        currentOffset += pageBoundary;
+                        sizeLeft -= pageBoundary;
+                        // Cross the page boundary
+                        if (!flashWritePageBreak(address + currentOffset, data + currentOffset, 4)) {
+                            return false;
+                        }
+                        currentOffset += 4;
+                        sizeLeft -= 4;
+                    } else {
+                        // We do not cross page boundary
+                        if (!flashWrite(address + currentOffset, (uint32_t *)(data + currentOffset), sizeLeft)) {
+                            return false;
+                        }
+                        currentOffset += sizeLeft;
+                        sizeLeft = 0;
+                    }
+                }
+            } else {
+                // Pointer is properly aligned and write does not cross page boundary,
+                // so use aligned write
+                if (!flashWrite(address, (uint32_t *)data, sizeLeft)) {
+                    return false;
+                }
+                currentOffset = sizeLeft;
+                sizeLeft = 0;
+            }
+        }
+    }
+    sizeLeft = size - currentOffset;
+    if (sizeLeft > 0) {
+        // Size was not aligned, so we have some bytes left to write, we also need to recheck for
+        // page boundary crossing
+        bool pageBreak = ((address % 4) != 0 && (address / FLASH_PAGE_SIZE) != ((address + sizeLeft - 1) / FLASH_PAGE_SIZE));
+
+        if (pageBreak) {
+            // Cross the page boundary
+            if (!flashWritePageBreak(address + currentOffset, data + currentOffset, sizeLeft)) {
+                return false;
+            }
+        } else {
+            // Just write partial block
+            flashReplaceBlock(address + currentOffset, data + currentOffset, sizeLeft);
+        }
+    }
+
+    return true;
+}
+
+bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) {
+    size_t sizeAligned = size & ~3;
+    size_t currentOffset = 0;
+
+    if ((uintptr_t)data % 4 != 0) {
+        uint32_t alignedData[FLASH_PAGE_SIZE / sizeof(uint32_t)] __attribute__((aligned(4)));
+        size_t sizeLeft = sizeAligned;
+
+        while (sizeLeft) {
+            size_t willCopy = std::min(sizeLeft, sizeof(alignedData));
+            // We read to our aligned buffer and then copy to data
+            if (!flashRead(address + currentOffset, alignedData, willCopy))
+            {
+                return false;
+            }
+            memcpy(data + currentOffset, alignedData, willCopy);
+            sizeLeft -= willCopy;
+            currentOffset += willCopy;
+        }
+    } else {
+        // Pointer is properly aligned, so use aligned read
+        if (!flashRead(address, (uint32_t *)data, sizeAligned)) {
+            return false;
+        }
+        currentOffset = sizeAligned;
+    }
+
+    if (currentOffset < size) {
+        uint32_t tempData;
+        if (spi_flash_read(address + currentOffset, &tempData, 4) != SPI_FLASH_RESULT_OK) {
+            return false;
+        }
+        memcpy((uint8_t *)data + currentOffset, &tempData, size - currentOffset);
+    }
+
+    return true;
+}
+
+bool EspClass::flashRead(uint32_t address, uint32_t *data, size_t size) {
+    if ((uintptr_t)data % 4 != 0 || size % 4 != 0) {
+        return false;
+    }
+    return (spi_flash_read(address, data, size) == SPI_FLASH_RESULT_OK);
 }
 
 String EspClass::getSketchMD5()

cores/esp8266/Esp.h

@@ -150,8 +150,48 @@ class EspClass {
         bool checkFlashCRC();
 
         bool flashEraseSector(uint32_t sector);
-        bool flashWrite(uint32_t offset, uint32_t *data, size_t size);
-        bool flashRead(uint32_t offset, uint32_t *data, size_t size);
+        /**
+         * @brief Write @a size bytes from @a data to flash at @a address
+         * This overload requires @a data and @a size to be always 4 byte aligned and
+         * @a address to be 4 byte aligned if the write crossess page boundary,
+         * but guarantees no overhead (except on PUYA flashes)
+         * @param address address on flash where write should start, 4 byte alignment is conditional
+         * @param data input buffer, must be 4-byte aligned
+         * @param size amount of data, must be a multiple of 4
+         * @return bool result of operation
+         * @retval true success
+         * @retval false failure to write to flash or incorrect alignment of params
+         */
+        bool flashWrite(uint32_t address, const uint32_t *data, size_t size);
+        /**
+         * @brief Write @a size bytes from @a data to flash at @a address
+         * This overload handles all misalignment cases
+         * @param address address on flash where write should start
+         * @param data input buffer, passing unaligned memory will cause significant stack usage
+         * @param size amount of data, passing not multiple of 4 will cause additional reads and writes
+         * @return bool result of operation
+         */
+        bool flashWrite(uint32_t address, const uint8_t *data, size_t size);
+        /**
+         * @brief Read @a size bytes to @a data to flash at @a address
+         * This overload requires @a data and @a size to be 4 byte aligned
+         * @param address address on flash where read should start
+         * @param data input buffer, must be 4-byte aligned
+         * @param size amount of data, must be a multiple of 4
+         * @return bool result of operation
+         * @retval true success
+         * @retval false failure to read from flash or incorrect alignment of params
+         */
+        bool flashRead(uint32_t address, uint32_t *data, size_t size);
+        /**
+         * @brief Read @a size bytes to @a data to flash at @a address
+         * This overload handles all misalignment cases
+         * @param address address on flash where read should start
+         * @param data input buffer, passing unaligned memory will cause significant stack usage
+         * @param size amount of data, passing not multiple of 4 will cause additional read
+         * @return bool result of operation
+         */
+        bool flashRead(uint32_t address, uint8_t *data, size_t size);
 
         uint32_t getSketchSize();
         String getSketchMD5();
@@ -175,6 +215,40 @@ class EspClass {
 #else
         uint32_t getCycleCount();
 #endif // !defined(CORE_MOCK)
+    private:
+        /**
+         * @brief Replaces @a byteCount bytes of a 4 byte block on flash
+         *
+         * @param address flash address
+         * @param value buffer with data
+         * @param byteCount number of bytes to replace
+         * @return bool result of operation
+         * @retval true success
+         * @retval false failed to read/write or invalid args
+         */
+        bool flashReplaceBlock(uint32_t address, const uint8_t *value, uint32_t byteCount);
+        /**
+         * @brief Write up to @a size bytes from @a data to flash at @a address
+         * This function takes case of unaligned memory acces by copying @a data to a temporary buffer,
+         * it also takes care of page boundary crossing see @a flashWritePageBreak as to why it's done.
+         * Less than @a size bytes may be written, due to 4 byte alignment requirement of spi_flash_write
+         * @param address address on flash where write should start
+         * @param data input buffer
+         * @param size amount of data
+         * @return size_t amount of data written, 0 on failure
+         */
+        size_t flashWriteUnalignedMemory(uint32_t address, const uint8_t *data, size_t size);
+        /**
+         * @brief Splits up to 4 bytes into 4 byte blocks and writes them to flash
+         * We need this since spi_flash_write cannot handle writing over a page boundary with unaligned offset
+         * i.e. spi_flash_write(254, data, 4) will fail, also we cannot write less bytes as in
+         * spi_flash_write(254, data, 2) since it will be extended internally to 4 bytes and fail
+         * @param address start of write
+         * @param data data to be written
+         * @param size amount of data, must be < 4
+         * @return bool result of operation
+         */
+        bool flashWritePageBreak(uint32_t address, const uint8_t *data, size_t size);
 };
 
 extern EspClass ESP;