Completely inline the helper pure abstract __FlashStringHelper class

[dok-net]

On ESP32, it's not needed - all the files using it are different from their ESP8266 counterparts anyway.

Fixes #7940

[mrengineer7777]

Breaking change! Affects ESPAsyncWebServer and possibly other libraries.

[dok-net]

@mrengineer7777 Thanks for pointing this out. Fixed.

[me-no-dev]

Why remove code instead of adding what is incompatible with 8266? Now you are risking causing harm to other libraries that might be using those functions.

[dok-net]

Why remove code instead of adding what is incompatible with 8266? Now you are risking causing harm to other libraries that might be using those functions.

Remove? What exactly are you seeing that I have missed? There should be no removals, everything is just inlined now because most of it can be / has to be no-ops.

[me-no-dev]

Why remove code instead of adding what is incompatible with 8266? Now you are risking causing harm to other libraries that might be using those functions.

Remove? What exactly are you seeing that I have missed? There should be no removals, everything is just inlined now because most of it can be / has to be no-ops.

Ah yes... my bad :)

[github-actions]

External libraries build test

Library	ESP32	ESP32C3	ESP32S2	ESP32S3
Adafruit NeoPixel	1 ✅	1 ✅	1 ✅	1 ✅
ArduinoBLE	1 ✅	1 ✅	N/A	1 ✅
ESP32Servo	4 ✅	4 ✅	4 ✅	4 ✅
ESPAsyncWebServer	3 ✅ 1 ⚠️	4 ❌	3 ✅ 1 ⚠️	3 ✅ 1 ⚠️
FastLED	1 ✅	1 ✅	1 ✅	1 ✅
IRremote	1 ⚠️	1 ❌	1 ✅	1 ✅

[github-actions]

External libraries build test

Library	ESP32	ESP32C3	ESP32S2	ESP32S3
Adafruit NeoPixel	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅
ArduinoBLE	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅	N/A	1 ✅ -> 1 ✅
ESP32Servo	4 ✅ -> 4 ✅	4 ✅ -> 4 ✅	4 ✅ -> 4 ✅	4 ✅ -> 4 ✅
ESPAsyncWebServer	3 ✅ 1 ⚠️ -> 3 ✅ 1 ⚠️	4 ❌ -> 4 ❌	3 ✅ 1 ⚠️ -> 3 ✅ 1 ⚠️	3 ✅ 1 ⚠️ -> 3 ✅ 1 ⚠️
FastLED	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅
IRremote	1 ⚠️ -> 1 ⚠️	1 ❌ -> 1 ❌	1 ✅ -> 1 ✅	1 ✅ -> 1 ✅

[brentru]

@P-R-O-C-H-Y @me-no-dev Thank you very much for adding a check for the external libraries. This will be incredibly useful down the line!

However, FYI (unless I am reading into this incorrectly), there are 10 errors and 1 warning within the external library check on this breaking change (mentioned above): https://github.com/espressif/arduino-esp32/actions/runs/4627758830 yet the CI shows it passes. Is this an issue with the CI tooling?

[P-R-O-C-H-Y]

@P-R-O-C-H-Y @me-no-dev Thank you very much for adding a check for the external libraries. This will be incredibly useful down the line!

However, FYI (unless I am reading into this incorrectly), there are 10 errors and 1 warning within the external library check on this breaking change (mentioned above): https://github.com/espressif/arduino-esp32/actions/runs/4627758830 yet the CI shows it passes. Is this an issue with the CI tooling?

Hi @brentru,

As the CI job is universal for PRs and Schedule runs, I did not think about this point of view that it can be confusing, that the CI passed successfully, when there were errors. When the External Library Test is successful, it means that the test run finishes without any complications, like wrong inputs (json with libs and sketches) or issues with CI actions. It needs to be successful as the compilation error is a result too. But I can think of it a bit and maybe do some changes.

As you can see there were errors in the run you mentioned, doesn't have to mean, that the PR breaks anything, it may even fix some libraries or at least fix some warnings. Where to look first, is the result table printed to comment by the GH action. You have to do manual check for now, to see if the PR breaks some library. As you can see, in this PR the compilation BEFORE is the same as AFTER (BEFORE->AFTER PR). You can check docs to read more information about the External Library Test.

Hope this help, but feel free to ask there or maybe in the discussions CI Testing for External Libraries. Even if you know some libraries that are highly used, feel free to add a comment there and we can add them later on :)

[bblanchon]

This change breaks the following use case:

const __FlashStringHelper* str = F("lorem ipsum");

[mrengineer7777]

@bblanchon We are aware this PR has issues. Does this fix help?

[bblanchon]

Thank you for the quick reply, @mrengineer7777.
Yes, #8147 should fix this issue.