Some tiny fixes

[rbergen]

Description

This implements a small number of tiny fixes that were triggered by recent changes or conversations.
Concretely:

• It changes the way effects calculate the memory they need to reserve to serialize themselves to JSON.
• It fixes an invalid NUM_INFO_PAGES define for TTGO.
• It removes defines of ONSCREEN_SPECTRUM_PAGE from globals.h, because it is no longer used anywhere.
• It sanitizes the value of NUM_INFO_PAGES down to the values that are actually valid for screen.cpp (1 or 2).
• It replaces an index-based for loop in webserver.cpp by a range-based one, which makes more sense at that spot.

Contributing requirements

• I read the contribution guidelines in CONTRIBUTING.md.
• I understand the BlinkenPerBit metric, and maximized it in this PR.
• I selected main as the target branch.
• All code herein is subjected to the license terms in COPYING.txt.

[robertlipe]

Nice cleanups, but I'm still troubled by sprinkling happy little numbers around for sizing JsonDocument. Is there a way to make it more self evident why you replaced one 256 with a +128 and anodther with a +64 ? It'll help other effect creators get it right if you can point to guidance where these numbers came from.

[robertlipe]

It doesn't seem as meaningful when you replace most occurrences of magic numbers with named constants (yay!) when you then turn around and immediately add more magic numbers.
Please consider a class enum or something not as much for constraining the values (is 132 more meaningful than 128?) that at least gives some hint why some classes get different values. "JsonSize:ThreeToFiveMembers" or something else to hint why they're different would be really helpful.

[rbergen]

I understand the plea, but the answer is that this is just a massive oddity of how ArduinoJson works, and it goes too far to replicate half their documentation throughout half our codebase to try to explain how and why this works the way it does.

[rbergen]

@robertlipe

I've extended the documentation about JSON (de)serialization in the main README a little (it even has its own section now!)
That's as far I think we can take this - and should want to. It is and remains a guessing game, and that's never an exact science.

[robertlipe]

The three called methods look like they should be const if they aren't. As such, can effect be a const& ? (I wish "auto" were more "auto" in this regard, but here we are.)

[rbergen]

Maybe? But if I'm honest then I feel we're splitting hairs a bit, considering it's a local loop variable that goes out of scope 15 lines further down. Also, the comment you make is one that one could make about many similar loop variables throughout the codebase. You're welcome to own consistently implementing this change throughout the project if you want, but I'd recommend spending your time and energy on other things. :)

[robertlipe]

That's better. Thank you. My concern wasn't so much that wanting to call out the doc as it was calling out one magic number that was replaced by one magic number adding another magic constant to it. This is all effectively single threaded, right? We can't have multiple effects writing to the Jason at once. Instead od a dozen buffers that are designed to be barely big enough could we use one big one (followed by a hard throw if too big) that's safe? Should we be looking for a streaming serializer that doesn't need to keep a complete copy on core until we can write it? Should we be thinking about template voodoo to find the size of the thing we're about to serialize (which is a constant, right?) So we can create an appropriately sized one or toss the build? Thanks for listening.

…

On Thu, Oct 19, 2023, 5:43 AM Rutger van Bergen ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In include/effects/strip/bouncingballeffect.h <#463 (comment)> : > @@ -91,7 +91,7 @@ class BouncingBallEffect : public LEDStripEffect bool SerializeToJSON(JsonObject& jsonObject) override { - StaticJsonDocument<256> jsonDoc; + StaticJsonDocument<LEDStripEffect::_jsonSize + 128> jsonDoc; @robertlipe <https://github.com/robertlipe> I've extended the documentation about JSON (de)serialization in the main README a little (it even has its own section now!) That's as far I think we can take this - and should want to. It is and remains a guessing game, and that's never an exact science. — Reply to this email directly, view it on GitHub <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD37JTJT4DMWXBCK74C3YAD74FAVCNFSM6AAAAAA6ETCXIOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOBXGMZDONBTGM> . You are receiving this because you were mentioned.Message ID: ***@***.*** com>

[rbergen]

This is all effectively single threaded, right? We can't have multiple effects writing to the Jason at once. Instead of a dozen buffers that are designed to be barely big enough could we use one big one (followed by a hard throw if too big) that's safe?

