search

adafruit / Adafruit_nRF52_Bootloader

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

Ignore uf2 with size too large #215

Open hathach opened 3 years ago

hathach commented 3 years ago

Is your feature request related to a problem? Please describe. Writing the uf2 file which size larger than supported will cause partial flashing, and the end part of application is not flashed.

Describe the solution you'd like We should pre-calculate the end address based on starting address + total block to actively reject/ignore the uf2 instead.

carlosperate commented 3 years ago

The UF2 format can also contain metadata, which I assume counts on the total block calculation and the bootloader could be rejecting a file that does fit?

hathach commented 3 years ago

@carlosperate can you post an example of such uf2 image with the layout breakdown

carlosperate commented 3 years ago

Oh, I haven't really used the UF2 format all that much (other than flashing files in this format), but this was my understanding from the spec: https://github.com/microsoft/uf2#flags

The file could have blocks with the not main flash flag, which can be used for things like MakeCode to store the programme metadata. A single UF2 file can also contain multiple concatenated files for different payloads.

Having a quick look at the files produced by https://makecode.adafruit.com it doesn't look like it's doing it at the moment, so I'm not sure what would be a good way to find an example of this configuration. I think maybe MakeCode could be doing something similar to what it used to do for the micro:bit before. It'd always try to put the metadata in flash and only resort to these other methods when it doesn't fit (so you can restore a project by reading back the flash). But I couldn't easily get it to use the not main flash flag.

Of course, something like MakeCode would normally not target a bootloader, and there isn't a lot of scenarios where this could be a problem, but I thought it was worth mentioning to keep this in mind and/or document it if not supported.

hathach commented 3 years ago

the not-flash meta data won't be counted as part of the total num block (different from the total file size of uf2), it is probably counted separated. I didn't look at this, but you could probably confirm this by looking at the uf2conv conversion script in the repo. Otherwise, it would be an bug of the script

carlosperate commented 3 years ago

Oh yeah, uf2conv.py is not really using that flag to create a UF2 file. Mostly I assume it might be because it doesn't really have anything useful to write to it: https://github.com/microsoft/uf2/blob/master/utils/uf2conv.py#L129-L134

the not-flash meta data won't be counted as part of the total num block (different from the total file size of uf2), it is probably counted separated.

Are you sure? I guess it's not really specified in the README, but things like this makes me think it should count:

The total number of blocks in the file and the sequential block number make it easy for the bootloader to detect that all blocks have been transferred

Also, the File Container section indicates:

The fields blockNo and numBlocks refer to the entire UF2 file, not the current file. https://github.com/microsoft/uf2#file-containers

hathach commented 3 years ago

Are you sure? I guess it's not really specified in the README, but things like this makes me think it should count:

It is my guess, you should ask this on uf2 repo for clarification. Overall, I don't think we need to worry about this until there is an image generated from the uv2conv from uf2 repo that cause the issue. It is best not to write more code than it should be.

carlosperate commented 3 years ago

It is best not to write more code than it should be.

Totally agree 👍 Maybe it could be worth documenting the incompatibility in case it catches somebody by surprised, but I'd be surprised if it ever becomes a problem.