ESP32-S2 wifi radio: check whether already connected before trying to connect by anecdata · Pull Request #4017 · adafruit/circuitpython · GitHub
Skip to content

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

Merged
dhalbert merged 6 commits into
adafruit:mainfrom
anecdata:connect
Jan 22, 2021
Merged

ESP32-S2 wifi radio: check whether already connected before trying to connect#4017
dhalbert merged 6 commits into
adafruit:mainfrom
anecdata:connect

Conversation

@anecdata

@anecdata anecdata commented Jan 18, 2021

Copy link
Copy Markdown
Member

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 anecdata added the espressif applies to multiple Espressif chips label Jan 18, 2021
@anecdata

anecdata commented Jan 19, 2021

Copy link
Copy Markdown
Member Author

@anecdata anecdata marked this pull request as ready for review January 19, 2021 02:14
@anecdata anecdata marked this pull request as draft January 19, 2021 18:54
@anecdata anecdata marked this pull request as ready for review January 19, 2021 19:32
@askpatrickw

Copy link
Copy Markdown

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

@tannewt tannewt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

anecdata commented Jan 21, 2021

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Collaborator

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

anecdata commented Jan 21, 2021

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

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

@dhalbert

Copy link
Copy Markdown
Collaborator

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

Copy link
Copy Markdown
Collaborator

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

@anecdata

anecdata commented Jan 21, 2021

Copy link
Copy Markdown
Member Author

tannewt
tannewt previously approved these changes Jan 21, 2021

@tannewt tannewt left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good! Thank you.

@dhalbert dhalbert merged commit ec8a42d into adafruit:main Jan 22, 2021
@anecdata anecdata deleted the connect branch January 22, 2021 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

espressif applies to multiple Espressif chips

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Calling wifi.radio.connect more than once causes wifi to disconnect

4 participants