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

Feature/wifi clear config #1761

Merged
merged 6 commits into from
Feb 8, 2017
Merged

Conversation

jfollas
Copy link
Contributor

@jfollas jfollas commented Jan 26, 2017

Fixes #1706 .

  • This PR is for the dev branch rather than for master.
  • This PR is compliant with the other contributing guidelines as well (if not, please describe why).
  • I have thoroughly tested my contribution.
  • The code changes are reflected in the documentation at docs/en/*.

Added wifi.sta.clearconfig() to remove the saved Wi-Fi configuration from flash. Useful in a couple of scenarios:

  • Factory Reset: To clear the saved configuration as a last step before giving the device to someone else
  • End User Setup: If the currently configured network is accessible, then EUS will stop immediately after starting. If the currently configured network is not available, then the SoftAP will not lock onto a radio channel since the Station will be trying to find the previously configured AP (if Auto Connect is enabled, that is). A solution to both is to clear the config before starting EUS.

Also included: Eliminated the minimum password length requirement for station passwords. It was 8 characters, but this prevented WEP from being used with 5-character keys (ASCII representation of a 40-bit key). See the discussion in issue #1706.

## wifi.sta.clearconfig()

Clears the currently saved WiFi station configuration. This will erase the saved configuration from the flash (useful for
factory-reset scenarios, or to prepare for [End-User Setup](enduser-setup) so that the SoftAP can lock onto a single channel)
Copy link
Contributor

Choose a reason for hiding this comment

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

useful for factory-reset scenarios

The function node.restore() should be used to ensure complete erasure of WiFi configuration, since it completely overwrites the system parameter sectors that store WiFi configuration and other configuration information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole time, I never knew about node.restore().

Reset default settings of following APIs: wifi_station_set_auto_connect, wifi_set_phy_mode, wifi_softap_set_config related, wifi_station_set_config related, wifi_set_opmode, and APs’ information recorded by #define AP_CACHE.

memset(temp, 0, sizeof(temp));
if(strlen(config[i].password) >= 8)
memset(temp, 0, sizeof(temp));
if(strlen(config[i].password) >= 0) /* WPA = min 8, WEP = min 5 ASCII characters for a 40-bit key */
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's 5 for the 40-bit WEP, then shouldn't 5 be the new minimum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See #1706. tl;dr: 5 seems to be the smallest that would ever be used, but the SDK doesn't enforce a minimum.

Copy link
Member

Choose a reason for hiding this comment

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

I suspect that this ought to be >= 1. I can't think of any way that strlen could return < 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, strlen will never be <0. Looking at this now after having cooled down from when I made the change, I think >0 is what is appropriate for this code that is copying the passwords only if they exist. The question becomes, should "pwd" be included the Lua table even if there is no password for the entry?

Copy link
Member

Choose a reason for hiding this comment

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

I'd leave out the pwd field if there is no password.

jfollas and others added 2 commits January 26, 2017 10:54
…g() docs, and added SDK verbiage to describe what node.restore() impacts.
@@ -506,8 +506,8 @@ static int wifi_station_get_ap_info4lua( lua_State* L )
c_sprintf(debug_temp, " %-6d %-32s ", i, temp);
#endif

memset(temp, 0, sizeof(temp));
if(strlen(config[i].password) >= 8)
memset(temp, 0, sizeof(temp));
Copy link
Member

Choose a reason for hiding this comment

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

I also wonder -- is the size of the ssid field the same as the password field? If not, then trouble is nearby. Would prefer to eliminate the temp variable and just do a pushlstring of the correct 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.

Looking at the SDK docs, ssid=32, password=64. Looks like they're defining temp's length here as the max password length, and zeroing out before each write (so it is big enough to hold null-terminated strings for both password and ssid).

Copy link
Member

Choose a reason for hiding this comment

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

Yeah -- it looks OK.

wifi_station_disconnect();

bool config_success;
if(save_to_flash) config_success = wifi_station_set_config(&sta_conf);
Copy link
Member

Choose a reason for hiding this comment

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

please format so that else lines up with if. The clauses should be on lines by themselves. I would prefer to use { and } in each case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was existing code - not changed for this PR. I'm all for applying the Boy Scout Rule, but think it makes it harder for reviewers to see what exactly is new.

Copy link
Member

@marcelstoer marcelstoer left a comment

Choose a reason for hiding this comment

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

This looks now complete to me.

@marcelstoer marcelstoer merged commit 2ab28df into nodemcu:dev Feb 8, 2017
@marcelstoer marcelstoer added this to the 2.0.0-follow-up milestone Feb 8, 2017
eiselekd pushed a commit to eiselekd/nodemcu-firmware that referenced this pull request Jan 7, 2018
* Add wifi.sta.clearconfig(). Adjust password validation to match 2.0.0 SDK rules (no min length enforced, i.e. for WEP)
* Updat comments about WEP key not having a minimum
* Documentation: add note about node.restore() to wifi.sta.clearconfig() docs, and add SDK verbiage to describe what node.restore() impacts.
* Normaliz if statements
* Convert leading tabs to leading spaces for consistency
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants