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

add begin(port) to esp8266webserver, move some strings to flash, some refactoring #4148

Merged
merged 18 commits into from
Feb 7, 2018
Merged

add begin(port) to esp8266webserver, move some strings to flash, some refactoring #4148

merged 18 commits into from
Feb 7, 2018

Conversation

devyte
Copy link
Collaborator

@devyte devyte commented Jan 12, 2018

No description provided.

@devyte devyte self-assigned this Jan 17, 2018
@devyte devyte added this to the 2.4.1 milestone Jan 30, 2018
@devyte
Copy link
Collaborator Author

devyte commented Feb 4, 2018

I'm including some string stuff here: moving strings to flash, remove duplicates, and so on.
WIP

@devyte
Copy link
Collaborator Author

devyte commented Feb 5, 2018

Alright, moving all the strings and so on saves 368b of heap on build with the HttpBasicAuth example, and increases the bin size by 496b. I actually expected the bin size to decrease as well, given that I removed several duplicate strings. No idea why it went the opposite direction.

_nc = _exractParam(authReq,"nc=",',');
_cnonce = _exractParam(authReq,"cnonce=\"");
if(authReq.indexOf(FPSTR(qop_auth)) != -1) {
_nc = _extractParam(authReq, F("nc="), ',');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for fixing this typo in the method name! _exractParam always gave me a headache!

@devyte
Copy link
Collaborator Author

devyte commented Feb 6, 2018

Any other comments? corrections? suggestions? observations? indications? tribulations?

@earlephilhower
Copy link
Collaborator

earlephilhower commented Feb 6, 2018

The mimetable array and enum being in separate files will be a pain to keep in sync, if we ever add things (but that seems unlikely). Only [gz] is used, so why not just hardcode ".gz". It's not likely that the standard compression format identifier is going to change in the lifetime of this code (famous last words). You could drop the enum then, no?

You were very thorough in moving strings to PMEM, but as .rodada does dedupe, I'm not sure you gained anything from changing "" to F("") (and in some spots "" -> String()).

OTW looks good to me, no problems.

@devyte
Copy link
Collaborator Author

devyte commented Feb 6, 2018

About the mimetable in its own file, I thought it might be useful for some users. I even need it myself for something stupid. Maybe it shouldn't be under details directory, but if it's to be moved, I'm thinking do it later.
About the "", that was to soothe my OCD (it's done elsewhere).
... and now that I just had another look, I seem to have missed one, and my right eye is twitching :P
Assuming no further comments, I'll do a couple of experiments to check size changes for heap and flash usage, maybe do an Arduino autoformat, and then merge.

@devyte
Copy link
Collaborator Author

devyte commented Feb 7, 2018

@earlephilhower looks like you were right: adding F() to short strings like "" or "/r/n" doesn't improve heap, and increases bin size. I'm gonna guess that the inflection point is strings of size 4, so 3 chars.
In other words, strings of 4 chars or more should be moved to flash, while strings shorter than that should not.

More importantly: is the mimetable correctly declared to hold everything in flash?

@devyte devyte merged commit bcbd596 into esp8266:master Feb 7, 2018
@devyte devyte deleted the esp8266webserver branch February 7, 2018 03:34
@earlephilhower
Copy link
Collaborator

earlephilhower commented Feb 7, 2018

Ignore previous comment. I see the SDWebServer itself is the one causing these to be in RODATA. It basically repeats all MIME types in loadFromSdCard(). I think you're golden.

@devyte
Copy link
Collaborator Author

devyte commented Feb 7, 2018

The mimetable is an array of structs with 2 string members, which is different than an array of pointer to strings, which is why that last look made my spidey sense tingle...
I'm not sure what the declaration is supposed to look like here.

@earlephilhower
Copy link
Collaborator

You missed my updated comment. You're all good, the strings are actually all repeated in the SDWebServer example that I compiled to test. So, no problems w/the changes, but we may want to refactor the examples at some point...

@devyte
Copy link
Collaborator Author

devyte commented Feb 7, 2018

Oh, I did miss it. Great! Nothing further to do here then :)

@devyte devyte mentioned this pull request Feb 7, 2018
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.

3 participants