Improve native wifi API compatibility #199

anecdata · 2024-04-30T19:14:34Z

• connect now matches (subset of) native wifi API, and deprecates the old secrets-dict-based API, ~~but does still return the esp.status instead of the native wifi None~~ and (usually) returns None.

• new ipv4_address matches native wifi API

• new connected matches native wifi API

This allows the following code, regardless of whether the board has native wifi, or an ESP32SPI (Airlift) co-processor:

radio.connect(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))
radio.connected
radio.ipv4_address

esp32spi_simpletest.py works fine: it puts settings.toml credentials into a secrets dict, then passes the elements of that dict to the connect_AP() param fields. If we want to encourage the native methods, or deprecate the old methods someday, the examples could be changed.

anecdata · 2024-04-30T19:16:20Z

Tested on:
Adafruit CircuitPython 9.1.0-beta.1-10-gc92bb9b3cc on 2024-04-25; FeatherS3 with ESP32S3

import time
import os
import board
import digitalio
import wifi
from adafruit_esp32spi import adafruit_esp32spi


time.sleep(3)

# native wifi
print("\nNative wifi...\n")

radio = wifi.radio
# radio.disconnect() not implemented; use radio.enabled = False or radio.stop_station()
print(f'{radio.connect(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=}')
print(f'{radio.connected=}')
print(f'{radio.ipv4_address=}')

# ESP32SPI
print("\nESP32SPI...\n")

spi = board.SPI()
esp32_cs = digitalio.DigitalInOut(board.D13)
esp32_reset = digitalio.DigitalInOut(board.D12)
esp32_ready = digitalio.DigitalInOut(board.D11)
radio = adafruit_esp32spi.ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset)

# new APIs
print(f'NEW {radio.connect(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=}')
print(f'{radio.connected=}')
print(f'{radio.ipv4_address=}')

print(f'\nradio.disconnect()=')
print(f'NEW {radio.connect(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"), timeout=10)=}')
print(f'{radio.connected=}')
print(f'{radio.ipv4_address=}')

# compatibility:
print(f'\nradio.disconnect()=')
print(f'ORIG {radio.connect_AP(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=}')
print(f'{radio.connect_AP(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=}')
print(f'{radio.is_connected=}')
print(f'{radio.ip_address=}')
print(f'{radio.pretty_ip(radio.ip_address)=}')
print(f'{radio.status=}')  # 3 == WL_CONNECTED

print(f'\nradio.disconnect()=')
print(f'ORIG {radio.connect_AP(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"), timeout_s=10)=}')
print(f'{radio.is_connected=}')
print(f'{radio.ip_address=}')
print(f'{radio.pretty_ip(radio.ip_address)=}')
print(f'{radio.status=}')  # 3 == WL_CONNECTED

# these should never disagree:
# esp.connected
# esp.is_connected
# esp.status == 3:  # "WL_CONNECTED"

code.py output:

Native wifi...

