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

Switch to HTTP1.1 so that we can reuse sockets. #18

Open
wants to merge 6 commits into
base: master
from

Conversation

@tannewt
Copy link

tannewt commented Feb 7, 2020

Note that this changes the set_socket API so that interface is encompassed within socket.

@tannewt tannewt requested a review from brentru Feb 7, 2020
"""Helper to set the global socket and optionally set the global network interface.
:param sock: socket object.
:param iface: internet interface object
def set_socket(socket):

This comment has been minimized.

Copy link
@brentru

brentru Feb 11, 2020

Member

Could you edit the simpletest examples to reflect the removal of the iface kwarg?

https://github.com/adafruit/Adafruit_CircuitPython_Requests/blob/master/examples/requests_simpletest.py#L32

This comment has been minimized.

Copy link
@brentru

brentru Feb 11, 2020

Member

Also - why was the call to set_interface (https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi_socket.py#L40) removed? Would setting the interface be handled in user-code, instead?

This comment has been minimized.

Copy link
@tannewt

tannewt Feb 12, 2020

Author

Ok, examples updated.

Setting the interface is usually done by WifiManager now but is manual in these examples. Further into the future I think the socket should assume the interface instead. We should be able to isolate it to there in the future.

Ideally this library and others like MQTT should work with CPython's socket alone and then the BLE, ESP32SPI and ethernet libraries provide their own socket implementations.

Copy link
Member

brentru left a comment

@tannewt I have some q's about the new set_socket, rest looks OK - thanks for adding stability fixes and socket reuse.

tannewt added 5 commits Feb 12, 2020
# for SSL we need to know the host name
sock.connect((host, port), _socket.TLS_MODE)
else:
sock.connect(addr_info[-1], _socket.TCP_MODE)

This comment has been minimized.

Copy link
@brentru

brentru Feb 13, 2020

Member

@tannewt

Traceback (most recent call last):
  File "code.py", line 46, in <module>
  File "/lib/adafruit_requests.py", line 340, in get
  File "/lib/adafruit_requests.py", line 300, in request
  File "/lib/adafruit_requests.py", line 89, in _get_socket
AttributeError: 'module' object has no attribute 'TCP_MODE'

_socket does not have a _socket.TLS_MODE/TCP_MODE attribute, the attribute is defined within the sockets interface as socket._the_interface.TLS_MODE

Would we want to define the TLS/TCP modes in ESP32SPI?

This comment has been minimized.

Copy link
@tannewt

tannewt Feb 13, 2020

Author

That is part of the change I have pending. We can wait for this PR until the ESP32SPI changes are in.

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

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.