[TW#25581] Partition Table Feature

[gilpinheiro]

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.

[gilpinheiro]

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]

@gilpinheiro
very thanks for your detailed descriptions and recommendations.

1. user_rf_cal_sector_set officially is recommended for init phy freq trace and partition table. we would mark it to documents.
2. map parameter is defined at compile time depended on compile option STEP 5, other than depended on flash time --flash_size.

• System would check partition table starting address according to SPI_FLASH_SIZE_MAP and check partition table overlap.
• System would also check whether partition table exceeds the flash size when called system_partition_table_regist.

3. attribute((weak)) for user_pre_init() is a nice idea, would mark it to inner.

[ustccw]

@FayeY please addition titile to #TW25581

[ustccw]

Closing due to inactivity. Please feel free to re-open or file a new issue if you have any more questions.

[Evert8266]

Using Download tool v3.6.4 with the latest firmware (v1.7) I have the same error trying to flash a esp 01 with 4M chip.

[eriksl]

I have experienced most of the above (and works now).

The important thing to remember that rboot does, according to the SDK NOT implement OTA. The OTA handling is handled completely by rboot itself, the SDK code has no role in it. So we should that fact and mark it as "simple" non-OTA image.

Indeed, don't create "boot" or "OTA" partitions and it will work. Create your "rboot" partitions as "user" partitions.

[eriksl]

I also noted when switching to the current v3 SDK:

• user_rf_pre_init is NO LONGER called (contrary what is said above). You can set a variable in your function and check it later to see.
• user_rf_cal_sector_set is NO LONGER called (contrary what is said above). You can set a variable in your function and check it later to see.
• IRAM space has shrinked considerately, I lost about 450 bytes.
• "heap free" has increased by 16k. I suspect the "use IRAM for DRAM" function is enabled by default (contrary to the documentation), let's see how we switch that off, it would be nice to have a function to check IRAM usage
• instead of using IRAM for DRAM, I'd rather use IRAM flash cache for IRAM flash code (i.e. the 32 k part that is always almost full because SDK code uses most of it)