No, for a reason that's also very peculiar. There's only a few ArduinoJson functions that indicate if they run out of buffer memory or not, and they're the ones that effectively add a smaller JSON document to a bigger one. That basically forces one to use the approach we use here: use small documents for individual objects (but not too small) and check for buffer overflow when adding them to the "master document", if one wants any insight into how things are progressing.

Should we be looking for a streaming serializer that doesn't need to keep a complete copy on core until we can write it?

ArduinoJson is the library to handle JSON for this platform, and I'm comfortable assuming they structure things this way for a reason.

Should we be thinking about template voodoo to find the size of the thing we're about to serialize (which is a constant, right?)

No, it depends on the contents of the serialized properties as well. Which obviously varies when strings are involved, but not only then: bigger numbers require more characters in the JSON that's created, so more buffer memory too. You could maybe cut up the guesswork into smaller chunks, but you'd still be guessing in the end. And I think the benefit gained wouldn't justify the work involved with that exercise; it seems more balanced to me to guess the "magic number" based on the collection of object properties one needs to serialize, of which there don't tend to be that many anyway in any individual case.

[robertlipe]

I know you've long let this go, but it troubles me that we could have as much as 32MB RAM (honestly, those are still expensive enough we don't use them to blink lights...but time will change that) and we have a key subsystem that silently fails unless we make meaningful guesses to statically assign buffer size. That just seems so very 1960's to me. These objects don't really leave the device and into a web interface, do they? They could be TinyCBOR or Protobuf (or a mutations of it like Capn' Proto)? What's the achilles heel configuration? A module/board with no PSRAM - so 512K, but sliced up funny - with lots of LEDs/buffers? Instead of fighting for dozens of bytes to avoid silent failure, should we just cap some of those configurations? I can hear your exasperation. I think you can hear mine (and it's that "silent failures and manual tuning are bad", not the specific tunings themselves). I'm interested in helping to solve underlying root causes - how can we avoid this? If we could safely move these into JsonAllocatableBuffers if we had another few KB of space on the heap o something, I'm willing to help find those dozens of KB somewhere. If we need to issue warnings or reduce buffer/LED maximums in some configurations, I'm open to implementing that. We shouldn't be in "fight for our ability to survive" mode in the code where we allocate static buffers for everything all of the time because we can't survive in the worst case. I'm about to leave for an appt. but if that means getting my head around https://blog.espressif.com/esp32-programmers-memory-model-259444d89387 and studying our own idf.py size-components I'm willing to dig in and help maximize what's in our heap if that would help. Likewise, if we want to experient with TinyCBOR, I'm willing to prototype that so we can measure it. Is there something architecturally I can do do help or should I just shut the hell up? :-)

…

On Thu, Oct 19, 2023 at 12:46 PM Rutger van Bergen ***@***.***> wrote: This is all effectively single threaded, right? We can't have multiple effects writing to the Jason at once. Instead of a dozen buffers that are designed to be barely big enough could we use one big one (followed by a hard throw if too big) that's safe? No, for a reason that's also very peculiar. There's only a few ArduinoJson functions that indicate if they run out of buffer memory or not, and they're the ones that effectively add a smaller JSON document to a bigger one. That basically forces one to use the approach we use here: use small documents for individual objects (but not *too* small) and check for buffer overflow when adding them to the "master document", if one wants *any* insight into how things are progressing. Should we be looking for a streaming serializer that doesn't need to keep a complete copy on core until we can write it? ArduinoJson is *the* library to handle JSON for this platform, and I'm comfortable assuming they structure things this way for a reason. Should we be thinking about template voodoo to find the size of the thing we're about to serialize (which is a constant, right?) No, it depends on the contents of the serialized properties as well. Which obviously varies when strings are involved, but not only then: bigger numbers require more characters in the JSON that's created, so more buffer memory too. You could maybe cut up the guesswork into smaller chunks, but you'd still be guessing in the end. And I think the benefit gained wouldn't justify the work involved with that exercise; it seems more balanced to me to guess the "magic number" based on the collection of object properties one needs to serialize, of which there don't tend to be that many anyway in any individual case. — Reply to this email directly, view it on GitHub <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD366JUKPGN2TS4PRIGDYAFRRFAVCNFSM6AAAAAA6ETCXIOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZRGQ2DONRYGQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rbergen]

