wificfg bugfix and enhancements #459

kanflo · 2017-10-10T14:03:29Z

I have started using the excellent wificfg code and found some "undocumented features". The Espressif code gets upset if the SSID contains certain characters or is less than 8 characters in length. This would result in an unsecured AP called "ESP_<last_three_bytes_of_macaddr>"

Additionally, it is not possible to change the SSID following a first init unless the entire flash is erased. I added a note on that.

I took the liberty to redefine wificfg_init to return a bool indicating if the AP start was successful. My application would like to know this.

sanitize_ssid clears up the SSID so as not to upset the Espressif code.

A bug was fixed in the wifi_sta_enable / wifi_ap_enable logic.

While on the topic I would like to discuss extras/wificfg. I would very much like to move everything under content/ and the entire wificfg_dispatch_list to the application inexamples/wificfg. The reason is to be able to use a "clean" wificfg extras and add content per application.

UncleRus · 2017-10-10T16:24:40Z

Great work!
Just one note: sanItize, not sanEtize
https://www.merriam-webster.com/dictionary/sanitize

kanflo · 2017-10-10T18:06:12Z

The mechanism behind changing SSID needs to be looked into. I tried #458 and noted the SSID changed. It could be that whenever AP/STA mode is changed, the Espressif code persists the SSID change that is used to name the AP.

ourairquality · 2017-10-10T20:22:49Z

examples/wificfg/wificfg.c

-
-    wificfg_init(80, dispatch_list);
+    if (!wificfg_init(80, dispatch_list)) {
+        printf("Failed to start AP\n");


This is not necessarily a failure. In fact the configuration can correctly not start wifi. It seems fine to return a bool flag indicating if any wifi was started, if the http server was started, but that should not be interpreted as an error value. The above warning seems unnecessary.

To me, not having an AP up and running on port 80 following the call to wificfg_init is nothing but a failure that needs dealing with. If nothing else, inform the user that the device is malfunctioning.

ourairquality · 2017-10-10T21:15:36Z

extras/wificfg/wificfg.c

+ *
+ * @return     true if name got sanetized.
+ */
+static bool sanetize_ssid(char *ssid)


I am not aware of any such restrictions. All these characters are accepted in the SSID here and an android tablet still noted it as secure. If there are issues in libmain then I think those should be corrected. If there is some restriction in the wpa library then that would be something else to consider.

ourairquality · 2017-10-10T21:18:06Z

extras/wificfg/wificfg.c

 {
    char *wifi_sta_ssid = NULL;
    char *wifi_sta_password = NULL;
    char *wifi_ap_ssid = NULL;
    char *wifi_ap_password = NULL;
+    bool ap_started = false;


As noted this name does not look appropriate. If you need a flag then perhaps wifi_started, but it is not an error for wifi to not be started.

I might have misinterpreted wificfg_init(...) as only starting an AP if so requested (for wifi configuration of virgin devices).

wificfg_init initializes wifi as set in the sysparam, which might be to start the AP and/or the station, or to not start either. It also does do some defaulting to get things started. If you have other wifi config code then perhaps that explains some of the issues you were seeing, perhaps there were conflicts.

ourairquality · 2017-10-10T22:27:03Z

extras/wificfg/wificfg.c

@@ -1856,8 +1894,10 @@ void wificfg_init(uint32_t port, const wificfg_dispatch *dispatch)
        wifi_mode = STATIONAP_MODE;
    else if (wifi_sta_enable)
        wifi_mode = STATION_MODE;
-    else
+    else if (wifi_ap_enable)


Yes, good catch, thanks.

ourairquality · 2017-10-10T22:27:07Z

extras/wificfg/wificfg.c

+           The SSID must be at least 8 characters, if not we will get an
+           insecure AP named ESP_<macaddr> */
+        if (!wifi_ap_ssid || strlen(wifi_ap_ssid) < 8 || strlen(wifi_ap_ssid) >= 32 ||
+            !wifi_ap_password || strlen(wifi_ap_password) < 8 || strlen(wifi_ap_password) >= 64) {


I don't think there is such a limit on the SSID length, and small ssids work here. The error here was the checking of the wifi_ap_password length which must be at least 8, the check of the ap_ssid length was a typo, sorry.

ourairquality · 2017-10-10T22:29:01Z

extras/wificfg/wificfg.c

        wifi_mode = SOFTAP_MODE;
+    else
+        printf("Warning: No AP/STA enabled by wificfg.\n");


It is quite normal for the wifi to not be running, so I would not bother with this. There is already enough output to see which interfaces have been started.

ourairquality · 2017-10-10T22:29:59Z

extras/wificfg/wificfg.c

-
-    xTaskCreate(server_task, "WiFi Cfg HTTP", 464, params, 2, NULL);
+    if (wifi_mode != NULL_MODE) {
+        server_params *params = malloc(sizeof(server_params));


Yes, that looks appropriate, thanks.

ourairquality · 2017-10-10T22:30:33Z

extras/wificfg/wificfg.h

@@ -88,9 +88,10 @@ typedef struct {
 /*
 * Start the Wifi Configuration http server task. The IP port number
 * and a path dispatch list are needed. The dispatch list can not be
- * stack allocated as it is passed to another task.
+ * stack allocated as it is passed to another task. Returns true if the
+ * selected AP was successfully started.


Comment not appropriate, see above.

ourairquality · 2017-10-10T22:32:43Z

extras/wificfg/wificfg.c

@@ -1819,7 +1855,7 @@ void wificfg_init(uint32_t port, const wificfg_dispatch *dispatch)
    /* Validate the configuration. */

    if (wifi_sta_enable && (!wifi_sta_ssid || !wifi_sta_password ||
-                            strlen(wifi_sta_ssid) < 1 ||
+                            strlen(wifi_sta_ssid) < 8 ||


I don't think there is such a limit on the ssid length.

I will revisit this one as I ran into the insecure ESP_xxxxxx AP being started (instead of the secured one with the selected name) when using a short SSID.

ourairquality · 2017-10-10T22:46:28Z

extras/wificfg/wificfg.c

+ *
+ * @return     true if name got sanitized.
+ */
+static bool sanitize_ssid(char *ssid)


Just in case the comment on this was lost: this does not seem necessary, these work here.

Please see my above ESP_xxxxxx comment. I will revisit this one too.

ourairquality · 2017-10-11T03:01:13Z

It works for me. Setting the SSID to "<_+- _>" and Android shows a secure connection and it connects with the password. This is an SSID shorter than 8 character and using all the characters noted in the sanitiser list. If there is a problem then I think more information is needed. I also don't see any problem changing the SSID - it does need a restart to take effect.

Btw The goal with having a separate dispatch list and content for the code under extras is to allow all apps that uses this to benefit from fixes and improvements without each having to transfer these into their own local copy - that is the whole point of sharing code. If you want to make extensive app specific changes then perhaps it would be best to just copy the lot into an app specific location rather than using the generic version from extras.

Further it does firstly check the app specific dispatch first, so an app can opt in to overriding the default handlers.

I suggest you try to work with it a little before you start to rework it all.

UncleRus · 2017-10-11T12:26:36Z

Closing/reopening to trigger CI build

kanflo · 2017-10-12T17:11:27Z

That is very strange. Obviously I tried the very simple example of changing a dash to an underscore in my SSID and reran the code. With the result of the ESP starting up in unsecured ESP_xxxxxx mode.

Regarding extras/wificfg it really should be done in two steps, the first being to include the code to handle the dispatch engine and the second step including all the vanilla HTML. The HTML obviously should be shared but one should not have to hack a copy of wificfg.c in order to get rid of it. That's not good code sharing ;)

ourairquality · 2017-10-12T21:20:50Z

The SSID works for me. Have you re-tested it with the fixes suggested to be adopted. There might be subtle bugs in libmain, and if so they need fixing, have you tried to trace the paths? Do other wpa_supplicants have any such limitations on the SSID?? Perhaps I am missing something, but why would the esp library specifically bother changing the security mode if these characters were used??

The wificfg http server is really minimal. It handles just one connection at a time. It uses one 54 byte stack allocated buffer for all its parsing and output. It does not rewrite html, no pattern matching etc, rather has the pieces in an array. All five of the html files included are just that, arrays of strings. There is no static html content.

As already noted, you can override the default content files per-application and individually, you do not have to copy the lot, but if you change the order or number of pieces in an any of the default content then yes you will need to change the code which is in a single file so you will need app specific code changes to the wificfg code. If people need some common changes then perhaps some flags can be added to effect that.

Perhaps you would prefer the esphttpd. I gave it a quick try, and it was in a very poor state, not usable, and was crashing the device running out of memory with only itself running. The wificfg is what it is for a reason, it is an extremely low overhead solution, and thus it might not ever meet your expectations in terms of abstraction. That's ok, just come up with another separate solution, however it would likely not be usable in my main app as memory is already very tight.

I really appreciate people looking over the code, and am glad you found some issues and reported them. But would not adopt everything in this patch, would not sanitise the strings, would not move the default content, and do not consider it an error if no wifi is started. You have not addressed any of this feedback.

The AP structures are not statically allocated, so is uses more memory when enabled. The station mode is more reliable, the main operating mode. Wificfg allows the AP mode to be disable when the station mode is working, but at the next restart, and that is a good thing.

People in my community do need to disable wifi routinely, walking around mapping, or locating next to sensitive equipment, and the wificfg allows both the AP and station modes to be disable for a number of restarts for such use cases.

wificfg bugfix and enhancements

d05a637

UncleRus added the wifi label Oct 10, 2017

Fixed typo

fed9056

ourairquality reviewed Oct 10, 2017

View reviewed changes

UncleRus closed this Oct 11, 2017

UncleRus reopened this Oct 11, 2017

ourairquality mentioned this pull request Oct 12, 2017

wificfg: correct a few initialization issues #464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wificfg bugfix and enhancements #459

wificfg bugfix and enhancements #459

kanflo commented Oct 10, 2017 •

edited

Loading

UncleRus commented Oct 10, 2017

kanflo commented Oct 10, 2017

ourairquality Oct 10, 2017

kanflo Oct 12, 2017

ourairquality Oct 10, 2017

ourairquality Oct 10, 2017

kanflo Oct 10, 2017

ourairquality Oct 11, 2017

ourairquality Oct 10, 2017

ourairquality Oct 10, 2017

ourairquality Oct 10, 2017

ourairquality Oct 10, 2017

ourairquality Oct 10, 2017

ourairquality Oct 10, 2017

kanflo Oct 10, 2017

ourairquality Oct 10, 2017

kanflo Oct 10, 2017

ourairquality commented Oct 11, 2017

UncleRus commented Oct 11, 2017

kanflo commented Oct 12, 2017

ourairquality commented Oct 12, 2017

wificfg bugfix and enhancements #459

Are you sure you want to change the base?

wificfg bugfix and enhancements #459

Conversation

kanflo commented Oct 10, 2017 • edited Loading

UncleRus commented Oct 10, 2017

kanflo commented Oct 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ourairquality commented Oct 11, 2017

UncleRus commented Oct 11, 2017

kanflo commented Oct 12, 2017

ourairquality commented Oct 12, 2017

kanflo commented Oct 10, 2017 •

edited

Loading