Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
anecdata opened this issue Mar 28, 2024 · 25 comments
Open

Comments

@anecdata
Copy link
Member

anecdata commented Mar 28, 2024

Thought spurred by a review discussion in adafruit_httpserver:
adafruit/Adafruit_CircuitPython_HTTPServer#91 (comment)

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
Copy link
Collaborator

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
Copy link
Member Author

anecdata commented Mar 28, 2024

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
Copy link

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

@anecdata
Copy link
Member Author

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

@dhalbert
Copy link
Collaborator

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
Copy link

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
Copy link
Collaborator

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

@justmobilize
Copy link

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

@tannewt tannewt added this to the Long term milestone Mar 29, 2024
@tannewt
Copy link
Member

tannewt commented Mar 29, 2024

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
Copy link
Member

tannewt commented Mar 29, 2024

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
Copy link
Member Author

anecdata commented Mar 29, 2024

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
Copy link

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
Copy link
Member Author

anecdata commented Mar 29, 2024

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
Copy link

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

@tannewt
Copy link
Member

tannewt commented Apr 1, 2024

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
Copy link

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
Copy link
Member Author

anecdata commented Apr 1, 2024

...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
Copy link
Collaborator

dhalbert commented Apr 1, 2024

@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.

  • Using CIRCUITPY_WIFI_SSID and CIRCUITPY_WIFI_PASSWORD causes auto-connect.
  • Using CIRCUITPY_WEB_API_PASSWORD enables web workflow if auto-connect has happened.
  • The CIRCUITPY_ prefix on the settings.toml values is meant to signify that CircuitPython does something itself with these values.
  • There is currently no way to manually enable web workflow after a manual connect. There could be.
  • Auto-connect and web workflow will both restart after waking up from deep sleep. This can slow down sleep cycle times.
  • Auto-connect could cache the previous wifi channel or maybe do other things to speed up discovering whether auto-connect is possible and speed up the connection initialization.
  • Enabling auto-connect can cause slow program startup if the auto-connect network is not present: you have to wait for a timeout. @jepler gave the example of a MEMENTO camera with auto-connect enabled being taken out of the house.
  • Auto-connect makes the wifi-startup boilerplate simpler since you don't need .connect().
  • WIFI_SSID and WIFI_PASSWORD are good choices for the non-auto-connect case.
  • Documentation:
    • Example settings.toml files could include comments that explain about auto-connect, etc.
    • Auto-connect should be better documented in various Learn Guides.

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
Copy link

@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
Copy link
Collaborator

dhalbert commented Apr 1, 2024

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
Copy link

Oh I would love to put that all in the ConnectionManager

@anecdata
Copy link
Member Author

anecdata commented Apr 1, 2024

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
Copy link

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
Copy link
Member Author

anecdata commented Apr 1, 2024

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
Copy link
Member

tannewt commented Apr 2, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants