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

introduce yieldUntil(untilFunction, timeoutMs) #8317

Closed
wants to merge 12 commits into from

Conversation

d-a-v
Copy link
Collaborator

@d-a-v d-a-v commented Sep 20, 2021

yieldUntil(untilFunction, timeoutMs) is a delay interruptible under a provided condition.

It is useful for ethernet drivers using recurrent scheduled functions.

edit Some WiFi examples are converted to ethernet (mdns, ssl, dhcp/static).

replaces #6212
related: #8120
fixes: #8291

…erruptible under a provided condition

useful for ethernet drivers using recurrent scheduled functions
@d-a-v d-a-v changed the title introduce yieldUntil(untilFonction, timeoutMs) introduce yieldUntil(untilFunction, timeoutMs) Sep 20, 2021
cores/esp8266/Schedule.cpp Outdated Show resolved Hide resolved
libraries/ESP8266WiFi/src/ESP8266WiFiGeneric.cpp Outdated Show resolved Hide resolved
@@ -36,7 +36,7 @@ class LwipIntfDev: public LwipIntf, public RawDev
_mtu(DEFAULT_MTU),
_intrPin(intr),
_started(false),
_default(false)
_default(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

was this intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes. Changing it to true allows to avoid calling manually eth.setDefault(), which is necessary for valid routing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

makes sense for one instance, true. ok if not a problem
(but, I wonder if default=false would be safer when there are more)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If there are more than one interface, only the last "defaulted" one becomes the default route.
I will investigate further on why this is necessary, but last time I checked packets are not routed without it.
I could have delegated the directive to "userland" but I think it is better to let the library deal with defaults so we don't need to change the API or the guidelines / examples when something changes or is fixed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see also WiFi

Copy link
Collaborator

Choose a reason for hiding this comment

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

so the instance variable does not make sense and it should be a static (and shared between the wifi and the lwipintf class), if every interface wants to become the default on callback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair enough.
I have moved the default value back to false and updated examples to explicitly use the new interface as default route.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Should the lwip part also be made aware of default modifications? (with another issue / pr, I gather, not to pollute the changes too too much)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Modification for "default" is reverted to what it was before the PR no ?
If the question is about "lwip2 - the driver adapter", it is very delicate to update the WiFi interface management code per possible hidden side effect. Maybe a new gh-issue describing the API question might be indeed opened for that purpose.

@d-a-v d-a-v added the alpha included in alpha release label Sep 23, 2021
dok-net added a commit to dok-net/arduino-esp8266 that referenced this pull request Oct 6, 2021
dok-net added a commit to dok-net/arduino-esp8266 that referenced this pull request Oct 9, 2021
mcspr pushed a commit that referenced this pull request Oct 16, 2021
esp_yield() now also calls esp_schedule(), original esp_yield() function renamed to esp_suspend().

Don't use delay(0) in the Core internals, libraries and examples. Use yield() when the code is
supposed to be called from CONT, use esp_yield() when the code can be called from either CONT or SYS.
Clean-up esp_yield() and esp_schedule() declarations across the code and use coredecls.h instead.

Implement helper functions for libraries that were previously using esp_yield(), esp_schedule() and
esp_delay() directly to wait for certain SYS context tasks to complete. Correctly use esp_delay()
for timeouts, make sure scheduled functions have a chance to run (e.g. LwIP_Ethernet uses recurrent)

Related issues:
- #6107 - discussion about the esp_yield() and esp_delay() usage in ClientContext
- #6212 - discussion about replacing delay() with a blocking loop
- #6680 - pull request introducing LwIP-based Ethernet
- #7146 - discussion that originated UART code changes
- #7969 - proposal to remove delay(0) from the example code
- #8291 - discussion related to the run_scheduled_recurrent_functions() usage in LwIP Ethernet
- #8317 - yieldUntil() implementation, similar to the esp_delay() overload with a timeout and a 0 interval
@d-a-v d-a-v added invalid merge-conflict PR has a merge conflict that needs manual correction and removed alpha included in alpha release labels Oct 19, 2021
@mcspr
Copy link
Collaborator

mcspr commented Oct 19, 2021

Closing via #7148

@mcspr mcspr closed this Oct 19, 2021
dok-net added a commit to dok-net/arduino-esp8266 that referenced this pull request Mar 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
invalid merge-conflict PR has a merge conflict that needs manual correction
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WiFiClientSecure not working lwip mode ! (insecure)
2 participants