Create ConnectionManager #1

justmobilize · 2023-12-22T17:27:50Z

ConnectionManager

The goal of Adafruit_CircuitPython_ConnectionManager is to make using sockets easier and the pool (socketpool, adafruit_esp32spi_socket, etc) managed in one place.

A global ConnectionManager is created and so it doesn't matter in which file or where you create Sessions since they all use the same one.

To test, you will want the following branches:

Example code for using `adafruit_requests` on either a board with wifi or an airlift:

import os

import adafruit_requests

import adafruit_connection_manager

try:
    import wifi

    radio = wifi.radio
    onboard_wifi = True

except ImportError:
    import board
    import busio
    from adafruit_esp32spi import adafruit_esp32spi
    from digitalio import DigitalInOut

    spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
    esp32_cs = DigitalInOut(board.D13)
    esp32_ready = DigitalInOut(board.D11)
    esp32_reset = DigitalInOut(board.D12)
    radio = adafruit_esp32spi.ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset)
    onboard_wifi = False

wifi_ssid = os.getenv("CIRCUITPY_WIFI_SSID")
wifi_password = os.getenv("CIRCUITPY_WIFI_PASSWORD")

print("Connecting to WiFi")
if onboard_wifi:
    while not radio.connected:
        radio.connect(wifi_ssid, wifi_password)
else:
    while not radio.is_connected:
        try:
            radio.connect_AP(wifi_ssid, wifi_password)
        except OSError as os_exc:
            print(f"could not connect to AP, retrying: {os_exc}")
            continue

pool = adafruit_connection_manager.get_radio_socketpool(radio)
ssl_context = adafruit_connection_manager.get_radio_ssl_context(radio)
requests = adafruit_requests.Session(pool, ssl_context)

print("Making request")
url = "https://www.adafruit.com/api/quotes.php"
response = requests.get(url)
print(response.json())

Minimum code on a board with WiFi:

import os

import adafruit_requests
import wifi

import adafruit_connection_manager

wifi_ssid = os.getenv("CIRCUITPY_WIFI_SSID")
wifi_password = os.getenv("CIRCUITPY_WIFI_PASSWORD")

radio = wifi.radio
while not radio.connected:
    radio.connect(wifi_ssid, wifi_password)

pool = adafruit_connection_manager.get_radio_socketpool(radio)
ssl_context = adafruit_connection_manager.get_radio_ssl_context(radio)

requests = adafruit_requests.Session(pool, ssl_context)
print(requests.get("http://www.adafruit.com/api/quotes.php").json())

Not knowing how people like to overload methods, I would love to make adafruit_requests.Session like this:

class Session:
    def __init__(self, radio=None, socket_pool=None, ssl_context=None, session_id=None):

And make sure either radio or socket_pool are passed in. if radio, call get_radio_socketpool and get_radio_ssl_context internally.

Thus have even less imports.

MQTT example:

import os
import time

import adafruit_minimqtt.adafruit_minimqtt as MQTT

import adafruit_connection_manager

try:
    import wifi

    radio = wifi.radio
    onboard_wifi = True

except ImportError:
    import board
    import busio
    from adafruit_esp32spi import adafruit_esp32spi
    from digitalio import DigitalInOut

    spi = busio.SPI(board.SCK, board.MOSI, board.MISO)
    esp32_cs = DigitalInOut(board.D13)
    esp32_ready = DigitalInOut(board.D11)
    esp32_reset = DigitalInOut(board.D12)
    radio = adafruit_esp32spi.ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset)
    onboard_wifi = False

wifi_ssid = os.getenv("CIRCUITPY_WIFI_SSID")
wifi_password = os.getenv("CIRCUITPY_WIFI_PASSWORD")
aio_username = os.getenv("AIO_USERNAME")
aio_key = os.getenv("AIO_KEY")

print("Connecting to WiFi")
if onboard_wifi:
    while not radio.connected:
        radio.connect(wifi_ssid, wifi_password)
else:
    while not radio.is_connected:
        try:
            radio.connect_AP(wifi_ssid, wifi_password)
        except OSError as os_exc:
            print(f"could not connect to AP, retrying: {os_exc}")
            continue

pool = adafruit_connection_manager.get_radio_socketpool(radio)

