Skip to content

Move String constants to FLASH from heap/SRAM (Esp.cpp) #5037

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
jindrichsirucek opened this issue Aug 13, 2018 · 15 comments
Closed

Move String constants to FLASH from heap/SRAM (Esp.cpp) #5037

jindrichsirucek opened this issue Aug 13, 2018 · 15 comments

Comments

@jindrichsirucek
Copy link

Hello, I have quite big project and dealing with small heap, so I did big PROGMEM modifications of my code to release HEAP.
When I was moving string constants to flash in my program I have found s. consts. in basic arduino framework like here:

sprintf(&buff[0], "Fatal exception:%d flag:%d (%s) epc1:0x%08x epc2:0x%08x epc3:0x%08x excvaddr:0x%08x depc:0x%08x", resetInfo.exccause, resetInfo.reason, (resetInfo.reason == 0 ? "DEFAULT" : resetInfo.reason == 1 ? "WDT" : resetInfo.reason == 2 ? "EXCEPTION" : resetInfo.reason == 3 ? "SOFT_WDT" : resetInfo.reason == 4 ? "SOFT_RESTART" : resetInfo.reason == 5 ? "DEEP_SLEEP_AWAKE" : resetInfo.reason == 6 ? "EXT_SYS_RST" : "???"), resetInfo.epc1, resetInfo.epc2, resetInfo.epc3, resetInfo.excvaddr, resetInfo.depc);

So I suggest to put them in FLASH to save precious RAM. I didnt figure out how to make PR; neigther I know how to correctly (best optimized way) put these strings to FLASH.

In my local code I use this:
sprintf(&buff[0], String(F("Fatal exception:%d flag:%d (%s) epc1:0x%08x epc2:0x%08x epc3:0x%08x excvaddr:0x%08x depc:0x%08x")).c_str(), resetInfo.exccause, resetInfo.reason, (resetInfo.reason == 0 ? "DEFAULT" : resetInfo.reason == 1 ? "WDT" : resetInfo.reason == 2 ? "EXCEPTION" : resetInfo.reason == 3 ? "SOFT_WDT" : resetInfo.reason == 4 ? "SOFT_RESTART" : resetInfo.reason == 5 ? "DEEP_SLEEP_AWAKE" : resetInfo.reason == 6 ? "EXT_SYS_RST" : "???"), resetInfo.epc1, resetInfo.epc2, resetInfo.epc3, resetInfo.excvaddr, resetInfo.depc);

this is the changed part:

String(F("Fatal exception:%d flag:%d (%s) epc1:0x%08x epc2:0x%08x epc3:0x%08x excvaddr:0x%08x depc:0x%08x")).c_str()

which Im not sure if is the best way - but it works and 80B of ram is saved. Is it, or is there better way how to do it? Maybe some MACRO could be used..

There are of course other const strings in this file and i suppose more anywhere else..

I have noticed @devyte you have assigned project in TODO, maybe I could help with taht if somebody will tell me how it should be done correctly and how to post code changes..

@Pablo2048
Copy link

