Configurable buffer size for StatefulService related classes

[raomin]

Hi,
It would be great to have MAX_JWT_SIZE overwritable somewhere.
My json settings is getting quite big. I guess an #ifndef would do the trick.

Thanks for the great work btw! :)

[proddy]

@rjwats you could consider using MessagePack which ArduinoJSON supports. It'll shave off ~30% of the json file size and you only need to change two lines in FSPersistance.h:

DeserializationError error = deserializeJson(jsonDocument, settingsFile) => DeserializationError error = deserializeMsgPack(jsonDocument, settingsFile) 

serializeJson(jsonDocument, settingsFile) => serializeMsgPack(jsonDocument, settingsFile)

[proddy]

also @raomin if you're on ESP8266 you could try adding this to the platformio.ini file:

board_build.ldscript = eagle.flash.4m2m.ld

which will give you 2024 KB SPIFFS instead of the default 1000 KB. Only the OTA buffer is less but if you switch to compressing the firmware image (new in core 2.7.0) you'll have no issues.

[rjwats]

On msgpack, it might be an idea. I'm much more worried about the buffer sizes in the heap though than size of the files on the flash. All-in the default factory settings come in at 862 bytes so there's not a huge saving to be had there. If we were to use it as the wire format, updating the UI to parse msgpack would be easy enough but there's the arguement for it being harder to debug with normal tools and interface with external services if you do that. Just checking @raomin, are referring to MAX_JWT_SIZE for the user JWT payload and not the other various MAX_{XXX}_SIZE defines you see scattered about? I'm working with another contriubtor at looking at these defaults over in this branch at the moment: #114 Might be able to look at making this configurable as part of that.

…

On Tue, May 19, 2020 at 2:57 PM Proddy ***@***.***> wrote: @rjwats <https://github.com/rjwats> you could consider using MessagePack <https://hackaday.com/2020/03/12/messagepack-is-a-more-efficient-json> which ArduinoJSON supports. It'll shave off ~30% of the json file size and you only need to change two lines in FSPersistance.h: DeserializationError error = deserializeJson(jsonDocument, settingsFile) => DeserializationError error = deserializeMsgPack(jsonDocument, settingsFile) serializeJson(jsonDocument, settingsFile) => serializeMsgPack(jsonDocument, settingsFile) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#115 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAKE4VFKJCPTLGGWD473XB3RSKF2ZANCNFSM4NFAETYQ> .

[raomin]

My json object needs a bit less than 4000 bytes according to the arduinojson's assistant.
It's also handle in FSPersistence so, you're right, I guess MAX_FILE_SIZE should be changed, and MAX_CONTENT_LENGTH in HttpEndpoint too.

[rjwats]

@raomin Your weatherstation project looks very cool, love the effort you've gone to. I hope the upgrade with all of the latest framework changes is not too painful. I'm intrested in any feedback :)

4k sounds a large a buffer to me on this device. You may run into allocation errors so trying to get that down might be a good idea, if you can.

In particular is it the logged data which is getting large? I had similiar issues with my fermentation chamber. I ended up implementing a paginated endpoint and writing struct based serialization for the data to save space. It was a bit rough and ready but it might give you some ideas:

Paginated endpoint: https://github.com/rjwats/esp8266-react/blob/fermentation-chamber2/src/FermentationChamber.cpp#L198

Circular log: https://github.com/rjwats/esp8266-react/blob/fermentation-chamber2/src/CircularLog.h
(this is horrible and bypasses the wear leveling, your time slice rolling file approach is better)

Regardless, I think it's worth using ifndefs for these settings anyway and having a global "DEFAULT_JSON_BUFFER_SIZE" option which control the unset ones. Will look at doing that thx.

[raomin]

Thanks for the feedback on DataLab 👍 . I left it a bit aside for now, but will come back to it eventually.
I'm on another project now that has manages a schedule configuration in JSON that could potentially go up to 4k, but it's more for being safe, usually it's less. On my esp32, I still have 170k heap free when esp8266-react is running.

Now I'm having problem after increasing the MAX* variable: the AsyncCallbackJsonWebHandler updateHandler does not manage information for the dynamicJSON buffer size, just on the content-lenght. So the update handler is stuck with the default dynamicJSON buffer size of 1024.
We could probably need to handle the DynamicJson buffer size in a property of the Settings type of the statfulservice and it should be passed as the 3rd param of the call to the AsyncCallbackJsonWebHandler constructor.

Currently, if you send a small but complex JSON that would result in a dynamicJSON size>1024 you'll get a 400 and the data won't reach updateSettings().

A side note on that:
We have 2 buffer sizes here: the content-length buffer for the http request or the file size and the dynamicJson buffer size. The later has little to do with the size of the content-length: we can have a small json in size but very complex that would require much more buffer size than its content-length.
Note on this: we might have an issue in FSPersistence > readFromFS() as you infer the json buffer size from the (max) file size.

[rjwats]

Some thoughts on this one, in the form of a PR:

#118

[rjwats]

The original reasoning behind capping the content length in the POST handler and likewise the filesystem code was to eagerly fail.

But you are right, the JsonObject won't necessarrly always be larger than the stringified representation as formatted json can have a bunch of whitespace data which won't be held in the object representation.

Humm.. needs some thinking about.

[rjwats]

@raomin

This PR shows one possible way we might solve this in the framework. Does this look like it will support your use-case?

#118

I feel like the constructors are getting quite long now in terms of the number of arguments...

[raomin]

Thanks for the PR and your consideration of this issue.

I had a look at the PR. I think defining a buffer size per StatefulServices is the right approach, with a default size around 1k.
But we have 2 buffer sizes: one used for the content-length, the other one for the JsonDocument.

Personally, I would not care about the content-length buffer. It is not needed as the content-length is given in the HTTP header. AsyncWeb caps it to 16k by default but it's never allocated to its max unless the actual request is as big.

The JsonObject on the other hand needs an allocation a priori. And the space of the JsonObject has little to do with its string length but more with its complexity. Eg, these 355 bytes long json needs 911 bytes for its JsonDocument
[{"fn":"a","ln":"v","c":"Boston","s":"MA","ch":[{"n":"Callie","a":5},{"n":"Griffin","a":3},{"n":"Luke","a":1}]},{"fn":"Henry","ln":"Rhodes","c":"ny","s":"NY","ch":[{"n":"Howard","a":15},{"n":"Robert","a":11}]},{"fn":"Allison","ln":"Berman","c":"la","s":"CA","ch":[{"n":"Jeff","a":35},{"n":"Roxanne","a":33},{"n":"Claudia","a":31},{"n":"Denzel","a":11}]}] (see https://arduinojson.org/v6/assistant/)

Therefore, I believe that the only buffer we should need to specify is the one for the DynamicJsonDocument, that would go in the third argument to the AsyncCallbackJsonWebHandler (currently missing here) ; and just drop the AsyncCallbackJsonWebHandler->setMaxContentLength.

This should lighten a bit the PR and the constructors code :)

Thanks again!

[rjwats]

Yes I had similar thoughts about dropping the content length while looking at this - see my previous comments on that. My theory was that the content length of the string would always be smaller than the object representation, but that's not the case. Your example is object heavy, you could just as easly have a massive string with very little actual content if it were padded with whitespace, so I think dropping the cap makes technical sense.

Good catch on the update handler, oversight! Have added that in.

[raomin]

Great! Glad we got on the same line :)
I'll test the PR and let you know.

[raomin]

Everything looks good :)
Thanks a lot for the fast reaction on this 👍👍
(i'll close when merged to master?)

[rjwats]

Super, thx for the input! I'll get this into master after work tomorrow, sleep beckons.