Skip to content

fix #1002 ::Flush() wait for empty send buffer #3967

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Dec 17, 2017
Merged

fix #1002 ::Flush() wait for empty send buffer #3967

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6a933ef
fix #1002 ::Flush() wait for empty send buffer
d-a-v Dec 13, 2017
a6a7eb5
WiFiClient::Flush() guarantees that the data has been delivered
d-a-v Dec 14, 2017
657cf44
Merge branch 'master' into 1002
devyte Dec 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libraries/ESP8266WiFi/src/WiFiClient.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ size_t WiFiClient::peekBytes(uint8_t *buffer, size_t length) {
void WiFiClient::flush()
{
if (_client)
_client->flush();
_client->arduinoFlush();
}

void WiFiClient::stop()
Expand Down
3 changes: 1 addition & 2 deletions libraries/ESP8266WiFi/src/WiFiUdp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,8 +243,7 @@ int WiFiUDP::peek()

void WiFiUDP::flush()
{
if (_ctx)
_ctx->flush();
endPacket();
}

IPAddress WiFiUDP::remoteIP()
Expand Down
11 changes: 11 additions & 0 deletions libraries/ESP8266WiFi/src/include/ClientContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,17 @@ class ClientContext
_rx_buf_offset = 0;
}

void arduinoFlush()
{
#if 1
// this is useless since _write_from_source() always exits with _datasource==NULL
while (state() == ESTABLISHED && _datasource && _datasource->available()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, assuming we are calling this function from Arduino loop context (not from system callback), _datasource is NULL, and the remaining data, if any, has been passed to LwIP. FWIW, i don't think that the official WiFi101 library actually guarantees that the data has been delivered (only that it was passed to the WiFi modem). But let's assume we want to do better than that.

I see two ways of implementing this:

  1. Wait until TCP window size is reset by LwIP. That involves checking if tcp_sndbuf(_pcb) == TCP_WND_MAX(_pcb), and delaying, say, for 1 ms while it is not. Normal write timeout needs to apply, i think.

  2. Introduce a flag for "some data not acked", and set it to 1 when writing the last chunk of data from the datasource. Set it to 0 from _sent callback, if _datasource == NULL. When entering flush method, check if the flag is set. If it is set, set _send_waiting to 1, and call esp_yield (or delay(timeout)). When _sent callback is called by LwIP, it will call _write_some_from_cb, which will esp_schedule, resuming the main task. At this point we know that the data is written.

Option 2 looks more "correct" to me, because it doesn't involve delaying and semi-busy-waiting with an arbitrary interval, but it adds another state flag which can potentially introduce bugs more severe than the one we want fixed here. So given that rc-2 is already behind, i would personally go for option 1 and add a ticket to implement option 2 later.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, how about renaming the existing flush to discard_received and this new function to something like wait_until_sent or similar?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did my homework

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

notes:
It appears that TCP_WND_MAX() is for the receive windows. Max value of snd_buf is a defined TCP_SND_BUF constant.
I raised the max delay to 10ms after measuring it with a heavily loaded tcp echo tester (both side on wifi)

_write_some();
delay(1); // esp_ schedule+yield
}
#endif
}

uint8_t state() const
{
if(!_pcb) {
Expand Down
9 changes: 7 additions & 2 deletions libraries/Ethernet/src/EthernetUdp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ size_t EthernetUDP::write(const uint8_t *buffer, size_t size)
int EthernetUDP::parsePacket()
{
// discard any remaining bytes in the last packet
flush();
clear_incoming();

if (recvAvailable(_sock) > 0)
{
Expand Down Expand Up @@ -204,7 +204,7 @@ int EthernetUDP::peek()
return b;
}

void EthernetUDP::flush()
void EthernetUDP::clear_remaining()
{
// could this fail (loop endlessly) if _remaining > 0 and recv in read fails?
// should only occur if recv fails after telling us the data is there, lets
Expand All @@ -216,3 +216,8 @@ void EthernetUDP::flush()
}
}

void EthernetUDP::flush()
{
endPacket();
}

3 changes: 3 additions & 0 deletions libraries/Ethernet/src/EthernetUdp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,9 @@ class EthernetUDP : public UDP {
uint16_t _remotePort; // remote port for the incoming packet whilst it's being processed
uint16_t _offset; // offset into the packet being sent
uint16_t _remaining; // remaining bytes of incoming packet yet to be processed

protected:
void clear_remaining();

public:
EthernetUDP(); // Constructor
Expand Down