Store strings inside library, instead using external char * #100
Conversation
romansavrulin
changed the title
|
I respectivly disagree, allowing you to use pointers means that you can delare the host as a constant or fixed width char eg Also core libaries should always try avoid using String beacuse there quite costly on ebedded devices. https://hackingmajenkoblog.wordpress.com/2016/02/04/the-evils-of-arduino-strings/ |
|
@timpur I know the cost of the solution. Performance: You nothing to do with that Strings during normal work. It's only convenient container for char *. So no any measurable performance drop. The memory: Maintenance: Obviously, using Strings makes things much cleaner and allows better user experience. So, Using Strings makes sense. |
|
Did you actually read the article ? Strings make copies all other the place with out you knowing, this can lead the heap fragmentation, something I personally try to avoid. |
|
char arrays are preferred over Strings, Strings will eventually break your program in the long run due to it’s dynamic allocation |
|
...and causes memory leaks... |
|
@romansavrulin |
|
const char* doesnt waste memory, it uses storage, same as PROGMEM. |
|
@timpur @tonton81 Okay, let's go step-by step. To PROGMEM or to CONSTRead this links and think about world order one more time: If you are still not confident about Variables placement testHere's a simple test sketch. //36 bytes in total (0x24 bytes)
char * char_variable = "this is char *"; //15 with trailing zero
const char * const_char_variable = "this is const char *"; //21 with trailing zero
void setup() {
// put your setup code here, to run once:
}
void loop() {
// put your main code here, to run repeatedly:
String progmem_char_variable = (F("This is real progmem")); //21 bytes with trailing zero (0x15 bytes)
Serial.print(char_variable);
Serial.print(const_char_variable);
Serial.print(progmem_char_variable);
}
I compile it with Map file analysisThe map file says the following this is the pointers. it's placed into RAM, as expected. Here's .rodata section and ALL To FLASH goes ONLY strings that is used with
ELF analysisTo ensure this is the data we are looking for, let's dump sections
We will get the following and
So, assumption that Test dataHere's |
|
Next, You've mentioned heap fragmentation. What can go wrong with heap fragmentation after setting If you really care about heap fragmentation, please, be consistent and carefully read this library's code. Or better admit that you are using modern cpp and stop the holly war between Peace... |
|
@romansavrulin Edit: also you make a good point about heep and the lib, I might go though and optimise it a bit. Yes std vectors and std functions might not be best for heap, but they have valid use (could be cleaned up). |
|
@timpur Good question. I haven't touched headers for interface compatibility reason. If you agree to pull-in The current PR solves memory management issues. For now, if user will somehow free the memory where parameters points to, the library will crash or go crazy. It can be easily achieved by both stack and dynamic allocations. All the following snippets will cause panic, because variables will be out of the scope when Using Strings inside the lib will ensure the data that is needed by the lib will be safe and sound, as it is copied inside String containers under the hood. It also ensures little memory overhead for data storage, due to dynamic allocations. You don't need to keep big static buffers for that data. |
|
on teensy, const is automatically out in flash during compile, no need for progmem or F() macros, the static print/println() are stored on flash as well without F() macro surely before you complain about marvin’s library not working on teensy, ive ported his library over as well as many of the core functions of ESP, without using silly AT protocols. 3 megabaud rates i had marvins mqtt running for months... but enough about that, not all microcontrollers put strings (small ‘s’) in flash, and F() is not needed by all microcontrollers if the compiler puts it in flash automatically :) |
|
Tbh, if the user isn't benifiting any any way then there's not much point ? And if using strings for the user means memory management issues ? Then not sure about this ... ? |
|
how would it panic out of setup() scope? you do realize, internally, the class stores the username/password. i fail to see how a panic would occur “out of scope” |
|
i would also refrain from using STL c++ containers, they use dynamic allocations and adding Strings to the mix is bad, especially if the user is using interrupts. vectors (one of many STL containers) can corrupt the data when dealing with ISRs, none of the STL containers are ISR safe, and the random dynamic allocations on these microcontrollers are asking for trouble. |
|
@timpur this examples will most likely panic or misbehave without my PR. This PR adds automatic memory management. No additional actions need to be taken by user. Without it user must keep an eye on structures where pointers point to. And if it points to the place that used to be valid at some point (e.g. setup in our case), but at the moment when the library needs this data again it's destroyed (say, reconnect event, that is definitely a bit later, when you've already exit |
|
Yeah true let me talk to Marvin. |
|
I'm absolutely sure about that cases. When you use But, wait... You've passed The same thing is with you will see The stack of the setup is definitely out of the scope when you need to reconnect. |
|
And what is interesting, if you rewrite the same example the following way you won't get panic anymore, because pointers will point to So, take care! p.s. To take a single function dizassembly, just run |
|
for classes as well, when initializing i would never put a variable storage as a const or a pointer, i prefer copying it directly and using the internal copy in future when i ported over the ESP core to teensy with mqtt etc, i never used pointers or const variables because of issues like this, and it allowed dynamic updates.. |
|
Hello, @timpur ! Is there any news on that PR? Will it appear in mainline? |
|
Hmmm, I understand the problem, just not keen on using the String class to solve this. TBH I'd prefer copying the mem manually. Thinking depending on the input type it will copy or not, that way if you know what your doing you can use a global const that doesn't copy the mem, other wise it will be copied. |
|
@timpur what are you actually fighting for when copying manually? Saving few bytes? You won't save - |
|
@romansavrulin, okay, but still not really keen on using String, but can you update this PR to include the function signatures also and anything else that makes sense, then well review :) |
|
I disagree with the initial premise:
In my use-cases most of the long-lived strings, such as user name, password, etc come from settings in flash that are loaded into memory at boot time and pretty much stay there. This means I already have memory allocated and can just point the MQTT library at that. If the library made a copy, then those strings would be in memory twice, because I can't easily get rid of the copy I already have. |
|
I'm with @tve on this PR. Okay to close? |
|
In the past 2 days I've been searching for a bug, why the client is not able to establish connection with the mosquitto broker. In the logs I've seen It would be nice to have a documentation saying that the memory passed to setters needs to be valid throughout the lifetime of client object. Preferably, such documentation should be available not in the |
This strings should be stored somewhere, let it be inside the library, because in that case we can use temp variables for them outside the library.
Usecase:
Without patch external variables will be out of the scope when mqtt will try to reconnect and one will get exception.