Remove bespoke nvs

[lukecyca]

I was wondering why this library has its own bespoke NVS functionality. The esp-idf's wifi API already optionally stores SSID and password in NVS. It seems that this library could be simplified by removing this code and relying on the built-in functionality. This also has the advantage of better interoperability with other code that wants to set or retrieve wifi credentials.

In this PR I've attempted this. Consider this a work-in-progress and request-for-comments. Surely there are edge cases that I have not considered.

For my purposes, I'm looking for ways to simplify this library, and give more control back to the calling application. I really like having an AP + captive portal to configure wifi credentials, but currently this library exerts too much control over all aspects of networking. My devices need the flexibility to try a default wifi network, use ethernet if equipped, or even disable networking altogether. If this goal doesn't align with yours, let me know and I can hack away and maintain my own fork and not waste your time. :)

[tonyp7]

First of all, the reason why wifi manager is saved using a custom bit of code is because... I didn't know there was a native functionality ;-)

Now, I actually don't think this can work because the custom code does two things:

• Save STA ssid and password
• Save the whole softAP configuration in a blob

You need both because the native function is only getting/saving STA it seems.

The truth is the function is called wifi_manager_fetch_wifi_sta_config but it's a misnomer and it's confusing.

This line:

esp_err = nvs_get_blob(handle, "settings", buff, &sz);

actually also fetches the AP settings.

Now you could argue that fetching AP setting is pretty useless at this point, because these are defined at compile time in the kconfig.

In that case we could really simplify the code indeed. I am debating. If you don't mind I'll have a look at your branch and might change a few things.

On a separate topic, why delete nvs_sync? I think it's important to keep, because without it there's no telling if user code tries to access NVS at the same time as esp32-wifi-manager. Most people won't bother, but it's better to have it there don't you think?

[lukecyca]

First of all, the reason why wifi manager is saved using a custom bit of code is because... I didn't know there was a native functionality ;-)

Haha fair enough, that's a good reason!

Now you could argue that fetching AP setting is pretty useless at this point, because these are defined at compile time in the kconfig.

Yes, I noticed that some of the library parameters are also stored in NVS. In my opinion these parameters should be specified by the calling application, either through compile-time kconfig, or as parameters to wifi_manager_start() as I suggest in #114. Either way I don't think it's necessary or even appropriate for this library to also persist these settings on its own. For most use cases, the application code will decide how it wants to use wifi_manager. If it wants to make some settings configurable and saveable in NVS, then I can handle that outside this library.

In that case we could really simplify the code indeed. I am debating. If you don't mind I'll have a look at your branch and might change a few things.

Certainly! I'm calling for some fairly major simplifications here, that may have ramifications that I haven't considered. Please do play around with the code.

On a separate topic, why delete nvs_sync? I think it's important to keep, because without it there's no telling if user code tries to access NVS at the same time as esp32-wifi-manager. Most people won't bother, but it's better to have it there don't you think?

The NVS api functions are already threadsafe according to two engineers at Espressif. It is not necessary for us to guard our concurrent calls to NVS functions, as it is already guarded at a lower level.

[tofublock]

Sorry to drag out a year old PR, @lukecyca, but are you sure saving credentials works like that? In general it seems to save them, but loses credentials when flashing a new firmware (while other data stored in NVS is not lost!) and even when doing a regular button reset sometimes. Any idea what might be going on?

[robert-alfaro]

In my experience, the NVS partition only gets erased if you erase the whole chip.. when flashing you can just flash the app partition (or whatever suits your needs). Moreover, I think the wifi subsystem only stores ssid/pass not other information like static ip, channel, bssid, etc. I too store a wifi ap struct as a blob in nvs myself and configured esp-wifi to not use nvs.

[tofublock]

NVS doesn't get erased so much as the specific wifi data just gets lost somewhere. Other values in NVS are fine. It doesn't happen all the time either. I use NVS in other parts of my code - starting to wonder about that thread safety of the save functions... I might try what you suggest - to manually save it myself.