fixing OTA write up to SPI_FLASH_SEC_SIZE margins (IDFGH-11311) #12460
Conversation
👋 Welcome kohait00, thank you for your first contribution to 📘 Please check Contributions Guide for the contribution checklist, information regarding code and documentation style, testing and other topics. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for espressif/esp-idf project. Pull request review and merge process you can expectEspressif develops the ESP-IDF project in an internal repository (Gitlab). We do welcome contributions in the form of bug reports, feature requests and pull requests via this public GitHub repository.
|
github-actions
bot
changed the title
Sorry but I fail to understand the fix here. If the data goes above the specified partition size then erase is likely to fail. Could you please create maybe a small application with restricted flash size and partition table to reproduce the problem? That will help to understand this scenario.
Wouldn't this cause a problem for a case where the partition size is 8K and input arguments are offset = 4K, size = 4K? |
|
I try to show it by a printf output.. esp_ota_ops.c:211 (esp_ota_write())
///////////////////////////////// ///////////////////////////////// SPIFFS Writing partition: type 1, subtype 130, offset 0x002e0000 W (29637) esp_ota_ops: OTA image wrote_size = 0, size = 100, wrote_size+size = 100 (first 0x00, last 0x00) ... W (92717) esp_ota_ops: OTA image wrote_size = 11F6FC, size = 100, wrote_size+size = 11F7FC (first 0x11F, last 0x11F) ///////////////////////////////// as you see, the writes thas "just fill a sector to the end" alsways already end up erasing already the next sector too.. imagine you write from beginning of a sector just a sector full. the following sector gets deleted |
|
so the fix turns out to be even simpler.. which produces first/last sector ids for just the affected data. I committed a new version of the fix a necessary precondition check.. RFC.. |
|
Okay, thanks for providing more information. To summarize, OTA update would fail if Left one comment, please squash the commits post update. Thanks. |
espressif-bot
added
Status: In Progress
kohait00
force-pushed
the
ota_ops_fix_master
branch
from
0c5b782
to
9a8796d
Compare
|
corrected and squashed |
| if(size == 0) | ||
| { | ||
| ESP_LOGD(TAG, "write data size is 0"); | ||
| return ESP_OK; | ||
| } |
There was a problem hiding this comment.
| if(size == 0) | |
| { | |
| ESP_LOGD(TAG, "write data size is 0"); | |
| return ESP_OK; | |
| } | |
| if (size == 0) { | |
| ESP_LOGD(TAG, "write data size is 0"); | |
| return ESP_OK; | |
| } |
|
@kohait00 Also, please check the commit title and the description once. Right now, it still points to a squashed summary of different commit. Otherwise, LGTM! |
…xt sector anymore
kohait00
force-pushed
the
ota_ops_fix_master
branch
from
9a8796d
to
61699ba
Compare
Exactly that |
kohait00
changed the title
|
sha=61699ba6d67427679132528250e17befcd141002 |
espressif-bot
added
Status: Done
…ed size OTA update used to fail if `firmware_size == partition_size`, because the code was trying to erase one additional sector beyond the space reserved for the firmware partition. This commit fixes the problem and OTA update can work if the firmware size exactly matches the allocated partition size. Closes espressif#12460
…ed size OTA update used to fail if `firmware_size == partition_size`, because the code was trying to erase one additional sector beyond the space reserved for the firmware partition. This commit fixes the problem and OTA update can work if the firmware size exactly matches the allocated partition size. Closes #12460
…ed size OTA update used to fail if `firmware_size == partition_size`, because the code was trying to erase one additional sector beyond the space reserved for the firmware partition. This commit fixes the problem and OTA update can work if the firmware size exactly matches the allocated partition size. Closes #12460
…ed size OTA update used to fail if `firmware_size == partition_size`, because the code was trying to erase one additional sector beyond the space reserved for the firmware partition. This commit fixes the problem and OTA update can work if the firmware size exactly matches the allocated partition size. Closes #12460
The size of partition of type APP should be multiple of 4 KB. Partition generation tool now make this as a mandatory requirement. This is minimum flash erase size. If the size of the APP type partition is not aligned to 4 KB then the last erase operation could go beyond the allocated partition and hence may fail. This issue would only be observed when the firmware size grows very close to the allocated partition size, and hence causing the OTA update to fail. For already deployed devices on-field with the size of APP partition not aligned to flash sector boundary, it is best to ensure that firmware size always remains within the lower 4 KB boundary of the total allocated space. While migrating to ESP-IDF 5.3 release, partition table for an existing project can be adjusted accordingly for the build to succeed. Found during discussion in #12460
…ed size OTA update used to fail if `firmware_size == partition_size`, because the code was trying to erase one additional sector beyond the space reserved for the firmware partition. This commit fixes the problem and OTA update can work if the firmware size exactly matches the allocated partition size. Closes #12460
…ed size OTA update used to fail if `firmware_size == partition_size`, because the code was trying to erase one additional sector beyond the space reserved for the firmware partition. This commit fixes the problem and OTA update can work if the firmware size exactly matches the allocated partition size. Closes espressif#12460
when OTA update is performed on a partition that extends to the very last byte of the flash, then ...
esp_partition_erase_range, called by esp_ota_write(), will get to erase the sector past flash size which it will decline with an error.
example:
120 sectors to write, (0 to 119), sector 120 is just outside the flash
writing the last bytes <= SPI_FLASH_SEC_SIZE will produce this:
first_sector will be 119 (already erased in previous write operations, has a ramainder to write)
last_sector will be 120 (just outside flash)
the fix determines this situation (condition could probably even better be
if((it->wrote_size + size) >= it->part->size)
and repositions the last_sector to really be the last, so when it is compared to be == first_sector, it will be ignored..
I also believe, that esp_partition_erase_range has a latent issue
...
esp_err_t esp_partition_erase_range(const esp_partition_t *partition,
size_t offset, size_t size)
{
assert(partition != NULL);
if (offset > partition->size) { //<<----------- should be >=
return ESP_ERR_INVALID_ARG;
}
if (offset + size > partition->size) { //<<----------- should be >=
return ESP_ERR_INVALID_SIZE;
}
...
but I probably didnt get all the internal info right...