IMHO all Your "WDT", "SOFT_WDT" will remain in RAM. So maybe better solution can be:
sprintf_P(buff, PSTR("Fatal exception:%d flag %d (%S) << mark the big S here... ...(resetInfo.reason == 0 ? PSTR("DEFAULT") : resetInfo.reason == 1 ? PSTR("WDT").... So no String object involved here and all strings are in Flash.

@jindrichsirucek
Copy link
Author

jindrichsirucek commented Aug 13, 2018

@Pablo2048 there are many of them like this in this file, i guess more over whole framework and libs..
Thx for pointing out the %S - i didnt know about it! I have to try it :-D

@devyte
Copy link
Collaborator

devyte commented Aug 13, 2018

@jindrichsirucek you are correct: this is an ongoing initiative, to increase available heap, so all help in this direction is welcome.

About the different ways discussed here:

  • The current code makes the string reside in RAM all the time. This is the worst way.
  • @jindrichsirucek 's way loads the string from flash into RAM temporarily, uses it to print to buffer, and then immediately discards it. This is an intermediate way, taking up RAM only for a moment.
  • @Pablo2048 's way prints to buffer directly from flash. This is the best way, as it requires no RAM. However, it's not always possible to do this everywhere without changing current core api.

@jindrichsirucek feel free to make a PR to help with this. Please include fixes for all cases that you see. I strongly suggest to test building each string that you move individually, i.e.: don't move a bunch of them and then build to check how it went. The reason for this is that not all moves will bring a benefit. Here is some info from what I remember (memory is frail, so please double check this yourself):

  1. Small strings up to 3 chars (i.e.: 4 bytes total with nullterm) don't give any RAM benefit, and in fact only increase bin size.
  2. Duplicate strings declared the classic way, i.e.: those that reside in RAM, are unified by the compiler, so duplicates are removed.
  3. Duplicate strings declared in flash are not optimized, so they are in fact stored with all duplicates in the bin. This means that you have to unify them yourself manually, i.e.: make a static global, then use the pointer in place of the duplicates.
  4. There are problems with declaring flash strings in a .h. There is no problem with declaring flash strings in a .cpp, and exporting a getter function in the .h to access them.

@jindrichsirucek
Copy link
Author

jindrichsirucek commented Aug 13, 2018

@devyte Thx a lot for explaining and sharing..

a) Which branch I should use for PR? - I have never done that before..

b) Is this the easiest way how to write that code? I mean String(F("text")).c_str() - isn't there some more straight forward way? If not, can we use some macro to it? How to implement macros into PR?

c) I don't know how to test yet at all, in here :-D.. And also according what I found, it looks like, that all texts except "" takes HEAP - so we can optimize them all in one time, right?

  1. I did some tests, and what you say it does not seem correct. According my test only string of zero length "" really takes 0 HEAP, even one letter string "A" takes 16B of HEAP
    Here are my tests and results:
void function()
{
  String shortStringLocal = "A"; //HEAP 16 // 40 after use of variable
  Serial.println(shortStringLocal); 

  Serial.println(F("123"));//HEAP 0
  Serial.println("a"); // HEAP 16
  Serial.println(""); // HEAP 0 - so no reason to F("")

  char c_str[] = "a"; //HEAP 0
  char c_str[] = "ab"; //HEAP 16
  Serial.println(c_str);

  char* stringPointer = "a"; //HEAP 16
  Serial.println(stringPointer);
}

  1. Confirmed, but is difficult for me to find and unify strings, so I would go in a way of optimising HEAP and not FLASH (that is too difficult for small price)

  2. viz 2.

  3. This one is really interesting. I found solution how to use FLASH strings/arrays in .h files:
    according this pretty well documented solution: Using PSTR and PROGMEM string with template class causes section type conflict #3369 (comment)
    I use this, but I dont really understand why is this needed, and it need to be in each .h file:
    #define LE(string_literal) (reinterpret_cast<const __FlashStringHelper *>(((__extension__({static const char __c[] __attribute__((section(".irom.text.template"))) = ((string_literal)); &__c[0];})))))
    than in code:
    print(LE("Super loooooong text")); // 0 HEAP // compile OK

@devyte
Copy link
Collaborator

devyte commented Aug 14, 2018

@jindrichsirucek to make a PR:

  1. fork this repo (here in the webpage)
  2. clone your fork locally
  3. set this as remote (git remote add upstream https://github.com/esp8266/Arduino.git)
  4. make a branch in your local git (git checkout -b somename)
  5. implement your changes
  6. commit your changes locally (git commit -m "some meaningful msg here")
  7. it's ok to make more than one commit locally
  8. push to your fork (git push origin master)
  9. in your fork's webpage or in this one (doesn't matter same thing) you'll then get a green button to review the PR and start it

@devyte
Copy link
Collaborator

devyte commented Aug 14, 2018

