Reorg code per request from orphan pr 69 #107
Conversation
There was a problem hiding this comment.
Thanks for updating these changes!
Please only add CPython compatible APIs to socket. Looks like you want getpeername: https://docs.python.org/3/library/socket.html#socket.socket.getpeername
You cannot add it to the constructor because CPython doesn't have it.
|
@tannewt there is no Trying to add this API would be a major amount of work and to be honest without it following the convention of the rest of this lib I would not even know where to begin. Do you think this PR should simply be closed if it might not be a feature worth implementing? |
|
@mytechnotalent I don't think you need getpeername exposed from Nina because you already have a way to get the remote ip. I think you can call that API from your |
|
Can you provide more information? Perhaps an example? I am not following. |
| self._remote_ip = remote_ip | ||
| self._remote_port = remote_port |
There was a problem hiding this comment.
| self._remote_ip = remote_ip | |
| self._remote_port = remote_port |
| resp = self._send_command_get_response( | ||
| _GET_REMOTE_DATA_CMD, self._socknum_ll, reply_params=2 | ||
| ) | ||
| return {"ip_addr": resp[0], "port": resp[1]} |
There was a problem hiding this comment.
| return {"ip_addr": resp[0], "port": resp[1]} | |
| return (resp[0], resp[1]) |
| @property | ||
| def remote_ip(self): | ||
| """The remote ip""" | ||
| return self._remote_ip | ||
|
|
||
| @property | ||
| def remote_port(self): | ||
| """The remote port""" | ||
| return self._remote_port | ||
|
|
There was a problem hiding this comment.
| @property | |
| def remote_ip(self): | |
| """The remote ip""" | |
| return self._remote_ip | |
| @property | |
| def remote_port(self): | |
| """The remote port""" | |
| return self._remote_port | |
| def getpeername(self): | |
| ip, port = _the_interface.get_remote_data(self._socknum) | |
| # Parse so that ip is a string and port is an int | |
| # Second bullet here: https://docs.python.org/3/library/socket.html#socket-families | |
| return (ip, port) | |
| remote_ip=None, | ||
| remote_port=None, |
There was a problem hiding this comment.
| remote_ip=None, | |
| remote_port=None, |
There was a problem hiding this comment.
Oh that makes sense @tannewt! I will work on these changes as soon as I get a second and will test and push an update. Thank you again.
There was a problem hiding this comment.
@tannewt I have implemented the changes. I wanted ask specifically how you would like this tested. Here is the result in the REPL.
>>> socket.socket. close connect connected read readline recv send settimeout write socknum available getpeername
I obviously cant call socket.socket.getpeername() so if you have a quick test I can code up I will begin testing so that I know what you are looking for specifically. Any simple example will do thanks!
There was a problem hiding this comment.
I'd imagine you need to create a socket and connect before calling it. The new requests library can work with CPython sockets so that's probably the best place to start.
There was a problem hiding this comment.
@tannewt I created an access point called cool and then I connected my iPhone to it. It successfully connected however when I call socket.socket.getpeername() i get TypeError: function takes 1 positional arguments but 0 were given as it should be returning the peer ip of 192.168.4.2 as that is the iPhone and the server is 192.168.4.2.
Any suggestions on how to better check the getpeername call?
There was a problem hiding this comment.
You actually need to connect a TCP socket and then call .getpeername() on it. The error is because you are calling it on the class, not an instance, and therefore no self is passed.
Why are you adding this overall? It doesn't seem like you have a use for it.
There was a problem hiding this comment.
Great question @tannewt. Over a year ago when I did the original PR I had issues where I had the device set up as an Access Point and if I accidently logged in from multiple browsers on various machines it would crash.
This original PR was designed to check for that remote IP and if it was not 192.168.4.2 then don't connect. It was to add the ability to get this information if a developer wanted to implement functionality such as this.
I am ok with closing the PR however. Thoughts?
There was a problem hiding this comment.
This is what I tried to do originally as well. I can't seem to even create a socket object here.
>>> import adafruit_esp32spi.adafruit_esp32spi_socket as socket
>>> s = socket.socket()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/lib/adafruit_esp32spi/adafruit_esp32spi_socket.py", line 62, in __init__
AttributeError: 'NoneType' object has no attribute 'get_socket'
>>>
There was a problem hiding this comment.
With the current ESP32-SPI implementation you need to call set_interface on the socket module: https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi_socket.py#L24 I believe you pass it the ESP32SPI object.
I could see this PR being helpful but if no one is using it enough to test, then we could just close for now.
This PR deprecates PR 69 as a year has passed as I was tied up with other responsibilities. The code base has changed significantly so there was a large merge conflict with the prior work. This new PR takes all of the recommendations of @mscosti @brentru @jerryneedell into account. @kattni apology for taking so long.