-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Handle unaligned address in EspClass::flashWrite u8 overload #8605
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
a83c0cd
9a3a4fe
553913c
361b060
e3ce307
d7425f3
349cbe0
d4dcbe7
2530e91
8f198da
1752272
78e236a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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)) { | ||
mcspr marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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<uint32_t>(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<uint32_t*>(data), size); | ||
}; | ||
|
||
auto flash_read = [](uint32_t address, uint8_t* data, size_t size) { | ||
return spi_flash_read(address, reinterpret_cast<uint32_t*>(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<uint32_t*>(&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<uint32_t*>(&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)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe have a double check on what is returned by There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No point guessing when IDE / language server in any editor will display this b/c it's a compile time value :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well my "code review IDE" (also called "brain") is lacking this feature. ;) It is only highlighting code which looks like code I have spent numerous hours on to fix in the past where I missed such a mistake. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which is why I mentioned the IDE as a tool to help with guesses, since we don't have this issue here. Review with a real test case might've helped too, would you mind re-checking with some ESPeasy builds if it's possible? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I did just now test ESPEasy with the code changes of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. puya supports looks fine as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't have any Puya device here at hand, so have not tested it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please do. |
||
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<uint32_t *>(data), size); | ||
result = spi_flash_write_puya(address, const_cast<uint32_t *>(data), size); | ||
} | ||
else | ||
#endif // PUYA_SUPPORT | ||
#endif | ||
{ | ||
rc = spi_flash_write(address, const_cast<uint32_t *>(data), size); | ||
result = spi_flash_write_page_break(address, const_cast<uint32_t *>(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<const uint32_t *>(data), size) == size; | ||
} | ||
|
||
return true; | ||
return false; | ||
} | ||
|
||
bool EspClass::flashRead(uint32_t address, uint8_t *data, size_t size) { | ||
|
Uh oh!
There was an error while loading. Please reload this page.