radio.connect(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=None
radio.connected=True
radio.ipv4_address=192.168.6.183

ESP32SPI...

NEW radio.connect(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=None
radio.connected=True
radio.ipv4_address=192.168.6.74

radio.disconnect()=
NEW radio.connect(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"), timeout=10)=None
radio.connected=True
radio.ipv4_address=192.168.6.74

radio.disconnect()=
ORIG radio.connect_AP(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=3
radio.connect_AP(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"))=3
radio.is_connected=True
radio.ip_address=bytearray(b'\xc0\xa8\x06J')
radio.pretty_ip(radio.ip_address)=192.168.6.74
radio.status=3

radio.disconnect()=
ORIG radio.connect_AP(os.getenv("WIFI_SSID"), os.getenv("WIFI_PASSWORD"), timeout_s=10)=3
radio.is_connected=True
radio.ip_address=bytearray(b'\xc0\xa8\x06J')
radio.pretty_ip(radio.ip_address)=192.168.6.74
radio.status=3

dhalbert

Since there are still a number of differences between this API and Radio, I was thinking about making it upward compatible for a time, change all the examples, add new Radio-compatible API calls, and then remove the old ones. For instance, ESP32SPI has create_AP(), Radio has start_ap(), etc. Radio also has stop_ap(), etc.; this does not, but could it?

This could be done in the existing class, or we could make a new wrapper class
around adafruit_esp32spi, calling it, say radio, and mimic wifi.radio. This would increase the code size, though.

anecdata · 2024-04-30T22:20:21Z

It would grow during the overlap interval. Yes, there are a number of functions that could be normalized ...though not all params would necessarily match in either direction. When it gets into the socket stuff... that's a whole other ballgame.

anecdata · 2024-04-30T22:24:20Z

NINA could be a bottleneck for some missing functions (most NINA functions are by now exposed in ESP32SPI, but not 100%). For example, no stop_AP in NINA, but maybe could fudge it by setting an invalid AP and ignore the error (or just esp.reset) ...could get a little weird.

justmobilize · 2024-04-30T23:30:07Z

@dhalbert do you know if we added the wrapper if it would fit in all the places it's frozen?

dhalbert · 2024-05-01T01:31:58Z

@dhalbert do you know if we added the wrapper if it would fit in all the places it's frozen?

Yes, I'd be worried about that, so maybe we have to do it all at once. But we'd need to measure the sizes.

dhalbert · 2024-05-01T01:33:08Z

@anecdata Another thing I was thinking about was, for a transition period, making connect() take either a dict or SSID and password. It could look at the args and choose what to do. I'm not sure if this is worthwhile or not.

anecdata · 2024-05-01T02:05:02Z

for a transition period, making connect() take either a dict or SSID and password

I had thought about that, and it was my first inclination. Then it seemed like the other vestiges of the secrets dict had been removed from the API and settings.toml is k-->v only without dicts, so I just changed the function. But I don't have a strong feeling about it. If there are learn guides or examples out there, or people still using secrets, then maybe we don't cut the cord so fast.

dhalbert · 2024-05-01T02:24:23Z

I will take a look at the guides tomorrow and see how much needs to be changed.

anecdata · 2024-05-01T02:48:19Z

I did a quick Github search on the guides repo, and it looks pretty prevalent there. I should be able to work up a code option tomorrow for handling both cases, I've seen it in libraries for other situations.

dhalbert · 2024-05-01T03:25:36Z

I am willing to change all the guides examples coincident with the library release.

justmobilize · 2024-05-01T03:58:28Z

Hmmm, I'm thinking. Do we really want to name it radio? Would we do:

from adafruit_esp32spi import radio
??? = radio(spi_bus, esp32_cs, esp32_ready, esp32_reset)

or add a __init__.py with imports so we could do:

imoprt adafruit_esp32spi
radio = adafruit_esp32spi.radio(spi_bus, esp32_cs, esp32_ready, esp32_reset)

And this started as a thought if we want to rename adafruit_esp32spi_socketpool to just socketpool?

anecdata · 2024-05-01T16:53:03Z

Just in case we want to support secrets stragglers I tested this with and without the timeout kwarg, plus a few error scenarios (it's not committed to the pr at the moment):

    def connect(self, *args, timeout=10):
        """Connect to an access point with given name and password."""
        if len(args) == 2:
            ssid, password = args
        elif len(args) == 1:
            if isinstance(args[0], dict):  # secrets
                ssid, password = args[0]["ssid"], args[0]["password"]
            elif isinstance(args[0], str):  # open AP
                ssid, password = args[0], None
            else:
                raise ConnectionError("Invalid credentials format")
        else:
            raise ConnectionError("Invalid credentials format")
        self.connect_AP(ssid, password, timeout_s=timeout)

(connect_AP(self, ssid, password, timeout_s=10) does the heavy lifting of converting parameter formats, calling lower-level functions, and enforcing any timeout)

It handles all three cases:

separate ssid and password strings passed in
secrets dict passed in
ssid string only passed in (open network)

dhalbert · 2024-05-01T21:57:40Z

I was also thinking whether we might just start a new library that uses the low-level SPI code here but has much more of a wifi.radio API. That would avoid issues about compatibility and incompatibility. It could be shipped as the frozen library in 9.2 or 10.0, say. It might be called Adafruit_CircuitPython_AirLift_WiFi or something. There is already a small Adafruit_CircuitPython_AirLift that just manages setup (choosing BLE or WiFi).

anecdata · 2024-05-02T01:27:44Z

Should I close this in favor of one for the alternatives for some future import radio-like_esp32spi?

justmobilize · 2024-05-02T01:29:48Z

I personally would love to see this go in as is, and then build a better one...

Happy to go update docs

dhalbert · 2024-05-02T01:32:29Z

Should I close this in favor of one for the alternatives for some future import radio-like_esp32spi?

No, I think we can do this as is. I will look tomorrow about how many guides need to be edited, and plan time tomorrow or soon after to merge this, push the release and then edit the guides immediately.

justmobilize · 2024-05-02T03:03:37Z

@dhalbert let me know how I can help.

@anecdata I will test tomorrow

dhalbert · 2024-05-02T14:36:03Z

OK, this is a problem: ESP32SPI is frozen into several boards: PyPortal, Titano, and MatrixPortal M4, and the i.MX 101x boards. Those boards need a frozen version because they don't have enough RAM to support a non-frozen version for other than small programs. If we change the API now, and change the Guide code, then the examples will no longer work on those boards with 9.0.x stable builds.

So I propose:

Include the backwards compatible API (with appropriate documentation), so that old code will work. Merge and release this any time between now and just before 9.1.0 final.
Just before releasing 9.1.0 final, update frozen libraries for 9.1.0 final with the upward compatible release. 9.1.0 will then work with the secrets dict and with the new API.
After 9.1.0 final is released, we can change the Guide code to use the separate SSID and password arguments.
Sometime after that, maybe 9.2 or 10.0, remove the backward compatibility.

justmobilize · 2024-05-02T15:06:16Z

So like this:

    def connect(self, ssid, password, timeout=10):
        """Connect to an access point with given name and password."""
        # previous versions of connect were passed in a secrets dict.
        # temporarily continue supporting this
        if isinstance(ssid, dict):
            ssid = ssid["ssid"]
            password = ssid["password"]

        self.connect_AP(ssid, password, timeout_s=timeout)

dhalbert · 2024-05-02T15:11:44Z

So like this:

@justmobilize I think your version would require a second argument in the case when just a dict was passed in.

See #199 (comment). I think that could also be done with ssid, password=None instead of *args

justmobilize · 2024-05-02T15:15:09Z

Also, finished testing. The only thing I might add is updating mac address:

>>> r1.mac_address
b'|\xdf\xa1\xf8\xfa\xf8'
>>> r2.MAC_address
bytearray(b'\\\xc1V\x12\xcf\xa4')
>>> r2.MAC_address_actual
<reversed>
>>> bytes(r2.MAC_address_actual)
b'\xa4\xcf\x12V\xc1\\'

and so would do:

    @property
    def mac_address(self):
        return bytes(reversed(self.MAC_address))

justmobilize · 2024-05-02T15:16:49Z

So like this:

@justmobilize I think your version would require a second argument in the case when just a dict was passed in.

See #199 (comment). I think that could also be done with ssid, password=None instead of *args

True, I like password=None, this allows new code to specify the args and pass them in named.

anecdata · 2024-05-02T16:29:22Z

OK, showing my ignorance, but if password is a kwarg in the function def, will it work if passed as a non-keyword arg?

edit:: n/m, TIL

>>> def myfunc(a, b=None, t=10):
...     print(a, b, t)
>>> myfunc("ssid", "password", 10)
ssid password 10

dhalbert · 2024-05-02T16:32:43Z

OK, showing my ignorance, but if password is a kwarg in the function def, will it work if passed as a non-keyword arg?

Is this what you mean? (EDIT: fixed example)

>>> def connect(ssid, password=None, timeout=10):
...     print(ssid, password, timeout)
... 
>>> connect("myssid")
myssid None 10
>>> connect("myssid", "mypassword")
myssid mypassword 10
>>> connect()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: connect() missing 1 required positional argument: 'ssid'

anecdata · 2024-05-02T16:33:22Z

Yes, forgot about the whole * and kw-only thing, thanks.

dhalbert · 2024-05-02T16:35:57Z

@anecdata, I fixed my example above. timeout should not be keyword only in that signature, but yes, it's still fine.

anecdata · 2024-05-02T16:36:40Z

Shouldn't it be keyword-only?
https://docs.circuitpython.org/en/latest/shared-bindings/wifi/index.html#wifi.Radio.connect

anecdata · 2024-05-02T16:37:37Z

Oh, it may break backwards compatibility if it fully matches native.

dhalbert · 2024-05-02T16:40:11Z

Oh, it may break backwards compatibility if it fully matches native.

We can fix that when we remove the dict capability and rewrite the examples and Guide code.

anecdata · 2024-05-02T16:56:04Z

OK, this is odd, I'm almost certain it used to work:

print(f'MAC (actual)    {radio.MAC_address_actual=}')

prints:

MAC (actual)    radio.MAC_address_actual=<reversed>

edit: probably nobody noticed since it's almost always iterated and converted to hex, I'll fix that while I'm in there

…crets` dict, `ssid, password`, and `ssid`-only (open) all work as before.

adafruit_esp32spi/adafruit_esp32spi.py

Co-authored-by: Justin Myers <justmobilize@users.noreply.github.com>

anecdata · 2024-05-02T17:49:21Z

(I fixed radio.MAC_address_actual to return a bytearray instead of a reversed iterator. Shouldn't affect anything since a bytearray can be iterated just like the reversed iterator.)

anecdata · 2024-05-02T18:19:11Z

@justmobilize would you mind re-testing in case I missed something?

justmobilize · 2024-05-02T18:22:20Z

Yup. On it

justmobilize · 2024-05-02T18:40:03Z

@anecdata looks good. Added this test to what you have above:

radio.connect({"ssid": os.getenv("WIFI_SSID"), "password": os.getenv("WIFI_PASSWORD")})

anecdata · 2024-05-02T18:53:00Z

Yes, good point, I didn't update the original posted test code when we changed the direction. Also tested a local open network:
radio.connect("open")
radio.connect("open", timeout=10)

adafruit_esp32spi/adafruit_esp32spi.py

Co-authored-by: Justin Myers <justmobilize@users.noreply.github.com>

dhalbert

I added some more documentation about connect(). Thanks @anecdata and @justmobilize for all your work on this. It was tricky to decide what to do.

Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 8.1.0 from 8.0.0: > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#199 from anecdata/connect Updating https://github.com/adafruit/Adafruit_CircuitPython_AzureIoT to 2.6.0 from 2.5.18: > Merge pull request adafruit/Adafruit_CircuitPython_AzureIoT#65 from justmobilize/fix-esp32spi Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA: > Updated download stats for the libraries

Fully deprecate secrets and improve native wifi API compatibility.

8349d77

anecdata requested a review from a team April 30, 2024 19:53

dhalbert reviewed Apr 30, 2024

View reviewed changes

ad mac_address and fix connect per discussion. connect with `se…

a589e81

…crets` dict, `ssid, password`, and `ssid`-only (open) all work as before.

justmobilize reviewed May 2, 2024

View reviewed changes

adafruit_esp32spi/adafruit_esp32spi.py Outdated Show resolved Hide resolved

Update adafruit_esp32spi/adafruit_esp32spi.py

cfb61f5

Co-authored-by: Justin Myers <justmobilize@users.noreply.github.com>

justmobilize reviewed May 2, 2024

View reviewed changes

adafruit_esp32spi/adafruit_esp32spi.py Outdated Show resolved Hide resolved

Update adafruit_esp32spi/adafruit_esp32spi.py

e69c950

Co-authored-by: Justin Myers <justmobilize@users.noreply.github.com>

anecdata changed the title ~~Fully deprecate secrets and improve native wifi API compatibility.~~ Improve native wifi API compatibility May 2, 2024

dhalbert and others added 2 commits May 2, 2024 22:03

add documentation for .connect(secrets_dict).

5a64044

remove extraneous whitespace

42dc7f5

dhalbert approved these changes May 3, 2024

View reviewed changes

dhalbert merged commit 69b6e97 into adafruit:main May 3, 2024
1 check passed

dhalbert mentioned this pull request May 3, 2024

Remove allowing dict arg to connect() and other backward compatible #200

Open

4 tasks

anecdata deleted the connect branch May 3, 2024 02:39

Improve native wifi API compatibility #199

Improve native wifi API compatibility #199

Conversation

anecdata commented Apr 30, 2024 • edited Loading

anecdata commented Apr 30, 2024 • edited Loading

dhalbert left a comment

Choose a reason for hiding this comment

anecdata commented Apr 30, 2024

anecdata commented Apr 30, 2024 • edited Loading

justmobilize commented Apr 30, 2024

dhalbert commented May 1, 2024

dhalbert commented May 1, 2024

anecdata commented May 1, 2024

dhalbert commented May 1, 2024

anecdata commented May 1, 2024

dhalbert commented May 1, 2024

justmobilize commented May 1, 2024

anecdata commented May 1, 2024 • edited Loading

dhalbert commented May 1, 2024

anecdata commented May 2, 2024

justmobilize commented May 2, 2024

dhalbert commented May 2, 2024

justmobilize commented May 2, 2024

dhalbert commented May 2, 2024 • edited Loading

justmobilize commented May 2, 2024

dhalbert commented May 2, 2024

justmobilize commented May 2, 2024 • edited Loading

justmobilize commented May 2, 2024

anecdata commented May 2, 2024 • edited Loading

dhalbert commented May 2, 2024 • edited Loading

anecdata commented May 2, 2024 • edited Loading

dhalbert commented May 2, 2024

anecdata commented May 2, 2024

anecdata commented May 2, 2024

dhalbert commented May 2, 2024

anecdata commented May 2, 2024 • edited Loading

anecdata commented May 2, 2024 • edited Loading

anecdata commented May 2, 2024

justmobilize commented May 2, 2024

justmobilize commented May 2, 2024

anecdata commented May 2, 2024

dhalbert left a comment

Choose a reason for hiding this comment

anecdata commented Apr 30, 2024 •

edited

Loading

anecdata commented Apr 30, 2024 •

edited

Loading

anecdata commented Apr 30, 2024 •

edited

Loading

anecdata commented May 1, 2024 •

edited

Loading

dhalbert commented May 2, 2024 •

edited

Loading

justmobilize commented May 2, 2024 •

edited

Loading

anecdata commented May 2, 2024 •

edited

Loading

dhalbert commented May 2, 2024 •

edited

Loading

anecdata commented May 2, 2024 •

edited

Loading

anecdata commented May 2, 2024 •

edited

Loading

anecdata commented May 2, 2024 •

edited

Loading