ESP32-S2 wifi radio: check whether already connected before trying to connect

[anecdata]

Fixes #3735 by checking to see if the station has wifi enabled and is not already connected before trying to connect. If wifi is not enabled, an exception is raised. If the station is already connected, connect returns immediately. Some early notes are in the issue comments.

This is fine now:

while True:
    # optionally, do other stuff...
    wifi.radio.connect(secrets["ssid"], secrets["password"])
    # optionally, do other stuff...

Since there's no harm now to try to connect when already connected, using wifi.radio.enabled = to toggle wifi is a little more resilient. This is (still) fine:

wifi.radio.enabled = False  # stops any scanning, and stops wifi (implicit disconnect)
# optionally, do other stuff...
wifi.radio.enabled = True  # starts wifi (station), but no automatic connection
wifi.radio.connect(secrets["ssid"], secrets["password"])

There are no changes to socketpool / socket, or to ping in this PR. Disconnections have the following consequences:

wifi.radio.enabled = False stops wifi (disconnecting in the process), so attempts to connect() when not enabled will result in various adafruit_requests RuntimeError, BrokenPipeError, and OSError exceptions. But after re-enabling with wifi.radio.enabled = True and connecting again, requests will work again.

Similarly, ping returns None when wifi is disconnected / not enabled, but will work again after enabled and connected. There is an occasional ping IDF error even when connected that I'll try to characterize.

Addendum for the second commit:

Turns out esp_wifi_connect() is not resilient to being called while wifi is stopped, so we also need to check enabled state (whether wifi is started or stopped), and not allow connect calls when wifi is stopped (raise exception):
ESP_ERROR_CHECK failed: esp_err_t 0x3002 (ESP_ERR_WIFI_NOT_STARTED) at 0x400303f8

Since connect() can now be called any time by user code, regardless of whether wifi is enabled or whether the station is already connected to an AP, it also becomes easy for user code to re-connect after a case where the AP goes away after connection (in this case, no disconnection exception was previously raised, but sockets would fail).

Addendum for the final commit:

The check to make sure wifi is enabled is added before starting a scan. If wifi isn't enabled, common_hal_wifi_radio_start_scanning_network() will raise an exception.

[anecdata]

Connect is a critical function, I'd really appreciate a careful code review, and hopefully some independent testing.

I'm still new to this, and the asynchronous nature of the event_handler relative to connect makes things interesting.

Sample test code:

import sys
import os
import time
import ipaddress
import wifi
import socketpool
import ssl

import adafruit_requests

from secrets import secrets


ITERATIONS = 5
DELAY = 0


print("""-"""*49)
print(os.uname().machine)
print(os.uname().version)

# Nice scenario: connect only when enabled
print("""-"""*49)
for _ in range(ITERATIONS):
    wifi.radio.enabled = True
    print("Enabled?  ", wifi.radio.enabled)
    print("Connect...", wifi.radio.connect(secrets["ssid"], secrets["password"]))
    print("RSSI      ", wifi.radio.ap_info.rssi)
    print()
    wifi.radio.enabled = False
    time.sleep(DELAY)

# Same, with multiple sequential connects while already connected
print("""-"""*49)
for _ in range(ITERATIONS):
    wifi.radio.enabled = True
    print("Enabled?  ", wifi.radio.enabled)
    for _ in range(ITERATIONS):
        print("Connect...", wifi.radio.connect(secrets["ssid"], secrets["password"]))
    print("RSSI      ", wifi.radio.ap_info.rssi)
    print()
    wifi.radio.enabled = False
    time.sleep(DELAY)

# Stress test: try connect and GET while wifi started, then stopped
# it should recover each time wifi is re-enabled
# user code should enable before connect to avoid socket exceptions
pool = socketpool.SocketPool(wifi.radio)
requests = adafruit_requests.Session(pool, ssl.create_default_context())

for _ in range(ITERATIONS):
    print("""-"""*49)
    wifi.radio.enabled = True
    print("Enabled?  ", wifi.radio.enabled)
    print("Connect...", wifi.radio.connect(secrets["ssid"], secrets["password"]))

    try:
        r = requests.get("http://wifitest.adafruit.com/testwifi/index.html")
        print(r.text)
    except (BrokenPipeError) as e:
        sys.print_exception(e)
    try:
        r = requests.get("https://httpbin.org/get")
        print(r.json())
    except (OSError) as e:
        sys.print_exception(e)

    print("""-"""*49)
    wifi.radio.enabled = False
    print("Enabled?  ", wifi.radio.enabled)
    try:
        print("Connect...", wifi.radio.connect(secrets["ssid"], secrets["password"]))
    except (ConnectionError, RuntimeError) as e:
        sys.print_exception(e)

    try:
        r = requests.get("http://wifitest.adafruit.com/testwifi/index.html")
        print(r.text)
    except (RuntimeError, BrokenPipeError, OSError) as e:
        sys.print_exception(e)
    try:
        r = requests.get("https://httpbin.org/get")
        print(r.json())
    except (RuntimeError, OSError) as e:
        sys.print_exception(e)

    print()
    time.sleep(DELAY)

print("Done.")

[askpatrickw]

I'm offering my approval based on testing comments in the issue. Someone else should review the code.

[tannewt]

This looks good to me too but I think we should hold this to after 6.1.0. @dhalbert can approve and merge after.

[anecdata]

I didn't mean to merge that yet. A couple of small clean-ups to radio.c. Should I back it out and make a separate PR after this one, or leave it in?

Test code for checking enabled before scan:

ITERATIONS = 10
DELAY = 0

for _ in range(ITERATIONS):
    print("Enabled?  ", wifi.radio.enabled)
    try:
        for network in wifi.radio.start_scanning_networks():
            print("{0:02X}:{1:02X}:{2:02X}:{3:02X}:{4:02X}:{5:02X}".format(*network.bssid), end=" ")
            print("{:>2d}".format(network.channel), end=" ")
            print(network.rssi, network.authmode, network.country, network.ssid)
        wifi.radio.stop_scanning_networks()
    except (RuntimeError) as e:
        print("RuntimeError:", e)
    wifi.radio.enabled = not wifi.radio.enabled
    time.sleep(DELAY)

print("Done.")

[dhalbert]

Do you mean the commit was premature? Just push another commit when you're done. No need to start over.

It's not merged yet. If it's in progress, you can mark it as a draft PR.

[anecdata]

It's done, it's just a little tangential to the original intent of this PR. The latest commit is independent of the earlier commits except for an error string. It's ready to go once the checks are done if you want me to leave it in this PR.

[anecdata]

failing on xtensa builds, which if I'm reading Discord right, is dependent on the rp2040 merge

[dhalbert]

right, I will re-run these failing PR's after the #4031 merge (which is having a small issue with the size of one build).

[dhalbert]

Oh, I misunderstood. No problem. Does it fix another issue, or is just an improvement?

[anecdata]

Just two small improvements, no associated issues. Should @tannewt review the new commit?

[tannewt]

Looks good! Thank you.