WPS functionality added #1694

FrankX0 · 2017-01-01T19:28:11Z

Fixes #1556 .

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/*.

Although there seems to be some opposition against using WPS, I still think it is worthwhile making it available to whomever wants to use it.
Please bare in mind that my software/Github skills are somewhat limited.
Looking forward to your comments/acceptance.

pjsg · 2017-01-01T20:30:27Z

app/modules/wps.c

+static int ICACHE_FLASH_ATTR wps_start(lua_State* L)
+{
+  // retrieve callback arg (optional)
+  if (wps_callback_ref != LUA_NOREF)


You don't need to do the test -- you can unref LUA_NOREF

pjsg · 2017-01-01T20:30:40Z

app/include/user_modules.h

@@ -68,6 +68,7 @@
 //#define LUA_USE_MODULES_UCG
 //#define LUA_USE_MODULES_WEBSOCKET
 #define LUA_USE_MODULES_WIFI
+#define LUA_USE_MODULES_WPS


This should default to OFF

- both WiFi and Wi-Fi are valid but we seem to have opted for WiFi (although not 100% consistently) - we don't use "Errors" sections - we omit the "Examples" section if they'd be trivial

marcelstoer · 2017-01-01T20:56:21Z

docs/en/modules/wps.md

@@ -0,0 +1,63 @@
+# WPS Module


A reference to this file needs to be added to /mkdocs.yaml.

marcelstoer · 2017-01-01T21:07:26Z

The CI build fails with

section .text will not fit in region iram1_0_seg

Looks like the fix that @djphoenix provided in #1566 didn't last long.

Fix whitespace

marcelstoer · 2017-01-01T21:16:54Z

I don't mind having this available as an off-by-default module. However, I think we should prominently mention the security concerns our WiFi module maintainer @dnc40085 raised a long time ago in the docs.

FrankX0 · 2017-01-01T21:54:51Z

I guess we can add a statement to the WPS module documentation:

Use with caution: there are (serious) security concerns about using WPS.

FrankX0 · 2017-01-03T15:27:54Z

@marcelstoer, @djphoenix: any ideas how to fix the CI build error?

marcelstoer · 2017-01-03T20:49:49Z

I don't, not my area of expertise 😞

djphoenix · 2017-01-04T00:00:41Z

Rebased this PR on current dev and built all-modules + ssl configuration. Build succeeds for integer version but fails for float.

After temporary increase iram0 segment size I tried to link binary again and it succeeds, and so size of iram is 0x8045 (69 bytes overflow).

For dev branch iram size is 0x7d45 (difference with wps is 768 bytes).

And the winner is...
libwps.a is placed in irom at most. But size of all symbols in text section is...

$ gobjdump -h sdk/esp_iot_sdk_v2.0.0/lib/libwps.a | grep ' .text' | awk '{print $3}' | grep -v 00000000
0000000b
000000aa
00000094
00000082
00000068
000000aa
$ node
> 0xb + 0xaa + 0x94 + 0x82 + 0x68 + 0xaa
733

Very close to what we see...

The solution is patch LD script as I was originally propose - keep only main, pp, phy, net80211 and gcc libs of SDK in iram. But I cannot guarantee that wps will keep work in irom. Cannot test this solution myself because in my router WPS completely disabled for security reasons.

FrankX0 · 2017-01-07T16:44:55Z

@djphoenix: I am willing to test, but it's not clear how to modify the current dev LD script.

djphoenix · 2017-01-07T18:17:47Z

In ld/nodemcu.ld file, near line 105:

+    *libmain.a:*(.literal .text)
+    *libnet80211.a:*(.literal .text)
+    *libphy.a:*(.literal .text)
+    *libpp.a:*(.literal .text)
+    *libgcc.a:*(.literal .text)
-    *sdk/esp_iot_sdk_*lib/lib*.a:*(.literal .text)

FrankX0 · 2017-01-07T19:41:45Z

Thanks @djphoenix: no problems compiling.
WPS functionality still works OK.
I tested using the physical button implementation, not through PIN.
This I have disabled (for what it is worth).

Keep only main, pp, phy, net80211 and gcc libs of SDK in iram, and therefore wps in irom.

djphoenix · 2017-01-08T17:29:00Z

app/modules/wps.c

+// Lua: wps.disable()
+static int ICACHE_FLASH_ATTR wps_disable(lua_State* L)
+{
+  wifi_wps_disable();


I think there should be error handler, as far as wps-related functions (in includes) returns bool as success/failure status.

djphoenix · 2017-01-08T17:32:39Z

app/modules/wps.c

+  if (lua_type(L, 1) == LUA_TFUNCTION || lua_type(L, 1) == LUA_TLIGHTFUNCTION)
+    wps_callback_ref = luaL_ref(L, LUA_REGISTRYINDEX);
+  else
+    return luaL_error (L, "Argument not a function");


With luaL_checkanyfunction no needed to manually raise an error.
And so safer way is check arguments before any internal state change.

djphoenix · 2017-01-08T17:34:33Z

app/modules/wps.c

+
+  wps_callback_ref = LUA_NOREF;
+
+  if (lua_type(L, 1) == LUA_TFUNCTION || lua_type(L, 1) == LUA_TLIGHTFUNCTION)


luaL_checkanyfunction should be there

djphoenix · 2017-01-08T17:35:23Z

app/modules/wps.c

+  wps_callback_ref = LUA_NOREF;
+
+  if (lua_type(L, 1) == LUA_TFUNCTION || lua_type(L, 1) == LUA_TLIGHTFUNCTION)
+    wps_callback_ref = luaL_ref(L, LUA_REGISTRYINDEX);


There will be issue when more than one argument is passed, so call lua_pushvalue(L, 1) before luaL_ref

djphoenix · 2017-01-08T17:38:08Z

ld/nodemcu.ld

+    *libnet80211.a:*(.literal .text)
+    *libphy.a:*(.literal .text)
+    *libpp.a:*(.literal .text)
+    *libgcc.a:*(.literal .text)


I think this change is outside this PR's scope.
@marcelstoer @jmattsson should we move it in another issue/PR or keep there?

I prefer a dedicated PR. Once it's merged we can restart the CI build here.

I'm really wary of this change altogether. As you already pointed out Yury, Espressif has put almost everything into irom0.text, except for what appears to be a few internal pieces. Presumably they had a really good reason for keeping those in IRAM. Maybe it would be worthwhile raising the large iram1_0_seg size with Espressif as a potential bug? If they respond, we'd know for certain whether it has to be kept in IRAM or not.

@jmattsson Johny, as I reviewed, a lot of Espressif code is a mess. I think there was some internal Espressif's issues e.g. "radius of the developers hands curvature does not comply with the platform requirements" :)
Anyway for WPS we decide that it works when fully putted in irom. And for most places it right (with exceptions for interrupts-related stuff). I haven't see any of interrupt-related code here.

Hah! Well I can't argue too strongly here seeing as I've used the "I've reviewed and tested it, and it works" argument myself in regard to flash vs ram.

Could we perhaps leave a comment in the nodemcu.ld file indicating that we're specifically excluding ilbwps.a due to size constraints?

@jmattsson I think we should move this discussion to #1710.
For @FrankX0 - just revert this part, we will fix this issue in other way.

For @FrankX0 - just revert this part, we will fix this issue in other way.

The cool thing is that GitHub allows us to do that yourselves right in the browser. You can't revert but you can resolve conflicts by editing code - which is just did.

marcelstoer · 2017-01-09T07:42:28Z

I'll add the doc amendment we talked about at #1694 (comment) after merging.

Thanks Frank, Yury and Johny.

FrankX0 and others added 2 commits January 1, 2017 20:19

WPS functionality added

281b982

Module name corrected

6a3dc14

pjsg reviewed Jan 1, 2017

View reviewed changes

FrankX0 and others added 3 commits January 1, 2017 21:46

WPS module switched off by default

771bb56

Update wps.md

5568553

- both WiFi and Wi-Fi are valid but we seem to have opted for WiFi (although not 100% consistently) - we don't use "Errors" sections - we omit the "Examples" section if they'd be trivial

Update wps.c

58e7374

marcelstoer reviewed Jan 1, 2017

View reviewed changes

docs/en/modules/wps.md

@@ -0,0 +1,63 @@

# WPS Module

Copy link

Member

marcelstoer Jan 1, 2017

Choose a reason for hiding this comment

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

A reference to this file needs to be added to /mkdocs.yaml.

FrankX0 and others added 2 commits January 1, 2017 21:58

Wikipedia link repaired

1acae3b

wps reference added

8d2bd2e

Update mkdocs.yml

338ed34

Fix whitespace

marcelstoer added the new module label Jan 2, 2017

Patch LD script

815d483

Keep only main, pp, phy, net80211 and gcc libs of SDK in iram, and therefore wps in irom.

djphoenix suggested changes Jan 8, 2017

View reviewed changes

djphoenix mentioned this pull request Jan 9, 2017

Leave only some parts of SDK in iram #1710

Merged

marcelstoer added 2 commits January 9, 2017 08:30

Merge branch 'dev' into dev

2a09597

Remove extra NL

a27e3e8

marcelstoer merged commit 378e5eb into nodemcu:dev Jan 9, 2017

marcelstoer added this to the 2.0.0 milestone Jan 9, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WPS functionality added #1694

WPS functionality added #1694

FrankX0 commented Jan 1, 2017

pjsg Jan 1, 2017

pjsg Jan 1, 2017

marcelstoer Jan 1, 2017

marcelstoer commented Jan 1, 2017

marcelstoer commented Jan 1, 2017

FrankX0 commented Jan 1, 2017

FrankX0 commented Jan 3, 2017

marcelstoer commented Jan 3, 2017

djphoenix commented Jan 4, 2017 •

edited

Loading

FrankX0 commented Jan 7, 2017

djphoenix commented Jan 7, 2017

FrankX0 commented Jan 7, 2017

djphoenix Jan 8, 2017

djphoenix Jan 8, 2017

djphoenix Jan 8, 2017

djphoenix Jan 8, 2017

djphoenix Jan 8, 2017

marcelstoer Jan 8, 2017 •

edited

Loading

jmattsson Jan 8, 2017

djphoenix Jan 9, 2017

jmattsson Jan 9, 2017

djphoenix Jan 9, 2017

marcelstoer Jan 9, 2017

marcelstoer commented Jan 9, 2017


		wps_callback_ref = LUA_NOREF;

		if (lua_type(L, 1) == LUA_TFUNCTION \|\| lua_type(L, 1) == LUA_TLIGHTFUNCTION)

WPS functionality added #1694

WPS functionality added #1694

Conversation

FrankX0 commented Jan 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelstoer commented Jan 1, 2017

marcelstoer commented Jan 1, 2017

FrankX0 commented Jan 1, 2017

FrankX0 commented Jan 3, 2017

marcelstoer commented Jan 3, 2017

djphoenix commented Jan 4, 2017 • edited Loading

FrankX0 commented Jan 7, 2017

djphoenix commented Jan 7, 2017

FrankX0 commented Jan 7, 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

marcelstoer Jan 8, 2017 • edited Loading

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

marcelstoer commented Jan 9, 2017

djphoenix commented Jan 4, 2017 •

edited

Loading

marcelstoer Jan 8, 2017 •

edited

Loading