[Replacement for 2493] EUS: Save the post data in a file on the filesystem

[marcelstoer]

This replaces #2493 (and thus fixes #1302) which has been laying around for so long that it now has a couple of merge conflicts. I pulled in that branch and rebased it on dev (rather than bothering the author with this).

Tagging everyone who's been involved in the other PR: @Gui13, @davidcbittner, @Jcrash29, @Pirngruber, @hanfengcan, @IDispose. I invite you to take the module for one last test ride before we merge these changes.

Here's a binary with the following properties for testing:

NodeMCU 3.0.0.0 built with Docker provided by frightanic.com
	branch: eus_params_post
	commit: efe1103d9c9c7cde62cffcef9de16c371803e498
	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 2019-06-22 21:53
 powered by Lua 5.1.4 on SDK 3.0.1-dev(fce080e)

nodemcu_float_eus_params_post_20190622-2153.bin.zip

[HHHartmann]

Just reading the docs I see two weaknesses.
As i recall it has been mentioned in the original issue that writing a file and then having to read it afterwards wears SPIFFS and is unnecessarily slow.
It is also not desirable to generate global variables when reading the file via dofile.
Would be nice here to return an array here.
The file could look like this:

local p = {}
p.wifi_ssid="ssid"
p.wifi_password="password"
-- your own parameters:
p.timeout_delay="xxx"
p.device_name="yyy"

return p

But alltogether it seems to be better to return the array right away.
Saving as json might also make sense, but then the sjson module would be required.

But this could also be done in a separate PR so this one will be integrated finally.

[marcelstoer]

It is also not desirable to generate global variables when reading the file via dofile.
Would be nice here to return an array here.

👍 definitely. I think we shouldn't merge it without that fix as changing the file format later would mean a breaking change. Are you up for it?

Update: there's even a TODO in the code that points into that direction

nodemcu-firmware/app/modules/enduser_setup.c

Line 777 in e98a104

// TODO: we could call back in the LUA with an associative table setup, but this is MVP2...

Saving as json might also make sense

I personally see little to no benefit with that.

[HHHartmann]

I'm not sure what you mean. No benefit in json or no benefit in CB with array?

I would like to implement returning the data in a CB and not writing a file at all.

Having to dofile leaves a dirty heap. I don't know how bad sjson is in this respect. But probably better.

[marcelstoer]

IMO C modules shouldn't depend on each other. Being dependent on SJSON to use EUS is a no-go for me. Even more so as I see no benefit in storing the data as JSON. After all it's a simple key-value structure.

I would like to implement returning the data in a CB and not writing a file at all.

From firmware perspective this would be nice indeed. However, you're likely only delegating the "dirty" work - to the application developer. For all the use cases I can imagine (I am sure there are many more) you will want to keep the custom settings around in a persistent manner. Hence, I expect most developers will write them to a file in the callback.

Having to dofile leaves a dirty heap.

No way around that?

[HHHartmann]

Looking at this I have some more ideas :-)
The file enduser_setup.html.gz does not need to be checked in, it is only needed to create http_html_backup.def
It should not be named "backup" but rather "default" or maybe "fallback". The wording should be changed in documentation also.
And why not enduser_setup.html.gz.def.c or whatever. to better see that it is connected to the html file.
Last the folder eus should be renamed to enduser_setup. Whoever is not involved in enduser_setup cannot easily see the connection to eus.

@marcelstoer what do you think about that? Should I change it while we are at it?

[marcelstoer]

The file enduser_setup.html.gz does not need to be checked in, it is only needed to create http_html_backup.def

True. Strictly speaking neither does the .html. We could reverse the xxd and then uncompress the data (provide a script in the repo for that) if we need to make changes.

It should not be named "backup" but rather "default" or maybe "fallback". The wording should be changed in documentation also.

Both "default" and "fallback" sound good to me.

And why not enduser_setup.html.gz.def.c or whatever. to better see that it is connected to the html file.

Yeah, it's odd to include a .def file: #include "eus/http_html_backup.def"

Last the folder eus should be renamed to enduser_setup. Whoever is not involved in enduser_setup cannot easily see the connection to eus.

👍

Should I change it while we are at it?

Go ahead! Can you also include the type=text to type=password fix? If I did that we'd certainly end up with conflicting changes.

[HHHartmann]

There is also a breaking change which should be mentioned in the release notes:
The /setwifi route has changed from GET to POST. So any individual adaption of enduser_setup.html will break.

[marcelstoer]

There is also a breaking change which should be mentioned in the release notes:

True, thanks for mentioning it again. I added a breaking change label we can add to PRs for such cases.