Add fix for override macro check on ESP32 atm

[DeflateAwning]

Appears to fix this issue: bxparks/AceTime#114

Perhaps there's a better solution to use though

[bxparks]

The ESP32 core changed its Print::flush() to be a virtual method in Jan 2022, and released in v2.0.3. See
espressif/arduino-esp32#6084

It seems to me that PlatformIO needs to be updated to match the current ESP32 API. I'm not inclined to fix this in AceCommon, because this causes warning messages to be generated on ESP32 core >= 2.0.3.

[DeflateAwning]

I think that makes sense. Is there a workaround to make it support both versions though, with some sort of version checker in the ifdef macro?

[bxparks]

If there is a #define macro that identifies PlatformIO, then maybe. But I don't use PlatformIO, and I would consider it only if you are unsuccessful at pushing that fix into PlatformIO itself. Adding little hacks like this makes it harder and harder for me to maintain my code. This one thing won't break the bank, but a hundred little things like this add up in the long run.

[DeflateAwning]

I agree completely. I don't see any option in PlatformIO to change that version, and it looks like a stale debate since 2021. Low chance I'll be able to force an upgrade on a major version number like that (they're still on v1.0.x)

[bxparks]

I don't know anything about the PlatformIO community, but I find that to be an odd situation. If they choose to remain stuck in the v1.x version of ESP32 Core, then over time, more and more 3rd party Arduino libraries will become incompatible with PlatformIO, since those will written and tested against v2.x.

As for me, I don't use PlatformIO, I really don't want to spend any extra effort writing, reviewing, and testing any code specifically for PlatformIO. You are free to fork your own copy of AceCommon and add what you need to make things work for you. If the code is minor, I may consider accepting a pull request, but I am reluctant to spend much mental energy on PlatformIO. I got limited time, and I have to pick my battles.

[bxparks]

One additional thought: If I recall, I actually use the fact that Print::flush() is virtual in a handful of places (probably in unit tests, but maybe not? I don't remember). That code would compile but break at runtime on ESP32, under PlatformIO. Will I get some obscure bug report from someone using one of my libraries on PlatformIO in the future? I don't know, and I'd rather not have to spend my time debugging such an obscure bug.

[DeflateAwning]

All sounds reasonable to me. Thanks for your contributions to these projects!

The most infuriating part is that I just need it to compile; I don't even need to use this darn function

[bxparks]

I really hope that you can push PlatformIO to fix this. It's bad enough that, as a library developer, I have to remember all the idiosyncratic differences of the various flavors of the "Arduino API" implemented by 3rd party Cores (Arduino, Adafruit, ESP8266, ESP32, Seeeduino, Teensyduino, Sparkfun, etc.) But now you are telling me that PlatformIO may be implementing those 3rd party Cores differently. Which increases the number of variations by a factor of 2X. That's too much.

As far as I can tell, PlatformIO piggybacks on top of the 3rd party Arduino library ecosystem. I would have thought that it would be their top priority to be as compatible and up-to-date as possible with the various Arduino Cores for the various microcontrollers.

[DeflateAwning]

I mean, Platform.io isn't doing it completely differently; they're just using an older version of the API to avoid making a breaking change. But I agree.

[bxparks]

I'm surprised that PlatformIO does not provide a mechanism for selecting the ESP32 core version. Then the end-user can make that choice.

Anyway, now that I understand that they are using the same source code as the ESP32 Arduino core, I'm willing to add a conditional, like this:

#if ... \
  || (defined(<<PLATFORM_IO>>) && defined(ESP32))
    void flush() {
  #else
    void flush() override {
#endif

Just let me know what that <<PLATFORM_IO>> macro is.

[DeflateAwning]

The macro is called PLATFORMIO.

Some other useful macros to reference may be: ESP_PLATFORM and ARDUINO_ARCH_ESP32. I'm not certain if ESP_PLATFORM is better than ESP32, but it might be; it'll include ESP8266s as well, which I'm not sure if they have this issue.

Thank you for working to solve this issue. Really appreciate it.

[bxparks]

I believe the ARDUINO_ARCH_ESP32 and ESP32 are functionally identical. TheARDUINO_ARCH_ESP32 is the correct macro following Arduino conventions, but it was added only in later versions. The ESP32 was the original. Since PlatformIO is stuck in the past, it seems like ESP32 would be the more reliable macro.

Let's go with PLATFORMIO, since it's self-descriptive. Can you add that to your PR, and test it? If it works, then I'll send an approval to merge.

As far as I am aware, the ESP8266 always defined its Print::flush() as a virtual, so PlatformIO should have picked up the same code, so the ESP8266 doesn't need this hack.

[DeflateAwning]

Changes made, pushed, and tested! Thank you!

[bxparks]

Released v1.6.2 with this fix.