WiFiClient - op bool should return false only for empty Client

[JAndrassy]

in 'Arduino language' which doesn't use pointers or references, an empty Client with operator bool returning false is used instead of a NULL pointer. an example is the return value of server.accept().

Implementations of Client operator bool in libraries by Arduino:
the WiFiNINA library

WiFiClient::operator bool() {
  return _sock != 255;
}

the classic Ethernet library

virtual operator bool() { return _sockindex < MAX_SOCK_NUM; }

the MbedCore base class for WiFiClient and EthernetClient

  operator bool() {
    return sock != nullptr;
  }

[TD-er]

This will probably break a lot of existing code.

TBH, it makes sense to see operator bool() reflecting some "I can do something with it" instead of "No idea what I can do with it as I don't even know if it is connected".

I get it when you try to make the interface of each library acting the same, but when looking at the WiFiClient code, I don't think checking whether _client is a nullpointer is not really useful for the user of WiFiClient.
You still need to check whether it is connected and/or has data available to read.

And while we're looking at what to expect from a function call, please have a look at this:

uint8_t WiFiClient::connected()
{
    if (!_client || _client->state() == CLOSED)
        return 0;

    return _client->state() == ESTABLISHED || available();
}

I can imagine why it seemed useful to have the || available() added here, so you can still read any remaining data even though state is something other than CLOSED or ESTABLISHED.
But from the perspective of the user of the class, it is unexpected behavior. Well at least I was quite surprised to see this when I looked at the code.

So please can you elaborate when it would be of any use to the user of this class to know whether the _client pointer is set?
The only ways you can set this pointer is by assigning/copying another client, or by calling connect().
To me it seems like operator bool() should be an alias to connected() to make sense to a user.
Whether this connected() should also check available() is up for debate.

I'm a full supporter of making code better and easier to understand.
But changing it to be more in line with the style of some other code whose design choices may either be debatable or not applicable to this contect is IMHO not a good idea.

[JAndrassy]

the way you would manage clients connected to a server in 'Arduino language' is to have an array of WiFiClient objects. lets name it clients. now you need to now which item of the array is 'null'. you can test it with if (clients[i]).

To get a client you set the value with clients[i] = server.accept(); and the copy constructor fills the object clients[i].
(and if no new client is waiting,server.accept() should return a WiFiClient (by copy) which evaluates false for bool operator).

in 'Arduino language' it is normal to pass WiFiClient object to a function as a copy (to avoid writing C's * or &).
It is normal to return WiFiClent or Arduino String from a function (as a copy).

String testFnc(WiFiClient client) {
  return client.readString();
}

this is a good code in 'Arduino language'. String and Client are classes which just wrap a pointer and have methods to manage it.
and the C++ compiler can optimize it and maybe even Bjarne Stroustrup would like it.

my connected() issue #6701

[TD-er]

String testFnc(WiFiClient client) {
  return client.readString();
}

this is a good code in 'Arduino language'. String and Client are classes which just wrap a pointer and have methods to manage it. and the C++ compiler can optimize it and maybe even Bjarne Stroustrup would like it.

I hope you don't mean this WiFiClient should also be copied when calling?

About this array of clients.
I still don't get it.
What are you trying to fetch from this array of clients? One which is null or isn't?
Even if you call abort() or stopAll(), the _client object isn't deleted.
So once used until the WiFiClient object is destructed, this pointer will remain initialized.

There are only 2 exceptions:

• When an attempt to connect() immediately fails
• When assigning an other client to it which has _client set as a nullptr

It is not deleted when a connection is closed or aborted.

So I still don't get it why it is of any use to the user of such a class to know the state of this pointer.

Maybe I am just in dire need of some more coffee, but I really don't see it.

[JAndrassy]

deleted

[TD-er]

That's exactly what I meant with this:

When assigning an other client to it which has _client set as a nullptr

It isn't such a strange way of 'resetting' an object as it allows to set some defaults without making assumptions of the default constructor implementation of that class.

But here you check for connected() and not operator bool().
So it seems to me it is more useful to check for the connected state and not the pointer state of _client.

[mcspr]

Seems platform-specific thing? No mention of bool operator() in reference doc, so I would assume this would only help out with consistency between Cores
https://reference.arduino.cc/reference/en/libraries/wifi/

Should the method then check for everything that connected() does, without || available()
i.e. WiFiClient::operator bool() { return status() =! CLOSED; }?

[JAndrassy]

https://www.arduino.cc/reference/en/libraries/ethernet/if-ethernetclient/

@mcspr please read second comment above. the first replay to TD-er

[TD-er]

Should the method then check for everything that connected() does, without || available()
i.e. WiFiClient::operator bool() { return status() =! CLOSED; }?

Not sure if there are other values for status() (or will be added in the future), but the current implementation of connected() checks for ESTABLISHED.

[TD-er]

https://www.arduino.cc/reference/en/libraries/ethernet/if-ethernetclient/

This mentions "Indicates if the specified Ethernet client is ready."
But I find this a bit unclear.
What is "ready"?
When looking at it from an ethernet standpoint, it is present, connected (as in a wire is inserted and connected to a switch or other Ethernet device on the other end) and initialized.
From a user standpoint, it might also need to have an IP-address.

Edit:
Nevermind, this is EthernetClient, not Ethernet....
Need more coffee...

[mcspr]

OK, so I would agree on pointer comment. Re-reading ethernet, c/p from connected() without available() makes more sense imo

[d-a-v]

It can happen that a remote peer sends a final value (which is received locally) then remotely close the connection before this last data is read and still not lost.
Checking available() allows to be aware that data is still to be read though the connection has already been closed.
When not checking with available() there are two issues:

• main issue: received data is ignored
• network API can emit an error because a connection is closed with something still present in the pipes

The general issue with the arduino API is its inability to distinguish input and output (like the misunderstanding of Serial.flush() or network.connected() -> which way of input vs output are they ?)

When is this Client::operator bool used ?
To check for received data ?
To check whether data can be sent ?
To check whether data can be sent or received ?

The current implementation is true when "data can be sent" or "data are ready to be read = already received".
Meaning it is false when "data cannot be sent anymore" AND "there is no and there will be no more data to read, ever".
As this implementation is specific to the RAW API of lwIP, it is unclear what the proposed change means.

[JAndrassy]

The main use of op bool is to check if server.accept() returned a valid client or a 'null' client.

[JAndrassy]

now I see that _client is not set to null when it is closed and doesn't have data to read