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

End User Setup: Save the post data in a file on the filesystem #2493

Closed
wants to merge 6 commits into from

Conversation

Gui13
Copy link

@Gui13 Gui13 commented Sep 20, 2018

Possibly Fixes #1302, and is a follow up on #2132 .

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

Several PR have tried to implement this, and I took to myself to try and finish this.

The idea is to save the parameters that the user sent in his EndUser setup into a file on the ESP filesystem.
Once we have that, you can modify the eus HTML file and add several fields that will be sent along with the wifi credentials. These fields and values will be saved in a LUA file on the filesystem, that you can now load when needed to access it.

I implemented several things:

  • The /setwifi API now is a standard form POST, which is way easier to handle (the body contains the key/values) and more correct if you want to send lots of data.
  • I parse the body and save the keypairs in a specialized struct, which I later serialize on the ESP filesystem

I have tested the code in a separate standalon project with Valgrind for the parsing/fill up of the struct part, and I'm fairly confident that it should be leak free.

As an MVP2, we could also pass the onConnected callback a structured LUA table which contains all the variables set in the POST.

Please, feel free to suggest code enhancements, my C is a bit rusty.

@Gui13 Gui13 changed the title EUS: allo to save the post data in a file on the filesystem End User Setup: Save the post data in a file on the filesystem Sep 20, 2018
- make the scanning work
- since we now use a POST, we need to redirect to the same page with a specific mode (trying=true) so that the page displays the "wait for it" message
@marcelstoer
Copy link
Member

Cool to see another community contribution for that feature, thanks!

Would you agree then that this supersedes #2132 from @Jcrash29? Then I will definitely close that PR since the author seems to have given up anyway.

P.S. Lua is not an acronym - it's Lua and not LUA 😉

@Gui13
Copy link
Author

Gui13 commented Sep 21, 2018

Yes, I took up his branch and actualy ended up rewriting the entirety of the patch to be able to use POST instead of GET.
I'm still ironing out the edges, but this is functional for my own project of sensors.

Sorry for the acronym, I'm not that good in Lua.. :)

@Gui13
Copy link
Author

Gui13 commented Sep 21, 2018

I have the feeling that the method "eus.start()" should allow to enable/disable the write to file, otherwise we well always write them out, which might not be necessary.

@Jcrash29
Copy link

Thanks @Gui13. I like the way you laid the code out. Glad someone was able to come in and complete the code.

When I finally get back around to my project I'll attempt it off your code.

@marcelstoer
Copy link
Member

marcelstoer commented Oct 19, 2018

I have the feeling that the method "eus.start()" should allow to enable/disable the write to file

I agree

@Gui13 do you have plans to address this before we merge?

@davidcbittner
Copy link

This is great I have been waiting for this for a long time. Is there any documentation on how to access this e.g. modify the web page to add custom fields to be collected during the AP sign in/set up process and also how to access them once AP setup is completed? I would really like to do some testing before this is merged possibly give some additional feedback.
Regards
David

@marcelstoer
Copy link
Member

I would really like to do some testing

Great!

If you look at the files this PR changes (tab at the top) you'll also find the updated documentation: https://github.com/nodemcu/nodemcu-firmware/blob/95c07234f1eae1d43eb46541cfa55b6c0f0c25ec/docs/en/modules/enduser-setup.md

@davidcbittner
Copy link

Thanks! perfect exactly what I was looking for. I'll let you know how it goes.
Regards
David

@Gui13
Copy link
Author

Gui13 commented Nov 14, 2018

@marcelstoer I can look into adding a function parameter, indeed. Give me a couple of days (this is a week end project..)

@davidcbittner
Copy link

I need to create my firmware build from the dev branch correct? Sorry if this is stupid question I've been away for a while.
Thanks
David

@davidcbittner
Copy link

I've been stressing for a while now that "Host name" should be part of the default along with SSID and PW. Hostname is a critical part of any device setup in my opinion. It should automatically be stored so the device can be accessed as 'http://kitchen:1234' It would save a step for certain otherwise one has to go into their router and/or hosts file to set this up. My particular router has the capability of doing the DNS as long as the host name is set.

@marcelstoer
Copy link
Member

I need to create my firmware build from the dev branch correct?

The code will only be on dev after the PR has been merged. Until then you need to clone @Gui13's fork, change to its eus_params_post branch and build the firmware yourself e.g. using Docker.

@davidcbittner
Copy link

Is there any ETA on when the merge will be done to dev?

@marcelstoer
Copy link
Member

As soon as you're done testing 😜 Seriously, I take it building your own firmware is too much of an obstacle; no problem. I built a binary from that branch and attached it as ZIP here:

NodeMCU 2.2.0.0 built with Docker provided by frightanic.com
	branch: eus_params_post
	commit: 95c07234f1eae1d43eb46541cfa55b6c0f0c25ec
	SSL: true
	Build type: float
	LFS: disabled
	modules: enduser_setup,file,gpio,http,i2c,mdns,mqtt,net,node,spi,tmr,uart,wifi
 build created on 2018-11-14 20:02
 powered by Lua 5.1.4 on SDK 2.2.1(6ab97e9)

nodemcu_float_eus_params_post_20181114-2003.bin.zip

@davidcbittner
Copy link

Bless you. ha..... I'll admit I had no idea how to do that thanks for building this for me. I'm going to flash this, this afternoon, and begin doing some testing hopefully
Regards
David

@davidcbittner
Copy link

Marcel, Thanks very much for the bin file. I flashed it could I ask you to create a new one and include SNTP? My project includes that and getting errors because it is missing. Despite that there appears to be issues with the web page/module itself I think. I think there is a problem with the wifi scanner drop down I want to research.
Thanks
David

@davidcbittner
Copy link

I found a couple of issues with the stock web page enduser_setup.html

  1. for the input fields the ID parm was removed however it is used in the javascript to reference those elements. ID needs to be put back in or the function needs to be changed for the selector. posts will send the form name variables which is why I suspect the ID was removed kind of not necessary if you change the function for the select. I added ID's back in and set the ID to the same of the name.
  2. throughout the functions it still referenced #ssid however since ID was removed those fail. when I added ID back in and set it the same as the name wifi_ssid I went through the code and update #ssid to #wifi_ssid

Opps must not have tested with IE this won't work in IE (at least not IE 11. Chrome seemed fine
params = new URLSearchParams(window.location.search);

It also fails to return anything when you save the parms. They do actually save and I've confirmed and eus_parms.lua file contains the values I have selected along with the new "hostname" field I added.

@davidcbittner
Copy link

Since all browsers don't support URLSearchParams API I've updated and added a routine to properly parse those. I've also update the items I mentioned with regards to the ID's
enduser_setup.txt

print("loading AP mode server")
if(wifi.sta.status()~=wifi.STA_IDLE)then
   wifi.sta.disconnect()
end
wifi.setmode(wifi.STATIONAP)
wifi.ap.config({ssid="MyAPNode",auth=wifi.OPEN})
enduser_setup.manual(true)
enduser_setup.start(
  function()
    print("Connected to wifi as:" .. wifi.sta.getip())
    tmr.alarm(0, 12000, 0, function() enduser_setup.stop();node.restart() end)
  end,
  function(err, str)
    print("enduser_setup: Err #" .. err .. ": " .. str)
  end
)

however this reports back an error instead of showing the IP address assigned. Also it does not show "Success" on the AP setup web page. Not sure if that is something in the module itself or the web page

enduser_setup: Err #3: 1693: dns_recv_callback failed. Send UDP data failed

@davidcbittner
Copy link

davidcbittner commented Nov 15, 2018

It appears to be something tied to the enduser_setup(true) which I did so I could set up a specific SSID to make it easy for the end user in my application and documentation.
If I comment that line out and run through the AP setup it works fine.

print("loading AP mode server")
if(wifi.sta.status()~=wifi.STA_IDLE)then
   wifi.sta.disconnect()
end
wifi.setmode(wifi.STATIONAP)
wifi.ap.config({ssid="MyAPNode",auth=wifi.OPEN})
--enduser_setup.manual(true)
enduser_setup.start(
  function()
    print("Connected to wifi as:" .. wifi.sta.getip())
    tmr.alarm(0, 12000, 0, function() enduser_setup.stop();node.restart() end)
  end,
  function(err, str)
    print("enduser_setup: Err #" .. err .. ": " .. str)
  end
)

However when this is commented out it falls back on the SSID as being "SetupGadgetxxxxx"

@davidcbittner
Copy link

davidcbittner commented Nov 16, 2018

Another feature consideration fetch any of the parms set in eus parm file created from the post. This would allow us to populate additional div elements other than <div id=deviceId></div>
In my case as I mentioned hostname is important (IMO) and plays a key role in locating the device. It would be nice to see after the setup.

Device: 123.123.123.123
Host Name: Master_Bedroom

and possibly the SSID and PW as well to insure that what was stored was correct. Obviously if it connects and we get success we know but it would still be nice to be able to see this information as an option.
I wish I had better skills at coding and reading C

@davidcbittner
Copy link

It drove crazy for about a day and a half. I managed to get the pamrs from the eus_params.lua file for ssid, password and hostname. I was able to parse out the file to parms array so

myconfig.ssid=parms.wifi_ssid
myconfig.pwd=parms.wifi_password
wifi.setmode(wifi.STATION)
wifi.sta.config(myconfig)
wifi.sta.sethostname(parms.hostname) <----error--->

hostname happened to be last in the file and contained a line feed that sethostname would not accept and threw an error. Once I striped the line feed it worked fine. Not sure if that is something that the code should do when writing the file and/or it should be documented in the command.

@davidcbittner
Copy link

the other thing I just noticed that needs stripped for this to work properly is the data is stored in the file with double quotes and line feeds.
wifi_ssid="mywifinetwork"/n
wifi_password="123connect"/n
hostname="kitchen"/n
those all need to be striped out before they can be used.
It would be better to store that information in the file
wifi_ssid=mywifi
etc...

@marcelstoer marcelstoer removed this from the Next Release milestone Nov 22, 2018
@Gui13
Copy link
Author

