From a83c0cddd9fc9627414834423f01b76d97e511ac Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Fri, 17 Jun 2022 01:33:26 +0300 Subject: [PATCH 01/10] break pages --- cores/esp8266/Esp.cpp | 291 +++++++++++++++++++----------------------- cores/esp8266/Esp.h | 32 +---- 2 files changed, 137 insertions(+), 186 deletions(-) diff --git a/cores/esp8266/Esp.cpp b/cores/esp8266/Esp.cpp index d517f6a496..801d20916a 100644 --- a/cores/esp8266/Esp.cpp +++ b/cores/esp8266/Esp.cpp @@ -673,7 +673,48 @@ bool EspClass::flashEraseSector(uint32_t sector) { return rc == 0; } +// Adapted from the old version of `flash_hal_write()` (before 3.0.0), which was used for SPIFFS to allow +// writing from both unaligned u8 buffers and to an unaligned offset on flash. +// Updated version re-uses some of the code from RTOS, replacing individual methods for block & page +// writes with just a single one +// https://github.com/espressif/ESP8266_RTOS_SDK/blob/master/components/spi_flash/src/spi_flash.c + +// Wrapper around spi_flash_write, handling offset + size crossing page boundaries +// Note that we expect that both `offset` and `data` are properly aligned +static SpiFlashOpResult spi_flash_write_page_break(uint32_t offset, uint32_t* data, size_t size) { + static constexpr uint32_t PageSize { FLASH_PAGE_SIZE }; + size_t page_size = PageSize - (offset % PageSize); + + // most common case, we don't cross a page and simply write the data + if (size < page_size) { + return spi_flash_write(offset, data, size); + } + + // otherwise, write the initial part and continue writing breaking each page interval + SpiFlashOpResult result = SPI_FLASH_RESULT_ERR; + if ((result = spi_flash_write(offset, data, page_size)) != SPI_FLASH_RESULT_OK) { + return result; + } + + uint32_t page_offset = PageSize; + for (uint32_t page = 0; page < (size - page_size) / PageSize; ++page) { + if ((result = spi_flash_write(offset + page_offset, data + (page_offset >> 2), PageSize)) != SPI_FLASH_RESULT_OK) { + return result; + } + + page_offset += PageSize; + } + + if ((offset + page_offset) < (offset + size)) { + return spi_flash_write(offset + page_offset, data + (page_offset >> 2), size - page_offset); + } + + return SPI_FLASH_RESULT_OK; +} + #if PUYA_SUPPORT +// Special wrapper for spi_flash_write *only for PUYA flash chips* +// Already handles paging, could be used as a `spi_flash_write_page_break` replacement static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, size_t size) { if (data == nullptr) { return SPI_FLASH_RESULT_ERR; @@ -720,195 +761,127 @@ static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, si } #endif -bool EspClass::flashReplaceBlock(uint32_t address, const uint8_t *value, uint32_t byteCount) { - uint32_t alignedAddress = (address & ~3); - uint32_t alignmentOffset = address - alignedAddress; +static constexpr uint32_t Alignment { 4 }; - 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; +static uint32_t alignAddress(uint32_t address) { + static constexpr uint32_t Mask { Alignment - 1 }; + return (address + Mask) & ~Mask; +} + +static uint32_t alignBeforeAddress(uint32_t address) { + return alignAddress(address) - Alignment; +} + +static bool isAlignedAddress(uint32_t address) { + return (address & (Alignment - 1)) == 0; +} + +static bool isAlignedSize(size_t size) { + return (size & (Alignment - 1)) == 0; +} + +static bool isAlignedPointer(const uint8_t* ptr) { + return isAlignedAddress(reinterpret_cast(ptr)); } 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)); + auto flash_write = [&](uint32_t address, uint8_t* data, size_t size) { + return flashWrite(address, reinterpret_cast(data), size); + }; + + auto flash_read = [](uint32_t address, uint8_t* data, size_t size) { + return spi_flash_read(address, reinterpret_cast(data), size) == SPI_FLASH_RESULT_OK; + }; + + alignas(alignof(uint32_t)) uint8_t buf[FLASH_PAGE_SIZE]; - if (pageBreak) { - size_t byteCount = 4 - (address % 4); + size_t written = 0; - if (!flashReplaceBlock(address, data, byteCount)) { + if (!isAlignedAddress(address)) { + auto r_addr = alignBeforeAddress(address); + auto c_off = address - r_addr; + auto wbytes = Alignment - c_off; + + wbytes = std::min(wbytes, size); + + if (spi_flash_read(r_addr, reinterpret_cast(&buf[0]), Alignment) != SPI_FLASH_RESULT_OK) { 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; +#if PUYA_SUPPORT + if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) { + for (size_t i = 0; i < wbytes ; ++i) { + buf[c_off + i] &= data[i]; + } + } else { +#endif + memcpy(&buf[c_off], data, wbytes); +#if PUYA_SUPPORT } - sizeLeft -= willCopy; - currentOffset += willCopy; - } +#endif - return currentOffset; -} + if (spi_flash_write(r_addr, reinterpret_cast(&buf[0]), Alignment) != SPI_FLASH_RESULT_OK) { + return wbytes; + } -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; + address += wbytes; + data += wbytes; + written += wbytes; + size -= wbytes; } - 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; + while (size > 0) { + auto len = std::min(size, sizeof(buf)); + auto wlen = alignAddress(len); + + if (wlen != len) { + auto l_b = wlen - Alignment; + if (!flash_read(address + l_b, &buf[l_b], Alignment)) { + return written; + } + } + + memcpy(&buf[0], data, len); + if (!flash_write(address, &buf[0], wlen)) { + return written; + } + + address += len; + data += len; + written += len; + size -= len; } - return true; + + return written; } 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; - } + SpiFlashOpResult result; #if PUYA_SUPPORT if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) { - rc = spi_flash_write_puya(address, const_cast(data), size); + result = spi_flash_write_puya(address, const_cast(data), size); } else -#endif // PUYA_SUPPORT +#endif { - rc = spi_flash_write(address, const_cast(data), size); + result = spi_flash_write_page_break(address, const_cast(data), size); } - return rc == SPI_FLASH_RESULT_OK; + return result == 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); + if (data && size) { + if (!isAlignedAddress(address) + || !isAlignedPointer(data) + || !isAlignedSize(size)) + { + return flashWriteUnalignedMemory(address, data, size) == size; } + + return flashWrite(address, reinterpret_cast(data), size) == size; } - return true; + return false; } bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) { diff --git a/cores/esp8266/Esp.h b/cores/esp8266/Esp.h index 773d3cd192..b25e02901e 100644 --- a/cores/esp8266/Esp.h +++ b/cores/esp8266/Esp.h @@ -103,11 +103,11 @@ class EspClass { static void reset(); static void restart(); - /** - * @brief When calling this method the ESP8266 reboots into the UART download mode without - * the need of any external wiring. This is the same mode which can also be entered by - * pulling GPIO0=low, GPIO2=high, GPIO15=low and resetting the ESP8266. - */ + /** + * @brief When calling this method the ESP8266 reboots into the UART download mode without + * the need of any external wiring. This is the same mode which can also be entered by + * pulling GPIO0=low, GPIO2=high, GPIO15=low and resetting the ESP8266. + */ [[noreturn]] static void rebootIntoUartDownloadMode(); static uint16_t getVcc(); @@ -252,17 +252,6 @@ class EspClass { */ static void resetHeap(); 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 - */ - static 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 access by copying @a data to a temporary buffer, @@ -274,17 +263,6 @@ class EspClass { * @return size_t amount of data written, 0 on failure */ static 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 - */ - static bool flashWritePageBreak(uint32_t address, const uint8_t *data, size_t size); }; extern EspClass ESP; From 9a3a4fe840be7ea8f25fd1488dd74ed09ebb1ac0 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Sat, 25 Jun 2022 14:13:33 +0300 Subject: [PATCH 02/10] oops --- cores/esp8266/Esp.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cores/esp8266/Esp.cpp b/cores/esp8266/Esp.cpp index 801d20916a..345c808972 100644 --- a/cores/esp8266/Esp.cpp +++ b/cores/esp8266/Esp.cpp @@ -878,7 +878,7 @@ bool EspClass::flashWrite(uint32_t address, const uint8_t *data, size_t size) { return flashWriteUnalignedMemory(address, data, size) == size; } - return flashWrite(address, reinterpret_cast(data), size) == size; + return flashWrite(address, reinterpret_cast(data), size); } return false; From 553913cc3913f521309bc8def46736c61311b9cc Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Sat, 25 Jun 2022 14:46:48 +0300 Subject: [PATCH 03/10] words. fix page offset when writing --- cores/esp8266/Esp.cpp | 79 +++++++++++++++++++++---------------------- 1 file changed, 39 insertions(+), 40 deletions(-) diff --git a/cores/esp8266/Esp.cpp b/cores/esp8266/Esp.cpp index 345c808972..b5b563769d 100644 --- a/cores/esp8266/Esp.cpp +++ b/cores/esp8266/Esp.cpp @@ -678,38 +678,35 @@ bool EspClass::flashEraseSector(uint32_t sector) { // Updated version re-uses some of the code from RTOS, replacing individual methods for block & page // writes with just a single one // https://github.com/espressif/ESP8266_RTOS_SDK/blob/master/components/spi_flash/src/spi_flash.c +// (if necessary, we could follow the esp-idf code and introduce flash chip drivers controling more than just writing methods?) -// Wrapper around spi_flash_write, handling offset + size crossing page boundaries -// Note that we expect that both `offset` and `data` are properly aligned -static SpiFlashOpResult spi_flash_write_page_break(uint32_t offset, uint32_t* data, size_t size) { +// This is a generic writer that does not cross page boundaries. +// Offset, data address and size *must* be 4byte aligned. +static SpiFlashOpResult spi_flash_write_page_break(uint32_t offset, uint32_t *data, size_t size) { static constexpr uint32_t PageSize { FLASH_PAGE_SIZE }; - size_t page_size = PageSize - (offset % PageSize); + size_t size_page_aligned = PageSize - (offset % PageSize); // most common case, we don't cross a page and simply write the data - if (size < page_size) { + if (size < size_page_aligned) { return spi_flash_write(offset, data, size); } // otherwise, write the initial part and continue writing breaking each page interval SpiFlashOpResult result = SPI_FLASH_RESULT_ERR; - if ((result = spi_flash_write(offset, data, page_size)) != SPI_FLASH_RESULT_OK) { + if ((result = spi_flash_write(offset, data, size_page_aligned)) != SPI_FLASH_RESULT_OK) { return result; } - uint32_t page_offset = PageSize; - for (uint32_t page = 0; page < (size - page_size) / PageSize; ++page) { - if ((result = spi_flash_write(offset + page_offset, data + (page_offset >> 2), PageSize)) != SPI_FLASH_RESULT_OK) { + for (uint32_t page = 0; page < (size - size_page_aligned) / PageSize; ++page) { + if ((result = spi_flash_write(offset + size_page_aligned, data + (size_page_aligned >> 2), PageSize)) != SPI_FLASH_RESULT_OK) { return result; } - page_offset += PageSize; + size_page_aligned += PageSize; } - if ((offset + page_offset) < (offset + size)) { - return spi_flash_write(offset + page_offset, data + (page_offset >> 2), size - page_offset); - } - - return SPI_FLASH_RESULT_OK; + // finally, the remaining data + return spi_flash_write(offset + size_page_aligned, data + (size_page_aligned >> 2), size - size_page_aligned); } #if PUYA_SUPPORT @@ -763,13 +760,15 @@ static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, si static constexpr uint32_t Alignment { 4 }; -static uint32_t alignAddress(uint32_t address) { - static constexpr uint32_t Mask { Alignment - 1 }; - return (address + Mask) & ~Mask; +template +static T aligned(T value) { + static constexpr auto Mask = Alignment - 1; + return (value + Mask) & ~Mask; } -static uint32_t alignBeforeAddress(uint32_t address) { - return alignAddress(address) - Alignment; +template +static T alignBefore(T value) { + return aligned(value) - Alignment; } static bool isAlignedAddress(uint32_t address) { @@ -784,13 +783,14 @@ static bool isAlignedPointer(const uint8_t* ptr) { return isAlignedAddress(reinterpret_cast(ptr)); } + size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data, size_t size) { - auto flash_write = [&](uint32_t address, uint8_t* data, size_t size) { - return flashWrite(address, reinterpret_cast(data), size); + auto flash_write = [](uint32_t address, uint8_t* data, size_t size) { + return spi_flash_write(address, reinterpret_cast(data), size) == SPI_FLASH_RESULT_OK; }; auto flash_read = [](uint32_t address, uint8_t* data, size_t size) { - return spi_flash_read(address, reinterpret_cast(data), size) == SPI_FLASH_RESULT_OK; + return spi_flash_read(address, reinterpret_cast(data), size) == SPI_FLASH_RESULT_OK; }; alignas(alignof(uint32_t)) uint8_t buf[FLASH_PAGE_SIZE]; @@ -798,13 +798,11 @@ size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data size_t written = 0; if (!isAlignedAddress(address)) { - auto r_addr = alignBeforeAddress(address); + auto r_addr = alignBefore(address); auto c_off = address - r_addr; - auto wbytes = Alignment - c_off; - - wbytes = std::min(wbytes, size); + auto wbytes = std::min(Alignment - c_off, size); - if (spi_flash_read(r_addr, reinterpret_cast(&buf[0]), Alignment) != SPI_FLASH_RESULT_OK) { + if (!flash_read(r_addr, &buf[0], Alignment)) { return 0; } @@ -820,7 +818,7 @@ size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data } #endif - if (spi_flash_write(r_addr, reinterpret_cast(&buf[0]), Alignment) != SPI_FLASH_RESULT_OK) { + if (!flash_write(r_addr, &buf[0], Alignment)) { return wbytes; } @@ -832,17 +830,17 @@ size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data while (size > 0) { auto len = std::min(size, sizeof(buf)); - auto wlen = alignAddress(len); + auto wlen = aligned(len); if (wlen != len) { - auto l_b = wlen - Alignment; - if (!flash_read(address + l_b, &buf[l_b], Alignment)) { + auto partial = wlen - Alignment; + if (!flash_read(address + partial, &buf[partial], Alignment)) { return written; } } memcpy(&buf[0], data, len); - if (!flash_write(address, &buf[0], wlen)) { + if (!flashWrite(address, reinterpret_cast(&buf[0]), wlen)) { return written; } @@ -889,23 +887,24 @@ bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) { size_t currentOffset = 0; if ((uintptr_t)data % 4 != 0) { - uint32_t alignedData[FLASH_PAGE_SIZE / sizeof(uint32_t)] __attribute__((aligned(4))); + constexpr size_t BufferSize { FLASH_PAGE_SIZE / sizeof(uint32_t) }; + alignas(alignof(uint32_t)) uint32_t buf[BufferSize]; size_t sizeLeft = sizeAligned; while (sizeLeft) { - size_t willCopy = std::min(sizeLeft, sizeof(alignedData)); + size_t willCopy = std::min(sizeLeft, BufferSize); // We read to our aligned buffer and then copy to data - if (!flashRead(address + currentOffset, alignedData, willCopy)) + if (!flashRead(address + currentOffset, &buf[0], willCopy)) { return false; } - memcpy(data + currentOffset, alignedData, willCopy); + memcpy(data + currentOffset, &buf[0], willCopy); sizeLeft -= willCopy; currentOffset += willCopy; } } else { // Pointer is properly aligned, so use aligned read - if (!flashRead(address, (uint32_t *)data, sizeAligned)) { + if (!flashRead(address, reinterpret_cast(data), sizeAligned)) { return false; } currentOffset = sizeAligned; @@ -913,10 +912,10 @@ bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) { if (currentOffset < size) { uint32_t tempData; - if (spi_flash_read(address + currentOffset, &tempData, 4) != SPI_FLASH_RESULT_OK) { + if (spi_flash_read(address + currentOffset, &tempData, sizeof(tempData)) != SPI_FLASH_RESULT_OK) { return false; } - memcpy((uint8_t *)data + currentOffset, &tempData, size - currentOffset); + memcpy(data + currentOffset, &tempData, size - currentOffset); } return true; From e3ce307e6c688aee9fc461e54f2575991652e741 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Mon, 27 Jun 2022 20:10:04 +0300 Subject: [PATCH 04/10] slight indirection for size numbers --- cores/esp8266/Esp.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cores/esp8266/Esp.cpp b/cores/esp8266/Esp.cpp index b5b563769d..1b78ca16b3 100644 --- a/cores/esp8266/Esp.cpp +++ b/cores/esp8266/Esp.cpp @@ -793,7 +793,8 @@ size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data return spi_flash_read(address, reinterpret_cast(data), size) == SPI_FLASH_RESULT_OK; }; - alignas(alignof(uint32_t)) uint8_t buf[FLASH_PAGE_SIZE]; + constexpr size_t BufferSize { FLASH_PAGE_SIZE }; + alignas(alignof(uint32_t)) uint8_t buf[BufferSize]; size_t written = 0; @@ -829,7 +830,7 @@ size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data } while (size > 0) { - auto len = std::min(size, sizeof(buf)); + auto len = std::min(size, BufferSize); auto wlen = aligned(len); if (wlen != len) { From d7425f321a062ed75b0e7210d5eb7184cc7ca25f Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Mon, 27 Jun 2022 22:10:04 +0300 Subject: [PATCH 05/10] test case failing with old version --- tests/device/test_spi_flash/test_spi_flash.ino | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/device/test_spi_flash/test_spi_flash.ino b/tests/device/test_spi_flash/test_spi_flash.ino index b1e18241f5..4436f96d47 100644 --- a/tests/device/test_spi_flash/test_spi_flash.ino +++ b/tests/device/test_spi_flash/test_spi_flash.ino @@ -1,5 +1,4 @@ #include -#include BS_ENV_DECLARE(); @@ -176,6 +175,11 @@ TEST_CASE("Unaligned page cross only", "[spi_flash]") CHECK(testFlash(0xa0000 + 255, 1, 2)); } +TEST_CASE("Unaligned page cross with unaligned size (#8588)", "[spi_flash]") +{ + CHECK(testFlash(0xa00b, 0, 202)); +} + void loop () { } From 349cbe08e495f8d903a8362bf85d04e67e1c48bc Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Mon, 27 Jun 2022 22:13:11 +0300 Subject: [PATCH 06/10] consistent `pointeralignment: right` --- cores/esp8266/Esp.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cores/esp8266/Esp.cpp b/cores/esp8266/Esp.cpp index 1b78ca16b3..0d9259dbbf 100644 --- a/cores/esp8266/Esp.cpp +++ b/cores/esp8266/Esp.cpp @@ -779,17 +779,17 @@ static bool isAlignedSize(size_t size) { return (size & (Alignment - 1)) == 0; } -static bool isAlignedPointer(const uint8_t* ptr) { +static bool isAlignedPointer(const uint8_t *ptr) { return isAlignedAddress(reinterpret_cast(ptr)); } size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data, size_t size) { - auto flash_write = [](uint32_t address, uint8_t* data, size_t size) { + auto flash_write = [](uint32_t address, uint8_t *data, size_t size) { return spi_flash_write(address, reinterpret_cast(data), size) == SPI_FLASH_RESULT_OK; }; - auto flash_read = [](uint32_t address, uint8_t* data, size_t size) { + auto flash_read = [](uint32_t address, uint8_t *data, size_t size) { return spi_flash_read(address, reinterpret_cast(data), size) == SPI_FLASH_RESULT_OK; }; @@ -841,7 +841,7 @@ size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data } memcpy(&buf[0], data, len); - if (!flashWrite(address, reinterpret_cast(&buf[0]), wlen)) { + if (!flashWrite(address, reinterpret_cast(&buf[0]), wlen)) { return written; } @@ -946,7 +946,7 @@ String EspClass::getSketchMD5() md5.begin(); while( lengthLeft > 0) { size_t readBytes = (lengthLeft < bufSize) ? lengthLeft : bufSize; - if (!flashRead(offset, reinterpret_cast(buf.get()), (readBytes + 3) & ~3)) { + if (!flashRead(offset, reinterpret_cast(buf.get()), (readBytes + 3) & ~3)) { return emptyString; } md5.add(buf.get(), readBytes); From d4dcbe7e86ea5b9a71134cd258b5c4729945d49f Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Mon, 27 Jun 2022 22:17:58 +0300 Subject: [PATCH 07/10] same names --- cores/esp8266/Esp.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/cores/esp8266/Esp.cpp b/cores/esp8266/Esp.cpp index 0d9259dbbf..4579938dbe 100644 --- a/cores/esp8266/Esp.cpp +++ b/cores/esp8266/Esp.cpp @@ -758,7 +758,7 @@ static SpiFlashOpResult spi_flash_write_puya(uint32_t offset, uint32_t *data, si } #endif -static constexpr uint32_t Alignment { 4 }; +static constexpr size_t Alignment { 4 }; template static T aligned(T value) { @@ -799,34 +799,34 @@ size_t EspClass::flashWriteUnalignedMemory(uint32_t address, const uint8_t *data size_t written = 0; if (!isAlignedAddress(address)) { - auto r_addr = alignBefore(address); - auto c_off = address - r_addr; - auto wbytes = std::min(Alignment - c_off, size); + auto before_address = alignBefore(address); + auto offset = address - before_address; + auto wlen = std::min(Alignment - offset, size); - if (!flash_read(r_addr, &buf[0], Alignment)) { + if (!flash_read(before_address, &buf[0], Alignment)) { return 0; } #if PUYA_SUPPORT if (getFlashChipVendorId() == SPI_FLASH_VENDOR_PUYA) { - for (size_t i = 0; i < wbytes ; ++i) { - buf[c_off + i] &= data[i]; + for (size_t i = 0; i < wlen ; ++i) { + buf[offset + i] &= data[i]; } } else { #endif - memcpy(&buf[c_off], data, wbytes); + memcpy(&buf[offset], data, wlen); #if PUYA_SUPPORT } #endif - if (!flash_write(r_addr, &buf[0], Alignment)) { - return wbytes; + if (!flash_write(before_address, &buf[0], Alignment)) { + return 0; } - address += wbytes; - data += wbytes; - written += wbytes; - size -= wbytes; + address += wlen; + data += wlen; + written += wlen; + size -= wlen; } while (size > 0) { From 2530e9198e19b4aec9611b04980945aa105cea89 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Mon, 27 Jun 2022 22:19:53 +0300 Subject: [PATCH 08/10] fix last page recalc --- cores/esp8266/Esp.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/cores/esp8266/Esp.cpp b/cores/esp8266/Esp.cpp index 4579938dbe..8cddd13353 100644 --- a/cores/esp8266/Esp.cpp +++ b/cores/esp8266/Esp.cpp @@ -697,7 +697,8 @@ static SpiFlashOpResult spi_flash_write_page_break(uint32_t offset, uint32_t *da return result; } - for (uint32_t page = 0; page < (size - size_page_aligned) / PageSize; ++page) { + const auto last_page = (size - size_page_aligned) / PageSize; + for (uint32_t page = 0; page < last_page; ++page) { if ((result = spi_flash_write(offset + size_page_aligned, data + (size_page_aligned >> 2), PageSize)) != SPI_FLASH_RESULT_OK) { return result; } From 8f198da21bd6e4aeef4fb748edfcb1bcc4a9dd28 Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Mon, 27 Jun 2022 22:25:07 +0300 Subject: [PATCH 09/10] all related issues --- tests/device/test_spi_flash/test_spi_flash.ino | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/device/test_spi_flash/test_spi_flash.ino b/tests/device/test_spi_flash/test_spi_flash.ino index 4436f96d47..16fa4a9750 100644 --- a/tests/device/test_spi_flash/test_spi_flash.ino +++ b/tests/device/test_spi_flash/test_spi_flash.ino @@ -175,7 +175,7 @@ TEST_CASE("Unaligned page cross only", "[spi_flash]") CHECK(testFlash(0xa0000 + 255, 1, 2)); } -TEST_CASE("Unaligned page cross with unaligned size (#8588)", "[spi_flash]") +TEST_CASE("Unaligned page cross with unaligned size (#8372, #8588, #8605)", "[spi_flash]") { CHECK(testFlash(0xa00b, 0, 202)); } From 1752272ef625d0cb0fd5a791b2603296d3649aea Mon Sep 17 00:00:00 2001 From: Maxim Prokhorov Date: Sun, 3 Jul 2022 00:39:27 +0300 Subject: [PATCH 10/10] we write everything --- cores/esp8266/Esp.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/cores/esp8266/Esp.h b/cores/esp8266/Esp.h index b25e02901e..1bb793ef7f 100644 --- a/cores/esp8266/Esp.h +++ b/cores/esp8266/Esp.h @@ -254,9 +254,10 @@ class EspClass { private: /** * @brief Write up to @a size bytes from @a data to flash at @a address - * This function takes case of unaligned memory access 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 + * This function handles all cases of unaligned memory acccess; when either + * address is not aligned, data pointer is not aligned or size is not a multiple of 4. + * User of this function should note that @a data will be copied into a buffer allocated on stack. + * * @param address address on flash where write should start * @param data input buffer * @param size amount of data