Skip to content

F() for ESP8266WebServer.cpp.patch #4323

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

Closed
bstsoftorg opened this issue Feb 8, 2018 · 4 comments
Closed

F() for ESP8266WebServer.cpp.patch #4323

bstsoftorg opened this issue Feb 8, 2018 · 4 comments

Comments

@bstsoftorg
Copy link

not found how to attach file ESP8266WebServer.cpp.patch
before 41424 (50%) = 40496
after 41408 (50%) = 40512
need commit

Index: ESP8266WebServer.cpp
===================================================================
--- ESP8266WebServer.cpp	(revision 3662)
+++ ESP8266WebServer.cpp	(working copy)
@@ -128,7 +128,7 @@
         delete[] toencode;
         return false;
       }
-      sprintf(toencode, "%s:%s", username, password);
+      sprintf(toencode, String(F("%s:%s")).c_str(), username, password);
       if(base64_encode_chars(toencode, toencodeLen, encoded) > 0 && authReq.equalsConstantTime(encoded)) {
         authReq = "";
         delete[] toencode;
@@ -218,7 +218,7 @@
   char buffer[33];  // buffer to hold 32 Hex Digit + /0
   int i;
   for(i = 0; i < 4; i++) {
-    sprintf (buffer + (i*8), "%08x", RANDOM_REG32);
+    sprintf (buffer + (i*8), String(F("%08x")).c_str(), RANDOM_REG32);
   }
   return String(buffer);
 }
@@ -351,7 +351,7 @@
   String headerLine = name;
   headerLine += F(": ");
   headerLine += value;
-  headerLine += "\r\n";
+  headerLine += F("\r\n");
 
   if (first) {
     _responseHeaders = headerLine + _responseHeaders;
@@ -370,7 +370,7 @@
     response += String(code);
     response += ' ';
     response += _responseCodeToString(code);
-    response += "\r\n";
+    response += F("\r\n");
 
     using namespace mime;
     if (!content_type)
@@ -390,7 +390,7 @@
     sendHeader(String(F("Connection")), String(F("close")));
 
     response += _responseHeaders;
-    response += "\r\n";
+    response += F("\r\n");
     _responseHeaders = "";
 }
 
@@ -438,12 +438,12 @@
 }
 
 void ESP8266WebServer::sendContent(const String& content) {
-  const char * footer = "\r\n";
+  const char * footer = String(F("\r\n")).c_str();
   size_t len = content.length();
   if(_chunked) {
     char * chunkSize = (char *)malloc(11);
     if(chunkSize){
-      sprintf(chunkSize, "%x%s", len, footer);
+      sprintf(chunkSize, String(F("%x%s")).c_str(), len, footer);
       _currentClientWrite(chunkSize, strlen(chunkSize));
       free(chunkSize);
     }
@@ -462,11 +462,11 @@
 }
 
 void ESP8266WebServer::sendContent_P(PGM_P content, size_t size) {
-  const char * footer = "\r\n";
+  const char * footer = String(F("\r\n")).c_str();
   if(_chunked) {
     char * chunkSize = (char *)malloc(11);
     if(chunkSize){
-      sprintf(chunkSize, "%x%s", size, footer);
+      sprintf(chunkSize, String(("%x%s")).c_str(), size, footer);
       _currentClientWrite(chunkSize, strlen(chunkSize));
       free(chunkSize);
     }
@igrr
Copy link
Member

igrr commented Feb 8, 2018

Please note that the behaviour of String(F("text")).c_str() is undefined, since the lifetime of the String object is shorter than the function call. In single threaded systems which don't do much memory allocation you are probably fine, but not a good coding practice.

I think i saw this construct in our code base already, but unfortunately GCC 4 (or 5, for that matter) doesn't know how to generate warnings for such cases.

When igrr/newlib-xtensa#5 is merged, it will be possible to pass progmem strings directly to sprintf though, so the String(F(...)).c_str construct will not be needed.

@bstsoftorg
Copy link
Author

It's clear as the white snow. But we do not see a strong influence on the work. It is clear that RAM is much faster, but it is terribly lacking. After loading the initial settings, 15,000 bytes remain, and at less than 10,000 bytes, the WEB starts to fail.

PS who can from conducting "commit" in Russian to communicate, you will contact me. [email protected]

@devyte
Copy link
Collaborator

devyte commented Feb 16, 2018

The String().c_str() are indeed a bad construct, given that the returned pointer is provided by a temp object.
I am going to leave this open to cover the changes needed to *printf() format strings once the newlib changes are merged.

@earlephilhower
Copy link
Collaborator

#4371 did movement of most strings where it made sense to PMEM. Nothing left to do that pops out at me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants