Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Handle unaligned address in EspClass::flashWrite u8 overload #8605

Merged
merged 12 commits into from
Jul 3, 2022
Merged

Handle unaligned address in EspClass::flashWrite u8 overload #8605

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a83c0cd
break pages
mcspr Jun 16, 2022
9a3a4fe
oops
mcspr Jun 25, 2022
553913c
words. fix page offset when writing
mcspr Jun 25, 2022
361b060
Merge remote-tracking branch 'origin/master' into unaligned-everything
mcspr Jun 27, 2022
e3ce307
slight indirection for size numbers
mcspr Jun 27, 2022
d7425f3
test case failing with old version
mcspr Jun 27, 2022
349cbe0
consistent `pointeralignment: right`
mcspr Jun 27, 2022
d4dcbe7
same names
mcspr Jun 27, 2022
2530e91
fix last page recalc
mcspr Jun 27, 2022
8f198da
all related issues
mcspr Jun 27, 2022
1752272
we write everything
mcspr Jul 2, 2022
78e236a
Merge remote-tracking branch 'origin/master' into unaligned-everything
mcspr Jul 2, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
291 changes: 132 additions & 159 deletions cores/esp8266/Esp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
mcspr marked this conversation as resolved.
Show resolved Hide resolved
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;
Expand Down Expand Up @@ -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));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe have a double check on what is returned by sizeof(buf) here?
I always find it tricky to see for myself whether the sizeof is returning the size of the array or the size of a pointer.
In this instance, it should be FLASH_PAGE_SIZE

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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 :)
But I suppose this could be std::size(buf); instead

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
Apart from that you still need to hover your mouse on those parts of the code in an IDE that does show you what it will do and that was my suggestion, to actually do that ;)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.
It does seem to work fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

puya supports looks fine as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

puya supports looks fine as well?

I don't have any Puya device here at hand, so have not tested it.
If you like, I can have another look at the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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) {
Expand Down
32 changes: 5 additions & 27 deletions cores/esp8266/Esp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down Expand Up @@ -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,
Expand All @@ -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;
Expand Down