Comments (31)
Ah, ok! The difference to my code is that I don't have the *
.
@dukess: Is there a reason why you didn't use a pointer here, or can we change it without any other unintended effects?
from wifispi.
It seems to me an explicit virtual destructor is needed. The destructor should close the connection and free the socket, if used.
I'll fix it ASAP.
Thanks.
from wifispi.
That would be great, I just added an empty function
WiFiSpiClient::~WiFiSpiClient() {
}
in WiFiSpiClient.cpp as well as
virtual ~WiFiSpiClient();
in WiFiSpiClient.h and it prevents the warning (not sure if "virtual" is correct here), but you are right, the connection should be closed and the socket freed as well. Thanks for looking into this, looking for the patch!
from wifispi.
It seems to me an explicit virtual destructor is needed. The destructor should close the connection and free the socket, if used.
I'll fix it ASAP.
Thanks.
no. the WiFiClient of Arduino net API should not close the connection if destroyed. (server.available() returns a copy, some function can take it in parameter as a copy)
WiFiClient should be only a pointer to internal representation. In ESP8266WiFi library the WiFiClient does it with ClientContext. All objects of the WiFiClient class for the same TCP connection must see the same state of the connection including for example buffer and count of data available.
I recently found out that the WiFiClientSecure for BearSSL in esp8266 WiFi library doesn't work on this principle and the developers fixed it immediately with a not trivial change.
In this library the WiFiClient should only point to WiFiClient object on firmware side as it is in Ethernet library and other WiFi libraries which are only remote API of firmware.
from wifispi.
Thanks Juraj!
I see, the problem is really in the server code, thanks for mentioning it. Closing the connection when destroying the WiFiClient object would hopefully be ok and desirable when using ESP as a web client but disastrous when using it as a server.
Line 62 in ec4a9d4
But it seems to me, there should be a housekeeping after destroying the WiFiClient that is not linked to the server socket as there would be no chance to connect to the socket and close it afterwards.
from wifispi.
Thanks, that seems to correspond with what I found here:
https://stackoverflow.com/questions/12994920/how-to-delete-an-object-of-a-polymorphic-class-type-that-has-no-virtual-destruct
Does "server code" here mean the "WiFiSpiEsp" code/library? Or code inside the ESP8266-SDK?
In the meantime, should I then refrain from using "delete" or just ignore the warning if the necessary changes are not trivial and will take some time?
from wifispi.
The library can be used to run a TCP client or server.
The server opens a listener and every time a connection is made it creates a WiFiClient object. This object is not supposed to free the socket in the WiFiSpiClass::_state array because it serves for subsequent requests.
On the other hand, I think, closing the connection when WiFiClient goes out of scope or is deleted is perfectly fine for client connection (imagine e.g. sending data to a web service using REST API). I have always used the WiFiClient object as static object, so I did not have the problem. I'll fix it once I make a test case.
from wifispi.
Jiri, this is my PR to esp8266 WiFi library, which contains a discussion about correct Arduino Server and Client implementation
esp8266/Arduino#7612
from wifispi.
Juraj, the problem with multiple WiFiClient objects for one connection is constrained to TCP server functionality, right?
I admit, I am a little confused, must study some code and refresh my code as well.
from wifispi.
it is valid for arduino code to copy the Ethernet/WiFiClient object. as assignment or as function parameter by copy (parameter type without & or *). this is not (ab)used widely, but can occur. I can't now find an example. I checked some libraries which work with Client and they all use Client&
.
This is similar how String is used.
String someFns(String s) {
String s2 = s;
return s2;
}
is valid and works. it looks crazy for a C++ programmer and is not efficient (compiler optimizes it to some level)
from wifispi.
I see. The problem is quite bigger than I originally thought.
Even in my code, the implicit copy constructor is used (on return from the function):
WiFiSpiClient WiFiSpiServer::available(uint8_t* status)
{
WiFiSpiClient client(_sock);
uint8_t _client_status = client.status(); // creates Client object on ESP side if there is established connection
// uint8_t _server_status = this->status(); removed, may be related with the comment below, running fine without it
if (status != NULL)
*status = _client_status;
// TODO: If server is not in listen state, restart it (code present in original WiFiServer.h). I think it is useless.
if (_client_status == ESTABLISHED)
return client;
return WiFiSpiClient(SOCK_NOT_AVAIL); // closed Client
}
Definitely, WiFiClient destructor should not close the connection.
Thanks for the help.
from wifispi.
index to an array on the ESP side. See https://github.com/JiriBilek/WiFiSpiESP/blob/018349de89ef63ff8f4d01792f9bfda29b4c9eca/WiFiSPIESP/WiFiSPICmd.h#L39 and next few lines
It must be synchronized between the protocol (SPI) client and server.
Edit: maybe you are asking the meaning of the command WiFiSpiClient client(_sock);
It creates the client object for the given server one.
from wifispi.
sorry I deleted the comment to not open a new problem. I don't want to look into source code of your library now, but shouldn't the firmware return 'sock' index of the current client. there can be more then one client connected.
and next problem is that, the esp8266 WiFiServer available() implementation doesn't comply with Arduino.
https://www.arduino.cc/en/Reference/WiFiServerAvailable
from wifispi.
@fredlcore : weird thing: the following code compiles without error. Am I missing something?
Compiled with arm-none-eabi-gcc\4.8.3-2014q1
#include "WiFiSpiClient.h"
void setup() {
WiFiSpiClient *pp = new WiFiSpiClient();
delete pp;
}
void loop() {
}
from wifispi.
Reason is only one: pointer required only 2(4) bytes RAM when we won't need network connection. As usually it is not life critical for Due but for Mega.
In our code we use pointer.
#ifdef MAX_CUL
#ifdef WIFI
WiFiSpiClient *max_cul;
#else
EthernetClient *max_cul;
#endif
#endif
from wifispi.
Ah, ok! The difference to my code is that I don't have the
*
.
@dukess: Is there a reason why you didn't use a pointer here, or can we change it without any other unintended effects?
you have a pointer. max_cul is a pointer. it would not compile if it wouldn't
@dukess, repeated heap allocation fragments the heap and there is no memory management to defragment it.
from wifispi.
Sorry, I looked further down in our code where it says
max_cul = new WiFiSpiClient();
but of course max_cul was defined earlier as you point out correctly.
@JiriBilek: Hm, if you had a gcc version prior to 4.7, the warning was apparently not shown, see here:
https://lists.boost.org/Archives/boost/2012/02/190544.php
But with your compiler version, it therefore should - unless arm's gcc is still behind on that one?
from wifispi.
...and yes, that seems to be the case: If I compile your code snippet, I get the following warning:
/Users/frederik/Documents/PlatformIO/Projects/BSB-LAN/src/BSB_lan.ino: In function 'void setup()':
/Users/frederik/Documents/PlatformIO/Projects/BSB-LAN/src/BSB_lan.ino:5:12: warning: deleting object of polymorphic class type 'WiFiSpiClient' which has non-virtual destructor might cause undefined behavior [-Wdelete-non-virtual-dtor]
delete pp;
^~
from wifispi.
@fredlcore Now, I tested compilation under xpack-arm-none-eabi-gcc/9.2.1-1.1, got warning about deprecated boolean type but nothing more. Could you please post your compiler version?
from wifispi.
arm-none-eabi-g++ (GNU Tools for Arm Embedded Processors 7-2017-q4-major) 7.2.1 20170904 (release) [ARM/embedded-7-branch revision 255204]
Strange that your newer compiler version would not complain about this, but my older one would?
Have you tried with -Wdelete-non-virtual-dtor
?
from wifispi.
With -Wdelete-non-virtual-dtor
I get the warning (on 9.2.1), thanks.
from wifispi.
I'm nowhere near being a C++ guru, but maybe this is helpful?
https://stackoverflow.com/questions/43282826/suppress-delete-non-virtual-dtor-warning-when-using-a-protected-non-virtual-dest
The selected answer mentions several ways to deal with this, but my OO-related knowledge is too limited to tell which one would be the most suitable here.
It also mentions the two different error messages where one says "will cause undefined behaviour" and one says "might cause undefined behaviour". We're in the "might" area here, so maybe the problem will only become manifest if we were to inherit from WiFiSpiClient. But still it would probably be good to adopt one of the approaches mentioned there to prevent any possible undefined behaviour?
from wifispi.
Fixed: 28f491e
from wifispi.
Great, thanks!
from wifispi.
@JAndrassy Yesterday I tried turning the connection on and off multiple times to see if there would be memory leaks. Maybe I was lucky, but the size of free memory did not decrease.
from wifispi.
@JiriBilek thank you!
from wifispi.
@ everybody: you're welcome
@dukess It's great you have no problems in heap fragmentation.
Nevertheless, Juraj is right, in general. If you are RAM constrained, you should not repeatedly allocate big objects on the heap. I learned this in automotive applications, where you'd agree, the reliability is more than desired. Some practices forbid using heap at all, another allow heap usage but you must do all your allocation in boot time of the app.
I haven't measured it, but WiFiClient class should be not big, so the risk of heap fragmentation should be low.
from wifispi.
@JiriBilek Thanks for the explanation. I think that in our project, we should not be too afraid of fragmentation: configuration changes (enabling/disabling modules) are rare, and we can send the controller to restart after saving the configuration. The main purpose of using pointers is to save memory when the module is not in use.
from wifispi.
BTW: the same warning also occurs with the official Ethernet 2.0 library...
from wifispi.
We have two way: add destructor independently or ask developers.
from wifispi.
I opened a pull request, but this is no longer the scope of this issue here, just wanted to mention that this seems to be a more general problem...
from wifispi.
Related Issues (20)
- Using library with Arduino Zero clone. HOT 2
- Example wifiManager HOT 1
- Problem with 3 second delay when calling WiFiSpiServer::avaliable() HOT 28
- Allow more than one connection in WiFiSPIServer HOT 3
- WiFi module not present HOT 23
- Protocol version mismatch. Please upgrade the firmware HOT 2
- Possible to run this on Arduino Due? HOT 10
- Add .remoteIP() function to WiFiSpiClient HOT 1
- How to manage maximum socket connections? HOT 16
- ESP8266: change AxTLS to BearSSL HOT 1
- Deprecated boolean type HOT 1
- Problems using ArduinoMDNS with WiFiSpi HOT 13
- Possible to set MAC address of ESP? HOT 1
- Reason for limitation of 4 sockets? HOT 1
- Protocol version mismatch. Please upgrade the firmware HOT 8
- Connect fail using BearSSL branch HOT 26
- Version mismatch HOT 2
- WiFiSpi.config(...) problems with number of parameters HOT 6
- Unstable HOT 7
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
🖖 Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. 📊📈🎉
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google ❤️ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from wifispi.