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.

Sign up for GitHub

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

wificfg bugfix and enhancements #459

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

wificfg bugfix and enhancements #459

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d05a637
wificfg bugfix and enhancements
kanflo Oct 10, 2017
fed9056
Fixed typo
kanflo Oct 10, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions examples/wificfg/wificfg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ void user_init(void)
printf("SDK version:%s\n", sdk_system_get_sdk_version());

sdk_wifi_set_sleep_type(WIFI_SLEEP_MODEM);

wificfg_init(80, dispatch_list);
if (!wificfg_init(80, dispatch_list)) {
printf("Failed to start AP\n");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
69 changes: 56 additions & 13 deletions extras/wificfg/wificfg.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1775,24 +1775,60 @@ static void dns_task(void *pvParameters)
}
}

/**
* @brief Sanetize SSID. If configuring an AP containing the characters in
* 'illegals', the ESP will start an insecure AP named ESP_<macaddr>
* Any illegal characters will be replaced by _
*
* @param ssid the SSID
*
* @return true if name got sanetized.
*/
static bool sanetize_ssid(char *ssid)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
bool sanetized = false;
char *illegals = "+-<> "; // There might me more characters that are illegal
uint8_t num_illegals = strlen(illegals);
if (!ssid) {
return sanetized;
}
while(*ssid) {
for (uint32_t i = 0; i < num_illegals; i++) {
if (*ssid == illegals[i]) {
*ssid = '_';
sanetized = true;
break;
}
}
ssid++;
}
return sanetized;
}

void wificfg_init(uint32_t port, const wificfg_dispatch *dispatch)
bool wificfg_init(uint32_t port, const wificfg_dispatch *dispatch)
{
char *wifi_sta_ssid = NULL;
char *wifi_sta_password = NULL;
char *wifi_ap_ssid = NULL;
char *wifi_ap_password = NULL;
bool ap_started = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


uint32_t base_addr;
uint32_t num_sectors;
if (sysparam_get_info(&base_addr, &num_sectors) != SYSPARAM_OK) {
printf("Warning: WiFi config, sysparam not initialized\n");
return;
return false;
}

sysparam_get_string("wifi_ap_ssid", &wifi_ap_ssid);
if (sanetize_ssid(wifi_ap_ssid)) {
sysparam_set_string("wifi_ap_ssid", wifi_ap_ssid);
}
sysparam_get_string("wifi_ap_password", &wifi_ap_password);
sysparam_get_string("wifi_sta_ssid", &wifi_sta_ssid);
if (sanetize_ssid(wifi_sta_ssid)) {
sysparam_set_string("wifi_sta_ssid", wifi_sta_ssid);
}
sysparam_get_string("wifi_sta_password", &wifi_sta_password);

int8_t wifi_sta_enable = 1;
Expand All @@ -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 ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

strlen(wifi_sta_ssid) > 32 ||
!wifi_sta_password ||
strlen(wifi_sta_password) < 8 ||
Expand All @@ -1844,9 +1880,11 @@ void wificfg_init(uint32_t port, const wificfg_dispatch *dispatch)
}
}

/* If the ssid and password are not valid then disable the AP interface. */
if (!wifi_ap_ssid || strlen(wifi_ap_ssid) < 1 || strlen(wifi_ap_ssid) >= 32 ||
!wifi_ap_password || strlen(wifi_ap_ssid) < 8 || strlen(wifi_ap_password) >= 64) {
/* If the ssid and password are not valid then disable the AP interface.
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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

wifi_ap_enable = 0;
}
}
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, good catch, thanks.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sdk_wifi_set_opmode(wifi_mode);

if (wifi_sta_enable) {
Expand Down Expand Up @@ -2013,10 +2053,13 @@ void wificfg_init(uint32_t port, const wificfg_dispatch *dispatch)
if (wifi_ap_ssid) free(wifi_ap_ssid);
if (wifi_ap_password) free(wifi_ap_password);

server_params *params = malloc(sizeof(server_params));
params->port = port;
params->wificfg_dispatch = wificfg_dispatch_list;
params->dispatch = dispatch;

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that looks appropriate, thanks.

params->port = port;
params->wificfg_dispatch = wificfg_dispatch_list;
params->dispatch = dispatch;
xTaskCreate(server_task, "WiFi Cfg HTTP", 464, params, 2, NULL);
ap_started = true;
}
return ap_started;
}
5 changes: 3 additions & 2 deletions extras/wificfg/wificfg.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment not appropriate, see above.

*/
void wificfg_init(uint32_t port, const wificfg_dispatch *dispatch);
bool wificfg_init(uint32_t port, const wificfg_dispatch *dispatch);

/*
* Support for reading a form name or value from the socket. The name or value
Expand Down