mqtt_client = MQTT.MQTT(
    broker="io.adafruit.com",
    username=aio_username,
    password=aio_key,
    socket_pool=pool,
)

mqtt_client.connect()
for i in range(10):
    print(i)
    mqtt_client.publish(f"{aio_username}/feeds/button", i)
    time.sleep(1)
mqtt_client.disconnect()

Example of using unique sessions (not all boards support this. Tested on `ESP32S3 TFT`):

import os

import adafruit_requests
import wifi

import adafruit_connection_manager

wifi_ssid = os.getenv("CIRCUITPY_WIFI_SSID")
wifi_password = os.getenv("CIRCUITPY_WIFI_PASSWORD")

radio = wifi.radio
while not radio.connected:
    radio.connect(wifi_ssid, wifi_password)

pool = adafruit_connection_manager.get_radio_socketpool(radio)
ssl_context = adafruit_connection_manager.get_radio_ssl_context(radio)

sessions = {}
session_id = 0
while True:
  session_id += 1
  try:
    requests = adafruit_requests.Session(pool, ssl_context, session_id=str(session_id))
    response = requests.get("http://www.adafruit.com/api/quotes.php")
    sessions[session_id] = {
      "requests": requests,
      "response": response,
      "result": "",
      "more": True,
    }
    print(f"Started session {session_id}")
  except:
    break

chunk_size = 5
b = bytearray(chunk_size)
while True:
  more = False
  for session_id, data in sessions.items():
    if data["more"]:
      size = data["response"]._readinto(b)
      if size:
        more = True
        data["result"] += str(b[:size], "utf-8")
      else:
        data["more"] = False
        data["response"].close()
    print(f"{session_id}: {data["result"]}")
  if not more:
    break

Interesting Note: on the ESP32S3 TFT, running 8.2.9 I got 3 sessions before it raised an error. On 9.0.0-beta.0 I got 4
Testing on 9.0.0-beta.1 on a UM FeatherS3 I got 7 sessions.

.gitignore

.pre-commit-config.yaml

tox.ini

…ger.py

justmobilize · 2023-12-27T16:12:06Z

adafruit_connection_manager.py

+# typing
+
+
+if not sys.implementation.name == "circuitpython":


This block pulled from Adafruit_CircuitPython_Requests

justmobilize · 2023-12-27T16:12:46Z

adafruit_connection_manager.py

+class SocketConnectMemoryError(OSError):
+    """ConnectionManager Exception class for an MemoryError when connecting a socket."""
+
+
+class SocketConnectOSError(OSError):
+    """ConnectionManager Exception class for an OSError when connecting a socket."""
+
+
+class SocketGetOSError(OSError):
+    """ConnectionManager Exception class for an OSError when getting a socket."""
+
+
+class SocketGetRuntimeError(RuntimeError):
+    """ConnectionManager Exception class for an RuntimeError when getting a socket."""


These added so Adafruit_CircuitPython_MiniMQTT can handle retries internally.

These seem weird to me. Shouldn't the outer code know the context already?

This is from MiniMQTT._get_connect_socket where different exceptions are flagged as a TemporaryError so it knows if it's retriable or not

These are removed

justmobilize · 2023-12-27T16:13:19Z

adafruit_connection_manager.py

+# classes
+
+
+class _FakeSSLSocket:


Also pulled from Adafruit_CircuitPython_Requests

justmobilize · 2023-12-27T16:14:02Z

adafruit_connection_manager.py

