Adding to the functionality of the ESP32 library in order to provide … #69

mytechnotalent · 2019-08-24T17:53:56Z

…a printable echo of a remote IP connection in addition to optional code to control specific IP access to the server as the server can only take one connection at a time.

jerryneedell · 2019-08-24T17:59:50Z

adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

+        remote_ip = _the_interface.get_remote_data(sock_num)
+        return _the_interface.pretty_ip(remote_ip)
+
+    def print_remote_ip(self):


Is it a good idea to have print functions like this in a library? The user can just print the return from check_remote_ip()

Are you suggesting to just utilize the check_remote_ip() method that I created instead of having both?

right above print_remote_ip()

Adafruit_CircuitPython_ESP32SPI/adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

Line 234 in 2990061

def check_remote_ip(self):

Just making sure you were referring to my method as I was not sure if you were referring to something already existing.

yes -- I think print statements are normally only used for debug.

The check_remote_ip() method is designed to allow forced control if needed otherwise the print method is just for echoing the connections. Two separate functionalities.

I'll leave it to others to comment. I'm not used to seeing prints in libraries but I'll defer to the other reviewers.

jerryneedell · 2019-08-24T18:00:35Z

adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

@@ -98,9 +98,23 @@ def update_poll(self):
        """
        self.client_available()
        if (self._client_sock and self._client_sock.available()):
+            self.print_remote_ip()


see note below - line 239 - regarding print statements.

I will amend the code.

I am not insisting -- just offering an opinion.

jerryneedell · 2019-08-24T18:14:22Z

adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

@@ -98,7 +98,7 @@ def update_poll(self):
        """
        self.client_available()
        if (self._client_sock and self._client_sock.available()):
-            self.print_remote_ip()
+            # self.print_remote_ip()


sorry -- commenting out code is not a good idea in my opinion. either remove the function or leave it in.

mytechnotalent · 2019-08-24T18:14:54Z

I don't understand what is causing Travis to fail any ideas?

jerryneedell · 2019-08-24T18:17:22Z

several issues reported here's https://travis-ci.com/adafruit/Adafruit_CircuitPython_ESP32SPI/builds/124470639#L288

mytechnotalent · 2019-08-24T18:39:30Z

I will remove it.

jerryneedell · 2019-08-24T18:45:01Z

Only remove it if you agree with my comments. I may have totally misunderstood your intent.

jerryneedell · 2019-08-24T18:58:32Z

adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

+        if (self._client_sock and self._client_sock.available()):
+            result = self.check_remote_ip()
+            if result == "192.168.4.2":
+                self.print_remote_ip()


If you have removed the print then this line should also be removed

Removed per request.

The purpose was to provide new users a usage case so they might be motivated to grow with the platform.

I had no concern with the example -- only with the line using print_remote_ip() since it was no longer present.

jerryneedell · 2019-08-24T19:07:03Z

Congratulations on satisfying PyLint!!

mytechnotalent · 2019-08-24T19:10:14Z

Thanks @jerryneedell and thank you for the help and suggestions to get the code more functional.

mscosti

Looks good! I think we can move some things around to make them more accessible to every socket instance, and making it available in a standardized way to wsgi applications.

mscosti · 2019-08-24T22:00:56Z

adafruit_esp32spi/adafruit_esp32spi.py

+        self._socknum_ll[0][0] = socket_num
+        resp = self._send_command_get_response(_GET_REMOTE_DATA_CMD,
+                                               self._socknum_ll, reply_params=2)
+        return resp[0]


the get_remote_data command in NINA returns 2 reply params. The first is the remote IP, the second is the remote port

instead of just returning the IP, I would return both.

something like

return { 'ip_addr': resp[0], 'port': resp[1] }

File "/lib/adafruit_esp32spi/adafruit_esp32spi.py", line 550, in pretty_ip
KeyError: 0

So by changing the return type an dictionary object means that when you call this method you will need to do an additional step to get the ip whenever you call this method. Dictionary objects have a method on it called .get(keyname). You'll need to update usages of this method so you actually grab the IP address from the dictionary.

Example:

remote_data = esp.get_remote_data(socknum) Ip = remote_data.get('ip_addr')

mscosti · 2019-08-24T22:05:41Z

adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

@@ -216,3 +217,11 @@ def _get_environ(self, client):
            env[key] = value

        return env
+
+    def check_remote_ip(self):


