Skip to content
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.

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

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

Merged
merged 3 commits into from
Dec 17, 2017
Merged

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Dec 14, 2017

ClientContext::arduinoFlush() needs review

{
#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)

option 1 of esp8266#3967 (comment)
10ms max wait according to loaded tcp echo/reply scheme
@devyte
Copy link
Collaborator

devyte commented Dec 17, 2017

At a glance, code looks good to me (untested).
Can this be merged, or is there anything pending for 2.4.0? (I know there is something pending for 2.5.0)

@d-a-v
Copy link
Collaborator Author

d-a-v commented Dec 17, 2017

I guess it can be merged (authors can't approve their own PR :-) )

@devyte devyte merged commit 26980b3 into esp8266:master Dec 17, 2017
@JAndrassy
Copy link
Contributor

and moving flush() from Stream to Print?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants