WiFiClient - rename flush() to clear() (breaking)

[JAndrassy]

With a major version release the flush() dilemma could be resolved for WiFiClient & co. as last on this platform.

If Arduino adds to Stream a method to clear the input buffer, it will be most probably named clear because Stream is taken from Processing and there it has clear().

[TD-er]

My concern with flush vs clear is that these functions should do what their names suggest.

• flush : Output whatever is in the queue/buffer via the device or port.
• clear: Throw away whatever is in the queue/buffer.

To be honest, your replacement of the flush function for the WiFiClient is more like a flush and not a clear as you actually actively flush some buffer.

[JAndrassy]

I renamed the flush which cleared the rx buffer to clear. but flush is pure virtual in Client so I had to add empty flush
The position of the function in file is to group Print (write) related and Stream (read) related functions.

EDIT: I removed the change from WiFiClientSecure. It was change "no change".

[VojtechBartoska]

Added "breaking change" label due to the PR name, please validate it @SuGlider.

[VojtechBartoska]

Arduino.cc API uses flush (https://www.arduino.cc/reference/en/libraries/wifi/client.flush/), we should keep it.

At this point, it does NOT make sense to merge this PR.

Anyway, thanks for your proposal @JAndrassy.

[JAndrassy]

that documentation is wrong. flush() is in Print which handles output. all other classes in this core which inherit from Print use 'flush()' on TX
https://github.com/arduino-libraries/Ethernet/blob/39103da0e1bc569023625ee4693272773397dbb6/src/EthernetClient.cpp#L123

related #943

[VojtechBartoska]

The documentation reflects flush() as it is in the code, I do not see any clear() there so we need to keep flush().

[JAndrassy]

sorry but https://www.arduino.cc/reference/en/libraries/wifi/client.flush/ is the documentation of the old and obsolete Arduino WiFi library written in 2011.

WiFiNINA documentation has other nonsens but talks about TX.

Clears the buffer once all outgoing characters have been sent.

Arduino release notes 1.0 - 2011.11.30:

the Serial.flush() command has been repurposed to wait for outgoing data to be transmitted, rather than dropping received incoming data.

Arduino release notes 1.6.7 - 2015.12.17

Ethernet, WiFi, SoftwareSerial: Fixed flush() behaviour: the flush function is no more dropping the receive buffer, as per 1.0 API specification

I analyzed the API of 18 Arduino networking libraries. ESP32 is the only one with wrong implementation of flush().
Don't you see the very bad consequence of using this flush() instead of proper flush(). It means RX data loss.

[VojtechBartoska]

@JAndrassy I see, thanks a lot for the references, we will take a look!

[TD-er]

Don't you see the very bad consequence of using this flush() instead of proper flush(). It means RX data loss.

I for sure see the significant difference and for sure I would expect a flush to actually transmit data which is in the TX buffers instead of dropping.
The result from calling flush() is that the (TX)buffer is empty.
However on the receiving end it doesn't seem clear to me that it should keep the data in the RX buffer.

Conceptually, when calling flush, the buffers are empty after this call.
If you want to clear the TX buffers and don't care whether they are being sent (or actually want to make sure they are not sent), such a function should be called clear() or clearTX().
But on the receiving end, there is not something like passively waiting for RX buffers to be transferred.

From a usecase point of view, I may have a sensor which sends data which I'm not interested in.
I send a pulse and everything received after this is of interest and nothing from before this pulse.
So what should I use then to clear the RX buffer?
Looping over the data till there is nothing available() anymore seems a bit wasteful and also is no guarantee there isn't a new byte received after this last check.
So for this use case, I really want to make sure the buffer is empty immediately after sending the pulse.

To give a more concrete example which is quite timing critical and doesn't need to actually send a pulse is receiving GPS data.
On GPS units that don't have a PPS pin, you can still get quite a good idea of the accurate timestamp by looking at when the first byte of a bulk transfer has been received.
However if you don't start with an empty RX buffer, the computed timing has a jitter of several msec, sometimes even > 10 msec.

Another example is when multiplexing the same serial port for several sensors which only send data and need no command to trigger sending data.
After switch to listen to the next sensor, you need to make sure there is no data in the RX buffers anymore, so all you receive is from a single sensor.

N.B. I know the examples above are about the serial port and not WiFiClient, but the principle remains the same.

TL;DR
I don't mind if flush will be changed to keep the RX buffer data, but this does not feel like expected behavior when looking at the function name.
And it should be made very clear which (other) function should be used to immediately have the RX buffer cleared. (and TX buffer also, as you sometimes also don't want data to be sent)

As I mentioned before (in other discussions), I can totally see why functions used all over the Arduino landscape should act predictable and similar.
And I also understand it might not be feasible to change X projects to match a single project's behavior.
But.... I also think one should take a good look at what should be expected behavior and not just change things to match something simply because "that's how everybody else does it".
IMHO is such a reasoning ("that's how everybody else does it") one of the worst possible reasons to do something.

[JAndrassy]

@TD-er
https://en.cppreference.com/w/cpp/io/manip/flush
https://www.tutorialspoint.com/c_standard_library/c_function_fflush.htm
https://www.tutorialspoint.com/java/io/outputstream_flush.htm

[TD-er]

These all do flush the output buffers, which makes perfect sense.

My point is about the meaning of the word flush regarding input buffers.

I did a quick look again at the PR code and noticed you did just change the function names of flush to clear, without looking further to what was actually done.
It looks like it is only clearing (thus not flushing) the RX buffers.

I still stand by what I have written, but I also have to conclude this was a matter of "disagree to agree" (from my part).

So to conclude, I think it is wise to actually rename and/or split (where appropriate) flush() to clear() when it concerns actually clearing buffers.
And since you can't really flush an RX buffer, those should be emptied using clear() and not flush().

[JAndrassy]

I would even recommend to remove the 'clear' functions. Clearing the input blindly is not a good idea.
For example to skip the rest of the line, one can use client.find('\n');. it is a method of Stream and it uses timeout so it works good even if the input line is split between two TCP packets so there is a pause between characters.

[JAndrassy]

we will take a look!

@VojtechBartoska

[VojtechBartoska]

Hello @JAndrassy, ipv6 have been merged, the main work is now being done in Networking refactoring PR, please refer to that one. Thanks

[JAndrassy]

work is now being done in Networking refactoring PR

that doesn't change the WiFiClient API

[JAndrassy]

@me-no-dev maybe look at this one too