Add documentation on threading issues with WiFi.onEvent() to examples

[egnor]

Description of Change

Adds comments explaining that the handler registered with WiFi.onEvent() will be called from another thread, and thus must be written carefully to be thread safe. Add // FIXME comments to specific handlers which are apparently written unsafely (or can't be easily validated as safe).

Tests scenarios

Untested, but only changes comments.

Related links

Addresses #6947 (comment)

[me-no-dev]

this is nothing strange. I am not sure it warrants the comments. Why setHostname is called where it is has it's reason and is not unknown. The method can not be called before the interface is started and you would also like it to be set before start talking to DHCP servers. Many things in our Arduino core run in threads and most of our APIs are thread-safe, or at least the ones that make sense to be, like SPI, Wire, Serial and so on. Some caution always needs to exercised.

[egnor]

Note that I was specifically directed to create a PR on this topic: #6947 (comment)

If you think the wording is bad or that I could explain things better or it shouldn't say // FIXME, let me know and I am happy to revise!

this is nothing strange. I am not sure it warrants the comments. Why setHostname is called where it is has it's reason and is not unknown. The method can not be called before the interface is started and you would also like it to be set before start talking to DHCP servers.

IMHO, those sequencing requirements are not clear for the end user, hence the various bugs in the topic of "I called setHostname() and it didn't work". The various sequencing conflicts with regard to the the TCP/IP subsystem are definitely an internal thing from the perspective of an Arduino user. And notably they differ between WiFi and ETH...

Many things in our Arduino core run in threads

Threads are often used internally but it's relatively uncommon that end-user code is run in a thread without that being mentioned. I have seen a large number of cases of code patterned after these examples which did non threadsafe things with the app's own data structures. A lot of programmers using the Arduino API have never even heard of threads.

and most of our APIs are thread-safe, or at least the ones that make sense to be, like SPI, Wire, Serial and so on.

It's far from clear to the observer which APIs might "make sense to be" thread safe. Specifically many of the methods on WiFi itself are not in fact thread safe!

Some caution always needs to exercised.

It's hard to exercise caution when you don't know the pitfalls-- the intent here was to provide a small warning sign next to one such pitfall.

In my understanding, generally speaking, Arduino APIs (as opposed to ESP-IDF APIs) are intended to be usable by less-technically-experienced users, and anything as gnarly as "this callback runs in a separate thread" really needs to be called out.

Normally I'd say that the fact that .onEvent is undocumented would be enough of a red flag, but the code I'm suggesting comments for is example code users are specifically encouraged to copy and adapt...

I'm certainly happy to revise these comments to give more explicit reasoning for why things are done the way they are, if that seems helpful to you?

[me-no-dev]

I'm certainly happy to revise these comments to give more explicit reasoning for why things are done the way they are, if that seems helpful to you?

I think it makes much more sense some info to be added to our documentation, rather than in examples. It could promote the functionality and warn the users that would not look at those examples as well. What do you think?

c.c. @pedrominatel

[pedrominatel]

@me-no-dev and @egnor,

We can add this information to the WiFi API and create a new document on Tutorials to show how to use it instead of adding comments on each example (hard to maintain in future examples or any changes to the WiFi API).

Do you want to propose this change on the docs, @egnor?

[mrengineer7777]

Do we have a WiFi .rst file were we can put comments rather than on the examples?

[me-no-dev]

@mrengineer7777 like this one?

[egnor]

Note that the current WiFi documentation does not mention WiFi.onEvent() at all; I have mixed feelings about documenting it given that it invokes threads, but if it's used in the example code it should be, and I can attempt to add a description. We can also document the limitations on WiFi.setHostname() there -- I'm not sure I 100% understand those limitations, but I can make a stab.

Note also that the current ETH documentation is barely more than a stub, and simply points to examples. We can try to expand it, or reference the WiFi document.

I would also put in a final plug for ALSO adding SOME comments to the example code. It's true that we can't ensure that every example gets documented, but many, many, maybe even most people use these APIs by starting out looking at examples, and it's quite handy if any significant pitfalls are called out directly therein. I think the surprise introduction of threads counts as a significant pitfall.

[mrengineer7777]

@egnor Adding comments to each example clutters them up unnecessarily. I have had no issues calling other functions from WiFi.onEvent() function handlers. That said, I believe documenting WiFi.onEvent() in WiFi.rst is very valuable. It's important to know about that separate thread in case the user notices odd behavior or crashes.

Documenting WiFi.setHostname() would also be interesting but may be a bigger task than you want to tackle. The short story on setHostname() is this: Arduino saves a copy of the hostname when you call the function, but it is not applied except when WiFi.mode goes from NONE -> STA. Not sure about Ethernet. It has something to do with the underlying IDF libraries. To make sure the hostname is set, you have to call it before WiFi.begin(). See the following related issues:

#7227
#6700
#6278
#6086
#2537

I agree more documentation on Ethernet would be good. Should probably do that on a separate PR.

[egnor]

I think my philosophy is different than yours about threads and debugging, but, I shall endeavor to add something to wifi.rst and thin out the comments, and circle back.

(I'd like to leave a small warning in comments; I submit that its value is extremely high, as threads are quite finicky and dangerous for the unfamiliar. Occasional weird hard-to-debug glitches are very. annoying. especially in embedded systems...)

[CLAassistant]

All committers have signed the CLA.

[egnor]

OK I've added some content to wifi.rst, and greatly thinned out the comments in example code. Take another look, @me-no-dev and @mrengineer7777 ?

for onEvent:

for setHostname:

[pedrominatel]

Sorry for the long delay, @egnor. LGTM.
cc @me-no-dev and @VojtechBartoska

[VojtechBartoska]

LGTM

[pedrominatel]

Can we merge, @VojtechBartoska and @me-no-dev? I need to resolve some conflicts in #8650 to finish the deployment process.