Skip to content

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

Merged
thelsing merged 1 commit into
thelsing:masterfrom
AODtorusan:master
Apr 13, 2022
Merged

Esp32Platform reinitializing EEPROM multiple times causing it to forget staged EEPROM changes#196
thelsing merged 1 commit into
thelsing:masterfrom
AODtorusan:master

Conversation

@AODtorusan
Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Owner

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

@thelsing
Copy link
Copy Markdown
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.

3 participants