Skip to content
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

Documentation for WiFi.persistent() is misleading #3641

Closed
herm opened this issue Sep 23, 2017 · 13 comments · Fixed by #5081
Closed

Documentation for WiFi.persistent() is misleading #3641

herm opened this issue Sep 23, 2017 · 13 comments · Fixed by #5081

Comments

@herm
Copy link

herm commented Sep 23, 2017

Documentation at http://arduino-esp8266.readthedocs.io/en/latest/esp8266wifi/generic-class.html says
Setting persistent to false will get SSID / password written to flash only if currently used values do not match what is already stored in flash.

But the code does something different:

if(sta_config_equal(current_conf, conf)) {
DEBUGV("sta config unchanged");
}
else {
ETS_UART_INTR_DISABLE();
if(WiFi._persistent) {
// workaround for #1997: make sure the value of ap_number is updated and written to flash
// to be removed after SDK update
wifi_station_ap_number_set(2);
wifi_station_ap_number_set(1);
wifi_station_set_config(&conf);
} else {
wifi_station_set_config_current(&conf);
}

It writes settings to flash if they don't match the values in flash if persistent is set to true. If it is false it will not write to flash independent of the current value in flash.

@tablatronix
Copy link
Contributor

All functions that use persistent and config equal or saves needs to have default/current conditional based on persistent. There are a few, I will make note of them.

@tablatronix
Copy link
Contributor

tablatronix commented Nov 5, 2017

ESP8266WiFiAPClass::softAP
ESP8266WiFiSTAClass::begin

This also affects functions that check mode, but save persistent
ESP8266WiFiGenericClass::mode

    if(wifi_get_opmode() == (uint8) m) {
        return true;
    }

It is also special note, that the docs say
This API can be called only if ESP8266 Station is enabled.
I am not sure if this applies to *_default config (docs only mention flash) or what the consequences are, it may be necessary to add a sanity check if it is prohibited.

The main problem with persistent is that is assumes current config has not been changed from default, which it can be by toggling persistent on and off. So both need to be checked in comparisons.

@tablatronix
Copy link
Contributor

This is what I am thinking

#3798

@herm
Copy link
Author

herm commented Nov 5, 2017

I'm not sure if the changes you propose are required or not. However they are unrelated to this issue. The problem here was that the code does something else than the documentation says. But the code's behavior is good so the documentation has to be changed.

@tablatronix
Copy link
Contributor

tablatronix commented Nov 5, 2017

Then I will create another issue, because I believe it is wrong and badly implemented. I see what you mean about the negated persistent functionality now, I missed that before.

@devyte devyte added this to the 2.5.0 milestone Feb 27, 2018
@devyte devyte self-assigned this Feb 27, 2018
@Raynman77
Copy link

@herm Just curious, whats the reason for the underscore in your line containing WiFi._persistent?

@tablatronix
Copy link
Contributor

That is the name of the variable used in the library.

@Raynman77
Copy link

Raynman77 commented Apr 25, 2018 via email

@tablatronix
Copy link
Contributor

No thats the private variable you have a function call your good

@Raynman77
Copy link

Raynman77 commented Apr 25, 2018 via email

@tablatronix
Copy link
Contributor

tablatronix commented Aug 29, 2018

Frequently calling these functions could cause wear on the flash memory

Only if ! notconfigequals, perhaps we should add debugging strings when doing set config defaults to help debug that?

@devyte
Copy link
Collaborator

devyte commented Aug 29, 2018

Paranoia is your friend in these cases, so I agree.

@tablatronix
Copy link
Contributor

there is currently
DEBUGV("sta config unchanged");
but that is the opposite of what you would want
Ill open an issue and try to pr it in the 2-3 places that it happens, would help find really bad stuff like loops

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

Successfully merging a pull request may close this issue.

5 participants