Gui13 commented Nov 28, 2018

Hi @davidcbittner, would you share the code you used to make the JS "UrlSearchParam" work on IE?

Otherwise, the \n you see are normal. You are supposed to 'do_file' the eus_params.lua like this:

dofile("eus_params.lua")
-- use parameters by their names (wifi_ssid, wifi_password etc.)
print("wifi_ssid=" .. wifi_ssid )
print("wifi_password=" .. wifi_password )

@Gui13
Copy link
Author

Gui13 commented Nov 28, 2018

Also, you were right about the missing 'id'. I had it lingering on my local changes.. sorry.

@davidcbittner
Copy link

Gui13, I don't know what I was thinking but you are correct dofile("eus_params.lua") I went crazy there for a bit and wrote a bunch of code that I didn't need to.. thanks.

Here is what I did to the web page to parse out the parms for the callback to it. a simple regular expression to pull those out works in both IE and Chrome... probably safari as well but I didn't test it. I also have the entire web page as a txt file above. Also I was going to do a validate on form submit because the hostname has some constraints:
"must only contain letters, numbers and hyphens('-') and be 32 characters or less with first and last character being alphanumeric"
I did a regex for that as well but got distracted and have not added it or the validate form yet.

	function getUrlParameter(name) {
		name = name.replace(/[\[]/, '\\[').replace(/[\]]/, '\\]');
		var regex = new RegExp('[\\?&]' + name + '=([^&#]*)');
		var results = regex.exec(location.search);
		return results === null ? '' : decodeURIComponent(results[1].replace(/\+/g, ' '));
	}
	window.onload = function() {
		var trying = getUrlParameter('trying');
		ab.innerText = 'Scan for Networks';
		ab.onclick = refrAp;
		$('#aplist').onchange = function () {
			$('#wifi_ssid').value = $('#aplist').value;
		};
		$('#bk2').onclick = function () {
			cur('#f1')
		}
		rs = to(refr, 0.5);
		if( trying ) cur("#f3");
	}

@marcelstoer
Copy link
Member

marcelstoer commented Nov 29, 2018

General note... @Gui13 it would be great if you could verify if/that David's comments and questions are addressed in the module documentation. I consider it likely that if things are unclear for David they are unclear for others as well 😄Same goes vice-versa for David: I invite you to propose changes to the documentation to enhance their expressiveness.

@Gui13
Copy link
Author

Gui13 commented Nov 29, 2018

Ok I'll add some doc on how to use the file. Regarding the IE compatibility, I have found the "polyfill" for it in a few lines, I have added it.

@Gui13
Copy link
Author

Gui13 commented Nov 29, 2018

@marcelstoer should be good now. @davidcbittner if you would like to test again with this version, be my guest!
Also, cosmetics: should I rebase the whole 6 commits to keep only one and force push?

@marcelstoer
Copy link
Member

should I rebase the whole 6 commits to keep only one and force push?

Thanks but that's not necessary. GitHub allows us to squash & merge with a single click.

@davidcbittner
Copy link

marcel could you build a new firmware for me and include the sntp this time? I will test this starting today if you can.
Thanks
David

@marcelstoer
Copy link
Member

David, I can't verify the firmware I just built for you atm but here it is: https://transfer.sh/wHpPy/nodemcu_float_eus_params_post_20181130-1048.bin

@saligerdo
Copy link

saligerdo commented Nov 30, 2018

hi david and marcel,
just needed exactly what you are building here today and so i did a try with the latest test bin. (nodemcu_float_eus_params_post_20181130-1048.bin)

sadly there is a problem within the "enduser_setup_http_handle_credentials" function:
it is returning "1" (ERROR, HTTP Status Code 400) when there is no custom additional input field AFTER the password input field.

int pwd_str_len = enduser_setup_srch_str(pwd_str_start, "& ");

@davidcbittner
Copy link

marcel, thanks I got sidetracked with my day job yesterday. I will give this a try today.
regards
David

@hanfengcan
Copy link
Contributor

I think that this module should be divide to captive portal and set wifi.

developer can implement other interesting web application the help of the captive portal

@NicolSpies
Copy link

I agree with @hanfengcan, the captive portal should be a module to upload configuration or any application required data to a JSON SPIFFS file that can then be used by the Lua application for anything.

@marcelstoer
Copy link
Member

@hanfengcan, @NicolSpies I tend to agree with you but this is the wrong place for such a fundamental proposal IMO. We're trying to get this PR to a ready-for-merging state. Regardless of the issues this module has the features in this PR are a significant step forward.

Feel free to propose this in a new feature but keep #1010 in mind.

@hanfengcan
Copy link
Contributor

@marcelstoer I am so sorry. I am just trying to express myself. maybe I can divide the module and push a new PR.

of course, I am looking forward to this merger.

@marcelstoer marcelstoer added this to the Next release milestone Dec 7, 2018
@IDispose
Copy link

so looking forward to this feature.

Are all form values double-quoted in eus_params.lua making them strings? I am not fluent in Lua, but is it expected that numeric values will need to be cast before usage?

@marcelstoer
Copy link
Member

Superseded by #2810.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants