mirror of
https://github.com/tzapu/WiFiManager.git
synced 2026-04-27 00:55:52 +03:00
[GH-ISSUE #1050] Char* conversion for default_value errornous when adding WiFiManagerParameter(); #895
Labels
No labels
📶 WiFi
🕸️ HTTP
Branch
DEV Help Wanted
Discussion
Documentation
ESP32
Example
Good First Issue
Hotfix
In Progress
Incomplete
Needs Feeback
Priority
QA
Question
Task
Upstream/Dependancy
bug
duplicate
enhancement
invalid
pull-request
wontfix
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
starred/WiFiManager#895
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @zen85 on GitHub (May 11, 2020).
Original GitHub issue: https://github.com/tzapu/WiFiManager/issues/1050
Basic Infos
Hardware
WiFimanager Branch/Release:
Esp8266/Esp32:
Hardware: ESP-12e, esp01, esp25
ESP Core Version: 2.4.0, staging
Description
Hi,
When we add a custom WiFiManagerParameter with a normal char* to initialize the default value it has a strange conversion error. Following code:
puts out on the Serial:
Settings in IDE
Module: NodeMcu, Wemos D1
Additional libraries:
Sketch
mounted file system
reading config file
opened config file
{"output":"2","mqtt_broker":"192.168.1.108","mqtt_pwd":"","session_key":"","entity_name":"","entity_type":"","wifi_enc":"","entity_version":"","mqtt_port":"1883","entity_id":"","mqtt_retain":"","mqtt_qos":""}
parsed json
loaded Json
now mqtt broker is:
192.168.1.108
SO, NOW:
192.168.1.108
*WM: [3] allocating params bytes: 20
*WM: [2] Added Parameter: mqttbroker
*WM: [1]
Setup mode...
*WM: [1] AutoConnect
*WM: [2] esp_wifi_set_country: US
*WM: [1] AutoConnect: ESP Already Connected
*WM: [3] STA static IP:
*WM: [2] setSTAConfig static ip not set, skipping
*WM: [1] AutoConnect: SUCCESS
*WM: [1] STA IP Address: 192.168.1.109
Connected to wifi network!
IOT_ESP_6184038
192.168.1.109
*WM: [1] Starting Web Portal
*WM: [3] dns server started with ip:
*WM: [2] HTTP server started
*WM: [2] WiFi Scan completed in 2182 ms
⸮⸮⸮?168.1.10
192.168.1.108
192.168.1.108
attempting connecting to mqtt
@rvt commented on GitHub (May 12, 2020):
You need to let c++ decide for the size:
So use ‘char mqtt_broker[] = "192.168.1.108";’
And male sure that the length parameter for WiFiManagerParameter is large enough for a
An IP address: 16 chars
@zen85 commented on GitHub (May 12, 2020):
unfortunatly this did not work and does not change anything no matter what i do there... the error occurs when i do the following:
it is so weird that this sequence of events works totally fine till it breaks...
this issue here seems to be related?
https://github.com/tzapu/WiFiManager/issues/1020
@tablatronix commented on GitHub (May 12, 2020):
Are you sure the parameters are still in scope?
Maybe reduce your test sketch so we can reproduce, there is too much going on there, try the examples
@rvt commented on GitHub (May 12, 2020):
You are re-assigning custom_mqtt_broker in your setup pfunction , don’t do it like that.
@zen85 commented on GitHub (May 13, 2020):
ok... progress :)
i did that and then
got rid of the weirdness... but i cant find a way to update the value of custom_mqtt_broker on runtime... so the value i get back at custom_mqtt_broker.getValue() will always be 192.168.1.108 no matter what i do in the portal and therefor the param-interface will always show that value. is there a method to do this cleanly?
@tablatronix commented on GitHub (May 13, 2020):
I have no idea, your sketch still contains other stuff not needed to reproduce, hell you have 2 webservers running...
@zen85 commented on GitHub (May 13, 2020):
you are right - you need to know my motivation to know why i left that part in:
the basic idea behind the sketch was to take the sketch posted on the end of this:
https://github.com/tzapu/WiFiManager/issues/656
and to merge it with the FS Example.
i guess the question turned into
"how can i define
WiFiManagerParameter custom_mqtt_broker("mqttbroker", "MQTT Broker", mqtt_broker, 40) ;as global and change the value of custom_mqtt_broker after i read the spiffs.
@zen85 commented on GitHub (May 13, 2020):
i refactored and still got to the root of the problem:
when i configure the first time everything is cool. after entering the configportal while on wireless with the local ip the fields are empty... therefor when i change a value in the custom parameters i rewrite the existing ones with "" - i wanted to get around of that and so my code above seems, and quite probably is, kind of weird...
so here is this version for reproducing that:
@tablatronix commented on GitHub (May 13, 2020):
ok this is pretty much the example, so where exactly is the problem, where in the code can we see the issue ? can you add a line in there or serial print ?
@zen85 commented on GitHub (May 13, 2020):
ok - above sketch never saved changed parameters to spiff... could not work anyway... but please try this one. there is an obvious serial print saying what i expect and this is not matching up with what i get at the end of the setup - so as you can see "mqtt_broker" is alright - i can use this to connect ... but custom_mqtt_broker.getValue() is not. and that also shows on the portal making it unusable.
@tablatronix commented on GitHub (May 13, 2020):
Why are you copying it back into the same variable though ?
strcpy(mqtt_broker, jsonBuffer["mqtt_broker"]);@tablatronix commented on GitHub (May 13, 2020):
you also should not be doing this much stuff in your callbacks, just set some flag and then save your spiffs later.
@zen85 commented on GitHub (May 13, 2020):
this is just done in setup... "mqtt_broker" is always alright. it behaves perfectly even when i put it in the code... but when i enter the configportal > setup it shows the first value it had and the changes are not reflected there... if i could just put something like "custom_mqtt_broker = mqtt_broker" everything would work :) . if i comment that out nothing is updated...
the callback also works fine i think. the config.json reflects everything i change. otherwise "strcpy(mqtt_broker, jsonBuffer["mqtt_broker"]);" would not work. its just the UI thats unusable because it will always say "192.168.100.108" even if i change it in the configportal while on wifi.
@tablatronix commented on GitHub (May 13, 2020):
Looks fine to me
I had to remove spiffs, it was crashing my esp, maybe your problem is with spiffs..
Not really understanding what you are trying to do though..
I am going to assume your issue is loading or saving to spiffs or json, so remove all the other WM code and use plain variables and see if you have the same problem.
Is this a typo?
apparently custom_mqtt_broker.getValue() never gets it value of the config.jsonbecause that doesn't make sense, wm params have nothing to do with json.. they are whatever you set them to or what you submit them as in the http get request.
@zen85 commented on GitHub (May 13, 2020):
yes... it works so far... if you restart the esp now it does not anymore
and it gets extremly close to working if i put:
if(setupSpiffs() == 0){
Serial.println("SO, NOW:");
Serial.println(mqtt_broker);
custom_mqtt_broker = WiFiManagerParameter("mqttbroker", "MQTT Broker", mqtt_broker, sizeof(mqtt_broker));
}
@tablatronix commented on GitHub (May 13, 2020):
If it is not set after resetting then you are not reading it from spiffs properly and then setting it..
Look for a bug there, either the spiffs read or the json decode, the answer was already posted above and you are still using the same variable char mqtt_broker[16] with the same size , reread the comments above by @rvt
@zen85 commented on GitHub (May 13, 2020):
but it is set properly... restart it after you did what you just did. and then enter the portal on the wifi, go to setup... now you will still see 192.168.100.108 and not 192.168.100.112 while the value of mqtt_broker is correctly 192.168.100.112.
here it is with the answer from above - no change:
@tablatronix commented on GitHub (May 13, 2020):
I cannot test that part, my spiffs is not working, reread my previous port I edited it.
you know you can init a param without a value and then set value later ?
your code just does this over and over on every restart, why would the value change ?
@tablatronix commented on GitHub (May 13, 2020):
oops I commented out the spiffs part
I see it now
Yeah so my guess is you are reading this in wrong into the same var[16] char, you need to not do that, as mentioned above.
@tablatronix commented on GitHub (May 13, 2020):
I am curious is it corrupt only after submitting and getting it? Or is this also corrupt ?
@zen85 commented on GitHub (May 13, 2020):
if i dont do
custom_mqtt_broker = WiFiManagerParameter("mqttbroker", "MQTT Broker", mqtt_broker, sizeof(mqtt_broker));nothing is updated at all and the value will always be 192.168.100.108.... if i do that i get at least "@⸮⸮?168.100.102"
@tablatronix commented on GitHub (May 13, 2020):
interesting
@tablatronix commented on GitHub (May 13, 2020):
oh sorry, I have that commented out, do not do // if(setupSpiffs() == 0){
remove that whole block
Just set and get the param value, that is all
@tablatronix commented on GitHub (May 13, 2020):
Then change this
strcpy(mqtt_broker, jsonBuffer["mqtt_broker"]);
to use a different VARIABLE
say mqtt_broker_RESTORE[];
then change your spiffs restore to
Sorry it took me like 30 minutes and 2 reboots to get my damn serial ports working, and now I have to format spiffs to actually run your code.
@zen85 commented on GitHub (May 13, 2020):
heyhey.... its completly amazing that there is somebody out there... i sunk about 20 hours in this now and it feels truly heartwarming that you are taking care...
so i changed my code to this but i still have the same outcome:
@tablatronix commented on GitHub (May 13, 2020):
Do you know how to init format spiffs?
@tablatronix commented on GitHub (May 13, 2020):
nm, // SPIFFS.format();
@tablatronix commented on GitHub (May 13, 2020):
And you have the same problem with the autoconnectwithparams example?
@tablatronix commented on GitHub (May 13, 2020):
ah crap is the old json lib
@zen85 commented on GitHub (May 13, 2020):
i like that neat little tool: https://github.com/nodemcu/nodemcu-flasher
no
yes... the sketch is supposed to run on arduinojson6...
@tablatronix commented on GitHub (May 14, 2020):
ok I have the same issue , no idea what is the deal, let me look into it
@tablatronix commented on GitHub (May 14, 2020):
in the mean time use setValue, it has something to do with calling WiFiManagerParameter twice and it not setting the or overriding the first obj memory properly
This should work fine, adjust length to match whatever you set it to before etc.
custom_mqtt_broker.setValue(mqtt_broker_RESTORE,20);@tablatronix commented on GitHub (May 14, 2020):
So here is the reproduction
What I don't understand is why setvalue is fine, and calling the constructor again is not, while they both internally call setValue and memset the same..
@tablatronix commented on GitHub (May 14, 2020):
Maybe someone can look at this for me and help me fix it or understand wtf is going on in the param class or with constructors, it should be the same memory, and if I check the ID its the same object..
@tablatronix commented on GitHub (May 14, 2020):
hmm ok so something I do not understand about constructors.. NOT the same memory..
@rvt commented on GitHub (May 14, 2020):
WiFiManagerParameter is missing a copy assignment operator. https://en.cppreference.com/w/cpp/language/copy_assignment
Doing this:
WiFiManagerParameter custom_mqtt_broker("mqttbroker", "MQTT Broker", mqtt_broker, 20);is different from doing this:
custom_mqtt_broker = WiFiManagerParameter("mqttbroker", "MQTT Broker", mqtt_broker, 20);what happens in that single line is this:
custom_mqtt_broker, c++ generates a assignment operator where each value is copied copied, but remember... char pointers is a address, only the address get's copied, not the string!_valueis not valid anymore, also not in the copied version because they point to the same deleted address.Hope that clarifies...
Is there any reason you want re-assign custom_mqtt_broker? Some comments are in German and although I got taught German at school 25 years ago it's not that good anymore...
@tablatronix commented on GitHub (May 14, 2020):
Thanks, Yeah I was reading about correct assignment but did not really understand what it meant. I am mostly curious how to handle this, how to make it work as expected or prevent it. As it is something users are going to try.
I have some reading to do on c++
@rvt commented on GitHub (May 14, 2020):
I agree that users will try to do this so we must prevent users from doing it or add the required code to handle it.
I will make a pull request to handle this.
@dontsovcmc commented on GitHub (May 14, 2020):
Maybe it's better to deny coping of WiFiParameter and add function "setValue()"?
No reasons to copy WiFiParameters.
@tablatronix commented on GitHub (May 14, 2020):
We have setValue and we can also check _id and deny I can throw a compiler error. It would be cool to just allow it though if its not too much trouble. At this point I really want to see how , there is also already an init helper in the constructor so maybe we can do something with that to reinit safely
@tablatronix commented on GitHub (May 14, 2020):
So is this correct or the gist?
Not sure if rhs is ref or pointer, I will wait for an actual PR from someone more confident in this than I, But I am trying to learn lol
Does this leak memory though?
Also I am not sure we are cleaning up this in the deconstructor properly.
@rvt commented on GitHub (May 14, 2020):
We could use
WiFiManagerParameter(const WiFiManagerParameter&) = delete; WiFiManagerParameter& operator=(const WiFiManagerParameter&) = delete;Somebody will always find a reason :)
@dontsovcmc commented on GitHub (May 14, 2020):
I found the reason: More code, more bugs.
Here is one of them:
We have memory leak if call setValue() in our program.
_value = new char[_length + 1]; //_value is't null cause setValue called early in constructor
@tablatronix i think:
@rvt commented on GitHub (May 14, 2020):
I like this one better:
That will be deleted again in the destructor
The copy assignment could be further optimized by using move semantics.
But the whole dynamic allocation within an embedded system should be prevent at all cost.
@dontsovcmc Perhaps you made a good point, just prevent copy assignment in the first place?
That will show also good design methodology for embedded software developers.
@dontsovcmc commented on GitHub (May 14, 2020):
@rvt "That will be deleted again in the destructor."
No, I mean:
@rvt commented on GitHub (May 14, 2020):
@dontsovcmc yes you are absolutely right, setValue does not release memory of previously allocated memory... I guess that´s why we love immutables :)
@zen85 commented on GitHub (May 14, 2020):
@tablatronix @rvt
sorry i left the discussion, it was 3 in the morning where i live and i had to work.
When i woke up i checked the thread in an instant and tried the solution before i had to leave and you guys made my day. it finally works. thank you so much!
I have to admit that i did not know about setValue() and never stumpled upon it in my extensive research. I am just wondering why this is considered a "meantime"-solution since it does exactly what i needed? is it not optimal to solve it like that? i am sorry for the german comments... i changed them to english. but no.... there was no particula reason to redeclare it - i just tried so much and this was the only thing that produced some kind of meaningful return.
also: the working sketch for anybody making it down here:
@tablatronix commented on GitHub (May 14, 2020):
Thanks all, yeah that is why I asked if this would leak memory, I assumed it would from the NEW value each time if there was any destruction allowed via assignment.
Ill let you all discuss this a little
@tablatronix commented on GitHub (May 15, 2020):
Made copy assignment private, so compiler will error out if you try this again.
Leaving open for improvements or alternatives.
@rvt commented on GitHub (May 15, 2020):
Not sure what compiler you are using but with c++11 you can do this:
WiFiManagerParameter& operator=(const WiFiManagerParameter&) = delete;so you don't have to set it private if you just want to prevent all users (including the class self) prevent using the copy assignment.
@tablatronix commented on GitHub (May 15, 2020):
ooh
@jonathanvanpeteghem commented on GitHub (Jun 15, 2022):
Hello,
I am curently using .setValue()
The reason is that when i update the parameters, and later go back to the configportal (via button for example) then I want to see the updated values. And .setValue seems the only way for that.
But my question is, is this now safe to use? Forgive my ignorance. I have read the post 1050 / 1341 and 1346. But I cannot understand if the memory leak issue is now gone or not?
In my case I always restart after going to the configportal (because, when returning from configportal i have to go back to an assync webserver, and that only works with a restart, since the cleanup from the wm webserver leaves "stuff behind")
==> so i guess with my restart, i wont notice the memory leak, since my restart cleaned it up.
But in anycase, it would be kind if you can let me know if the .setValue() is now leak safe.
Best Regards to Tablatronix! Thank you so much for making this wifimanager something that we can use daily.
@tablatronix commented on GitHub (Jun 15, 2022):
This is 2 years old, so no idea, pretty sure there have been at least 2 PRs to this