About point 1, maybe I didn't explain it correctly.
Say you have
const char *str = "lol";
That's a 4 byte string. Moving that string to flash like so:
const char str[] PROGMEM = "lol";
doesn't increase available heap.
Also, say you have this:
const char *str1 = "lol";
const char *str2 = "lol";
const char *str3 = "lol";
The compiler will optimize those into a single string "lol" in RAM. However, this:
const char str1[] PROGMEM = "lol";
const char str2[] PROGMEM = "lol";
const char str3[] PROGMEM = "lol";
won't get optimized.

As I said, I don't remember all the details anymore, I'd have to experiment to refresh memory. I suggest:

  • looking at the mem and bin sized reported right after a build
  • running a ESP.getFreeHeap() on startup to get a real value

As I explained above, String(F("text")).c_str() will load the string into a heap temporarily, then extract the c_str() from it, then discard it. So it is a temporary increase in heap usage.
In contrast, @Pablo2048 's suggestion is better, because the string is used directly from flash. It's not as easy or pretty to do that, though.

Interesting find about the LE macro, I was not aware of it. I also don't understand it :p maybe @earlephilhower will? It looks like a forced section or something.

@earlephilhower
Copy link
Collaborator

@Pablo2048 @jindrichsirucek FYI, %S does NOT work for PROGMEM strings in the current newlib. So formatted inputs can not be in flash, only the main format string. FWIW %S is supposed to be wchar per spec and the AVR Arduino just overloaded it, so I bet if you try to use it on the new ARM Arduino boards you'll get some very interesting outputs. :)

I've got a PR in our newlib to redo the *printf-type calls to be always safe for PROGMEM format strings and parameters (among other memory saving things), but it's not landed yet.

@jindrichsirucek
Copy link
Author

@earlephilhower pls can you provide code example of what you have explained

FYI, %S does NOT work for PROGMEM strings in the current newlib. So formatted inputs can not be in flash, only the main format string
please?

And can you pls let us know here when the PR will be usable part of code, so I can make changes using this new approach pls? Or is there a way how to github will notify me, that changes are incorporated? (I have subscribed to that PR you have linked)..
thx :-)

@earlephilhower
Copy link
Collaborator

Works: sprintf_P(PSTR("I have %1.9f feral %s\n"), 4.2, "cats");
Will crash today: sprintf_P(PSTR("I have %1.9f feral %s\n"), 4.2, PSTR("cats"));
Will crash today: sprintf_P(PSTR("I have %1.9f feral %S\n"), 4.2, PSTR("cats"));

About the newlib changes, I think you can follow the PR. If not, just add a reply to that PR chain and you'll be notified when it's incorporated or tossed.

@jindrichsirucek
Copy link
Author

jindrichsirucek commented Aug 17, 2018

@earlephilhower ok.. but when the change will be (hopefully) incorporated than both of the examples will work or only the one with capital S %S ?
I subscribed, so hopefully i will get notification :-) thx

@earlephilhower
Copy link
Collaborator

I think realistically it's a while away. Not been discussed so far for 2.5.0.

It's still possible to move lots of format strings to PROGMEM by modifying the *printf to *printf_P, so if you really do want to get them to flash only, it's possible for all but the parameters.

@jindrichsirucek
Copy link
Author

Its like going through the all code twice, seems like not good idea.. So i will wait with changes.. Is there a way how to push the request to discussion somehow?

But thanks for showing me already working Serial.printf_P(PSTR"...."), arg); I didnt know about this, i was using macro which did this Serial.printf(((String)F("...")).c_str(), arg); which is more complicated than yours :-)

@jindrichsirucek
Copy link
Author

I have just received notification, that the PR
igrr/newlib-xtensa#5 was merged.. is that mean that it will work in next version (2.5) ?..

@d-a-v
Copy link
Collaborator

d-a-v commented Aug 20, 2018

gcc/toolchain environment needs recompilation with this update.
This is a work in progress and hopefully will be available with next release.

@earlephilhower
Copy link
Collaborator

Closing as almost all core strings are now in flash and printfs() can handle progmem format and param strings.

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

No branches or pull requests

5 participants