gilpinheiro
commented
6 years ago I would also recommend defining a __attribute__((weak)) version of user_pre_init() that implements the current default positions for the {SYSTEM_PARTITION_RF_CAL, SYSTEM_PARTITION_SYSTEM_PARAMETER, and SYSTEM_PARTITION_PHY_DATA} partitions.
If that was in place, there would be no user code changes required, but you'd still be able to change these values by supplying a custom user_pre_init() in your project. As it is, this change will need to be implemented in every downstream project.
ustccw
Evert8266
eriksl
Thanks for the recent batch of work, I think a lot of these changes are very positive.
I wanted to give some feedback to help steer the work before the next release.
First, I think the addition of the partition-related functions is a positive - it was somewhat difficult to understand what was reserved in the past, and having a mechanism for checking this information is useful.
Based on the changes to the documentation, readme.md and new example code, I assumed that even though I don't use your OTA library I still needed to have all the corresponding entries in the system partition table (bootloader, ota1, ota2, rf_cal, phy_data, system_parameter_area).
The right thing to do seems to be to remove the {bootloader, ota1, ota2} entries from the table and things will work properly, but it took a long time to figure that out.
I'm guessing the documentation here is not yet complete - since you've been kind enough to share this code ahead of a formal release, I'm going to walk through this like a story, so you can see how an actual user approached this feature and the problems I had.
I use rboot, which reserves up the first 2 4k blocks or boot loader + boot configuration. The user_config.h says 'user can't modify this partition address, but can modify size' - so I thought it'd be fine to do so.
[reality: using anything other than size==0x1000 fails with "system_partition_table_regist fail"]
I do have two ota partitions, so I tried using the correct start/size values.
[reality: specifying any partition fails - I'm guessing because the partition start value isn't what was expected (which is documented, but why would you be able to change the size of the bootloader partition?)]
In implementing the pre-init function, I was confused by the 'map' parameter to 'system_partition_table_regist': Crawling through the examples gave me a bit of information, and I could extrapolate that I needed to set that value to '6' for my 4MB device since the values match my configuration. At this point I couldn't tell you what the 6 meant.
But the function failed on boot with 'mismatch map 6,spi_size_map 4'
[It is worth mentioning that having the error message be extra descriptive was really helpful in figuring out what was going on.]
Digging deeper, since I'm pretty familiar with the flash image layout, I guessed that this corresponds to to the 'flash_size_frequency' byte in the header (https://www.pushrate.com/blog/articles/esp8266_boot.html) - which is set by esptool when creating the image.
At this point I noticed the flash_size_map enum and realized that:
6 = FLASH_SIZE_32M_MAP_1024_1024
[It'd be worth making the 'map' parameter to the system_partition_table_regist either of type "enum flash_size_map' or reference that enumeration in the documentation. It took a long time to figure out what it was, and what the expected values were. Also in the examples/ code , the numeric value should be changed to the enumeration define instead, to make that connection clear.]
There isn't a good reference on how and when the flash_size_map can be set - in the documentation for 'system_get_flash_size_map' it says that 'Flash map depends on selection when compiling;' but as far as I can tell that is incorrect, as it only defined during image generation by esptool for the bootloader partition.
Digging in to the esptool sources, I found the '4M-c1' option which sets that flash_size to 6, which is what the table_regist function expected, and that all makes sense. I don't think this option to esptool is documented anywhere.
Now on booting the code continued past the flash_size check, but still fails with "system_partition_table_regist fail".
After much mucking about, I realize that if I remove the bootloader, and both OTA partitions, the device will boot again. i.e.: registering a partition table with only SYSTEM_PARTITION_RF_CAL, SYSTEM_PARTITION_SYSTEM_PARAMETER, and SYSTEM_PARTITION_PHY_DATA entries.
The initialization routines print some ominous messages ('boot not set', 'ota1 not set' and 'ota2 not set'), but things work, and it looks like the various parameters are placed as specified.
Summary:
Non-FOTA use of pre-init needs to be better documented. I like having one place for storing all the flash offset configurations, so I think this is a net positive.
Is user_rf_cal_sector_set(void) officially deprecated? If so, it should be noted somewhere in the release notes (when the release happens).
I'm not sure why 'map' is a parameter to 'system_partition_table_regist' in the first place. If you are creating a non-FOTA binary, you could previously flash the same image to any device and things would pretty much always work, since the configuration sectors are all defined relative to the --flash_size parameter to esptool (at flash time). Now you'll need to recompile the code to get it to work on different sized devices. I think the map parameter should be removed. It would be useful to do a runtime check of the partition table against flash_size to see if any partition exceeds the flash size of the device or has overlaps.