These objects don't really leave the device and into a web interface, do they?

Quite a few do. This applies to the JSON that's generated for consumption by the web UI as well, like device and effect settings, the effect summary list, and effectively everything any of the web API endpoints returns.

If we could safely move these into JsonAllocatableBuffers if we had another few KB of space on the heap o something, I'm willing to help find those dozens of KB somewhere.

You'd still need to guess the buffer size even if it was allocated on the heap - the documentation update I'm about to merge describes this. ArduinoJson just doesn't implement "dynamic allocation as/when needed", and actually explicitly stopped supporting that at some point in the past. The exact reason why I can't remember, but is discussed somewhere in their documentation.

In short, if you want to get rid of the guesswork, you'll have to replace ArduinoJson across the board. And you're right: I'm not going to go down that route. You're welcome to do so, but I'd only consider a change that implements the replacement comprehensively. If you consider pursuing that, do realize the "pick your buffer size before you start" pattern extends to the web server library we use as well, as it also uses ArduinoJson for the JSON handling it does.

Is there something architecturally I can do do help or should I just shut the hell up? :-)

It's not my style to tell people to shut up, but there is an expression that recommends one to "choose your battles". If you ask me, I'd let this one go.

[robertlipe]

I'd be willing to replace it with TinyCBOR in the C++ code to solve this problem, but I don't do enough web programming to get that over the line. I really like how their own doc tells you not to worry about it, omitting that if you guess wrong it silently fails. https://arduinojson.org/v5/faq/how-to-determine-the-buffer-size/#dont-overthink-this-problem Yet elsewhere, their doc says that sizing buffers is out anyway and "no need to specify the capacity anymore." https://arduinojson.org/v7/how-to/upgrade-from-v5/#jsondocument Whatever. This approach gives me the creeps, but it's not in my power to replace it or even improve it, it sounds, I surrender and am moving on.

…

On Thu, Oct 19, 2023 at 7:00 PM Rutger van Bergen ***@***.***> wrote: These objects don't really leave the device and into a web interface, do they? Quite a few do. This applies to the JSON that's generated for consumption by the web UI as well, like device and effect settings, the effect summary list, and effectively everything any of the web API endpoints returns. If we could safely move these into JsonAllocatableBuffers if we had another few KB of space on the heap o something, I'm willing to help find those dozens of KB somewhere. You'd still need to guess the buffer size even if it was allocated on the heap - the documentation update I'm about to merge describes this. ArduinoJson just doesn't implement "dynamic allocation as/when needed", and actually explicitly *stopped* supported that at some point in the past. The exact reason why I can't remember, but is discussed somewhere in their documentation. In short, if you want to get rid of the guesswork, you'll have to replace ArduinoJson across the board. And you're right: I'm not going to go down that route. You're welcome to do so, but I'd only consider a change that implements the replacement comprehensively. If you consider pursuing that, do realize the "pick your buffer size before you start" pattern extends to the web server library we use as well, as it also uses ArduinoJson for the JSON handling it does. Is there something architecturally I can do do help or should I just shut the hell up? :-) It's not my style to tell people to shut up, but there is an expression that recommends one to "choose your battles". If you ask me, I'd let this one go. — Reply to this email directly, view it on GitHub <#463 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACCSD3Z4S45ATAZFJXBYDC3YAG5JBAVCNFSM6AAAAAA6ETCXIOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONZRHA3DCNBXHE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[rbergen]

This is actually not a bad find on your part, because that documentation concerns ArduinoJson version 7, which is so new it hasn't even propagated to the Arduino library manager yet. We're using version 6.

Of course, version 7 comes with its own warning, being that it's a lot bigger than earlier versions. That's something we'd have to look into, in the sense that the projects we now have still need to fit on the devices we support, at least in principle. Also, we'd have to wait for ESPAsyncWebServer to be ported to ArduinoJson version 7, because including two ArduinoJson versions is the road to madness.

To be continued, at some point.

[rbergen]

Merging after confirmed working on Spectrum.