Rewrite PUYA patch to be more universal and mem friendly. #5493
Comments
|
Note: From #4061 : |
|
@devyte |
Applied the patch to get the starting point as described in esp8266#5493
|
Where is a good place to check for PUYA chip at boot? (and then allocate read buffer for the write function) 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:
core 2.5.0 PUYA patch, Faked Puya detect:
See the code (Work-in-progress): https://github.com/TD-er/Arduino/tree/bugfix/5493_puya_patch Edit: |
|
Do you mean the simple check similar to what's in the patch, or the general check I described in point 4? |
|
@devyte The moment to allocate the 4k buffer. 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. Or am I missing something in my thought experiment? |
|
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. |
|
Why must I write an entire flash sector? |
|
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:
|
|
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. Only thing is, I had trouble finding my "PUYA" nodes to test. |
|
One other thing I notice. What guarantee do we have the write cycle is always 4 kB max? I've been reading the datasheet.
Arduino/cores/esp8266/Updater.cpp Lines 142 to 147 in d8acfff So is it OK to set the PUYA buffer size as a define with default set to 512? |
|
In this last commit, I am using a 512 byte buffer and do checks on the amount of data to write. I have tested writing files (to SPIFFS) of 2 kB and partial updates in files, but not sure if SPIFFS also divided the amount of data to be sent in parts of 512 bytes. N.B. would be nice if it is also tested on real PUYA hardware. |
|
PUYA chips have software erase page size of 256 bytes, so 256 byte buffer should also be okay. |
|
What about the rest of the patch? |
|
It has been tested on a module with PUYA, with the suggested 256 Byte buffer and appears to be working. |
As discussed here: esp8266#5493 (comment)
* [PUYA] Applied ESPeasy puya_v3.patch Applied the patch to get the starting point as described in #5493 * [PUYA] Only allocate memory when PUYA detected core 2.5.0 PUYA patch, no puya: Description Function #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 Function #calls call/sec min (ms) Avg (ms) max (ms) Save File 2 0.04 41.332 57.544 73.756 Free Mem: 11560 * [PUYA] Check for PUYA chip as soon as possible at boot Check for PUYA chip in call for `getFlashChipId()` This will only be done once and the result of the get function is also cached. * [PUYA] Use limited buffer (512 byte) allocated at first write No need to allocate a buffer when not writing to flash. The default buffer size is 512 bytes, which is 2 pages in the flash chip. * [PUYA] Lower PUYA flash buffer to 1 page (256 B) As discussed here: #5493 (comment) * [PUYA] Fix indents naming and return conditions * [PUYA] Move Puya write code to spi_flash_write_puya * [PUYA] Make spi_flash_write_puya static and define PUYA_SUPPORT * [PUYA] Add some SPI flash vendor IDs As requested by @igrr #5504 (comment) * [PUYA] All suggested changes. See: #5504 (review)
|
Nice job @TD-er |
|
Hello I have tried pulling this in to my machine from Git but I still seem to have issues. Any hints on how to check this is working? I've run the test Sketches from the initial thread but running the test write code is still yielding empty lines? Sketches: WriteReadTest.ino Log FlashInfo.ino Log |
|
Did you set the build flag to activate this patch? |
|
AH! That makes sense, sorry 6 hours of debugging my own code then I found this :D I have added |
|
You must define it as a global define when building it and probably make a clean build. #ifndef PUYA_SUPPORT
#define PUYA_SUPPORT 1
#endif |
|
Whooo! Changing it in the |
|
Thanks for testing it. |
|
Hello @ALL you Gurus... Just a favor for us amateurs and ignorants in the matter: can you please give a link with the patch and a small explanation in mortal language of how to install (use) it? Many THANKS for your incredible work. |
|
@carlosmtolosa As an example you can see how we use it in the PlatformIO.ini file for ESPeasy: |
|
@TD-er in which version of platform IO was this patch merged? (If it's not merged yet in a published version, what are the steps to use it? I was waiting for months for this patch. Thank you for fixing this. |
|
same file, but look at lines 106-108 |
|
@daniftodi It is included in the staging version of core 2.5.0
I will try to make a git branch on my own repository, so it is included in the older core versions too. |
|
@daniftodi And also I made tar.gz downloads for the older core libraries. |
|
Thank @TD-er. I'm using espressif8266 and have written some code based on this library. Can you tell me how to apply your patch on this? Because I have tried with development version (indicated inside platformio.ini the github link to project) and with PUYA_SUPPORT=1 flag and it didn't worked for me. May be that I have to format somehow the SPIFFS before will be able to use PUYA_SUPPORT? |
|
@TD-er what should be done on our site? |
I think it would be nice to have the patch included in a platformio-archive of older core versions, like I did. Like @daniftodi already showed, it is apparently not that obvious how to change the platformio.ini to include the right platform (e.g. |
|
We stopped maintaining custom packages and use only official releases. You are able to create own package and use it. I hope this feature platformio/platformio-core#1367 will help to use a custom package per build environment. |
|
@ivankravets As you may have seen, I already created a custom version including this patch and we're using it for ESPeasy to build the nightly builds with it. But on the other hand, I get it when you don't want to add various patches and thus move away from the official core releases. Is there a way for me to get notified when you create a new version of those older core packages? |
|
Hi, I was having some issues with a json file inside the SPIFFS which I read it (open() with 'r'), parse it, add new values to the jsonObject, write it back to the file (open() with 'w'). So, I applied the patched described by @PaulBortnikov at #4061 at the Esp.cpp which checks the flash, but now I'm getting several problems trying to write to the SPIFFS. Someone else with the same problem? `
ctx: sys ets Jan 8 2013,rst cause:1, boot mode:(1,6) ets Jan 8 2013,rst cause:4, boot mode:(1,6) wdt reset |
|
I came to this issue from the original issue #4061.After much deliberation, I decided to save my settings into EEPROM. EEPROMs have limited write cycles, but they are pretty huge (4096 bytes for the ESP01), and ESP01's are cheap, but it is possible and it works. You will be able to see working sample code here. https://github.com/rupin/IoT-Projects/blob/master/TriggerAlarm/TriggerAlarm.ino I hope someone finds this useful. |
The (current) patch:
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)
The text was updated successfully, but these errors were encountered: