adafruit / Adafruit_nRF52_Bootloader

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

Increase NUM_FAT_BLOCKS to maximum as possible #129

Closed hathach closed 4 years ago

hathach commented 4 years ago

Is your feature request related to a problem? Please describe. Currently NUM_FAT_BLOCKS is hard-coded to 16MB. (0x8000 sectors) Is there any reason this is not set to a much larger number?

One reason to use a large (virtual) volume size:

_Originally posted by @henrygab in https://github.com/adafruit/Adafruit_nRF52_Bootloader/pull/128_

Describe the solution you'd like Increase the UF2 drive size to maximum within the specs and/or memory allowed by nRF52

hathach commented 4 years ago

More discussion by mmoskal

The ghostfat.c uses FAT16, so we could easily increase to 0xffff - 32M. To go higher we would need to change the sectors per cluster - not sure if that's difficult.

You make a very good point about concatenation - I actually didn't think about that when putting in the size limit.

FamilyID is meant as both board-id and MCU-id - for example, binaries generated from MakeCode Arcade would run on all NRF54840 boards provided they have the right bootloader settings (that define pinout, screen type, etc.). OTOH some other binaries may have pin configuration baked in, so they will only work on say 840 Feather. So it actually makes perfect sense for the bootloader to accept MCU-wide familyID and a specific boardID.

Generally, this can thought of as a sub-divsion tree of devices and bootloader accepts everything on one path from root to leaf. In this case I think we have 3 levels (all UF2 file - familyID==0, NRF52840, specific board), but it could be more if needed (eg different revisions of a specific board).

Just make sure to generate them randomly (eg with that CF2 website or your favorite RNG), to minimize risk of collision. We could move it out of README.md into a new file in the UF2 repo to keep track of all these for double-assurance.

_Originally posted by @mmoskal in https://github.com/adafruit/Adafruit_nRF52_Bootloader/pull/128_

mmoskal commented 4 years ago

BTW, I think it's actually 8000 not 0x8000, so 4M. Even more reason to increase.

henrygab commented 4 years ago

BTW, I think it's actually 8000 not 0x8000, so 4M. Even more reason to increase.

You're absolutely correct, it's even less than I thought:

https://github.com/adafruit/Adafruit_nRF52_Bootloader/blob/79a6a0c4f7f526704d5b47ffd4f6020265e01444/src/usb/uf2/uf2cfg.h#L3

Maximum sectors recommended to be 0x101E8

The [FAT12/16/32 specification](https://web.archive.org/web/20200227001937/http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/fatgen103.doc) indicates that the **_count of clusters_** must be in the range `[4085 .. 65524]`, aka `[0x0FF5 .. 0xFFF4]`. _((This makes sense if you consider the values used for invalid FAT entries.))_ [Brute force calculations](https://gist.github.com/henrygab/daa08d103a41bb349f922f36b9f9882b) show this range corresponds to a TotalSectors range of `[0x1019 .. 0x101F8]`. However, the above-linked specification also recommends to stay at least 16 away from the upper edge, to avoid bugs in implementations. This would give a recommended range of `[0x1029 .. 0x101E8]`. A final note: Where total sectors exceeds `0xFFFF`, it must be placed into `BPB_TotalSectors32`. If wanting to avoid that subtle change, then the final range would be `[0x1029 .. 0xFFFF]`. FYI, this is just shy of 32MB. Going larger, while staying as FAT16, would require either multiple sectors per cluster (making some of the code more complex), or moving to FAT32 (which would be fairly easy).


I could give a PR with minimum code changes + some static assertions to raise to this recommended maximum. However, I have do not have the ability to test the change properly. Would such a PR be desired?
hathach commented 4 years ago

BTW, I think it's actually 8000 not 0x8000, so 4M. Even more reason to increase.

You're absolutely correct, it's even less than I thought:

https://github.com/adafruit/Adafruit_nRF52_Bootloader/blob/79a6a0c4f7f526704d5b47ffd4f6020265e01444/src/usb/uf2/uf2cfg.h#L3

Maximum sectors recommended to be 0x101E8 I could give a PR with minimum code changes + some static assertions to raise to this recommended maximum. However, I have do not have the ability to test the change properly. Would such a PR be desired?

such an PR is more than welcome, I will be very appreciated, and can do the testing. the virtual disk capacity is only to trick OS for us to copy a large uf2 file, hopefully it is safe enough. Though you should wait for the PR #128 to be merged first, or branching from that, Since it made significant changes to ghotsfat which will likely to cause code conflict.

henrygab commented 4 years ago

Now open: PR #132