I would move this to the Socket class as a @property, like we did for socknum.(https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi_socket.py#L183-L186)
We could name the property remote_ip. You could add in a second property for remote_port as well.

then every socket instance could get the remote_ip by doing sock_instance.remote_ip, and the port by doing sock_instance.remote_port

agreed! socket would be the correct home for this method

mscosti · 2019-08-24T22:13:00Z

adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

@@ -216,3 +217,11 @@ def _get_environ(self, client):
            env[key] = value

        return env
+
+    def check_remote_ip(self):


if we move the remote ip fetching to a property on Socket, then we're still going to want to expose it some way to a server application. Luckily, the WSGI defines an environment request variable named REMOTE_ADDR. You could add that to the get_environ function here: https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py#L174-L179

it would look something like

env['REMOTE_ADDR'] = self._client_sock.remote_ip

The environment object gets passed to the application on every single incoming request, so all your request handlers would just automatically get the remote IP and can use it for processing if they need it.

siddacious · 2019-08-24T23:15:24Z

Great work @mytechnotalent , thanks for your contribution!

Thanks to @jerryneedell and @mscosti for the reviews

tannewt · 2019-08-27T00:13:13Z

print calls in libraries are almost always bad because they force output on the library user. In the case of formatting an ip, it's usually a matter of making a class for the type of data (IP) and then implementing __str__ to format it for humans.

brentru · 2019-08-28T13:50:51Z

adafruit_esp32spi/adafruit_esp32spi.py

@@ -768,3 +769,12 @@ def get_time(self):
            resp = self._send_command_get_response(_GET_TIME)
            return struct.unpack('<i', resp[0])
        raise RuntimeError("Must be connected to WiFi before obtaining NTP.")
+
+    def get_remote_data(self, socket_num):
+        """


Please document the argument, socket_num, using a docstring (see here :https://github.com/adafruit/Adafruit_CircuitPython_ESP32SPI/blob/master/adafruit_esp32spi/adafruit_esp32spi.py#L514)

brentru · 2019-08-28T13:51:38Z

adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py

@@ -216,3 +217,11 @@ def _get_environ(self, client):
            env[key] = value

        return env
+
+    def check_remote_ip(self):


agreed! socket would be the correct home for this method

kattni · 2020-01-08T18:43:17Z

@mytechnotalent @brentru @jerryneedell Thank you for all your work on this PR. It has gotten behind the repo and requires resolving a merge conflict now. I'd like to see this included, but the PR needs to be updated and the final change requests addressed. If you need assistance with this, please let us know. Brent or Jerry: if the original author is unavailable, either of you is welcome to resolve the conflicts and push to this PR.

mytechnotalent · 2020-09-29T14:26:52Z

This PR is deprecated in favor of #107.

Adding to the functionality of the ESP32 library in order to provide …

2990061

…a printable echo of a remote IP connection in addition to optional code to control specific IP access to the server as the server can only take one connection at a time.

ladyada requested a review from brentru August 24, 2019 17:58

jerryneedell reviewed Aug 24, 2019

View reviewed changes

Removed print echo from library function as requested. Left optional.

f577e93

jerryneedell reviewed Aug 24, 2019

View reviewed changes

Fixed linting errors and removed comment from library as requested.

eb80804

Fixing linting errors and removing print method from library.

32fb05f

Fixing linting errors.

5b88086

jerryneedell reviewed Aug 24, 2019

View reviewed changes

Removing example code per request.

ad976f4

mscosti suggested changes Aug 24, 2019

View reviewed changes

brentru requested changes Aug 28, 2019

View reviewed changes

anecdata mentioned this pull request Sep 20, 2019

Expose more NINA FW SPI command handlers: getCurrBSSID, getIdxBSSID, getIdxChannel #74

Closed

mytechnotalent closed this Sep 29, 2020

Adding to the functionality of the ESP32 library in order to provide … #69

Adding to the functionality of the ESP32 library in order to provide … #69

Conversation

mytechnotalent commented Aug 24, 2019

Choose a reason for hiding this comment

mytechnotalent Aug 24, 2019 • edited Loading

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

mytechnotalent commented Aug 24, 2019

jerryneedell commented Aug 24, 2019

mytechnotalent commented Aug 24, 2019

jerryneedell commented Aug 24, 2019

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

jerryneedell commented Aug 24, 2019

mytechnotalent commented Aug 24, 2019

mscosti left a comment

Choose a reason for hiding this comment

mscosti Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mscosti Aug 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

siddacious commented Aug 24, 2019

tannewt commented Aug 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kattni commented Jan 8, 2020

mytechnotalent commented Sep 29, 2020

mytechnotalent Aug 24, 2019 •

edited

Loading

mscosti Aug 24, 2019 •

edited

Loading

mscosti Aug 24, 2019 •

edited

Loading