Skip to content

fix: WiFiClient::flush() yields but can be called from events #5254

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
Oct 17, 2018
Merged

fix: WiFiClient::flush() yields but can be called from events #5254

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9f6481e
fix: WiFiClient::flush() yields but can be called from events
d-a-v Oct 17, 2018
f673fd7
yield every loop
d-a-v Oct 17, 2018
9024e39
Merge branch 'master' into flushyield
devyte Oct 17, 2018
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
6 changes: 4 additions & 2 deletions libraries/ESP8266WiFi/src/include/ClientContext.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,9 +331,11 @@ class ClientContext
prevsndbuf = sndbuf;
// We just sent a bit, move timeout forward
last_sent = millis();
}

yield();
// yield because we are staying longer
// (only if possible because we can be called from events)
esp_yield();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you mean to move the esp_yield inside the if-block? Before it would yield each loop, here it will only yield if it actually sends something so it seems like it'll hard-lock, no?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In doubt, you are right.

We are waiting for acks to give us the right to send, and they are supposed to come asynchronously. So yielding here is only for avoiding wdt.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The esp_yield() should not be conditional.
Also, while in the sys context, i.e.: in an async callback: if we're relying on receiving something and detecting that reception during the yield, that won't happen => infinite loop. The async way of doing this is to break the loop body into its own function, and schedule the function to get called by the sys context. Then, in the function body, if nothing has happened and we need to continue waiting, the function is re-scheduled, and return.
Personally, I don't like the idea of trying to allow sync calls during async callbacks. In fact, I would disallow such usage, period. The call sequence and use models are very different, and there's bound to be something that will bite hard. Async callbacks in the sys context have stricter timing requirements (e.g.: sending several packets and waiting for the acks or whatever could go boom), you can't yield and then continue execution, if you're waiting on something you have to return and come back later, etc.
What I would love to have is an async socket that can work from async callbacks, and a sync socket that is built on top of it by serializing (i.e.: waiting for) the async calls. One possibility would be to implement a new WiFiClient/Server built on top of @me-no-dev 's async libs. I'm not sure how that serialization would be implemented, I'd have to think about it, but if we did manage it, we could drop a lot of code, unify with the async libs, get an async framework into the core, and overall get lots of goodies.

}

if ((state() != ESTABLISHED) || (sndbuf == TCP_SND_BUF)) {
break;
Expand Down