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.

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

Fix flash with more than 512kb #214

Merged
merged 4 commits into from
Aug 4, 2021
Merged

Fix flash with more than 512kb #214

merged 4 commits into from
Aug 4, 2021

Conversation

hathach
Copy link
Member

@hathach hathach commented Aug 4, 2021

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 Arduino, all boards is served with 28KB for storing bonding information.
  • For Circuitpython, there seems to be 3 section FLASH_BLE_CONFIG (32KB), FLASH_NVM (8KB), FLASH_FATFS (can be 0 on qspi flash dev).

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 #213 (comment)

@hathach hathach merged commit 142ed60 into master Aug 4, 2021
@hathach hathach deleted the fix-flash-51kb branch August 4, 2021 11:02
@tannewt
Copy link
Member

tannewt commented Aug 4, 2021

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
Copy link
Member Author

hathach commented Aug 5, 2021

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
Copy link
Member

tannewt commented Aug 5, 2021

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
Copy link
Member Author

hathach commented Aug 6, 2021

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
Copy link
Member

tannewt commented Aug 6, 2021

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to flash UF2 where flash is >512kb
2 participants