Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to ArduinoJson 6 #1589

Open
mcspr opened this issue Feb 27, 2019 · 7 comments
Open

Update to ArduinoJson 6 #1589

mcspr opened this issue Feb 27, 2019 · 7 comments
Labels
core enhancement New feature or request

Comments

@mcspr
Copy link
Collaborator

mcspr commented Feb 27, 2019

ref #924, #1179

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

@mcspr mcspr added enhancement New feature or request core labels Feb 27, 2019
@Niek
Copy link
Contributor

Niek commented Jul 11, 2019

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

@mcspr
Copy link
Collaborator Author

mcspr commented Jul 24, 2019

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
Copy link
Collaborator Author

mcspr commented Jul 30, 2019

Half-broken, but buildable:
dev...mcspr:web/fixed-size-json
edit: And similar changes for V5
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
Copy link
Contributor

Niek commented Jul 31, 2019

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
Copy link
Collaborator Author

mcspr commented Jul 31, 2019

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
Copy link
Collaborator Author

mcspr commented Aug 1, 2019

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
Copy link
Collaborator Author

mcspr commented Dec 4, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants