Skip to content

Convert ESP8266WebServer* into templatized model #5982

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

Merged
merged 20 commits into from
Jul 4, 2019

Conversation

earlephilhower
Copy link
Collaborator

@earlephilhower earlephilhower commented Apr 12, 2019

Supercedes #4912

Refactor the three versions of ESP8266WebServer and *WebServerSecure to a
single templated class. Use "using" to enable old, non-templated names to b
used (so no user changes required to compile or run).

Fixes #4908 and clean up the code base a lot.

Basic tests run (the ones in the example code).

No code changes are required in userland except for setting the SSL
certificates which now use a cleaner "getServer()" accessor and lets the
app use the native BearSSL calls on the WiFiClientSecure object.

@devyte should be proud, it removes virtuals...

Supercedes esp8266#4912

Refactor the three versions of ESP8266WebServer and *WebServerSecure to a
single templated class. Use "using" to enable old, non-templated names to b
used (so no user changes required to compile or run).

Fixes esp8266#4908 and clean up the code base a lot.

Basic tests run (the ones in the example code).

No code changes are required in userland except for setting the SSL
certificates which now use a cleaner "getServer()" accessor and lets the
app use the native BearSSL calls on the WiFiClientSecure object.

@devyte should be proud, it removes virtuals and even has template specialization...
Allow existing code to use the same well known names for
HTTPUpdateSecure.
@devyte
Copy link
Collaborator

devyte commented Apr 12, 2019

Overall, nice work! I think I cried a little with so many virtuals removed. I see that request handler still has some, though.

I do have some comments about code structure, namespaces and refactoring, but it will take me a bit to go through the whole PR for the specifics.

In the meantime, some comments and questions:

  1. Is there any bin size impact? IRAM impact?
  2. This change means that the secure webserver is no longer a descendant of the non-secure webserver. As a result, any sketch out there that was implemented using the inheritance may no longer build. The way a user would need to address the change is to migrate any functions that were relying on the inheritance to a template version.
    => Breaking change.
  3. As I understand things, the server and client must always match, i.e.: both must either be secure, or both not secure. Is there any scenario where a mix makes sense?
    I'm thinking:
    WiFiServer returns only WiFiClient
    WiFiServerSecure returns only WiFiClientSecure
    => maybe WiFiServer and WiFiServerSecure should have a type member that reflect the WiFiClient type they return. Then, the template parameter here would be unique: WiFiServerType. The client type would then be WiFiServerType::WiFiClientType.
    Why do this? it disallows the possibility of declaring a webserver with mismatched server/client types.
  4. The templated object(s) should be put in a namespace. The specific types defined for secure/non-secure, i.e.: the using statements, should be outside the namespace in global space and reference the template with the specific type definitions. See polledTimeout for one way to handle this.
  5. At a glance I see several methods that aren't really specific to the class they are implemented in, e.g.: urlDecode. It is good practice to move those out of the class into their own .cpp as standalone functions. This is particularly important when dealing with a template class, because each combination of template args will generate a copy of the method, and all copies will essentially be the same code.
  6. One thing I have pending is to clean up the using namespace mime, which imports mime into the global space. If the template objects are moved into their own namespace, then mime could be imported there, and global namespace pollution would no longer be an issue.

@earlephilhower
Copy link
Collaborator Author

  1. RequestHandler needs to be virtual without some major work since we need to keep track of multiple different ones with different implementations per-web-node. I don't see that changing anytime soon.

  2. Bin size is about the same on things I tried. AdvancedWebServer actually shrinks by 704 bytes vs. master, and it uses 40 bytes less heap RAM as well. But, in the grand scheme, that's noise.

  3. Are you saying an app subclassing WebServerSecure (class MyServer : public WebServerSecure {}) won't work? I think it's a no-op as you can subclass the template-ized just like normal, can't you? There's the possibility of object slicing, I guess, but the WebServer never returns itself, and only uses WiFiClient/WiFiServer which we don't expect to be subclassed. At that point, they'd need to make a template-ized instantation to get it going anyway, I think.

  4. It's impossible for a WiFiServer to return a WiFiClientSecure, and vice versa. Let me see if I can use some typename magic.

  5. Will do.

  6. Agreed, but I was trying to minimized the refactoring involved. Wouldn't that be a breaking change? For a template-ized class, I thought all methods were template-type decorated so a naked "WebServer::parseURL()" declaration wouldn't be included as a class member. Moving the urlDecode to another class/global function would definitely be a breaking change (it's a public member). I would also suggest that apps will only be using secure or non-secure versions of the object, and not both, so duplicated code bloat is not going to be a common thing.

  7. I think we moved mimetable to its own file just because of the PROGMEM segment problems. I think it can be moved back to the class...

Remove the ClientType template parameter from all objects.  Simplifies
the code and makes it more foolproof.

Add a "using" in each server to define the type of connection returned
by all servers, which is then used in the above templates automatically.
@earlephilhower
Copy link
Collaborator Author

Got 3, 4, and 6 all taken care of.

earlephilhower and others added 3 commits July 3, 2019 19:56
Make the simple mock test CI job pass and clean up
any spurious warnings in the test directory.

There still are warnings in the libraries and core, but they
should be addressed in a separate PR.
@d-a-v d-a-v merged commit 7036297 into esp8266:master Jul 4, 2019
@earlephilhower earlephilhower deleted the webserver-template branch November 18, 2020 00:16
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.

ESP8266WebServerSecure.serveStatic() and BearSSL failed with "ERR_CONTENT_LENGTH_MISMATCH"
3 participants