From d0e2a35e4d78a5d407a00815fe094cb17c679c1b Mon Sep 17 00:00:00 2001 From: Adrian Bowyer Date: Fri, 4 Oct 2013 23:33:54 +0100 Subject: [PATCH] Web still not reliable. (A) Trouble is the dynamic allocation of http_state structs. This needs to be tracked through the RepRap network firmware. More tomorrow... --- Platform.cpp | 153 ++++++++++++++++++------------------------------ Platform.h | 21 ++++--- Webserver.cpp | 6 +- network/httpd.c | 73 ++++++++++++++--------- 4 files changed, 118 insertions(+), 135 deletions(-) diff --git a/Platform.cpp b/Platform.cpp index d120772..c82fbab 100644 --- a/Platform.cpp +++ b/Platform.cpp @@ -710,16 +710,11 @@ extern "C" // Transmit data to the Network -void SetNetworkDataToSend(char* data, int length); +void RepRapNetworkSendOutput(char* data, int length, void* https); // Close the connection -void CloseConnection(); - -bool NoMoreData() -{ - return !reprap.GetWebserver()->WebserverIsWriting(); -} +void CloseConnection(void* https); // Called to put out a message via the RepRap firmware. @@ -730,8 +725,9 @@ void RepRapNetworkMessage(char* s) // Called to push data into the RepRap firmware. -void RepRapNetworkReceiveInput(char* ip, int length) +void RepRapNetworkReceiveInput(char* ip, int length, void* https) { + reprap.GetPlatform()->GetNetwork()->PushHttp(https); reprap.GetPlatform()->GetNetwork()->ReceiveInput(ip, length); } @@ -743,32 +739,6 @@ void RepRapNetworkAllowWriting() reprap.GetPlatform()->GetNetwork()->SetWriteEnable(true); } -// Called by the RepRap firmware to transmit data, if there is -// any to send. - -void SendDataFromRepRapNetwork() -{ - if(!reprap.GetPlatform()->GetNetwork()->DataToSendAvailable()) - return; - - // Stop the generation of more data. - - reprap.GetPlatform()->GetNetwork()->SetWriteEnable(false); - - // Find where the data is. - - char* data = reprap.GetPlatform()->GetNetwork()->OutputBuffer(); - int length = reprap.GetPlatform()->GetNetwork()->OutputBufferLength(); - - // Send it. - - SetNetworkDataToSend(data, length); - - // Prepare to write more, when writing is re-enabled. - - reprap.GetPlatform()->GetNetwork()->ClearWriteBuffer(); -} - } @@ -782,11 +752,12 @@ Network::Network() void Network::Reset() { + reprap.GetPlatform()->Message(HOST_MESSAGE, "Reset.\n"); inputPointer = 0; inputLength = -1; outputPointer = 0; - outputLength = -1; writeEnabled = true; + reading = true; status = nothing; } @@ -805,27 +776,53 @@ void Network::Spin() if(inputPointer < inputLength) return; + // Check for more input or send more output + ethernet_task(); - - // Send any data that's available - - SendDataFromRepRapNetwork(); - - // If we've finished generating data, queue up the - // last bytes recorded (which may not fill the - // buffer) to send. - - if(!reprap.GetWebserver()->WebserverIsWriting()) - { - if(outputPointer > 0) - { - outputLength = outputPointer; - outputPointer = 0; - } - } - } +void Network::PushHttp(void* h) +{ + if(httpStateStackPointer >= HTTP_STATE_STACK_SIZE) + { + reprap.GetPlatform()->Message(HOST_MESSAGE, "httpStateStack overflow.\n"); + return; + } + httpStateStack[httpStateStackPointer] = h; + httpStateStackPointer++; +} + +void* Network::PopHttp() +{ + httpStateStackPointer--; + if(httpStateStackPointer < 0) + { + reprap.GetPlatform()->Message(HOST_MESSAGE, "httpStateStack underflow.\n"); + return 0; + } + return httpStateStack[httpStateStackPointer]; +} + + +void Network::SetReading(bool r) +{ + reading = r; + + // If we've just STOPPED writing (i.e. if reading has just been + // set to true) check if there's anything + // Left in the buffer to send. If we've already done this, + // outputPointer will be 0. + + if(!reading) + return; + + if(outputPointer > 0) + { + SetWriteEnable(false); + RepRapNetworkSendOutput(outputBuffer, outputPointer, PopHttp()); + outputPointer = 0; + } +} void Network::ReceiveInput(char* ip, int length) { @@ -836,24 +833,6 @@ void Network::ReceiveInput(char* ip, int length) } -int Network::OutputBufferLength() -{ - if(outputLength <= 0) - return 0; - return outputLength; -} - -char* Network::OutputBuffer() -{ - return outputBuffer; -} - -void Network::ClearWriteBuffer() -{ - outputPointer = 0; - outputLength = -1; -} - void Network::Write(char b) { // Check for horrible things... @@ -864,12 +843,6 @@ void Network::Write(char b) return; } - if(outputLength >= 0) - { - reprap.GetPlatform()->Message(HOST_MESSAGE, "Network::Write(char b) - Attempt to write to unflushed buffer.\n"); - return; - } - if(outputPointer >= STRING_LENGTH) { outputBuffer[STRING_LENGTH - 1] = 0; @@ -888,19 +861,12 @@ void Network::Write(char b) if(outputPointer >= STRING_LENGTH - 5) // 5 is for safety { - outputLength = outputPointer; + SetWriteEnable(false); // Stop further input + RepRapNetworkSendOutput(outputBuffer, outputPointer, PopHttp()); outputPointer = 0; - } else - outputLength = -1; + } } -// If outputLength has been set, there's some data ready -// to send. - -bool Network::DataToSendAvailable() -{ - return (outputLength >= 0); -} bool Network::CanWrite() { @@ -910,14 +876,6 @@ bool Network::CanWrite() void Network::SetWriteEnable(bool enable) { writeEnabled = enable; - - // Reset the write buffer if needs be. - - if(writeEnabled && outputLength >= 0) - { - outputLength = -1; - outputPointer = 0; - } } // This is not called for data, only for internally- @@ -928,7 +886,8 @@ void Network::SetWriteEnable(bool enable) void Network::Write(char* s) { int i = 0; - while(s[i]) Write(s[i++]); + while(s[i]) + Write(s[i++]); } bool Network::Read(char& b) @@ -948,7 +907,7 @@ bool Network::Read(char& b) void Network::Close() { if(Status() && clientLive) - CloseConnection(); + CloseConnection(PopHttp()); Reset(); } diff --git a/Platform.h b/Platform.h index c1024df..535ca18 100644 --- a/Platform.h +++ b/Platform.h @@ -59,7 +59,7 @@ Licence: GPL // Some numbers... -#define STRING_LENGTH 1000 +#define STRING_LENGTH 1029 #define TIME_TO_REPRAP 1.0e6 // Convert seconds to the units used by the machine (usually microseconds) #define TIME_FROM_REPRAP 1.0e-6 // Convert the units used by the machine (usually microseconds) to seconds @@ -181,7 +181,9 @@ Licence: GPL // Seconds to wait after serving a page -#define CLIENT_CLOSE_DELAY 0.001 +#define CLIENT_CLOSE_DELAY 0.002 + +#define HTTP_STATE_STACK_SIZE 5 /****************************************************************************************************/ @@ -236,15 +238,14 @@ public: int8_t Status(); // Returns OR of IOStatus bool Read(char& b); bool CanWrite(); - bool DataToSendAvailable(); void SetWriteEnable(bool enable); - void ClearWriteBuffer(); void Write(char b); void Write(char* s); void Close(); void ReceiveInput(char* ip, int length); - int OutputBufferLength(); - char* OutputBuffer(); + void SetReading(bool r); + void PushHttp(void* h); + void* PopHttp(); friend class Platform; @@ -258,10 +259,14 @@ private: void Reset(); char* inputBuffer; char outputBuffer[STRING_LENGTH]; - int inputPointer, inputLength; - int outputPointer, outputLength; + int inputPointer; + int inputLength; + int outputPointer; bool writeEnabled; + bool reading; int8_t status; + void* httpStateStack[HTTP_STATE_STACK_SIZE]; + int8_t httpStateStackPointer; }; // This class handles serial I/O - typically via USB diff --git a/Webserver.cpp b/Webserver.cpp index 9d9295c..8702642 100644 --- a/Webserver.cpp +++ b/Webserver.cpp @@ -179,6 +179,7 @@ bool Webserver::LoadGcodeBuffer(char* gc, bool convertWeb) void Webserver::CloseClient() { writing = false; + reprap.GetPlatform()->GetNetwork()->SetReading(!writing); //inPHPFile = false; //InitialisePHP(); clientCloseTime = platform->Time(); @@ -209,6 +210,7 @@ void Webserver::SendFile(char* nameOfFileToSend) fileBeingSent = platform->GetFileStore(platform->GetWebDir(), nameOfFileToSend, false); } writing = fileBeingSent != NULL; + reprap.GetPlatform()->GetNetwork()->SetReading(!writing); } platform->GetNetwork()->Write("HTTP/1.1 200 OK\n"); @@ -311,6 +313,7 @@ void Webserver::GetJsonResponse(char* request) { jsonPointer = 0; writing = true; + reprap.GetPlatform()->GetNetwork()->SetReading(!writing); if(StringStartsWith(request, "temps")) { @@ -642,8 +645,6 @@ void Webserver::Spin() { if(platform->Time() - clientCloseTime < CLIENT_CLOSE_DELAY || !platform->GetNetwork()->CanWrite()) return; - //if(!platform->GetNetwork()->CanWrite()) - // return; needToCloseClient = false; platform->GetNetwork()->Close(); } @@ -665,6 +666,7 @@ void Webserver::Init() char scratchString[STRING_LENGTH]; lastTime = platform->Time(); writing = false; + reprap.GetPlatform()->GetNetwork()->SetReading(!writing); receivingPost = false; postSeen = false; getSeen = false; diff --git a/network/httpd.c b/network/httpd.c index ed1722c..5815b4e 100644 --- a/network/httpd.c +++ b/network/httpd.c @@ -70,46 +70,49 @@ struct http_state { // Prototypes for the RepRap functions in Platform.cpp that we // need to call. -void RepRapNetworkReceiveInput(char* ip, int length); +void RepRapNetworkReceiveInput(char* ip, int length, void* https); void RepRapNetworkMessage(char* s); void RepRapNetworkAllowWriting(); -bool NoMoreData(); // Static storage for pointers that need to be saved when we go // out to the RepRap firmware for when it calls back in again. // Note that this means that the code is not reentrant, but in // our context it doesn't need to be. -static struct tcp_pcb* activePcb; static struct tcp_pcb* pcbToClose = 0; -static struct http_state* activeHttpState; +//static struct http_state* activeHttpState; static struct pbuf* pbufToFree = 0; static struct tcp_pcb* sendingPcb = 0; static int initCount = 0; /*-----------------------------------------------------------------------------------*/ + +// ******** Called from outside. + static void conn_err(void *arg, err_t err) { - //struct http_state *hs; + struct http_state *hs; LWIP_UNUSED_ARG(err); - //hs = arg; - //mem_free(hs); + hs = arg; + mem_free(hs); } /*-----------------------------------------------------------------------------------*/ // Added to allow RepRap to close the connection. -void CloseConnection() +void CloseConnection(void *arg) { if(pcbToClose == 0) return; + struct http_state *hs; + hs = arg; tcp_arg(pcbToClose, NULL); tcp_sent(pcbToClose, NULL); tcp_recv(pcbToClose, NULL); - //mem_free(hs); + mem_free(hs); tcp_close(pcbToClose); pcbToClose = 0; RepRapNetworkMessage("CloseConnection() called.\n"); @@ -124,7 +127,7 @@ close_conn(struct tcp_pcb *pcb, struct http_state *hs) tcp_arg(pcb, NULL); tcp_sent(pcb, NULL); tcp_recv(pcb, NULL); - //mem_free(hs); + mem_free(hs); tcp_close(pcb); } @@ -157,9 +160,9 @@ send_data(struct tcp_pcb *pcb, struct http_state *hs) } } while (err == ERR_MEM && len > 1); - tcp_output(pcb); - - if (err == ERR_OK) { + if (err == ERR_OK) + { + tcp_output(pcb); hs->file += len; hs->left -= len; /* } else { @@ -167,6 +170,9 @@ send_data(struct tcp_pcb *pcb, struct http_state *hs) } } /*-----------------------------------------------------------------------------------*/ + +// ******** Called from outside. + static err_t http_poll(void *arg, struct tcp_pcb *pcb) { @@ -211,7 +217,7 @@ http_sent(void *arg, struct tcp_pcb *pcb, u16_t len) } else { // See if there is more to send - // TODO - possible memory leak? + // TODO - possible memory leak? cf original program RepRapNetworkAllowWriting(); } @@ -226,11 +232,15 @@ http_sent(void *arg, struct tcp_pcb *pcb, u16_t len) // that prompted the transmission, as that must now have been fully read. // If RepRap ignores input, is this another potential memory leak? -void SetNetworkDataToSend(char* data, int length) +void RepRapNetworkSendOutput(char* data, int length, void* https) { + struct http_state *hs; + + hs = https; + //RepRapNetworkMessage("Some data arrived.\n"); - activeHttpState->file = data; - activeHttpState->left = length; + hs->file = data; + hs->left = length; /* printf("data %p len %ld\n", hs->file, hs->left);*/ if(pbufToFree != 0) @@ -239,30 +249,32 @@ void SetNetworkDataToSend(char* data, int length) pbufToFree = 0; } - send_data(sendingPcb, activeHttpState); + send_data(sendingPcb, hs); /* Tell TCP that we wish be to informed of data that has been successfully sent by a call to the http_sent() function. */ - tcp_sent(sendingPcb, http_sent); } +// ******** Called from outside. + static err_t http_recv(void *arg, struct tcp_pcb *pcb, struct pbuf *p, err_t err) { int i; - char *data; + struct http_state *hs; + + hs = arg; if (err == ERR_OK && p != NULL) { /* Inform TCP that we have taken the data. */ tcp_recved(pcb, p->tot_len); - if (activeHttpState->file == NULL) + if (hs->file == NULL) { - data = p->payload; - RepRapNetworkReceiveInput(data, p->len); + RepRapNetworkReceiveInput(p->payload, p->len, hs); pbufToFree = p; sendingPcb = pcb; } else @@ -272,11 +284,14 @@ http_recv(void *arg, struct tcp_pcb *pcb, struct pbuf *p, err_t err) } if (err == ERR_OK && p == NULL) { - close_conn(pcb, activeHttpState); + close_conn(pcb, hs); } return ERR_OK; } /*-----------------------------------------------------------------------------------*/ + +// ******** Called from outside. + static err_t http_accept(void *arg, struct tcp_pcb *pcb, err_t err) { @@ -287,12 +302,12 @@ http_accept(void *arg, struct tcp_pcb *pcb, err_t err) tcp_setprio(pcb, TCP_PRIO_MIN); - // Ignore arg; we know it must be our static activeHttpState + RepRapNetworkMessage("http_accept\n"); - hs = activeHttpState; + hs = (struct http_state *)mem_malloc(sizeof(struct http_state)); if (hs == NULL) { - //printf("http_accept: Out of memory\n"); + RepRapNetworkMessage("Out of memory!\n"); return ERR_MEM; } @@ -321,11 +336,13 @@ http_accept(void *arg, struct tcp_pcb *pcb, err_t err) void httpd_init(void) { + struct tcp_pcb* activePcb; + initCount++; if(initCount > 1) RepRapNetworkMessage("httpd_init() called more than once.\n"); - activeHttpState = (struct http_state *)mem_malloc(sizeof(struct http_state)); + //activeHttpState = (struct http_state *)mem_malloc(sizeof(struct http_state)); activePcb = tcp_new(); tcp_bind(activePcb, IP_ADDR_ANY, 80); activePcb = tcp_listen(activePcb);