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

Use new SocketPool for ESP32SPI and WIZNET5K #11

Merged
merged 10 commits into from
Apr 30, 2024

Conversation

justmobilize
Copy link
Collaborator

@justmobilize justmobilize commented Apr 24, 2024

@justmobilize justmobilize force-pushed the esp32spi-and-wiznet5k-socketpool branch from 0447e9f to 85713ef Compare April 24, 2024 15:41
@anecdata
Copy link
Member

Some day soon, I expect I'll be wiring up some monstrosity with native wifi and multiple ethernets and multiple ESP32SPIs ;-)

Copy link
Member

@anecdata anecdata left a comment

Choose a reason for hiding this comment

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

This is working as expected.

Test code here.

Config with a native wifi, a WIZnet, and an ESP32SPI. Each radio has a socketpool, each pool is managed by a unique ConnectionManager. HTTP sessions were observed at the router from each radio's IP address, corresponding to the serial output sequence.

P.S. SSL also works on all links in this config when using 20240425-main-PR8954-c92bb9b.uf2 (or newer) ...native ssl on native wifi and WIZnet; SSL via NINA on ESP32SPI.

@anecdata
Copy link
Member

anecdata commented Apr 26, 2024

Open question we were discussing on Discord is whether this PR is intended to handle multiple radios of the same class (native wifi, eth, or esp).

Code to loop and append to lists the multiple radios-pools-contexts-requests for each similar device works if I manually create the socketpools fed to Requests:

    radios.append(ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset))

    # -----------v
    pools.append(socketpool.SocketPool(radio[i]))

    ssl_contexts.append(adafruit_connection_manager.get_radio_ssl_context(radios[i]))
    requestss.append(adafruit_requests.Session(pools[i], ssl_contexts[i]))
    connection_managers.append(adafruit_connection_manager.get_connection_manager(pools[i]))

In that case, each radio, pool, context, requests, and connection manager is unique, and the router sees requests from each device's IP address.

If ConnectionManager creates the socketpools fed to Requests, each radio is still unique, but the pools and connection managers are the same for all devices in the same class, and only the first radio gets used:

    radios.append(ESP_SPIcontrol(spi, esp32_cs, esp32_ready, esp32_reset))

    # -----------v
    pools.append(adafruit_connection_manager.get_radio_socketpool(radios[i]))

    ssl_contexts.append(adafruit_connection_manager.get_radio_ssl_context(radios[i]))
    requestss.append(adafruit_requests.Session(pools[i], ssl_contexts[i]))
    connection_managers.append(adafruit_connection_manager.get_connection_manager(pools[i]))

Full code here.

@justmobilize
Copy link
Collaborator Author

@anecdata if you are up for a retest, I just pushed a fix for getting a new pool for a different radio. The wifi.radio was the problem as it can't be hashed to use for a key. So I wrote a quick wrapper to create a key and now it should work.

@dhalbert
Copy link
Contributor

@justmobilize On which chips does hash() not work?

Adafruit CircuitPython 9.0.0-alpha.2 on 2023-10-27; Adafruit Feather ESP32-S2 Reverse TFT with ESP32S2
>>> import wifi
>>> hash(wifi.radio)
1073582920

@justmobilize
Copy link
Collaborator Author

@dhalbert interesting... If you do:

x = {wifi.radio: 1}

It raises:

TypeError: unsupported type for __hash__: 'Radio'

So maybe it's an issue with dict?

I can test using a manual hash key, since that would be cleaner.

@dhalbert
Copy link
Contributor

>>> { wifi.radio: 1 }
{<Radio>: 1}

Which board are you testing on, with which version of CPy?

@justmobilize
Copy link
Collaborator Author

@dhalbert, scrolled through my tests (because I remembered this from when I built CM) - it's a CP8 issue:
image

adafruit_connection_manager.py Show resolved Hide resolved
@anecdata
Copy link
Member

Those hashing changes did the trick. adafruit_connection_manager.get_radio_socketpool(radio) now returns a unique socketpool for devices of the same class, and also results in unique ConnectionManagers. HTTP[S] sessions verified coming from both ESP32SPI peripherals.

Thanks @justmobilize!

@dhalbert dhalbert dismissed their stale review April 26, 2024 15:37

addressed

@justmobilize justmobilize marked this pull request as ready for review April 28, 2024 04:38
@justmobilize
Copy link
Collaborator Author

@anecdata when you have time (you've tested this so much for me already, thank you!) will you pull down the latest of all 3 branches. Had some conflicts and tweaks, and although I don't expect anything to not work, it's worth a quick check.

Copy link
Member

@anecdata anecdata left a comment

Choose a reason for hiding this comment

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

Confirmed everything still works as before. Tested HTTP and HTTPS URLs.

Libraries::

Adafruit_CircuitPython_ConnectionManager Use new SocketPool for ESP32SPI and WIZNET5K #11
Merge branch 'main' into esp32spi-and-wiznet5k-socketpool cd9f892

Adafruit_CircuitPython_Wiznet5k Convert to SocketPool #159
Merge branch 'main' into convert-to-socketpool e1c0ac7

Adafruit_CircuitPython_ESP32SPI Convert to SocketPool #198
Use standard super() for __new__ 06281a5

Hardware:

Tested on a config with native wifi, ethernet, and esp32spi (1 each), on a config with 3 ethernets, and on a config with 3 esp32spis. Each device gets a unique SocketPool and a unique ConnectionManager, repeated Requests are successful, and sessions / IP addresses of each device show up appropriately at the router..

@justmobilize
Copy link
Collaborator Author

@dhalbert limiting notifications, but this and the 2 PRs in the description are ready for review

Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

I asked some questions.

adafruit_connection_manager.py Outdated Show resolved Hide resolved
adafruit_connection_manager.py Outdated Show resolved Hide resolved
@justmobilize
Copy link
Collaborator Author

Copy link
Contributor

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Ok, thanks for your patience, and thanks @anecdata for testing.

@dhalbert dhalbert merged commit 2c79732 into adafruit:main Apr 30, 2024
1 check passed
@justmobilize justmobilize deleted the esp32spi-and-wiznet5k-socketpool branch April 30, 2024 16:16
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request May 1, 2024
Updating https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI to 8.0.0 from 7.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_ESP32SPI#198 from justmobilize/convert-to-socketpool

Updating https://github.com/adafruit/Adafruit_CircuitPython_Wiznet5k to 7.0.0 from 6.0.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_Wiznet5k#159 from justmobilize/convert-to-socketpool

Updating https://github.com/adafruit/Adafruit_CircuitPython_ConnectionManager to 2.0.0 from 1.2.1:
  > Merge pull request adafruit/Adafruit_CircuitPython_ConnectionManager#11 from justmobilize/esp32spi-and-wiznet5k-socketpool

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants