Skip to content

esp8266: share port 80 with regular webserver #575

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 10 commits into from
Nov 21, 2020

Conversation

d-a-v
Copy link
Contributor

@d-a-v d-a-v commented Nov 2, 2020

This PR can appear as huge changes, where it's not.

The goal is to allow sharing port 80 with native esp8266 arduino core webserver.
This is possible thanks to this change on esp8266 arduino core.

To summarize, the webserver is the one which accepts the new connections and reads the first line where method and URI are specified. Before going further, the webserver shows this line to optional hooks and let them take over the connection if they wish to.

For this to happen with arduinoWebSockets, the WebSocketsServer class has to be split into two classes:

  • WebSocketsServerCore which is mainly the original WebSocketsServer class, deprived of the TCP server
  • WebSocketsServer, derived from WebSocketsServerCore and holding the TCP server part.

and a new optional class is added:

  • WebSockets4WebServer which also derives from WebSocketsServerCore and connects to ESP8266WebServer.

The API is unchanged, the logic is not changed, examples are untouched (one is added). This PR is big because of the internal class name change and the splitting of some methods into two parts, one to WebSocketsServerCore and the other to WebSocketsServer.

Also to be noted:

  • A new internal method is added void dropNativeClient (WSclient_t * client) to properly close the TCP connection also when there is no more available place in the websocket array.
  • A websocket python client for quickly testing is added (in examples/esp8266/WebSocketServerHooked)
  • A linux script to compile&test this library without flashing (in the same directory)
  • This will work only with git head version of esp8266 arduino core (or with the unofficial snapshot release).

@d-a-v
Copy link
Contributor Author

d-a-v commented Nov 2, 2020

In this commit, adding missing String initializations was mandatory to avoid crash at run when debug mode is enabled (crash happens when displaying client->cExtensions.c_str()).

That should be done in master until this PR is merged, I can submit a PR is desired.

@mcspr
Copy link

mcspr commented Nov 2, 2020

(hit-n-run comment via esp8266 gitter)

In this commit, adding missing String initializations was mandatory to avoid crash at run when debug mode is enabled (crash happens when displaying client->cExtensions.c_str()).

Might I suggest to also take a look at initialization overall:

memset(&_clients[0], 0x00, (sizeof(WSclient_t) * WEBSOCKETS_SERVER_CLIENT_MAX));

Struct in question is initialized a-la C POD struct, which is probably not the thing we want :/ Since the struct already allows default values, this can be replaced with new / delete (placement or otherwise) or reset() method bound to the struct itself or just straight up copy assignment which will replace all members - *client = WSclient_t()

edit: specifically, because of this comment by the GCC:

.pio/libdeps/d1_mini/WebSockets/src/WebSocketsServer.cpp: In constructor 'WebSocketsServerCore::WebSocketsServerCore(const String&, const String&)':
.pio/libdeps/d1_mini/WebSockets/src/WebSocketsServer.cpp:42:83: warning: 'void* memset(void*, int, size_t)' clearing an object of type 'struct WSclient_t' with no trivial copy-assignment; use assignment or value-initialization instead [-Wclass-memaccess]
   42 |     memset(&_clients[0], 0x00, (sizeof(WSclient_t) * WEBSOCKETS_SERVER_CLIENT_MAX));
      |                                                                                   ^

edit2: assignment

@Links2004
Copy link
Owner

thanks for the work, hope I find the time to look at this I more detail at the weekend.

@d-a-v
Copy link
Contributor Author

d-a-v commented Nov 18, 2020

@mcspr I think the placement constructor is the best bet in this case.

this can be replaced with new / delete (placement or otherwise)

    // init client storage
    for(uint8_t i = 0; i < WEBSOCKETS_SERVER_CLIENT_MAX; i++) {
        // call WSclient_t constructor "in place" (=on already allocated pointer &_clients[i])
        // this ensures all String are properly initialized and sets all scalars to 0
        new (&_clients[i]) WSclient_t;
    }

Even though WSclient_t is a struct, I think it works.

edit A proper constructor is missing to initialize .num and .status,
=> new (&_clients[i]) WSclient_t(i);

@mcspr
Copy link

mcspr commented Nov 18, 2020

Maybe not placement (my mistake mentioning it, for some reason I read the code as it was using raw memory and _clients was allocated separatly...)
It will replace the object, but won't destroy the existing one that was constructed as part of the server object. Right now it does reset each member though through assignment / copy ctor, so we could call begin() multiple times. My 2nd idea was to place defaults into WSclient_t { ... } struct directly and copy-assign it back into existing client object, but it merely changes the style, it might be ok as-is.

Also note of the recent String PR to the esp8266 core which made String SSO flag 0 instead of 1, right now it will return ptr to itself aka sso buffer containing zeroes instead of 0 / nullptr, opposite of what it done before as the result of memset zeroing out memory and String assuming it's in heap.
(not tested, yet, just based on what I read in the esp8266/Arduino#7553. memset call should be removed:)

@Links2004 Links2004 changed the base branch from master to esp8266_share_port November 21, 2020 12:50
@Links2004 Links2004 merged commit 826c6b4 into Links2004:esp8266_share_port Nov 21, 2020
@d-a-v
Copy link
Contributor Author

d-a-v commented Nov 21, 2020

Thanks !

@d-a-v d-a-v deleted the hook branch November 21, 2020 13:04
@Links2004
Copy link
Owner

will be in master soon, will do some testing and init clean up.

@d-a-v
Copy link
Contributor Author

d-a-v commented Nov 21, 2020

@mcspr
My thinking is that placement constructor is the best bet with the condition it is done in class constructor (at the same time when the array is allocated), in place of the memset().
But I wonder if the memset should just be removed, because it is a regular class array and the constructors are supposed to be applied on each element. If there are defaults value to set, they might just be set in WSclient_t struct constructor.

@Links2004
Copy link
Owner

looks like the current platformio ESP8266 core does still miss the Webserver Hook Functions

@Links2004
Copy link
Owner

tested with Arduino IDE and merged to master :)
thanks for adding this feature.