+        return _FakeSSLSocket(socket, self._iface.TLS_MODE)
+
+
+def create_fake_ssl_context(


Modeled after Adafruit_CircuitPython_Requests.set_socket(), this makes it so Sessions are easier to use with esp32spi

justmobilize · 2023-12-27T16:15:00Z

adafruit_connection_manager.py

+                socket = self._socket_pool.socket(addr_info[0], addr_info[1])
+            except OSError as exc:
+                logger.debug(f"  OSError getting socket: {exc}")
+                last_exc_new_type = SocketGetOSError


These add support so MiniMQTT can handle retries

Why does MiniMQTT need additional retries?

Same comment as above. MiniMQTT has it's own built in retrying and some exceptions are retriable and others are fatal

I think this code should be able to implement the same policy. I don't know why they error behavior would be specific to minimqtt.

This logic is now removed

justmobilize · 2023-12-29T15:39:58Z

For information:

Sizes before:

14976 adafruit_minimqtt.mpy
 8578 adafruit_requests.mpy
-----
23554

Sizes after:

13828 adafruit_minimqtt.mpy
 6117 adafruit_requests.mpy
-----
19945

Sizes total:

13828 adafruit_minimqtt.mpy
 6117 adafruit_requests.mpy
 4825 adafruit_connection_manager.mpy
-----
24770

Increase: 1216 bytes

tannewt

Thanks for factoring this out! Just a couple questions.

tannewt · 2024-02-06T21:11:00Z

adafruit_connection_manager.py

+class SocketConnectMemoryError(OSError):
+    """ConnectionManager Exception class for an MemoryError when connecting a socket."""
+
+
+class SocketConnectOSError(OSError):
+    """ConnectionManager Exception class for an OSError when connecting a socket."""
+
+
+class SocketGetOSError(OSError):
+    """ConnectionManager Exception class for an OSError when getting a socket."""
+
+
+class SocketGetRuntimeError(RuntimeError):
+    """ConnectionManager Exception class for an RuntimeError when getting a socket."""


These seem weird to me. Shouldn't the outer code know the context already?

tannewt · 2024-02-06T21:14:28Z

adafruit_connection_manager.py

+
+
+def get_connection_members(radio):
+    """Helper to get needed connection mamers for common boards"""


I'm not really a fan of this because members is vague. Could this be the ConnectionManager constructor instead and then have requests.Session et al recognize when they are given a connection manager?

Another clearer way would be to have two functions, get_pool() and get_ssl_context().

Can make this get_connection_pool_and_ssl_context. I had this idea of just passing the radio to things like requests and having it get the pool and ssl_context. The thought was around simplifying imports.
I can also take this out of here for now, and add this as a second add.

This also allows you to:

requests = adafruit_requests.Session(*connection_members)

or even:

requests = adafruit_requests.Session(*get_connection_members(radio))

I'm not a huge fan of what it is hiding. I'd prefer it in a follow up PR.

I did break these into get_radio_socketpool and get_radio_ssl_contexts. Still happy to remove if we don't want theses yet

adafruit_connection_manager.py

tannewt · 2024-02-06T21:17:44Z

adafruit_connection_manager.py

+                socket = self._socket_pool.socket(addr_info[0], addr_info[1])
+            except OSError as exc:
+                logger.debug(f"  OSError getting socket: {exc}")
+                last_exc_new_type = SocketGetOSError


Why does MiniMQTT need additional retries?

Co-authored-by: Scott Shawcroft <scott@tannewt.org>

justmobilize · 2024-02-07T18:59:22Z

adafruit_connection_manager.py

+        while retry_count < max_retries and socket is None:
+            if retry_count > 0:
+                logger.debug(f"  retry #{retry_count}")
+                if any(self._socket_free.items()):
+                    self._free_sockets()
+                else:
+                    raise RuntimeError("Sending request failed") from last_exc
+            retry_count += 1


@tannewt I have a question on how you would like to fix this. In theory we try 5 times and if we fail we raise with Repeated socket failures, but that will never happen.

If it's the first request:
Try one: error
Try two: any(self._socket_free.items()) returns false, error right away

If it's not the first request:
Try one: error
Try two: any(self._socket_free.items()) returns true, clears _socket_free, error
Try three: any(self._socket_free.items()) returns false, error right away

The only case it would try 5 times is if you have an open and active socket, which seems an odd case to keep trying. Thoughts? Leave it as is for now?

Am writing tests and this made sense to change to:

if any(socket for socket, free in self._socket_free.items() if free is True)

to avoid extra loops but then couldn't get that last error at all

Sorry, my brain doesn't have enough context to be concrete. It seems like the retrying here should be robust enough that minimqtt doesn't need to do any retrying itself.

Okay, so 2 separate issues.

This code was pulled from requests and has built in retry, that's flawed

minimqtt also built in a retry mechanism that is similar but different on what they feel is retry-able

It might be easiest to look at the minimqtt PR listed above to see what I mean.

I'm happy to work towards finding something that works for all libraries, but don't know how to get consensus

justmobilize · 2024-02-10T19:59:55Z

@tannewt and @brentru I opened this PR so I can simplify the retry logic here.

adafruit_connection_manager.py

justmobilize · 2024-02-14T17:17:42Z

@tannewt simplified the retry logic and asked a question. After any back-and-forth I can start adding tests

tannewt

Retry logic looks good to me too. Let me know when you are ready for a final review.

justmobilize · 2024-02-14T21:38:03Z

@tannewt final quick questions.

On another PR you asked to have the for Adafruit removed since I'm not paid for it. But cookie-cutter put it in this. Should I remove it?
For logging, should I follow what's in MiniMQTT or what I have here? My thought was that you could:
- add adafruit_logging.mpy
- set adafruit_connection_manager.logger.setLevel(0)
- and logging would show. making it easy for troubleshooting
Can you look at examples/connectionmanager_ssltest.py? I grabbed data from here and not sure what I should put up top in the license info

tannewt · 2024-02-15T18:24:09Z

@tannewt final quick questions.

On another PR you asked to have the for Adafruit removed since I'm not paid for it. But cookie-cutter put it in this. Should I remove it?

I would. I just think that's the default.

For logging, should I follow what's in MiniMQTT or what I have here? My thought was that you could:
* add `adafruit_logging.mpy`
* set `adafruit_connection_manager.logger.setLevel(0)`
* and logging would show. making it easy for troubleshooting

I wouldn't. logging takes mpy space and code space if frozen in. It's pretty easy to add logging when it is needed.

Can you look at examples/connectionmanager_ssltest.py? I grabbed data from here and not sure what I should put up top in the license info

I'd just add a comment about the dict with a pointer to the source. It is Apache so it should be ok to include.

justmobilize · 2024-02-15T22:19:22Z

@tannewt final quick questions.

On another PR you asked to have the for Adafruit removed since I'm not paid for it. But cookie-cutter put it in this. Should I remove it?

I would. I just think that's the default.
For logging, should I follow what's in MiniMQTT or what I have here? My thought was that you could:
* add `adafruit_logging.mpy`
* set `adafruit_connection_manager.logger.setLevel(0)`
* and logging would show. making it easy for troubleshooting
I wouldn't. logging takes mpy space and code space if frozen in. It's pretty easy to add logging when it is needed.

Can you look at examples/connectionmanager_ssltest.py? I grabbed data from here and not sure what I should put up top in the license info

I'd just add a comment about the dict with a pointer to the source. It is Apache so it should be ok to include.

@tannewt - so since this will be a core library (and frozen for some boards) I should take all the logging out?

tannewt · 2024-02-16T17:46:56Z

@tannewt - so since this will be a core library (and frozen for some boards) I should take all the logging out?

Ya, I think so. It'll be frozen whenever requests is now.

justmobilize · 2024-02-17T01:01:41Z

@tannewt this is ready for re-review!!!

justmobilize · 2024-02-17T20:49:45Z

For information:

Sizes before (8.x/9x):

14,976 / 13,438 adafruit_minimqtt.mpy
 8,578 /  8,127 adafruit_requests.mpy
-----
23,554 / 21,565

Sizes after:

14,444 / 12,192 adafruit_minimqtt.mpy
 6,726 /  6,189 adafruit_requests.mpy
 3,183 /  2,875 adafruit_connection_manager.mpy
-----
24,353 / 21,256

adafruit_requests + adafruit_connection_manager increase for 9x: 1,245 bytes
adafruit_minimqtt + adafruit_requests + adafruit_connection_manager decrease for 9x: 301 bytes

tannewt · 2024-02-20T23:20:32Z

adafruit_requests + adafruit_connection_manager increase for 9x: 1,245 bytes

We'll want to make sure we can build boards with requests frozen in before updating requests. This PR is ok though.

tannewt

Looks good! Thank you!

Create ConnectionManager

cd719ea

justmobilize commented Dec 22, 2023

View reviewed changes

.gitignore Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

tox.ini Show resolved Hide resolved

tox.ini Show resolved Hide resolved

tox.ini Show resolved Hide resolved

tox.ini Show resolved Hide resolved

This was referenced Dec 22, 2023

Switch to using ConnectionManager adafruit/Adafruit_CircuitPython_MiniMQTT#197

Merged

Switch to using ConnectionManager adafruit/Adafruit_CircuitPython_Requests#147

Closed

justmobilize added 2 commits December 22, 2023 10:36

Switch from adafruit_connectionmanager.py -> adafruit_connection_mana…

9a82153

…ger.py

Add logging and better docstrings

da22cc2

justmobilize commented Dec 27, 2023

View reviewed changes

justmobilize mentioned this pull request Jan 23, 2024

Use ConnectionManager adafruit/Adafruit_CircuitPython_Requests#151

Merged

justmobilize added 2 commits February 3, 2024 14:13

Add get_connection_members and session_id

6a80954

Make sure session_id is a string

e61e880

tannewt requested changes Feb 6, 2024

View reviewed changes

justmobilize and others added 2 commits February 6, 2024 13:28

Update adafruit_connection_manager.py

a47e0ea

Co-authored-by: Scott Shawcroft <scott@tannewt.org>

Fix typo

8b178c8

justmobilize commented Feb 7, 2024

View reviewed changes

Remove unneeded typing

9b068a8

Simplify retry logic

796b9b0

justmobilize commented Feb 14, 2024

View reviewed changes

adafruit_connection_manager.py Show resolved Hide resolved

justmobilize requested a review from tannewt February 14, 2024 17:16

tannewt reviewed Feb 14, 2024

View reviewed changes

Add tests

392363d

justmobilize added 4 commits February 15, 2024 13:00

Add more sst test hosts

91e0cf7

isort updates

b407c02

Fix docs

4bb1be3

Update example for Pico W error codes and timeout

f278eab

justmobilize added 3 commits February 16, 2024 10:13

Remove logging

8cc513b

Raise proper exception if TLS isn't supported

9053549

Update connectionmanager_ssltest

398d1a1

justmobilize added 2 commits February 17, 2024 10:53

Get test coverage to 100%

39b39df

Add more info into docs

449194c

justmobilize requested a review from tannewt February 17, 2024 20:49

justmobilize added 5 commits February 17, 2024 13:35

Simplify helpers

21549cd

Fix test descriptions

e04e887

Fix test descriptions

e198210

Doc updates

e7cfca3

Standardise license text

9c6adc7

tannewt approved these changes Feb 20, 2024

View reviewed changes

tannewt merged commit fe040d8 into main Feb 20, 2024

justmobilize deleted the connection-manager branch February 21, 2024 00:30

RetiredWizard mentioned this pull request Apr 27, 2024

Close all and counts #13

Merged

		return _FakeSSLSocket(socket, self._iface.TLS_MODE)


		def create_fake_ssl_context(



		def get_connection_members(radio):
		"""Helper to get needed connection mamers for common boards"""

Create ConnectionManager #1

Create ConnectionManager #1

Conversation

justmobilize commented Dec 22, 2023 • edited Loading

ConnectionManager

Example code for using adafruit_requests on either a board with wifi or an airlift:

Minimum code on a board with WiFi:

MQTT example:

Example of using unique sessions (not all boards support this. Tested on ESP32S3 TFT):

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justmobilize commented Dec 29, 2023 • edited Loading

tannewt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

justmobilize commented Feb 10, 2024

justmobilize commented Feb 14, 2024

tannewt left a comment

Choose a reason for hiding this comment

justmobilize commented Feb 14, 2024

tannewt commented Feb 15, 2024 • edited Loading

justmobilize commented Feb 15, 2024

tannewt commented Feb 16, 2024

justmobilize commented Feb 17, 2024

justmobilize commented Feb 17, 2024 • edited Loading

tannewt commented Feb 20, 2024

tannewt left a comment

Choose a reason for hiding this comment

justmobilize commented Dec 22, 2023 •

edited

Loading

Example code for using `adafruit_requests` on either a board with wifi or an airlift:

Example of using unique sessions (not all boards support this. Tested on `ESP32S3 TFT`):

justmobilize commented Dec 29, 2023 •

edited

Loading

tannewt commented Feb 15, 2024 •

edited

Loading

justmobilize commented Feb 17, 2024 •

edited

Loading