adafruit / circuitpython

CircuitPython - a Python implementation for teaching coding with microcontrollers
https://circuitpython.org
Other
4.12k stars 1.22k forks source link

Add separate workflow credentials for native wifi without auto-connect #9112

Open anecdata opened 8 months ago

anecdata commented 8 months ago

Thought spurred by a review discussion in adafruit_httpserver: https://github.com/adafruit/Adafruit_CircuitPython_HTTPServer/pull/91#discussion_r1542612641

Example native wifi code commonly uses os.getenv("CIRCUITPY_WIFI_SSID") etc. with wifi connect logic, though that implies wifi has already auto-connected and is a bit redundant. Example. I think we want people to know how to connect to wifi, and how to detect a connection, but virtually all examples assume the connection is made once (really, it's made automatically at start up), and connection robustness is discussed more as a support issue on Discord and elsewhere.

Web workflow is enabled by CIRCUITPY_WEB_API_PASSWORD, so that's less of an issue (except in cases where a user HTTP server and the web workflow server need to have separate ports).

But using the auto-connect credentials, particularly in cases where auto-connect may not be wanted (for example: running a wifi access point without need for station), or having the distinction of auto-connect vs. manual [re-]connection code in an example.

I'm proposing a new separate set of credentials in the workflow document (and examples where it makes sense), some people (including me) I think have been using:

WIFI_SSID=
WIFI_PASSWORD=

Standardizing this would help with some example cases I think, and help highlight and distinguish auto-connect vs. manual connect.

dhalbert commented 8 months ago

It may be too late, but a names like CIRCUITPY_WIFI_SSID_AUTOCONNECT or CIRCUITPY_AUTOCONNECT_WIFI_SSID (and same for PASSWORD) would make this clearer.

anecdata commented 8 months ago

We could make the new manual name the longer one since auto-connect is the de facto (and still have the CIRCUITPY_ prefix. e.g., CIRCUITPY_WIFI_SSID_MANUAL or CIRCUITPY_MANUAL_WIFI_SSID.

justmobilize commented 8 months ago

I also had this idea, but I thought we could just add CIRCUITPY_AUTO_CONNECT and users could set it to False...

anecdata commented 8 months ago

I like that, seems cleaner than two sets of credentials.

dhalbert commented 8 months ago

I do like the CIRCUITPY_AUTO_CONNECT flag idea, and it defaults to true. We don't have support for TOML booleans (true and false) right now, but it could be added. We've kept the implementation very minimal so it fits on some small boards. Or we could use 0 and 1. Slightly related: #9113.

justmobilize commented 8 months ago

I'm also good with 1/0 or even "True"/"False"... Does the c code for auto connect use the same methods as the getenv does?

dhalbert commented 8 months ago

It uses some internal methods like common_hal_os_getenv_int() from shared-module/os/getenv.c.

justmobilize commented 8 months ago

Gotcha. So it would create a little bloat if added...

tannewt commented 8 months ago

My preference is to change the examples. My intention with the CIRCUITPY_ prefix was to mark values that are used by the CP core.

tannewt commented 8 months ago

I think we need to double check that web workflow will start when wifi is manually connected and the api password is set. That way user code can manage roaming multiple networks and still have web workflow work.

anecdata commented 8 months ago

My preference is to change the examples.

Change the examples to:

  1. assume auto-connect, and remove wifi.radio.connect logic?

  2. don't assume auto-connect, and keepwifi.radio.connect logic:

2a. continue to use the settings.toml auto-connect credentials? (potentially confusing or undesired per OP, and I think the comment above implies not doing this)

2b. use alternate credentials as discussed above? (a standard method will make support easier)

_2c. implement CIRCUITPY_AUTO_CONNECT as discussed above?_

  1. ?

I couldn't get web workflow to work with manual wifi connect + only:

CIRCUITPY_WEB_API_PASSWORD="passw0rd"

Autoconnect and Web workflow works for me if all three are defined:

CIRCUITPY_WIFI_SSID={redacted}
CIRCUITPY_WIFI_PASSWORD={redacted}
CIRCUITPY_WEB_API_PASSWORD="passw0rd"
justmobilize commented 8 months ago

It also gets tricky when using projects across boards. ESP32SPI won't auto connect. I'm pretty sure I've had code check for connection and return false because negotiation takes too long...

anecdata commented 8 months ago

I think @tannewt is suggesting (2b.) above. We could even devise a simple standard connect code block for examples with comments about auto-connect vs. manual connect, and I think we could even make the code for native wifi and esp32spi more similar (would require supporting esp32spi connect with 2 params rather than / in addition to a dict¹, and adding a connected alias²). Snippets borrowed from connection manager examples. Something like:

# "If the keys CIRCUITPY_WIFI_SSID and CIRCUITPY_WIFI_PASSWORD are set in settings.toml, 
# CircuitPython will automatically connect to the given Wi-Fi network on boot and upon reload."
# https://docs.circuitpython.org/en/latest/docs/workflows.html#web
# If auto-connect is not enabled with those keys, then manually connect using alternate keys in settings.toml:
wifi_ssid = os.getenv("WIFI_SSID")
wifi_password = os.getenv("WIFI_PASSWORD")

# native wifi:
radio = wifi.radio
# esp32spi (does not support auto-connect):
# radio = adafruit_esp32spi.ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset)
while not radio.connected:
    radio.connect(wifi_ssid, wifi_password)

¹Current esp32spi connect call: radio.connect({"ssid": os.getenv("WIFI_SSID"), "password": os.getenv("WIFI_PASSWORD")}) (there's also a radio.connect_AP(said, password, timeout_s=10)) ²Current esp32spi check for connection: radio.is_connected

justmobilize commented 8 months ago

Yeah, I would really like to make those the same...

tannewt commented 7 months ago

I think @tannewt is suggesting (2b.) above.

Yup, that's what I meant. Use settings.toml keys without CIRCUITPY_ prefix for user config values. CIRCUITPY_ ones are used by CP itself.

justmobilize commented 7 months ago

I think @tannewt is suggesting (2b.) above.

Yup, that's what I meant. Use settings.toml keys without CIRCUITPY_ prefix for user config values. CIRCUITPY_ ones are used by CP itself.

So we should update examples to not use CIRCUITPY_ and save those for examples when using webworkflow?

anecdata commented 7 months ago

...and many examples can probably assume auto-connect and not need any credentials (but perhaps some comments about connection).

Maybe we wait for Dan to chime in, I think he was trying to summarize the in-the-weeds discussion from the call today.

dhalbert commented 7 months ago

@justmobilize The meeting video is not up yet. I was multitasking during the meeting and don't remember if you attended, but we discussed this in In the Weeds today.

We didn't reach any conclusions that there is one right way to write the examples. It depends on the use case and the user's requirements. That merits more discussion.

justmobilize commented 7 months ago

@dhalbert thanks. Sadly I missed it today...

So we would probably want something like this in the examples:

if not radio.connected:
    # We got here because CIRCUITPY_WIFI_SSID/CIRCUITPY_WIFI_PASSWORD are either incorrect or not set
    # Try grabbing WIFI_SSID/WIFI_PASSWORD and connecting
    ssid = os.getenv("WIFI_SSID")
    password = os.getenv("WIFI_PASSWORD")
    while not radio.connected:
        try:
            radio.connect(ssid, password)
        except ConnectionError as e:
            print(f"Connection Error: {e})

And that way if they have set CIRCUITPY_ and it connected it's good. If not try the other ones?

dhalbert commented 7 months ago

Video is up. Discussion starts here: https://youtu.be/fbg2qtGjxC8?si=hlf-NvkHwluSLIRg&t=1987

So we would probably want something like this in the examples:

That is the most thorough boilerplate. The retry while loop could be omitted, I think, and then also the try-except. I notice @anecdata also includes that in their example.

I think one question is whether to bother to include this for all examples or just depend on auto_connect.

Something like this code could go in ConnectionManager: connect_if_needed() or similar. Maybe that obfuscates what is going on, or maybe not. It needs explanation, for sure.

justmobilize commented 7 months ago

Oh I would love to put that all in the ConnectionManager

anecdata commented 7 months ago

In any robust application, there would be a lot of exception-handling, etc. But for simple examples, there are two cases:

Auto-Connect enabled: No wifi.radio.connect() logic is needed, but some documentation about settings.toml and auto-connect would set the stage.

Auto-Connect not enabled: We could make it super-simple and simply call wifi.radio.connect(), no ifs or whiles even needed. .connect() is virtually a NOP if already connected, there is very little overhead to the internal check (~1ms or less). Though it is useful for users to soon learn how to detect if the connection is lost and can't be made, and how to recover.

I don't have strong opinions about these, I'm happy with whatever we end up with... hopefully some consistency and some comments / docs help.

We seem to be converging on the os.getenv("WIFI_SSID") form of credentials.

Any libraries that explicitly handle wifi connections would have to handle either case... are there many outside of esp32spi-land?

justmobilize commented 7 months ago

My biggest worry is discord help. Almost all user code I see starts from an example. There ideally should be something clear that says you aren't connected.

If we build that into a global helper, then it's super simple. It could check for env vars check connection state and even try to connect (knowing for each radio how to do it)

anecdata commented 7 months ago

That's true. And especially when reloading often in a flurry of development, initial connect exceptions happen more (for me, at least - maybe the AP hasn't fully dropped the old one yet). So acknowledging that connections are not guaranteed would be good.

wifi and sockets are distinct, though I'm hard-pressed to think of very many use cases¹ where wifi is used without sockets - wondering whether wifi connection helper belongs in ConnectionManager, or something separate even if just an example code block.

¹I do have APs that create informational SSIDs, but accept no connections. I suppose it's conceivable for an AP to accept connections but not need to use sockets?

tannewt commented 7 months ago

We could make it super-simple and simply call wifi.radio.connect(), no ifs or whiles even needed. .connect() is virtually a NOP if already connected, there is very little overhead to the internal check (~1ms or less). Though it is useful for users to soon learn how to detect if the connection is lost and can't be made, and how to recover.

This is my preference. I don't want to require connection manager for simple examples. Instead, maybe the examples need to set the timeout explicitly so that users can raise it if they have errors. Note, connect() will switch networks if it is connected to a different one.