@Links2004
Copy link
Owner

Released with version 2.3.1

@mcspr
Copy link

mcspr commented Nov 21, 2020

@d-a-v ref.
https://en.cppreference.com/w/cpp/language/default_initialization
https://en.cppreference.com/w/cpp/language/constructor#Initialization_order
(and pls forgive me beforehand if it is already obvious, but it felt like we are talking about slightly different things)

WSclient_t _clients[5]; indeed calls WSclient_t::WSclient_t() (which is just WSclient_t() = default; atm,) on each one of the array elements, which in turn calls String(), int(), uint8_t() and etc., for each of the client class members. Note that all member objects of the server are already constructed and initialized at the time we enter the ctor body, and placement-new yet has the same issue as memset - it overwrites the existing memory, ignoring things that already live there (https://godbolt.org/z/nbK5xT). It is just a happy coincidence that it works at all and assignment operator method allows objects to recover themselves.

For example, this way it is possible to lose String heap buffer, which will be reset to default-0 when constructor is called a 2nd time on the same memory location and does not expect the existing object. ~String() will never be called for the 1st one, unless it is done manually.

@Links2004
re. about the PlatformIO. It needs platform_packages = ... set like this:
https://github.com/xoseperez/espurna/blob/850db61b761ebb8a4496bb031922f091b6df563e/code/platformio.ini#L126-L127
Documentation is outdated atm and is only mentioning the framework git url. This was noticed at the end of the platformio/platformio-core#3612, but I'm going to create a separate espressif8266+docs issue since that one got nowhere :/

@d-a-v
Copy link
Contributor Author

d-a-v commented Nov 21, 2020

@mcspr
I think we do speak of the same thing (while I still can be wrong).

We agree on that memset() should not be here because this is not the proper way to re-initiatlize objects (even if it was working before, but as you said esp8266 core's String has recently changed and now not everything is 0 at start).
WSclient_t instances are already originally initialized (default value/constructor for all members), but WSclient_t:: in the array needs to be reinitialized in server::begin().

@Links2004 @mcspr
What would you think of removing the memset and calling a placement destructor followed by a placement constructor in server::begin() ?

diff --git a/src/WebSocketsServer.cpp b/src/WebSocketsServer.cpp
index 20b2373..072fa63 100644
--- a/src/WebSocketsServer.cpp
+++ b/src/WebSocketsServer.cpp
@@ -38,8 +38,6 @@ WebSocketsServerCore::WebSocketsServerCore(const String & origin, const String &
     _httpHeaderValidationFunc = NULL;
     _mandatoryHttpHeaders     = NULL;
     _mandatoryHttpHeaderCount = 0;
-
-    memset(&_clients[0], 0x00, (sizeof(WSclient_t) * WEBSOCKETS_SERVER_CLIENT_MAX));
 }
 
 WebSocketsServer::WebSocketsServer(uint16_t port, const String & origin, const String & protocol)
@@ -76,41 +74,18 @@ void WebSocketsServerCore::begin(void) {
     WSclient_t * client;
 
     // init client storage
-    for(uint8_t i = 0; i < WEBSOCKETS_SERVER_CLIENT_MAX; i++) {
+    for (int i = 0; i < WEBSOCKETS_SERVER_CLIENT_MAX; i++) {
         client = &_clients[i];
 
-        client->num    = i;
-        client->status = WSC_NOT_CONNECTED;
-        client->tcp    = NULL;
-#if(WEBSOCKETS_NETWORK_TYPE == NETWORK_ESP8266) || (WEBSOCKETS_NETWORK_TYPE == NETWORK_ESP32)
-        client->isSSL = false;
-        client->ssl   = NULL;
-#endif
-        client->cUrl  = "";
-        client->cCode = 0;
-
-        client->cIsClient    = false;
-        client->cIsUpgrade   = false;
-        client->cIsWebsocket = false;
-
-        client->cSessionId  = "";
-        client->cKey        = "";
-        client->cAccept     = "";
-        client->cProtocol   = "";
-        client->cExtensions = "";
-        client->cVersion    = 0;
-
-        client->cWsRXsize = 0;
-
-        client->base64Authorization = "";
-        client->plainAuthorization  = "";
-
-        client->extraHeaders = "";
-
-#if(WEBSOCKETS_NETWORK_TYPE == NETWORK_ESP8266_ASYNC)
-        client->cHttpLine = "";
-#endif
+        // reset instance
+        client->~WSclient_t();    // destructor in place
+        new (client) WSclient_t;  // constructor in place
 
+        // these could be set through constructor:
+        //      new (client) WSclient_t(i);
+        //      WSclient::WSclient(int index): num(index), status(WSC_NOT_CONNECTED), ...
+        client->num                    = i;
+        client->status                 = WSC_NOT_CONNECTED;
         client->pingInterval           = _pingInterval;
         client->pongTimeout            = _pongTimeout;
         client->disconnectTimeoutCount = _disconnectTimeoutCount;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants