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

ESP32S2: Implement TCP Server bindings #3854

Merged
merged 7 commits into from Jan 21, 2021
Merged

ESP32S2: Implement TCP Server bindings #3854

merged 7 commits into from Jan 21, 2021

Conversation

hierophect
Copy link
Collaborator

@hierophect hierophect commented Dec 20, 2020

This PR adds a few new features to support running a TCP server on the ESP32-S2:

  1. Adds bind, listen and accept, which allow connecting to remote sockets and sending/receiving data from them.
  2. Removes the property that all sockets of type SOCK_STREAM automatically use MbedTLS instead of the LWIP socket API, and makes that decision on the fly based on what functions are used first.
  3. Adds in additional logic to the various send and receive functions to determine when to use TLS vs LWIP.

The differentiation between sockets that use TLS and those that don't is still a bit hacky. Right now, it's determined by what functions the socket calls first - if you use connect after initialization, it'll become a TLS socket and will be ineligible for any other uses. Otherwise (ie, calling bind or sendto first) it becomes a regular socket. I'd like to eventually improve this to allow connect without TLS, possibly by putting the TLS init in the SSL wrapper step. We could also consider moving all TLS functionality to a separate module (possibly a duplicate Socket based API) instead of having both kinds of operation under the hood here.

Tested on a Saola 1 Wrover. Test programs below:

Resolves #3724
Resolves #3836

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Sorry I left these pending. Thanks for the ping!

ports/esp32s2/common-hal/socketpool/Socket.c Outdated Show resolved Hide resolved
ports/esp32s2/common-hal/socketpool/Socket.c Outdated Show resolved Hide resolved
ports/esp32s2/common-hal/socketpool/Socket.c Outdated Show resolved Hide resolved
shared-bindings/socketpool/Socket.c Outdated Show resolved Hide resolved
shared-bindings/socketpool/Socket.c Outdated Show resolved Hide resolved
@tannewt
Copy link
Member

tannewt commented Jan 7, 2021

@krittick want to test this PR?

@krittick
Copy link

krittick commented Jan 7, 2021

@krittick want to test this PR?

Sure, would love to. Will try to get to it later today.

@tannewt tannewt requested a review from jepler January 13, 2021 18:22
@hierophect
Copy link
Collaborator Author

Does the CI failure have to do with my code?

subprocess.CalledProcessError: Command '['/opt/hostedtoolcache/Python/3.8.7/x64/bin/python', '-m', 'virtualenv', '/home/runner/work/circuitpython/circuitpython/.idf_tools/python_env/idf4.2_py3.8_env']' returned non-zero exit status 1.

My changes don't affect anything related to this, as far as I can tell?

@tannewt
Copy link
Member

tannewt commented Jan 14, 2021

I don't think so. Dan and Jeff would have a better idea of the CI state atm.

@hierophect
Copy link
Collaborator Author

hierophect commented Jan 17, 2021

@tannewt looks like CI passed this time, should ready for re-review

Copy link
Member

@tannewt tannewt left a comment

Choose a reason for hiding this comment

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

This looks good to me! Thank you! I'm holding the approval so this is merged in for 6.2 rather than 6.1. @dhalbert can merge post-6.1

@hierophect
Copy link
Collaborator Author

hierophect commented Jan 20, 2021

This looks good to me! Thank you! I'm holding the approval so this is merged in for 6.2 rather than 6.1. @dhalbert can merge post-6.1

Sounds good. I have the TLS server separation PR finished and ready to go as soon as this gets merged, so we could potentially get both of them into the same release.

Copy link
Collaborator

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

Approving and merging as requested.

@hierophect hierophect dismissed tannewt’s stale review January 21, 2021 20:33

Discord merge approval

@hierophect hierophect merged commit f88a896 into adafruit:main Jan 21, 2021
@hierophect hierophect deleted the esp-tcpserver branch January 21, 2021 22:25
@netroy
Copy link

netroy commented Jan 29, 2021

Hey @hierophect,
I've been running a hacked together code for NTP sync on an ESP32S2 board, and it has stopped working since this change.
git bisect has located the breaking commit from this PR. I believe it might be the changes in the socketpool code.
Reverting the merge commit fixed the issue for me.

Here is the code

import struct
import time
import rtc
import socketpool
import wifi

TZ_OFFSET = 3600
NTP_TO_UNIX_EPOCH = 2_208_988_800  # 1970-01-01 00:00:00
BILLION = 1_000_000_000

wifi.radio.connect(ssid=secrets["ssid"], password=secrets["password"])
pool = socketpool.SocketPool(wifi.radio)
packet = bytearray(48)
packet[0] = 0b00100011

for i in range(1, len(packet)):
  packet[i] = 0

with pool.socket(pool.AF_INET, pool.SOCK_DGRAM) as sock:
  sock.sendto(packet, ("pool.ntp.org", 123))
  sock.recvfrom_into(packet)
  destination = time.monotonic_ns()

seconds = struct.unpack_from("!I", packet, offset=len(packet) - 8)[0]
monotonic_start = seconds - NTP_TO_UNIX_EPOCH - (destination // BILLION)

rtc.RTC().datetime = time.localtime(time.monotonic_ns() // BILLION + monotonic_start + TZ_OFFSET)

The call sock.recvfrom_into seems to be the one that fails with an EAGAIN.
Unfortunately my C foo isn't up to the mark to be able to debug this any further.
But, if you could point out what the issue might be (in this PR, or my code), I'd be really grateful.

Cheers :)

@hierophect
Copy link
Collaborator Author

Hi @netroy, an issue with this PR was that when it expanded the timeout system to support non-blocking calls, it inadvertently set the default timeout of a socket to 0 instead of the Cpython default of None (ie infinite timeout). #4049 will change back to None, but until then you can use settimeout(None) to extend the timeout yourself. I think that should resolve your problem, but feel free to follow up with an issue if it doesn't.

@tannewt
Copy link
Member

tannewt commented Jan 29, 2021

@hierophect Maybe we should fix this outside of #4049 so we can get it in faster.

@hierophect
Copy link
Collaborator Author

@tannewt nah it's already in, this problem was one of the simple ones. I do think we can go ahead and merge #4049. The big issue is what's going on in #4061 which I'm still hacking at but can put in a different PR

@hierophect
Copy link
Collaborator Author

hierophect commented Jan 29, 2021

Wait I probably read that wrong, you mean change the default timeout in a new PR so we can take more time on #4049?

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.

Socket closing if timed out and no data on socket Add socket bind/accept/listen support for esp32s2
5 participants