-
Notifications
You must be signed in to change notification settings - Fork 13.3k
CaptivePortalAdvanced: Fix compatibility with Android #5069
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
Conversation
libraries/DNSServer/examples/CaptivePortalAdvanced/handleHttp.ino
Outdated
Show resolved
Hide resolved
a68c875
to
d84a8d4
Compare
Android refuses to show page with missing Content-Length header. Prepare page data to String and send it with server.send Moved respose strings to PROGMEM
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.
Approving since it solves a compatibility issue.
As said, several other strings could be PROGMEMed, I guess a(nother) general pass over all examples may fix that.
A comment would be welcome for readers to understand that in that particular HTTP case, special care with String/PROGMEM should be considered to avoid ram waste and malloc holes.
Will this get merged as part of 2.5.0? |
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.
You're doing several String concats in a row, which leads to mem fragmentation. I suggest figuring out an approximate upper bound to the resulting String size, and using Page.reserve(blah) beforehand. Given that this is an example, I think it would help users who play with this code.
A bit of explanation at this point:
C-style strings, i.e.: "blah", that are identical are unified during the build process.into a single one that is held in RAM.
Moving multiple C-style strings to flash breaks that unification, i.e.: the build process can't unify the ones declared to be in flash. If all instances of a string are moved to flash, then the bin size increases by the string size * number of string instances, and I think plus a pointer (4 bytes) to each instance.
To avoid the problem, you have to manually unify the strings, e.g.: declare a single one globally to be in flash, then use it everywhere.
There is a certain string size threshold for deciding whether to move to flash. I think the following, but this should be verified:
- at 4 bytes (3 chars + null term) it makes no difference (move string to flash saves 4 bytes, but creates a pointer which is also 4 bytes)
- strings bigger than 4 bytes (or 3 chars) should be moved to flash, but it should be done unifying all instances.
- strings smaller than 4 bytes (or 3 chars) should not be moved to flash. It doesn't help and increases bin size.
Again, the previous should not be taken at face value, and should be verified.
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.
Tested, working, thanks!
It is a general comment on String usage. |
Android refuses to show page with missing Content-Length header.
Prepare page data to String and send it with server.send