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

Esp32Platform reinitializing EEPROM multiple times causing it to forget staged EEPROM changes #196

Merged
merged 1 commit into from
Apr 13, 2022

Conversation

AODtorusan
Copy link
Contributor

For the ESP32, you cannot call EEPROM.begin(size) multiple times, and get the same data buffer back. Each call re-creates the in-memory data array mirror of the flash contents. The ESP32 arduino library the code is clearing EEPROM emulation data array is here;
https://github.com/espressif/arduino-esp32/blob/02c3ec01cca90d3b84398791656599d8cb3bb668/libraries/EEPROM/src/EEPROM.cpp#L128

The result is that when loading a program from ETS doesn't work because it calls Esp32Platform::getEepromBuffer() several times (calling EEPROM.begin(size)), causing it for forget all previously changed bytes.

Ideally we should could use the .length() to determine if the EEPROM was initialized properly.
However this was broken until quite recently espressif/arduino-esp32#5775 (requiring v2.0.1+), therefore I just put a simple guard around the begin.

@Ing-Dom
Copy link
Collaborator

Ing-Dom commented Apr 12, 2022

this was already discussed and adressed here:
https://knx-user-forum.de/forum/projektforen/openknx/1749751-problem-mit-neuen-thelsing-knx-stack-versionen-und-esp32-esp8266

ESP (not only ESP32) Plattform should also be fixed.

@AODtorusan
Copy link
Contributor Author

Ah, ok, good to know.
It does look a bit nicer with the null pointer check instead of a new member variable.
I will need to dig out an esp8266 to verify it. Once done I'll create a new PR for both ESP platforms.

…times causing it to forget staged EEPROM changes
@AODtorusan
Copy link
Contributor Author

Changed the check for the EEPROM initialization to a null pointer check.
Verified that a full download works on both esp32 & esp8266 with a full download on a new physical address.

@Ing-Dom
Copy link
Collaborator

Ing-Dom commented Apr 13, 2022

hmmmm. wouldn't you think using the arg size is the better option (KNX_FLASH_SIZE could also be undef) ?
I see no reason changing that to the define, do you?

@thelsing thelsing merged commit b0153de into thelsing:master Apr 13, 2022
@thelsing
Copy link
Owner

Merged. The argument can be changed with a different PR as this one fixes a critical issue for esp*

@thelsing
Copy link
Owner

You're right. I only noticed this after the next PR. Sorry.

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.

None yet

3 participants