Avoid Arduino.h if its features are disabled (fixes #1692)

[paulocsanz]

Fixes #1692

It's not clear if this is the best solution. I'm open to suggestions and will modify the PR accordingly.

[bblanchon]

Hi @paulocsanz,

Thank you very much for this contribution 👍

I think including ArduinoJson.h from this file was a mistake, so I prefer we move the include elsewhere.
I suggest including this file closest to its actual usage, just as we do for STL headers.

Please remove #include <Arduino.h> from ArduinoJson/Configuration.hpp and add it in in the following files:

• ArduinoJson/Deserialization/Readers/ArduinoStreamReader.hpp
• ArduinoJson/Deserialization/Readers/ArduinoStringReader.hpp
• ArduinoJson/Deserialization/Readers/FlashReader.hpp
• ArduinoJson/Serialization/Writers/ArduinoStringWriter.hpp
• ArduinoJson/Serialization/Writers/PrintWriter.hpp
• ArduinoJson/Strings/Adapters/ArduinoString.hpp
• ArduinoJson/Strings/Adapters/FlashString.hpp
• ArduinoJson/Polyfills/pgmspace.hpp

Finally, include pgmspace.hpp from pgmspace_generic.hpp, so that static_array.hpp includes ArduinoJson.h transitively (I think this is what was missing for #1070).

Hopefully, everything should compile correctly.

Best regards,
Benoit

[paulocsanz]

Thank you very much for the help, it worked flawlessly, I've updated my fork and we are successfully using it in production while waiting for this merge to land :).

Happy holidays, if that's your thing!

Best Regards,
Paulo

[coveralls]

Coverage remained the same at 99.438% when pulling 812e096 on paulocsanz:6.x into ada1f2a on bblanchon:6.x.

[bblanchon]

AVR and SAMD builds are failing on platformio.

This is the error on AVR:

In file included from lib/ArduinoJson/src/ArduinoJson/Strings/Adapters/ArduinoString.hpp:7:0,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/StringAdapters.hpp:20,
                 from lib/ArduinoJson/src/ArduinoJson/Memory/MemoryPool.hpp:10,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/VariantData.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/SlotFunctions.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayIterator.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayRef.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson.hpp:17,
                 from lib/ArduinoJson/src/ArduinoJson.h:9,
                 from src/avr.cpp:1:
/home/runner/.platformio/packages/framework-arduino-avr/cores/arduino/Arduino.h:132:5: error: conflicting declaration of C function 'int atexit(void (*)(...))'
 int atexit(void (*func)()) __attribute__((weak));
     ^~~~~~
In file included from /home/runner/.platformio/packages/framework-arduino-avr/cores/arduino/Arduino.h:23:0,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/Adapters/ArduinoString.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/StringAdapters.hpp:20,
                 from lib/ArduinoJson/src/ArduinoJson/Memory/MemoryPool.hpp:10,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/VariantData.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/SlotFunctions.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayIterator.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayRef.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson.hpp:17,
                 from lib/ArduinoJson/src/ArduinoJson.h:9,
                 from src/avr.cpp:1:
/home/runner/.platformio/packages/toolchain-atmelavr/avr/include/stdlib.h:685:12: note: previous declaration 'int atexit(void (*)())'
 extern int atexit(void (*)(void));
            ^~~~~~

And on SAMD:

In file included from /home/runner/.platformio/packages/framework-arduino-samd/cores/arduino/api/Interrupts.h:8:0,
                 from /home/runner/.platformio/packages/framework-arduino-samd/cores/arduino/api/ArduinoAPI.h:29,
                 from /home/runner/.platformio/packages/framework-arduino-samd/cores/arduino/Arduino.h:23,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/Adapters/ArduinoString.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/StringAdapters.hpp:20,
                 from lib/ArduinoJson/src/ArduinoJson/Memory/MemoryPool.hpp:10,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/VariantData.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/SlotFunctions.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayIterator.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayRef.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson.hpp:17,
                 from lib/ArduinoJson/src/ArduinoJson.h:9,
                 from src/esp8266.cpp:1:
/home/runner/.platformio/packages/framework-arduino-samd/cores/arduino/api/Common.h:83:5: error: conflicting declaration of C function 'int atexit(void (*)(...))'
 int atexit(void (*func)()) __attribute__((weak));
     ^~~~~~
In file included from /home/runner/.platformio/packages/toolchain-gccarmnoneeabi/arm-none-eabi/include/string.h:10:0,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/Adapters/RamString.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/Adapters/JsonString.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Strings/StringAdapters.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson/Memory/MemoryPool.hpp:10,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/VariantData.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Variant/SlotFunctions.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayIterator.hpp:7,
                 from lib/ArduinoJson/src/ArduinoJson/Array/ArrayRef.hpp:8,
                 from lib/ArduinoJson/src/ArduinoJson.hpp:17,
                 from lib/ArduinoJson/src/ArduinoJson.h:9,
                 from src/esp8266.cpp:1:
/home/runner/.platformio/packages/toolchain-gccarmnoneeabi/arm-none-eabi/include/stdlib.h:76:5: note: previous declaration 'int atexit(void (*)())'
 int _EXFUN(atexit,(_VOID (*__func)(_VOID)));
     ^

It looks like we revealed a bug: including Arduino.h after stdlib.h causes a conflict.
Here are the faulty lines: on AVR and on SAMD.

Can you try to find an elegant solution?

[paulocsanz]

I see, thank you for the help, I will look into a more elegant solution, I've setup the CI in my own fork and am iterating over the code.

[paulocsanz]

I spent a long time iterating over alternatives and what I've found is that since modules haven't really landed/become mainstream any header can include any other header. So the only way I was able to fix this was by including <Arduino.h> before an include for anything that is outside of the <ArduinoJson/...> context, if ARDUINO was set (which goes against the purpose of this PR, but I was trying to narrow the cause down).

So it seems we could track down all headers included that includes <stdlib.h> transitively, but since we target too many environments it's hard to ensure some of them won't cause problems in the future or if we are excluding some that would work out of the box but is not tier 1.

<Arduino.h> is old and has too much backwards compatibility problems but it's widely used, I think we should just accept that it's the cost of legacy code and include it globally whenever a feature needs it. And in my experience a few environments have very slow pace of development and can't be counted on to update, and there are so many backends that it's hard to ensure all of them work properly and it's better to focus on the old "Accept everything possible, be strict in what you emit".

It seems that ARDUINOJSON_ENABLE_PROGMEM will cause a problem for these Arduino environments if the user disables all of the other three Arduino features. So it seems <Arduino.h> should be included if ARDUINOJSON_ENABLE_PROGMEM is set too, that's why I changed, but I can revert it.

Anyway, it seems too much resources spent on trying to fix a very small problem. Could we time-box it and move forward with a less elegant solution?

I am open to suggestions tho.

[paulocsanz]

Sorry, it was late, I screwed something up, I will fix it in my fork and then update here.

Can I bother you with a question to help speed things up? How can I run these tests locally to not depend on my fork's CI?

Only the linter and the four broken ones are more than enough. Is there any doc around that?

[bblanchon]

Don't worry, I'll take care of it; I'm unfortunately used to working around these quirks.

[paulocsanz]

Great to see this merged! Thank you very much for fixing my mess! But won't this cause problems for ARDUINOJSON_ENABLE_PROGMEM?

As if it's not defined it checks at /ArduinoJson/Configuration.hpp#L154 to see if PROGMEM or other defines that Arduino.h creates are there, so now users that include ArduinoJson.h before Arduino.h and don't define ARDUINOJSON_ENABLE_PROGMEM will have it disabled?

Or am I reading too much into it?

My last approach included Arduino.h before that line if ARDUINOJSON_ENABLE_PROGMEM wasn't set and ARDUINO was to avoid that.

[bblanchon]

You're right; it might be an issue if Arduino.h is included after ArduinoJson.h.
In practice, the environment (Arduino or PlatformIO) already includes Arduino.h for you; I think it's only a problem with Particle.
How did you get into this situation, @paulocsanz?
Are you sure that Arduino.h is not already included by your environment?
If so, we should roll back these changes.

[bblanchon]

I'm wondering if we could simply assume that PROGMEM is available as soon as ARDUINO is defined.
All Arduino cores I could check support or emulate PROGMEM.
I only added the test of the existence of all macros because of #1433, but I could add a polyfill to support this edge case.

[paulocsanz]

PIO doesn't actually include #include <Arduino.h> for me, I'm sure because I spent quite some time removing all references to it from headers and when I used to #include ArduinoJson.h I had to undef a bunch of stuff like OUTPUT and others that Arduino.h includes (which is why I created this issue/PR). Using the current main branch directly fixes it and I don't have to undef things anymore after #include <AduinoJson.h>.

We only #include <Arduino.h> in cpp files for the esp8266 driver. So the Arduino macros aren't available outside of it (we tried). But I'm sure this is a very uncommon situation.

I hadn't actually had problems with that part of the code as we don't use it, but since I saw that it expects PROGMEM to be defined before the #include <Arduino.h> and I had to #include <sys/pgmspace.h> whenever I wanted to use PROGMEM outside of the drivers in my system it seemed like a possible problem.

Weren't the CI failures we found related to Arduino.h not being included before stdlib.h? If it was automatically included it wouldn't have failed, right?

[paulocsanz]

I may have been mistaken about PROGMEM, it seems it's something defined by esp8266's core so it may be available without including Arduino.h, but I'm not sure yet and this is only for one environment. Sorry for crying wolf. This code is probably ok.

[bblanchon]

Indeed, PlatformIO only includes Arduino.h if the file extension is .ino; sorry about that.
I don't think it implicitly defines PROGMEM though, and if it does, it may not be a portable solution.
I think it's safer to assume that PROGMEM is supported or emulated on every Arduino core.
I can see that the Particle header still lacks the required functions, so I'll write the polyfills.