Copy-assignment operator needed in WiFiClient about wifi101 HOT 1 CLOSED

Agreed, this is not a clean encapsulation. As you pointed out the limitation is that we cannot rely on the heap (I don’t think we have a malloc on ArduinoZero right?), but moreover this library has to support ArduinoUno where memory space is really limited.
Your modification looks good to me.

Thibault Richard.

From: Jesse Frush [mailto:[email protected]]
Sent: Tuesday, December 29, 2015 11:19 AM
To: arduino-libraries/WiFi101
Subject: [WiFi101] Copy-assignment operator needed in WiFiClient (#15)

Pretext:

• WiFiServer::available(uint8_t*) returns WiFiClient as an object, not a pointer to an object.
• WiFiClass maintains WiFiClient* _client[] cache of pointers to WiFiClient objects, indexed by client socket.
• valid WiFiClient constructors (two of the three) assign WiFi._client[_socket] to this, attempting to cache the address for the most recently constructed WiFiClient object for a given socket.
• WiFiClient::available(), read(), and other functions use m2m_wifi_handle_events(NULL); to load data into a buffer for the client, as stored by WiFiClass cache described above.

Problem
No existing copy-assignment constructor means that the caching in WiFiClass member WiFiClient* _client[] is not always valid. Default implementation of this operator performs memory copy only. Examples provided below.

Examples::
A) WiFiClient object is stored as a global variable in a sketch

WiFiClient client; //stack memory allocated, default ctor invoked

WiFiServer server(23);

setup() { ... }

loop() {

client = server.available(); //(1) (2)

if( client.available() ) //(3)

{

...

}

}

(1) compiler optimization will invoke WiFiClient(uint8_t sock, uint8_t parentsock) ctor here when client is first created (as returned by WiFiServer.available()), this constructor will set WiFi._client[socket] = this. However, since client object is a global object, memory already exists for this object, and copy-assignment operator is invoked. WiFiClass _client cache is now invalid.

(2) Subsequent calls to server.available() will return the cached object, in which the copy-assignment operator will be invoked instead of the copy constructor. WiFiClass cache is invalidated.

(3) this line will update the buffers in the cached WiFiClient object via m2m_wifi_handle_events(NULL);, which is not the same as this client object. When m2m_wifi_handle_events(NULL); finishes, the return value of this client object is _head - _tail, which have not been updated for this object. return value is always 0, even when data has been placed into the cached object.

B) WiFiClient is stored as a member of another class

class WiFi101Stream : public Stream {

private:

WiFiServer _server = WiFiServer(23);

WiFiClient _client;

public:

inline int connect_client()

{

if( !( _client && _client.connected() ) ) {

  WiFiClient newClient = _server.available(); //(1) (2)

  if( !newClient )

  {

    return 0;

  }



  _client = newClient;  //(4)

}

return 1;

}

}

(4) Copy constructor is not invoked, only copy-assignment operator. WiFiClass cache still invalid.

Solution
I'd first like to ask why this WiFiClient caching design was used? Inter-dependent classes messing with a common cache in WiFiClass is spaghetti code, proper member encapsulation is not exercised. I imagine it was done to prevent WiFiClient objects from existing on the heap, but I doubt that a class of this very small size would cause issues if the buffers are maintained in a cleanly manner.
I have successfully tested a solution in which the WiFiServer::available() function is converted from a WiFiClient return type to WiFiClient*, in which a newly-constructed client object is first assigned to the WiFiClass _client cache and then returned. Subsequent calls to WiFiServer::available return this cached pointer. This still requires the WiFiClass _client cache to be updated by WiFiServer::available(), but at least the stored address does not have to be overwritten each time this object is passed from function to function, stored as a global object in a sketch, or encapsulated in another class as a member variable. This also prevents the copy-constructor from being invoked during every loop() (as in all of the example sketches).

However, I've assumed the existing design should not be changed (APIs/example sketches would be modified by the above change). I've instead implemented a fix by moving the logic inside the WiFiClient copy constructor to a new setMembersAndWiFiCache(const WiFiClient& other); (disclaimer: naming things is hard) function and adding a copy-assignment operator to this class, which will also call this new function. This will guarantee that if the WiFiClient object is stored in any other way than a scoped variable, the library will function properly. I will create the pull request shortly after this post is submitted and I will add a link in a comment below. I invite you to critique the naming of the new function or any other details.

—
Reply to this email directly or view it on GitHubhttps://github.com//issues/15.

from wifi101.