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

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

Open
wants to merge 6 commits into
base: master
from

Conversation

@mytechnotalent
Copy link

mytechnotalent commented Aug 24, 2019

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

…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 ladyada requested a review from brentru Aug 24, 2019
remote_ip = _the_interface.get_remote_data(sock_num)
return _the_interface.pretty_ip(remote_ip)

def print_remote_ip(self):

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

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()

This comment has been minimized.

Copy link
@mytechnotalent

mytechnotalent Aug 24, 2019

Author

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

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

right above print_remote_ip()

This comment has been minimized.

Copy link
@jerryneedell

This comment has been minimized.

Copy link
@mytechnotalent

mytechnotalent Aug 24, 2019

Author

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

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

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

This comment has been minimized.

Copy link
@mytechnotalent

mytechnotalent Aug 24, 2019

Author

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.

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

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.

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

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

see note below - line 239 - regarding print statements.

This comment has been minimized.

Copy link
@mytechnotalent

mytechnotalent Aug 24, 2019

Author

I will amend the code.

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

I am not insisting -- just offering an opinion.

@@ -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()

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

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

@mytechnotalent

This comment has been minimized.

Copy link
Author

mytechnotalent commented Aug 24, 2019

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

@jerryneedell

This comment has been minimized.

Copy link
Contributor

jerryneedell commented Aug 24, 2019

@jerryneedell

This comment has been minimized.

Copy link
Contributor

jerryneedell commented on adafruit_esp32spi/adafruit_esp32spi_wsgiserver.py in eb80804 Aug 24, 2019

i still don't see the need for this function -- the user can just:
print("some text" + check_remote_ip()) can't they ? or am I missing something?

@mytechnotalent

This comment has been minimized.

Copy link
Author

mytechnotalent commented Aug 24, 2019

I will remove it.

@jerryneedell

This comment has been minimized.

Copy link
Contributor

jerryneedell commented Aug 24, 2019

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

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

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

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

This comment has been minimized.

Copy link
@mytechnotalent

mytechnotalent Aug 24, 2019

Author

Removed per request.

This comment has been minimized.

Copy link
@mytechnotalent

mytechnotalent Aug 24, 2019

Author

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

This comment has been minimized.

Copy link
@jerryneedell

jerryneedell Aug 24, 2019

Contributor

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

@jerryneedell

This comment has been minimized.

Copy link
Contributor

jerryneedell commented Aug 24, 2019

Congratulations on satisfying PyLint!!

@mytechnotalent

This comment has been minimized.

Copy link
Author

mytechnotalent commented Aug 24, 2019

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

Copy link
Contributor

mscosti left a comment

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.

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]

This comment has been minimized.

Copy link
@mscosti

mscosti Aug 24, 2019

Contributor

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] }

This comment has been minimized.

Copy link
@mytechnotalent

mytechnotalent Aug 24, 2019

Author

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

This comment has been minimized.

Copy link
@mscosti

mscosti Aug 24, 2019

Contributor

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')
@@ -216,3 +217,11 @@ def _get_environ(self, client):
env[key] = value

return env

def check_remote_ip(self):

This comment has been minimized.

Copy link
@mscosti

mscosti Aug 24, 2019

Contributor

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

This comment has been minimized.

Copy link
@brentru

brentru Aug 28, 2019

Member

agreed! socket would be the correct home for this method

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

return env

def check_remote_ip(self):

This comment has been minimized.

Copy link
@mscosti

mscosti Aug 24, 2019

Contributor

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

This comment has been minimized.

Copy link
Contributor

siddacious commented Aug 24, 2019

Great work @mytechnotalent , thanks for your contribution!

Thanks to @jerryneedell and @mscosti for the reviews

@tannewt

This comment has been minimized.

Copy link

tannewt commented Aug 27, 2019

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.

@@ -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):
"""

This comment has been minimized.

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

return env

def check_remote_ip(self):

This comment has been minimized.

Copy link
@brentru

brentru Aug 28, 2019

Member

agreed! socket would be the correct home for this method

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.