-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
EndUser_setup will now be able to accept multiple other inputs via th… #2132
Conversation
…e .html page generated. These inputs will be saved to the file system under a JSON file which can than be read, and used further by the system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a feature that would be really good to have. It'll need a bit of work before it's ready for merging though.
) | ||
{ | ||
int p_file = 0; | ||
lua_State *L = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stylistically I'd prefer to see these declared right before use rather than old-C style top-of-function.
ENDUSER_SETUP_DEBUG("enduser: opening enduser_custom_parameters.json for write"); | ||
|
||
// setup the file output | ||
p_file = vfs_open("enduser_custom_parameters.json", "w"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May I suggest a shorted filename? Perhaps "eus_custom.json"? Feels like a lot of characters to type otherwise.
Actually, is it guaranteed to even be JSON? If not, maybe "eus_custom.txt"?
|
||
err |= enduser_setup_http_urldecode(decoded_config_str, config_str_start, config_str_len, sizeof(decoded_config_str)); | ||
|
||
enduser_setup_write_file_with_extra_configuration_data(decoded_config_str, config_str_len); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return value of enduser_setup_write_file_with_extra_configuration_data()
is being ignored here. It probably shouldn't be. Also, that's one long function name. Quite descriptive though, can't argue with that :)
return 1; | ||
} | ||
|
||
/* Not certain what this does */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gets you the correct Lua context to work on. Note that this lua_State
stack might already be in use, so care must be taken to ensure that the stack is left in the same condition as when the function was entered. Weird Stuff(tm) happens otherwise, of the variety that's hard to track down.
|
||
if (L != NULL) | ||
{ | ||
lua_pushlstring(L, configuration_string, configuration_length); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Without the lua_call()
below, this will end up leaving the stack in an inconsistent state. I'd suggest moving this down to the same block, there's no reason not to from what I can see.
int config_str_len = enduser_setup_srch_str(config_str_start, "& "); | ||
/* Create a new variable for storing the decoded string. | ||
* Add one more size(char) to allow for \0 */ | ||
uint8 decoded_config_str[config_str_len+1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note of caution that the stack space is rather limited. It would be safer to os_malloc()
this buffer (and later os_free()
it. As-is, someone could smash the stack by submitting a really long config parameter.
@@ -160,6 +160,8 @@ <h3>Connect device to your Wi-Fi</h3> | |||
</div> | |||
<input id=ssid type=text autocorrect=off autocapitalize=none placeholder='Wi-Fi Name' /> | |||
<input id=wifi_password type=text autocorrect=off autocapitalize=none autocomplete=off placeholder=Password /> | |||
<input id=TwitterAPI type=text autocorrect=off autocapitalize=none autocomplete=off placeholder='Twitter API Handler' /> | |||
<input id=TwitterMessage type=text autocorrect=off autocapitalize=none autocomplete=off placeholder='Twitter Message' /> | |||
<button id=submit type=button>Save</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These additions shouldn't be part of the default page.
config.twitterAPI = $('#TwitterAPI').value; | ||
config.twitterMessage = $('#TwitterMessage').value; | ||
json = JSON.stringify(config); | ||
var url = '/setwifi?wifi_ssid=' + encodeURIComponent($('#ssid').value) + '&wifi_password=' + encodeURIComponent($('#wifi_password').value) + '&config=' + encodeURIComponent(json); | ||
clearTimeout(rs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likewise, these shouldn't be part of the default page. It'd be a good example in the docs though.
@@ -1,205 +1,213 @@ | |||
static const char http_html_backup[] = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, changes should not be part of the default. Same goes for http_html_backup.def
.
* | ||
* | ||
* | ||
* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SPAAAAAAAAAAAAAAAAAAAAACE! :)
@jmattsson THANKS! I'll start making these changes. Really appreciate you taking the time to look at this! |
Johny, I'm really happy you took the time to review this PR. I was afraid no one else from us maintainers besides me thought this be a very useful addition to the current module. |
@@ -160,6 +160,8 @@ <h3>Connect device to your Wi-Fi</h3> | |||
</div> | |||
<input id=ssid type=text autocorrect=off autocapitalize=none placeholder='Wi-Fi Name' /> | |||
<input id=wifi_password type=text autocorrect=off autocapitalize=none autocomplete=off placeholder=Password /> | |||
<input id=TwitterAPI type=text autocorrect=off autocapitalize=none autocomplete=off placeholder='Twitter API Handler' /> | |||
<input id=TwitterMessage type=text autocorrect=off autocapitalize=none autocomplete=off placeholder='Twitter Message' /> | |||
<button id=submit type=button>Save</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with Johny here. I'm a bit surprised to see no changes to the documentation. I wouldn't want to change the default page but see documented how to change it in
- the module documentation
- or by creating a nice example at https://github.com/nodemcu/nodemcu-firmware/tree/master/lua_examples and linking documentation to it
Has this been added? Is there any documentation available how to make use of this functionality? I had posted a request earlier about adding the host name as one of the parameters that should be set during the configuration. This is really a fundamental field which IMO should be added to the default HTML page but if it's not I'm certainly willing to save my own on there but not sure how to actually implement this. |
As you can see this PR is still open i.e. the code is not available in the NodeMCU repository yet. Hence, if you wanted to use it right now you'd have to manually build from the author's repository https://github.com/Jcrash29/nodemcu-firmware.
If you follow the conversation above you'll also see that we requested a couple of changes from the author before we'd merge the feature into our repo. One of the comments asks for (more/better) documentation. |
@Jcrash29 I just realized you based your work on |
Marcel, thank you for the reply. I am a bit new to github and learning the procedures, flags, etc…
|
Hi, is anyone working to get it implemented? |
Sorry not currently.
I've got a couple other projects I am addressing.
I did implement several of the code changes recommended above, however have
to sufficiently tested them to create a commit.
|
I would suggest to modify the code in the following way:
|
Hey guys,
Feel free to tear the code apart, this is my first time making a PR to project with which I didn't know the dev team.
This code is working, but still has some debug code in there for items I was confused about.
What it does:
Allows for inputs to be added to the .html file for enduser_setup and those extra inputs can be added to a JSON file for later extraction and use.
links to similar PR's:
#1886