Skip to content

Commit

Permalink
Proof-of-concept: Remove oappend
Browse files Browse the repository at this point in the history 
Remove the large stack buffer as we're just going to copy it in to a
heap buffer anyways.  Later we can refine the length estimation or use a
rope-style dynamic data structure like DynamicBufferList.
  • Loading branch information
@willmmiles
willmmiles committed Sep 6, 2024
1 parent dfcfeaf commit 4dbe596
Show file tree
Hide file tree
Showing 8 changed files with 472 additions and 525 deletions.
74 changes: 37 additions & 37 deletions usermods/audioreactive/audio_reactive.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1885,57 +1885,57 @@ class AudioReactive : public Usermod {
}


void appendConfigData() override
void appendConfigData(Print* dest) override
{
#ifdef ARDUINO_ARCH_ESP32
oappend(SET_F("dd=addDropdown('AudioReactive','digitalmic:type');"));
dest->print(SET_F("dd=addDropdown('AudioReactive','digitalmic:type');"));
#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
oappend(SET_F("addOption(dd,'Generic Analog',0);"));
dest->print(SET_F("addOption(dd,'Generic Analog',0);"));
#endif
oappend(SET_F("addOption(dd,'Generic I2S',1);"));
oappend(SET_F("addOption(dd,'ES7243',2);"));
oappend(SET_F("addOption(dd,'SPH0654',3);"));
oappend(SET_F("addOption(dd,'Generic I2S with Mclk',4);"));
dest->print(SET_F("addOption(dd,'Generic I2S',1);"));
dest->print(SET_F("addOption(dd,'ES7243',2);"));
dest->print(SET_F("addOption(dd,'SPH0654',3);"));
dest->print(SET_F("addOption(dd,'Generic I2S with Mclk',4);"));
#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3)
oappend(SET_F("addOption(dd,'Generic I2S PDM',5);"));
dest->print(SET_F("addOption(dd,'Generic I2S PDM',5);"));
#endif
oappend(SET_F("addOption(dd,'ES8388',6);"));
dest->print(SET_F("addOption(dd,'ES8388',6);"));

oappend(SET_F("dd=addDropdown('AudioReactive','config:AGC');"));
oappend(SET_F("addOption(dd,'Off',0);"));
oappend(SET_F("addOption(dd,'Normal',1);"));
oappend(SET_F("addOption(dd,'Vivid',2);"));
oappend(SET_F("addOption(dd,'Lazy',3);"));

oappend(SET_F("dd=addDropdown('AudioReactive','dynamics:limiter');"));
oappend(SET_F("addOption(dd,'Off',0);"));
oappend(SET_F("addOption(dd,'On',1);"));
oappend(SET_F("addInfo('AudioReactive:dynamics:limiter',0,' On ');")); // 0 is field type, 1 is actual field
oappend(SET_F("addInfo('AudioReactive:dynamics:rise',1,'ms <i>(&#x266A; effects only)</i>');"));
oappend(SET_F("addInfo('AudioReactive:dynamics:fall',1,'ms <i>(&#x266A; effects only)</i>');"));

oappend(SET_F("dd=addDropdown('AudioReactive','frequency:scale');"));
oappend(SET_F("addOption(dd,'None',0);"));
oappend(SET_F("addOption(dd,'Linear (Amplitude)',2);"));
oappend(SET_F("addOption(dd,'Square Root (Energy)',3);"));
oappend(SET_F("addOption(dd,'Logarithmic (Loudness)',1);"));
dest->print(SET_F("dd=addDropdown('AudioReactive','config:AGC');"));
dest->print(SET_F("addOption(dd,'Off',0);"));
dest->print(SET_F("addOption(dd,'Normal',1);"));
dest->print(SET_F("addOption(dd,'Vivid',2);"));
dest->print(SET_F("addOption(dd,'Lazy',3);"));

dest->print(SET_F("dd=addDropdown('AudioReactive','dynamics:limiter');"));
dest->print(SET_F("addOption(dd,'Off',0);"));
dest->print(SET_F("addOption(dd,'On',1);"));
dest->print(SET_F("addInfo('AudioReactive:dynamics:limiter',0,' On ');")); // 0 is field type, 1 is actual field
dest->print(SET_F("addInfo('AudioReactive:dynamics:rise',1,'ms <i>(&#x266A; effects only)</i>');"));
dest->print(SET_F("addInfo('AudioReactive:dynamics:fall',1,'ms <i>(&#x266A; effects only)</i>');"));

dest->print(SET_F("dd=addDropdown('AudioReactive','frequency:scale');"));
dest->print(SET_F("addOption(dd,'None',0);"));
dest->print(SET_F("addOption(dd,'Linear (Amplitude)',2);"));
dest->print(SET_F("addOption(dd,'Square Root (Energy)',3);"));
dest->print(SET_F("addOption(dd,'Logarithmic (Loudness)',1);"));
#endif

oappend(SET_F("dd=addDropdown('AudioReactive','sync:mode');"));
oappend(SET_F("addOption(dd,'Off',0);"));
dest->print(SET_F("dd=addDropdown('AudioReactive','sync:mode');"));
dest->print(SET_F("addOption(dd,'Off',0);"));
#ifdef ARDUINO_ARCH_ESP32
oappend(SET_F("addOption(dd,'Send',1);"));
dest->print(SET_F("addOption(dd,'Send',1);"));
#endif
oappend(SET_F("addOption(dd,'Receive',2);"));
dest->print(SET_F("addOption(dd,'Receive',2);"));
#ifdef ARDUINO_ARCH_ESP32
oappend(SET_F("addInfo('AudioReactive:digitalmic:type',1,'<i>requires reboot!</i>');")); // 0 is field type, 1 is actual field
oappend(SET_F("addInfo('AudioReactive:digitalmic:pin[]',0,'<i>sd/data/dout</i>','I2S SD');"));
oappend(SET_F("addInfo('AudioReactive:digitalmic:pin[]',1,'<i>ws/clk/lrck</i>','I2S WS');"));
oappend(SET_F("addInfo('AudioReactive:digitalmic:pin[]',2,'<i>sck/bclk</i>','I2S SCK');"));
dest->print(SET_F("addInfo('AudioReactive:digitalmic:type',1,'<i>requires reboot!</i>');")); // 0 is field type, 1 is actual field
dest->print(SET_F("addInfo('AudioReactive:digitalmic:pin[]',0,'<i>sd/data/dout</i>','I2S SD');"));
dest->print(SET_F("addInfo('AudioReactive:digitalmic:pin[]',1,'<i>ws/clk/lrck</i>','I2S WS');"));
dest->print(SET_F("addInfo('AudioReactive:digitalmic:pin[]',2,'<i>sck/bclk</i>','I2S SCK');"));
#if !defined(CONFIG_IDF_TARGET_ESP32S2) && !defined(CONFIG_IDF_TARGET_ESP32C3) && !defined(CONFIG_IDF_TARGET_ESP32S3)
oappend(SET_F("addInfo('AudioReactive:digitalmic:pin[]',3,'<i>only use -1, 0, 1 or 3</i>','I2S MCLK');"));
dest->print(SET_F("addInfo('AudioReactive:digitalmic:pin[]',3,'<i>only use -1, 0, 1 or 3</i>','I2S MCLK');"));
#else
oappend(SET_F("addInfo('AudioReactive:digitalmic:pin[]',3,'<i>master clock</i>','I2S MCLK');"));
dest->print(SET_F("addInfo('AudioReactive:digitalmic:pin[]',3,'<i>master clock</i>','I2S MCLK');"));
#endif
#endif
}
Expand Down
14 changes: 6 additions & 8 deletions wled00/fcn_declare.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,7 +302,7 @@ class Usermod {
virtual bool handleButton(uint8_t b) { return false; } // button overrides are possible here
virtual bool getUMData(um_data_t **data) { if (data) *data = nullptr; return false; }; // usermod data exchange [see examples for audio effects]
virtual void connected() {} // called when WiFi is (re)connected
virtual void appendConfigData() {} // helper function called from usermod settings page to add metadata for entry fields
virtual void appendConfigData(Print*) {} // helper function called from usermod settings page to add metadata for entry fields
virtual void addToJsonState(JsonObject& obj) {} // add JSON objects for WLED state
virtual void addToJsonInfo(JsonObject& obj) {} // add JSON objects for UI Info page
virtual void readFromJsonState(JsonObject& obj) {} // process JSON messages received from web server
Expand All @@ -328,7 +328,7 @@ class UsermodManager {
bool getUMData(um_data_t **um_data, uint8_t mod_id = USERMOD_ID_RESERVED); // USERMOD_ID_RESERVED will poll all usermods
void setup();
void connected();
void appendConfigData();
void appendConfigData(Print*);
void addToJsonState(JsonObject& obj);
void addToJsonInfo(JsonObject& obj);
void readFromJsonState(JsonObject& obj);
Expand Down Expand Up @@ -362,10 +362,8 @@ void parseNumber(const char* str, byte* val, byte minv=0, byte maxv=255);
bool getVal(JsonVariant elem, byte* val, byte minv=0, byte maxv=255);
bool getBoolVal(JsonVariant elem, bool dflt);
bool updateVal(const char* req, const char* key, byte* val, byte minv=0, byte maxv=255);
bool oappend(const char* txt); // append new c string to temp buffer efficiently
bool oappendi(int i); // append new number to temp buffer efficiently
void sappend(char stype, const char* key, int val);
void sappends(char stype, const char* key, char* val);
void sappend(Print* dest, char stype, const char* key, int val);
void sappends(Print* dest, char stype, const char* key, char* val);
void prepareHostname(char* hostname);
bool isAsterisksOnly(const char* str, byte maxLen);
bool requestJSONBufferLock(uint8_t module=255);
Expand Down Expand Up @@ -443,7 +441,7 @@ void wsEvent(AsyncWebSocket * server, AsyncWebSocketClient * client, AwsEventTyp
void sendDataWs(AsyncWebSocketClient * client = nullptr);

//xml.cpp
void XML_response(AsyncWebServerRequest *request, char* dest = nullptr);
void getSettingsJS(byte subPage, char* dest);
void XML_response(Print* dest);
void getSettingsJS(byte subPage, Print* dest);

#endif
7 changes: 4 additions & 3 deletions wled00/mqtt.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,12 @@ void publishMqtt()
strcat_P(subuf, PSTR("/status"));
mqtt->publish(subuf, 0, true, "online"); // retain message for a LWT

char apires[1024]; // allocating 1024 bytes from stack can be risky
XML_response(nullptr, apires);
DynamicBuffer buf(1024);
BufferPrint<DynamicBuffer> pbuf(buf);
XML_response(&pbuf);
strlcpy(subuf, mqttDeviceTopic, 33);
strcat_P(subuf, PSTR("/v"));
mqtt->publish(subuf, 0, retainMqttMsg, apires); // optionally retain message (#2263)
mqtt->publish(subuf, 0, retainMqttMsg, buf.data(), pbuf.size()); // optionally retain message (#2263)
#endif
}

Expand Down
6 changes: 5 additions & 1 deletion wled00/set.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1196,7 +1196,11 @@ bool handleSet(AsyncWebServerRequest *request, const String& req, bool apply)

// internal call, does not send XML response
pos = req.indexOf(F("IN"));
if (pos < 1) XML_response(request);
if (pos < 1) {
auto response = request->beginResponseStream("text/xml");
XML_response(response);
request->send(response);
}

return true;
}
2 changes: 1 addition & 1 deletion wled00/um_manager.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ void UsermodManager::setup() { for (unsigned i = 0; i < numMods; i++
void UsermodManager::connected() { for (unsigned i = 0; i < numMods; i++) ums[i]->connected(); }
void UsermodManager::loop() { for (unsigned i = 0; i < numMods; i++) ums[i]->loop(); }
void UsermodManager::handleOverlayDraw() { for (unsigned i = 0; i < numMods; i++) ums[i]->handleOverlayDraw(); }
void UsermodManager::appendConfigData() { for (unsigned i = 0; i < numMods; i++) ums[i]->appendConfigData(); }
void UsermodManager::appendConfigData(Print* dest) { for (unsigned i = 0; i < numMods; i++) ums[i]->appendConfigData(dest); }
bool UsermodManager::handleButton(uint8_t b) {
bool overrideIO = false;
for (unsigned i = 0; i < numMods; i++) {
Expand Down
69 changes: 12 additions & 57 deletions wled00/util.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,88 +89,43 @@ bool updateVal(const char* req, const char* key, byte* val, byte minv, byte maxv


//append a numeric setting to string buffer
void sappend(char stype, const char* key, int val)
void sappend(Print* dest, char stype, const char* key, int val)
{
char ds[] = "d.Sf.";

const __FlashStringHelper* type_str;
switch(stype)
{
case 'c': //checkbox
oappend(ds);
oappend(key);
oappend(".checked=");
oappendi(val);
oappend(";");
type_str = F(".checked=");
break;
case 'v': //numeric
oappend(ds);
oappend(key);
oappend(".value=");
oappendi(val);
oappend(";");
type_str = F(".value=");
break;
case 'i': //selectedIndex
oappend(ds);
oappend(key);
oappend(SET_F(".selectedIndex="));
oappendi(val);
oappend(";");
type_str = F(".selectedIndex=");
break;
default:
return; //???
}

dest->printf_P(PSTR("d.Sf.%c%s%s%d;"), stype, key, type_str, val);
}


//append a string setting to buffer
void sappends(char stype, const char* key, char* val)
void sappends(Print* dest, char stype, const char* key, char* val)
{
switch(stype)
{
case 's': {//string (we can interpret val as char*)
String buf = val;
//convert "%" to "%%" to make EspAsyncWebServer happy
//buf.replace("%","%%");
oappend("d.Sf.");
oappend(key);
oappend(".value=\"");
oappend(buf.c_str());
oappend("\";");
dest->printf_P(PSTR("d.Sf.%s.value=\"%s\";"),key,val);
break;}
case 'm': //message
oappend(SET_F("d.getElementsByClassName"));
oappend(key);
oappend(SET_F(".innerHTML=\""));
oappend(val);
oappend("\";");
dest->printf_P(PSTR("d.getElementsByClassName%s.innerHTML=\"%s\";"), key, val);
break;
}
}


bool oappendi(int i)
{
char s[12]; // 32bit signed number can have 10 digits plus - sign
sprintf(s, "%d", i);
return oappend(s);
}


bool oappend(const char* txt)
{
unsigned len = strlen(txt);
if ((obuf == nullptr) || (olen + len >= SETTINGS_STACK_BUF_SIZE)) { // sanity checks
#ifdef WLED_DEBUG
DEBUG_PRINT(F("oappend() buffer overflow. Cannot append "));
DEBUG_PRINT(len); DEBUG_PRINT(F(" bytes \t\""));
DEBUG_PRINT(txt); DEBUG_PRINTLN(F("\""));
#endif
return false; // buffer full
}
strcpy(obuf + olen, txt);
olen += len;
return true;
}


void prepareHostname(char* hostname)
{
sprintf_P(hostname, PSTR("wled-%*s"), 6, escapedMac.c_str() + 6);
Expand Down
18 changes: 7 additions & 11 deletions wled00/wled_server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -512,27 +512,23 @@ void serveJsonError(AsyncWebServerRequest* request, uint16_t code, uint16_t erro

void serveSettingsJS(AsyncWebServerRequest* request)
{
char buf[SETTINGS_STACK_BUF_SIZE+37];
buf[0] = 0;
byte subPage = request->arg(F("p")).toInt();
if (subPage > 10) {
strcpy_P(buf, PSTR("alert('Settings for this request are not implemented.');"));
request->send(501, FPSTR(CONTENT_TYPE_JAVASCRIPT), buf);
request->send_P(501, FPSTR(CONTENT_TYPE_JAVASCRIPT), PSTR("alert('Settings for this request are not implemented.');"));
return;
}
if (subPage > 0 && !correctPIN && strlen(settingsPIN)>0) {
strcpy_P(buf, PSTR("alert('PIN incorrect.');"));
request->send(401, FPSTR(CONTENT_TYPE_JAVASCRIPT), buf);
request->send_P(401, FPSTR(CONTENT_TYPE_JAVASCRIPT), PSTR("alert('PIN incorrect.');"));
return;
}
strcat_P(buf,PSTR("function GetV(){var d=document;"));
getSettingsJS(subPage, buf+strlen(buf)); // this may overflow by 35bytes!!!
strcat_P(buf,PSTR("}"));

AsyncWebServerResponse *response;
response = request->beginResponse(200, FPSTR(CONTENT_TYPE_JAVASCRIPT), buf);
AsyncResponseStream *response = request->beginResponseStream(FPSTR(CONTENT_TYPE_JAVASCRIPT));
response->addHeader(F("Cache-Control"), F("no-store"));
response->addHeader(F("Expires"), F("0"));

response->print(F("function GetV(){var d=document;"));
getSettingsJS(subPage, response);
response->print(F("}"));
request->send(response);
}

Expand Down
Loading

5 comments on commit 4dbe596

@blazoncek
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This already looks wonderful!
Did you check before and after code size? I can see that it could be way smaller.

@willmmiles
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I haven't checked the code size yet. I expect this formulation will be slightly larger than before - we save a few bytes for oappend itself, but we'll likely pay for it with all the vtable lookups on the Print object. We could go through and replace a lot of the small append constructions with printf_P()s or the like which could reduce the call count by quite a bit, though. I tried a test example in sappend() to reduce some of the code duplication there, but the principle could be applied more broadly.

@willmmiles
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I'd suspected, this build is actually slightly bigger -- 2k on ESP8266, 1k on ESP32. (Apologies for my custom envs.) It can likely be improved a lot by using printfs instead, though.

nodemcv2_debug
aws-queue
RAM: [====== ] 57.9% (used 47420 bytes from 81920 bytes)
Flash: [========= ] 91.0% (used 950663 bytes from 1044464 bytes)
end-oappend
RAM: [====== ] 57.8% (used 47380 bytes from 81920 bytes)
Flash: [========= ] 91.2% (used 952147 bytes from 1044464 bytes)

esp32_new_ar
aws-queue
RAM: [== ] 18.0% (used 59104 bytes from 327680 bytes)
Flash: [==========] 95.3% (used 1499681 bytes from 1572864 bytes)
end-oappend
RAM: [== ] 18.0% (used 59104 bytes from 327680 bytes)
Flash: [==========] 95.4% (used 1500637 bytes from 1572864 bytes)

@blazoncek
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But the RAM decreased on ESP8266 which is excellent (if that is not attributed to some other change).
AFAIC this is another magnificent optimisation across the board.
If you can PR this to 0_15 (also replacing oappendi() in some usermods) I think we can work together to consolidate those strings.

@willmmiles
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hesitate to use the term optimization - it's a different tradeoff, a little more (and most likely slower) code for the benefit of using dynamic buffers. Glad you're still interested, I'll polish it up for PR today or tomorrow - I want to do a proper before and after comparison on all the XML responses to ensure they are unchanged. I also tweaked the AsyncStreamResponse for better allocator performance as well as doing early buffer free()s. Finally the MQTT case needs a flat buffer Print class that there's a thousand implementations of, but I haven't found one in one of our existing deps yet - so I added one to my development AsyncWebServer, but I might just inline it in the MQTT code to avoid the dependency change.

The ESP8266 RAM change is entirely from removing some of the short non-F strings. All those 2 byte shared string literals really do add up to a few k of RAM.

The latest tip gets:

nodemcuv2_debug
RAM: [====== ] 57.8% (used 47352 bytes from 81920 bytes) (-68 from aws-queue)
Flash: [========= ] 91.0% (used 950783 bytes from 1044464 bytes) (+120 from aws-queue)

esp32_new_ar
RAM: [== ] 18.0% (used 59096 bytes from 327680 bytes) (-8 from aws-queue)
Flash: [==========] 95.3% (used 1499265 bytes from 1572864 bytes) (-416 from aws-queue)

I did make some choices to improve the sizes at the cost of code readability, though. :(

Please sign in to comment.