search

adafruit / Adafruit_nRF52_Bootloader

USB-enabled bootloaders for the nRF52 BLE SoC chips
MIT License
438 stars 393 forks source link

Fix flash with more than 512kb #214

Closed hathach closed 3 years ago

hathach commented 3 years ago

fix #213 

Write addr = 0x000ACD00, block = 2142 (2142 of 2191)
Write addr = 0x000ACE00, block = 2143 (2143 of 2191)
Write addr = 0x000ACF00, block = 2144 (2144 of 2191)

I found the issue, the bootloader is limited its size to 512KB since it reserved the rest for internal file system ( for all boards), since the very first implementation of the bootloader.

For that I think we will increase reserved size to 40KB. The overflow to fatfs should be checked by circuitpyhon linker, just try DEBUG=1 with boards internal flash, it overflows and won't linked which is good.

_Originally posted by @hathach in https://github.com/adafruit/Adafruit_nRF52_Bootloader/issues/213#issuecomment-892501613_

tannewt commented 3 years ago

I'd expect the bootloader to only protect itself. Protecting user data is the job of the UF2 when it specifies what blocks to write. We've actually taken advantage of this behavior on samd because current.uf2 will include the fatfs data and make it possible to share it.

The CP linker script should already fail when not enough room is left after the fatfs partition is set aside.

hathach commented 3 years ago

yeah, you are right, but it is a bit more complicated and originally from existing nordic sdk code with dual bank (which our first version based on). And It is expanded to allow in-place update bootloader + sd without touching the user area region and/or avoid to write to user firmware if possible by using the highest flash as temporarily until whole image is receive. https://github.com/adafruit/Adafruit_nRF52_Bootloader/blob/master/src/usb/uf2/ghostfat.c#L449

samd upgrade is a bit different, by having the user application to write/update bootloader flash, then destroy itself to prevent re-updating. Which is more generic (which is currently used by tinyuf2 for most ports).

We can of course just define it as 0x0 and leave that to application linker, though it could take more effort for refactoring & testing. Which we could do it later, I could open an issue for that If you insist.

tannewt commented 3 years ago

What does this have to do with updating the bootloader? This should be a simpler case of updating a user application. What is the purpose of protecting app data? I'd only expect the bootloader and MBR to be protected.

Am I correct thinking it was partially flashing before failing? That would explain how part of our image worked but the end of it wasn't included. Perhaps the flash region needs to be validated at the start before any erase.

hathach commented 3 years ago

What does this have to do with updating the bootloader? This should be a simpler case of updating a user application. What is the purpose of protecting app data? I'd only expect the bootloader and MBR to be protected.

For this issue, it doesn't. but I just realized, circuipython use 40KB for bonding and nvm (user data) more than currently reserved 28KB. Therefore bootloader needs to adjust to increase that for not overwriting those region when updating itself. The reason is mentioned in above comment.

Am I correct thinking it was partially flashing before failing? That would explain how part of our image worked but the end of it wasn't included. Perhaps the flash region needs to be validated at the start before any erase.

Yes, it failed when it goes over 512 KB, which is fixed now, originally, it is the capped limit of user application due to internal filesystem when the very first port of circuitpython for nrf52840 and nrf52832. It is updated to remove that in https://github.com/adafruit/Adafruit_nRF52_Bootloader/pull/214/files#diff-d7b2438df61e4c16edd0db62cd50ddddb0e72b6984050d42ea9d7ae0bb94738bR21

Yes, we can pre-calculate the end address based on the start + total block to actively reject the uf2 instead of writing part of it. I just opened an issue for this, will fix it later on #215

tannewt commented 3 years ago

Updating the bootloader is pretty rare. I think it's ok to wipe user data in that process. Other bootloaders don't make any promise about it.

Thanks for opening #215!