xoseperez / espurna

Home automation firmware for ESP8266-based devices
http://tinkerman.cat
GNU General Public License v3.0
3k stars 638 forks source link

Update to ArduinoJson 6 #1589

Open mcspr opened 5 years ago

mcspr commented 5 years ago

ref #924, #1179

Since 6.9.0 ArduinoJson is no longer in beta. https://github.com/bblanchon/ArduinoJson/releases/tag/v6.9.0

Niek commented 5 years ago

Migration docs are available @ https://arduinojson.org/v6/doc/upgrade/

mcspr commented 5 years ago

Important note from the migration guide:

// ArduinoJson 5
DynamicJsonBuffer jb;

// ArduinoJson 6
DynamicJsonDocument doc(1024);

DynamicJsonDocument has a fixed capacity that you must specify to the constructor. Unlike the DynamicJsonBuffer, DynamicJsonDocument doesn’t automatically expand.

All dynamic memory buffers now need hard-coded max capacity value. Old DynamicJsonBuffer also had a fixed-size constructor option, but I think that was used here exactly once and since was removed from the code. Only 10 files use it: https://github.com/xoseperez/espurna/search?l=C%2B%2B&q=DynamicJsonBuffer HomeAssistant & WS are the main ones, which are known to crash specifically with json output. Web could also use some sane limits for the uploaded config.

mcspr commented 5 years ago

Half-broken, but buildable: https://github.com/xoseperez/espurna/compare/dev...mcspr:web/fixed-size-json edit: And similar changes for V5 https://github.com/xoseperez/espurna/compare/dev...mcspr:web/fixed-size-json-v5 Both are not tested, yet

What I am not liking is size increase, almost +4KB. Either what we are doing is really wrong, or this flow is not optimized in the new version.

Via ~/.platformio/packages/toolchain-xtensa/bin/xtensa-lx106-elf-nm --print-size --size-sort --radix=d .pio/build/<env>/src/espurna.ino.cpp.o (meaning, this is from arduinojson6 usage, since it is header-only and does not build any library):

version addr size symbol
dev 00000284 00000694 _Z25_relayWebSocketSendRelaysv
fixed-size-json `00000364 00001136 _Z25_relayWebSocketSendRelaysv

+442 bytes just there

edit: #undef FORCE_INLINE is slightly decreasing the overall size, but not to the v5 levels...

Niek commented 5 years ago

I don't know too much about the ArduinoJson code, but wouldn't it make sense to use StaticJsonDocument instead of DynamicJsonDocument, if you're not going to expand beyond the initially allocated size? The way I read the docs, that should be faster (and possibly smaller).

mcspr commented 5 years ago

IDK. Static one is a template, so we actually create a new class definition every time we use a different document size. Plus, it uses stack, and it might be problematic if there are some other heavy structs used.

by default, esp8266 stack is 4k deep. if WPS is enabled, it is 8k (but -4k less heap, because of the Core tweaks). still less than 50k of available heap

mcspr commented 5 years ago

There is also useful ::memoryUsage() that calculates used size. To test things, one can put some random document size and call this after all values are set. Resulting value can then be hard-coded (ofc, taking into consideration const / non-const strings)

Size still grows. Current nodemcu-lolin build from v6 branch is 483472 bytes versus 477504 from the latest nightly. I hope it is all those debug strings...

mcspr commented 4 years ago

Oh, I missed this release's changelog. While noticing the writer issue, its turns out there are readers too! https://arduinojson.org/2019/11/01/version-6-13-0/ https://github.com/bblanchon/ArduinoJson/releases/tag/v6.13.0

Added support for custom writer/reader classes (issue 1088)

Some weird dynamic things like ws incoming buffer could use non-linear memory a-la telnet outgoing buffer without using Stream