use arduinoJson for fields #216
Conversation
Use a pre-allocated DynamicJsonDocument to more easily create the JSON structure. As a nice side effect, it also performs only two allocations: One for the DynamicJsonDocument, One for `result.reserve()` call. The old function had 28 lines of the form: `(String)json += "foo" + (String)bar() + ","` Each line creates three temporary String objects: (e.g., "foo", result from bar()), ","). It then appends "foo" + "bar" (often another allocation), then appends the trailing close-quote and comma (unlikely, but potentially another allocation). Finally, the first temporary variable (now "foo : bar,") is appended to the current final result string (28 allocations).
The anonymous/unnamed namespace ensures no unintended pollution of the global namespace, and makes clear what the external API for this file contains. This makes it much easier to change the underlying implementation, as it's guaranteed that changes internal to the namespace won't break other source files. Use of a scoped enum for field type enables compiler warnings. Specifically, use of a `switch` statement with no `default` case will generate a compiler warning/error if missing any values. Tested to generate 100% identical JSON on Fib256.
Confirmed identical JSON output for at least Fib256.
Verified identical JSON output on at least Fib256.
|
@tobi01001 -- Awesome review, I really appreciate your ability to quickly pinpoint those additional areas for improvement! |
|
Hi @jasoncoon -- Are you good with these changes? |
|
I'm working on adding Fibonacci1024, see thread on Twitter here: https://twitter.com/jasoncoon_/status/1461852385656160261 The |
|
Hi Jason, I'm interested! Please message me on Tindie with details on expected costs, etc? You can use ArduinoJSON Assistant to help figure out the size of the buffer needed. I entered the longest possible strings in each field, then added ~100 bytes. Looks like I didn't add enough. Would it make sense to split the API? Yes, at least for pattern / palette lists. Maybe define a new data type, which is a string representing a relative URI, where the caller calls that URI as a REST API to get the additional data? I'm sure there are "correct" ways to do this in REST, but I'm not deeply familiar with how REST APIs normally self-document.... |
|
I do believe that this is good but maybe not neccessary ;-) The So rather than increasing the JSON Buffer I propose to:
/all is fixed as long as there is no change in the software affecting the fields and its contents. So why not letting the ESP app writing the config to a file on the LittleFS (e.g. config_all.json). And all the app would have to do with the API call to |
|
Caveats which Jason should confirm, but I will presume as true for this post: It's OK to break compatibilty withe old REST APIs.
If yes, this means it's OK to break compatibility if a 3rd party had used the REST APIs, such as using custom It's OK to take a permanent dependent on having LittleFS storage.
If yes, this means boards that don't have LittleFS (or SPIFFS) support will not work. I don't foresee this being a concern, but it's better to be sure. @tobi01001's solution is excellentIn fact, if Jason confirms those are OK, then it'll be time to drop the "EEPROM" code altogether, in favor of reading/writing the parameters through LittleFS, using ArduinoJson. |
|
It's definitely OK with me to break compatibility with old REST APIs. Might need to check in with @balloob to see if there's a good way to prevent breaking the HomeAssistant integration: https://github.com/home-assistant/core/tree/dev/homeassistant/components/evil_genius_labs Permanent LittleFS dependency is also fine with me. It'd be great if this could be done in a way that wouldn't require having to update multiple places in the code and/or file when new patterns/palettes are added, updated, renamed, etc. I also think the patterns and palettes list should probably be separate requests, as they are each so large. |
|
Can we add a version number to the API so I can differentiate the different APIs? |
|
@henrygab For easier maintenance and for the sake of reliability I would suggest that (modifiable) settings to be stored are managed in a structure like e.g.: structuretypedef struct {
uint8_t settingsMagicByte = SETTINGS_MAGIC_BYTE;
CRGB solidColor = CRGB::Blue;
uint8_t speed = 30;
uint8_t power = 1;
uint8_t brightness = brightnessMap[brightnessIndex];
uint8_t cooling = 49;
uint8_t sparking = 60;
uint8_t currentPatternIndex = DEFAULT_PATTERN_INDEX; // Index number of which pattern is current
uint8_t autoplay = 0;
uint8_t autoplayDuration = 10;
uint8_t showClock = 0;
uint8_t clockBackgroundFade = 160;
uint8_t utcOffsetIndex = 24;
uint8_t currentPaletteIndex = 0;
uint8_t twinkleSpeed = 4;
uint8_t twinkleDensity = 5;
uint8_t coolLikeIncandescent = 1;
} strEEPROMSettings;This way, the settings management incl. EEPROM write and read is just: HandlingstrEEPROMSettings EEPROMSettings;
...
EEPROM.begin(sizeof(EEPROMSettings))
...
void readSettings()
{
// check for "magic number" so we know settings have been written to EEPROM
// and it's not just full of random bytes
if (EEPROM.read(0) != SETTINGS_MAGIC_BYTE) {
return;
}
EEPROM.get(0, EEPROMSettings);
setUtcOffsetIndex(EEPROMSettings.utcOffsetIndex);
}
void writeAndCommitSettings() {
EEPROMSettings.settingsMagicByte = SETTINGS_MAGIC_BYTE;
EEPROM.put(0, EEPROMSettings);
EEPROM.commit();
}EDIT: The EEPROM uses a reserved section in the flash where a complete sector needs to be erased and rewritten. Reliability could further be increased by using e.g. EEPROM_Rotate which is basically a drop-in replacement of the EEPROM class and which will cycle through different sectors... Long story short: As the EEPROM is only emulated in the flash, I see no benefit (other than maybe readability and code maintenance) to switch to LittleFS with file sytem overhead for the settings to be saved and reread quite often... @jasoncoon If Fields (w/o values) are all written / stored in a file, I think there is no need to separate the "options" for patterns and palettes - unless one would only need to read those. But this could go into a separate API call. The webserver does serve all the other files (which are bigger in size) wihtout issues and so it can also serve this one easily (and quick). |
|
@tobi01001 just so I'm clear, we're talking about the fields (w/o values) being written/stored in a file automatically/dynamically, in setup, on startup? Not maintained separately in a static file that would need to be kept in sync with the patterns, palettes, etc in code? |
|
Yes, exactly. I did this already but as I am personally on ArduinoJson V5 and AsyncWebserver (and very limited time currently) I cannot come up with a PR on the fly... ;-) In my case with AsyncWebServer this would be as easy as: Update Filevoid getAllJSON(JsonArray &arr)
{
for (uint8_t i = 0; i < getFieldCount(); i++)
{
Field field = fields[i];
JsonObject& obj = arr.createNestedObject();
obj[F("name")] = field.name;
obj[F("label")] = field.label;
obj[F("type")] = (int)field.type;
if (field.type == NumberFieldType)
{
obj[F("min")] = field.min;
obj[F("max")] = field.max;
}
if (field.getOptions)
{
JsonArray &ar = obj.createNestedArray(F("options"));
field.getOptions(ar);
}
}
}
void updateConfigFile(void)
{
DynamicJsonBuffer buffer;
JsonArray& root = buffer.createArray();
getAllJSON(root);
File config_Json = LittleFS.open(F("/config/config_all.json"), "w");
root.printTo(config_Json);
config_Json.close();
}
Serve the file...
server.on("/all", HTTP_GET, [](AsyncWebServerRequest *request) {
if(!LittleFS.exists("/config/config_all.json"))
{
updateConfigFile();
}
request->send(LittleFS, "/config/config_all.json", "application/json");
});
server.on("/allvalues", HTTP_GET, [](AsyncWebServerRequest *request) {
AsyncJsonResponse * response = new AsyncJsonResponse();
JsonObject &root = response->getRoot();
JsonArray &arr = root.createNestedArray("values");
if(!getAllValuesJSONArray(arr))
{
JsonObject &obj = arr.createNestedObject();
obj[F("ValueError")] = F("Did not read any values!");
}
response->setLength();
request->send(response);
});And the app.js needs to perform a second request to e.g /allvalues to update the web app to the current values with something like: app.js $.get(urlBase + "all", function(data) {
})
.fail(function(errorThrown) {
console.log(errorThrown);
})
.done(function(name, value, test) {
$("#status").html("Done...");
$.get(urlBase + "allvalues", function(data) {
$("#status").html("Loading, current values...");
for(i=0; i<data.values.length; i++) {
updateFieldValue( data.values[i].name, data.values[i].value);
}
})
.fail(function(errorThrown) {
console.log("error: " + errorThrown);
})
.done(function(name, value, test) {
$("#status").html("Ready");
});
}); |
|
Having two methods to read/parse/store configuration (EEPROM and JSON) is going to be error-prone, and increases maintenance. I still think it makes sense to store any user-configurable data in Any objection if I work towards Tobi's solution, ditch all the EEPROM code, and store only in the file system? |
|
OK, I will open a new issue to track that work. Thanks! |
Yes, that's a fine idea. |
This should reduce heap fragmentation (by constant appending of small strings).
Uses arduinoJson to simplify (and make safer) the generation of JSON. Uses custom converter for
fieldstructure to make easy to use.Also explicitly makes most of the code in
fields.cppinternal, to ensure safe to modify the internals. Only public APIs are put outside the namespace.There's also some minor changes (removing
autoreferences in map.cpp) in anticipation of enabling ESP32.