Rewrite PUYA patch to be more universal and mem friendly.

[TD-er]

The (current) patch:

1. uses a static 4KB buffer, which means everyone using the patch has 4KB less heap, including those users who do not have a puya chip. Instead, a dynamically allocated buffer should be used.
2. evaluates the first if condition every single call of the function, although only the first one after boot actually does anything. Instead of a private static, a global could be used that is set elsewhere only once at bootup, and then if condition is no longer needed. Alternatively, The ESPClass could get a new data member (i.e.: instead of the global flag) which is set on construction, and a method to return its value.
3. calculates the flash_chip_id condition in the second if every single call of the function, which makes no sense, because it can't ever change at runtime. Instead, the condition should be fully calculated once and the result stored in a flag, which should then be checked.
4. detects the condition based on flash_chip_id, which isn't very robust, as has been shown with the flash chip size. Instead, a general purpose detection approach should be implemented that transparently covers other potential brands and manufacturers that have the same quirk, should they appear.
   Example: on first bootup after flashing, choose a sector, e.g.: within the update area of the flash layout., preferably one that is unlikely to be used. Do a write test and read back sequence known to behave differently with puya chips. If puya is detected, set a quirk flag somewhere. On all successive bootups, detect that the quirk flag is set, so don't do the test again. Use the quirk flag in the write function to decide whether the additional logic is done or not.

Given the above, the patch won't be accepted as-is into our core. There have been several internal discussions on exactly what to do, but given that the patch is available, there have been, and still are, higher priorities, e.g.: figuring out how to ease IRAM usage.

There have been a lot of comments from many users in this discussion, and I see the patch is even being applied elsewhere, but I still don't see a PR here. Instead of wondering what will happen, I invite those involved to make a PR with the proposed patch, and implement at least points 1-3 above on top of it. If you come up with a good solution for point 4, even better.

Originally posted by @devyte in #4061 (comment)

Will have a look at it tomorrow (well, after sleep)

[devyte]

Note: From #4061 :
Patch v3 in use by ESPEasy, which needs rework per points quoted above:
https://github.com/letscontrolit/ESPEasy/blob/mega/patches/puya_v3.patch#L8

[uzi18]

@devyte in #4061 we found it is only true for flash_chip_id 0x146085 PUYA,
even no other size of flash were found/reported in issue, other brands worked.

[mrWheel]

@devyte
Good to know, but I do a lot of development for various projects that use the ESP-01 and so .. I'm stuck with the PUYA chip problems.....

[TD-er]

Where is a good place to check for PUYA chip at boot? (and then allocate read buffer for the write function)
Inside the getFlashChipId() function?
Inside the flashRead() function is also possible, but then we're checking it for every read, which is silly.

By the way, this is a simple test with the dynamic allocation active in a simulated setup (faked PUYA detection) :

core 2.5.0 PUYA patch, no puya chip present:

Description	#calls	call/sec	min (ms)	Avg (ms)	max (ms)
Save File	4	0.25	34.755	45.264	67.620

• Free Mem: 16168

core 2.5.0 PUYA patch, Faked Puya detect:

Description	#calls	call/sec	min (ms)	Avg (ms)	max (ms)
Save File	2	0.04	41.332	57.544	73.756

• Free Mem: 11560

See the code (Work-in-progress): https://github.com/TD-er/Arduino/tree/bugfix/5493_puya_patch

Edit:
See TD-er@715fc99
Check for PUYA chip in call for getFlashChipId()
This will only be done once and the result of the get function is also cached.
I assume it will not change anymore, right?

[devyte]

Do you mean the simple check similar to what's in the patch, or the general check I described in point 4?

[TD-er]

@devyte The moment to allocate the 4k buffer.
When allocated as soon as possible, it will give the least amount of fragmentation. (and the most chance of success to allocate the block)

Point 1 - 3 should be done by the patch as far as I have it.

My idea to detect PUYA write behavior is to check for the first 4 bytes after the sketch to see if there is the word "PUYA" written.
In order to write it, I would like to write the NOT representation of "PUYA" in ASCII and then again the text "PUYA".
If it is a PUYA chip, it will contain the text "PUYA". If it is a normal flash chip, it will contain only zeroes.
If not tested, it will contain 0xFF.

Or am I missing something in my thought experiment?

[devyte]

It sounds reasonable. However, please investigate the flash mem layout and start/stop addresses thoroughly (SPIFFS/no SPIFFS and update area, btw from your description I understand you're using the very start of the update area), and keep in mind that you have to handle an entire 4KB flash sector when writing ~(PUYA)PUYA.
I suggest implementing a struct with 2 const data members that are const char uint8_t[4]. Then you write the struct as a whole to the flash address. If there are additional 1-time values to add in the future, the struct and logic could then be extended.

[TD-er]

Why must I write an entire flash sector?
The check does not work when it will do an erase like the normal write procedure.
The whole issue with SPIFFS not working was related to low-level writing without erasing.
Or can write calls at low level only write an entire sector at a time?

[devyte]

I didn't say you have to write the entire sector, I said you have to handle it. I don't know what goes on at low level for puya or non puya chips. But what I was getting at are two things:

• there could be data at the high end of the sector, e.g.: from a new sketch coming OTA. Care that you don't impact that.
• if you do a single malloc of 4KB at bootup, that heap is used up whether you do flash writes or not, and if you do a malloc inside the if in the patch code once per write, it could potentially fail when oom or due to mem fragmentation. Each has pros and cons.

[TD-er]

I will indeed look into the flash structure and right now it looks more complicated than expected.

And indeed 4 kB used even when no writes are needed is a waste.
I will think of a way to use less memory.
Would it be OK to preset some kind of 'flash profile' which determines the allocation strategy?

Only thing is, I had trouble finding my "PUYA" nodes to test.
I should have a bag full of them somewhere...

[TD-er]

One other thing I notice.
There isn't a while loop in there and also no check for the size.

What guarantee do we have the write cycle is always 4 kB max?

I've been reading the datasheet.

• A page is said to be 256 bytes. So why not write the data in chunks of 256 bytes?
• spiffs_hal uses a buffer of 512 bytes to correct unaligned data to be written.
• Updater class uses 256 as a fall-back when heap memory is low:

Arduino/cores/esp8266/Updater.cpp

Lines 142 to 147 in d8acfff

	if (ESP.getFreeHeap() > 2 * FLASH_SECTOR_SIZE) {
	_bufferSize = FLASH_SECTOR_SIZE;
	} else {
	_bufferSize = 256;
	}
	_buffer = new uint8_t[_bufferSize];

So is it OK to set the PUYA buffer size as a define with default set to 512?