Rewrite PUYA patch to be more universal and mem friendly.

[TD-er]

See #5493

Implemented all suggestions, except the extended detection of PUYA like flash operations.
Current detection is only to match the vendor ID from the flash chip ID.

[igrr]

• indentation has changed from spaces to tabs, please make it consistent with the original code

• what is the purpose of #define PUYASUPPORT in header file?

• flashIsPuya function doesn't need to be class member, right?

• please move puya-related write logic into a new function, e.g. spi_flash_write_puya, and call either that or spi_flash_write from flashWrite.

• inconsistent variable naming, bytes_now vs bytesLeft.

• missing ets_isr_unmask in the failed path of malloc-ing the temporary buffer

(Sorry for not leaving these as line comments; something's wrong with the comment UI there...)

[devyte]

#define PUYASUPPORT

I believe that define is supposed to be used to conditionally compile the puya-specific code, I.e.: enable/disable that supporrt via the define, but that isn't done.

I also have several comments, but I'll hold off until the current ones are addressed to reduce confusion.

[igrr]

enable/disable that supporrt via the define

That would make sense, in this case the more common pattern would be:

#ifndef FOO_SUPPORT
#define FOO_SUPPORT 1
#endif

// Later in the code
#if FOO_SUPPORT

[TD-er]

We used the define to see if the patch was applied.
And the flashIsPuya function was used to display info about the used core to see if the patch was applied and if it has a PUYA chip. (to have the detection of that chip in one place)
So I would like to have that function public accessible to have some indicator on future reported issues.

missing ets_isr_unmask in the failed path of malloc-ing the temporary buffer

That's a serious one, I will have a look at the suggested changes right now.

[TD-er]

@igrr

please move puya-related write logic into a new function, e.g. spi_flash_write_puya, and call either that or spi_flash_write from flashWrite.

Should it be in the same Esp.cpp file?
The spi_flash_write function is taken from the sdk, but we cannot put it there for obvious reasons.
I assume the puya version of this function should not be public?

[igrr]

It's okay to put spi_flash_write_puya into Esp.cpp as a static function.

Regarding detection of whether PUYA is supported or not, okay, understood. Can you modify the define to be conditional as shown in my previous comment? This was it will be possible to override the define and disable PUYA support via e.g. platform.txt.local file.
Also, not a fan of having flashIsPuya function in the public interface. Something like getFlashVendor() with an enum or list of constants for known vendors would be preferable.

[TD-er]

All suggestions should now be implemented.

Did it all on a Celeron Chromebook, so now I can appreciate a proper i7 or Xeon ;)

[igrr]

This version looks okay to me.

Having static state which can not be deallocated (flash_write_puya_buf) is usually a problem for unit tests which check for memory leaks. However assuming that we a) don't test PUYA write logic in host tests and b) we don't run device tests on modules with PUYA chips, this isn't a big issue.

[TD-er]

Is this also merged back to core 2.4.2, since 2.5.0 is beta?
Or should I create a new PR after this gets merged, to a core 2.4.2 branch?

[igrr]

We aren't maintaining bugfix releases for older versions, so this should only apply to 2.5.0.

[devyte]

I would have preferred an enum class instead of a classic C-style enum, but ok. Approving.

[mrWheel]

[PUYA] Support for PUYA now in core 2.5.0 (?)
Can anybody please explain how to activate this? I have several reports from people trying to get this working with a project of mine but it looks like it is still not.
It would really help if only some documentation would explain how to activate this.

Simply #define PUYA_SUPPORT 1 in the ino sketch does not show any changing in the building logs. No -DPUYA_SUPPORT=1 flags.
Changing line 27 in Esp.h to #define PUYA_SUPPORT 1 does not show any change in build/compile flags.

So, for the simple people among us (including myself): How to activate the PUYA chip functionality?

[d-a-v]

In platform.local.txt:

compiler.c.extra_flags=-DSTASSID="yourssid" -DSTAPSK="yourpassword" -DPUYA_SUPPORT=1
compiler.cpp.extra_flags={compiler.c.extra_flags}

[mrWheel]

@d-a-v
Thanks for that quick reply!

If there is no platform.local.txt should I just make one (in the same map?)? << YES!

And then #define PUYA_SUPPORT 1 in my sketch.ino? << Makes no difference :-(

OK!! That seems to work. But what about -DSTASSID=\"yourssid\" and -DSTAPSK=\"yourpassword\"?? << Removed both.

.. "-DSTASSID=\"yourssid\"" "-DSTAPSK=\"yourpassword\"" -DPUYA_SUPPORT=1 ..

First line now reads:

compiler.c.extra_flags=-DPUYA_SUPPORT=1

Ok, seems this is nót conditional!
Either you setup your system to always use the PUYA patch or not. #define PUYA_SUPPORT or not does not change that. I get the -DPUYA_SUPPORT=1 option anyway.
Is that correct behaviour?

[d-a-v]

platform.local.txt needs to be in the same directory where platform.txt is.
STASSID and STAPSK to allow examples running on your network without modifying them.

Is that correct behaviour?

Yes, no need to add this in your source code. It will be useless.

[mrWheel]

@d-a-v
Ok, thanks again.
Is there a drawback to adding this compiler option if you're compiling for/uploading to a ESP8266 that does not have a PUYA chip? Probably some performance degradation but no real problems?

[TD-er]

Nope, not a real problem.
In ESPeasy we build all versions including Puya support and it works just fine.