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

Resolve flash address issues with SDK v3.0.0 #8755

Merged
merged 11 commits into from
Dec 19, 2022

Conversation

mhightower83
Copy link
Contributor

@mhightower83 mhightower83 commented Dec 12, 2022

Fix EEPROM vs RF_CAL flash address conflict. The EEPROM address and RF_CAL address were the same.

Add support for Flash size: "Mapping defined by Hardware and Sketch" aka autoconfig

Change at_partition_table from dynamic to static.

When the Arduino IDE flash size selected is correct, the flash sector positions for EEPROM, RF_CAL, and SYSTEM_PARAMETER are kept constant across SDK selections; however, when you change SDK versions, you should erase WiFi settings. The file system should be portable across SDK selections; however, if you have valued data backup before changing. Data may be lost if the flash size setting and device mismatch.

Status: Ready

Fix EEPROM vs RF_CAL flash address conflict. The EEPROM address and
RF_CAL address were the same.

Add support for Flash size: "Mapping defined by Hardware and Sketch"

Change at_partition_table static from dynamic to static.
@mcspr
Copy link
Collaborator

mcspr commented Dec 13, 2022

Also missing / may be wrong

  • IDE menu Erase Flash > Sketch + WiFi Settings
  • EspClass::eraseConfig()
  • all non-auto-size .ld files

Embedding anything in .ld might be detrimental with simultaneous support of SDK2 and SDK3

As a hypothetical solution, what about restricting SDK3 to the auto-size flash mode only.
ref. #8595, everything until empty space between the app and fs is handled as pairs of {name, type, size}, allocated backwards from the end of the flash (to reduce boilerplate and avoid us remembering hard addrs)

EEPROM (optional, custom size), FS (optional, custom size) are user modifiable either through an override or something that appends things into the partition table object. Then it is system_partition_get_item or something custom for LittleFS, EEPROM, anything else storing data at fixed addresses

Changed set_pll() to mmu_set_pll() and made available for debug builds
and other settings where required.

Provide more checks and feedback in the debug builds and
trim code for production.
@mhightower83
Copy link
Contributor Author

mhightower83 commented Dec 13, 2022

I think things are covered with "non-auto-size .ld files"; however, I plan to change RF_CAL and parameters to be the last 4 sectors of the flash. Get rid of the EEPROM relative approach. That will be more consistent with ESP.eraseConfig(). I'll keep the PHY_DATA read spoofing over the EEPROM. Of course, my thinking may change when I go to do it.

Technically, because we have to tell the SDK the position of PHY_DATA, we tell the SDK that PHY_DATA is the address of the EEPROM. To prevent conflict detection, we never tell the SDK the address of the EEPROM. During the short spoofing window, all reads for 128 bytes receive PHY_DATA. This is similar to how it was done for earlier SDKs; however, for those, PHY_DATA and RF_CAL shared the same sector location.

With auto-size flash mode the data in PROGMEM is used before flashinit() has run. For user_pre_init, it runs before any C runtime code runs and before flashinit() in user_init() and other "C++" initializations. I think we will need a version that can be called from user_pre_init and __flashindex will need to move to the .noinit section. Simple "C" runtime init for variables in sections .bss and .data has finished prior to calling of user_pre_init. I think structures are done with "ctors". I am confused about global structures.

And, panic() will need to be dealt with differently.

@mhightower83 mhightower83 marked this pull request as ready for review December 15, 2022 18:04
@d-a-v d-a-v added this to the 3.1 milestone Dec 16, 2022
{ SYSTEM_PARTITION_SYSTEM_PARAMETER, system_parameter, 0x3000 }, // type 6
};
_at_partition_table = at_partition_table;
_at_partition_table_sz = sizeof(at_partition_table) / sizeof(at_partition_table[0]);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
_at_partition_table_sz = sizeof(at_partition_table) / sizeof(at_partition_table[0]);
_at_partition_table_sz = std::size(at_partition_table);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this will happen at C++ compile time and we can count on not needing C++ runtime init.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, number of elements is part of the type info.
template <typename T, size_t Size> constexpr size_t size(const T (&)[Size]) { return Size; }

cores/esp8266/core_esp8266_main.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_main.cpp Outdated Show resolved Hide resolved
cores/esp8266/core_esp8266_main.cpp Outdated Show resolved Hide resolved
// .bin image.
#endif

#if FLASH_MAP_SUPPORT & defined(DEBUG_ESP_PORT)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#if FLASH_MAP_SUPPORT & defined(DEBUG_ESP_PORT)
#if FLASH_MAP_SUPPORT && defined(DEBUG_ESP_PORT)

(would also suggest moving all #... to the beginning of line? things are pretty dense here)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have gone back and forth on this. I kept getting lost with all the #ifs in column one. The density of the #ifs is the reason I started indenting the innermost #ifs to reduce the visual disruption to a block of "C" code. When nested the outer #if are at the beginning of the line. I don't know if doing this is a style violation.

@mcspr mcspr merged commit 59b5bba into esp8266:master Dec 19, 2022
@mhightower83 mhightower83 deleted the pr-305-rf-cal-phy-data branch December 22, 2022 18:06
@amrlsayed
Copy link

Hello, sorry for commenting on an old thread, but I thought your comment will help me out.
I have a project using the SDK 3.0.1 just before this merge, and I have the problem that the EEPROM is overwritten randomly that I am not able to reproduce.
My question is does this issue relate to that ? as the EEPROM address and RF_CAL address were the same, so may be RF_CAL data overwrites EEPROM data ?
Thanks

@mhightower83
Copy link
Contributor Author

... EEPROM is overwritten randomly that I am not able to reproduce.

Yes, EEPROM writes may write over RF_CAL data. And SDK RF_CAL writes may write over EEPROM data. I suspect/expect the SDK implements wear leveling on their writes. This means the location of the writing might not be the same each time. This could contribute to the reproducibility problem.

hasenradball pushed a commit to hasenradball/Arduino that referenced this pull request Nov 18, 2024
* Reslove flash address issues with SDK v3.0.0

Fix EEPROM vs RF_CAL flash address conflict. The EEPROM address and
RF_CAL address were the same.

Add support for Flash size: "Mapping defined by Hardware and Sketch"

Change at_partition_table static from dynamic to static.

* Cleanup and improve comments

* Improve flash size and partition error reporting/indication

Changed set_pll() to mmu_set_pll() and made available for debug builds
and other settings where required.

Provide more checks and feedback in the debug builds and
trim code for production.

* Now supports FLASH_MAP_SUPPORT with SDKs v3.0

RF_CAL and system_parameter always occupy the last 5
sectors of flash memory.

* cleanup and refactoring
comment cleanup

* Add more build issolation when including flash_hal.h

* Improve details for autoconfig fail.

* requested changes
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.

4 participants