-
Notifications
You must be signed in to change notification settings - Fork 3
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
Created UDP sockets #36
Conversation
Wrapper for Python's socket module. | ||
""" | ||
|
||
def __init__(self, socket_instance: socket.socket) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to give it a default value of None for it to allow not passing in any arguments
else: | ||
self.__socket = socket_instance | ||
|
||
def send_to(self, data: bytes, address: tuple) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using address as a tuple, please send host and port as 2 different parameters, then you can put them into a tuple yourself in the implementation. It's kind of annoying for users to have to do this themselves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, UDP sockets will give an error when data size is too large. Can you setup a while loop to break up the data into smaller chunks, such as 4096 bytes (have this as some sort of constant)
|
||
return True | ||
|
||
def recv_from(self, buf_size: int) -> "tuple[bool, bytes | None]": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please edit the type hint. We want to return a tuple of length 2: (success, data). No need to return the address from which it was sent. Also, could we rename this to just recv
? It will be more consistent and easier for users to use.
|
||
Returns | ||
------- | ||
tuple: If the data and address were received |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you update the docstring to reflect the data returned?
|
||
""" | ||
try: | ||
data, addr = self.__socket.recvfrom(buf_size) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you put this in a while loop? I believe that networks usually split large data into smaller packets, and may not send the whole thing in one go. Thus, you need to keep on looping until you get all of the data. Additionally, you should check that it all comes from the same address (for now, use the first recv's returned address).
network/modules/UDP/server_socket.py
Outdated
print(f"Could not create socket, error: {e}.") | ||
return False, None | ||
|
||
def receive(self, buffer_size: int = 1024) -> "tuple[bytes, tuple]": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need to make a new recv function here, we can just use the base class's recv function (since technically both server and client can send and recv)
network/modules/UDP/server_socket.py
Outdated
print(f"Error receiving data: {e}.") | ||
return b"", () | ||
|
||
def close(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same with close, we can re-use base class's close
network/modules/UDP/client_socket.py
Outdated
print(f"Failed to send data: {e}") | ||
return False | ||
|
||
def close(self) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to customize close here
Parameters | ||
---------- | ||
host: str (default "") | ||
The hostname or IP address to bind the socket to. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add explanation that empty string is the open address
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
while data_sent < data_size: | ||
|
||
chunk = data[data_sent:data_sent+4096] | ||
packed_data = struct.pack(f'!{len(chunk)}s', chunk) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad. There's no need to pack our data using struct, I kind of overthinked for a moment. This is only necessary when you are say sending an array of integers, but we are sending raw bytes to be interpreted by the image_decode module later anyways.
|
||
while data_sent < data_size: | ||
|
||
chunk = data[data_sent:data_sent+4096] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use a global constant declared at the top ie CHUNK_SIZE
instead of a value here. Also, you should probably check for when data_sent + 4096 > data_size
before it throws index out of bounds or something like that.
try: | ||
unpacked_data = struct.unpack(f'!{len(data)}', data) | ||
except struct.error as e: | ||
print(f"Could not unpack data: {e}") | ||
return False, None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad, we don't need this anymore.
data += packet # Add the received packet to the accumulated data | ||
|
||
# Assuming that receiving less than buf_size means end of data | ||
if len(packet) < buf_size: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we were trying to receive exactly 4096 bytes? Then this would break as it will be listening for more when there is no more. Just have a variable tracking progress so far or use len(data)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little confused here. Did you want a condition that would break out of the loop if the length of the data was exactly 4096 bytes? I don't really see how that would work independently of the other condition unless the user somehow specified that they wanted exactly 4096 bytes beforehand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just meant you should have a variable that keeps track of how many bytes you've received so far, because this won't work if the user wanted to receive a byte length that is a multiple of 4096. Sort of like in the send function, just have a variable that you keep on adding to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
…y, addressed minor changes in socket_wrapper.py
elif addr != current_addr: | ||
print(f"Data received from multiple addresses: {addr} and {current_addr}") | ||
return False, None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than returning, we can just drop the unwanted packet and continue.
if data_size >= buf_size: | ||
break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's probably better to just put this in the while condition rather than down here, makes it more readable.
network/modules/UDP/server_socket.py
Outdated
self.server_address = server_address | ||
|
||
@classmethod | ||
def create(cls, host: str = "", port: int = 5000) -> "tuple[bool, UdpServerSocket | None]": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a parameter for setting a timeout? Have a default of like 10seconds (see settimeout()
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
self.server_address = server_address | ||
|
||
@classmethod | ||
def create( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a parameter for setting a timeout? Have a default of like 10seconds (see settimeout()
)
|
||
data_sent = 0 | ||
data_size = len(data) | ||
chunk_size = 4096 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we set this as a global constant like at the top of the file in all caps?
data_size = len(data) | ||
chunk_size = 4096 | ||
|
||
while data_sent + chunk_size < data_size: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't put the check here, because if the user wanted to send 5000 bytes, the last 904 bytes will not be sent. Please keep this as data_sent < data_size
and move the check to just prevent index out of bounds when (prevent it from trying to send 8192 bytes when there are only 5000 bytes)
For initializing Socket with an existing socket object. | ||
""" | ||
if socket_instance is None: | ||
self.__socket = socket.socket(socket.AF_INET, socket.SOCK_DGRAM) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the timeout here as well, just in case
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved!
* Created UDP Sockets * Addressed Comments * Added send() function and override recv() function in client_socket.py, addressed minor changes in socket_wrapper.py * Made requested changes * Updated docstrings
* Created UDP Sockets * Addressed Comments * Added send() function and override recv() function in client_socket.py, addressed minor changes in socket_wrapper.py * Made requested changes * Updated docstrings
* Created UDP Sockets * Addressed Comments * Added send() function and override recv() function in client_socket.py, addressed minor changes in socket_wrapper.py * Made requested changes * Updated docstrings
* Add image_encode and socket_wrapper in lte module * Refactor socket classes into client/server * Refactor network wrapper into "ClientSocket" and "ServerSocket" classes. Write tests for sending images * Run linters/formatters * Black and Pylint changes * Fixes for test_network.py * Refactor pytest into sender and receiver scripts * Fix in ServerSocket constructor * bug fixes for tests, and linter formats * - Refactored to indicate current sockets are TCP specific - Simplified many steps by using higher-level APIs - Added some comments and changed README * Created UDP sockets (#36) * Created UDP Sockets * Addressed Comments * Added send() function and override recv() function in client_socket.py, addressed minor changes in socket_wrapper.py * Made requested changes * Updated docstrings * Modify TCP unit tests to remove image encoding + linter and other fixes * fix rebase to main * add UDP unit tests + pytest automated unit tests for TCP & UDP. Refactored UDP class. * linter fixes * fix relative path issues * add integration test and slight modification to image_encoding * fix network address overuse in pytest due to teardown not fast enough * Update to python 3.11 (#47) * Using own forked dronekit, which is now compatible with python 3.11 * update to python 3.11 * incorporate WARG's dronekit fork as a submodule * fix new dronekit import in tests * fix linter issues * update submodules * minor fixes * add project setup scripts which update submodules as well * remove submodule libraries from requirements * remove empty comment in requirement.txt * Fix mavutil imports (#49) * mavutil is imported from pymavlink and not dronekit * fix linter and add __init__ file for root directory (since common is meant to be a module) * Apparently pytest cannot work if root dir has __init__ file * switch to relative imports * edit ci workflow to upgrade pip first * udpate CI workflow to newer versions * try adding __init__ again * move comment to doc string for test_image_encode_decode * Add image_encode and socket_wrapper in lte module * Refactor socket classes into client/server * Refactor network wrapper into "ClientSocket" and "ServerSocket" classes. Write tests for sending images * Run linters/formatters * Black and Pylint changes * Fixes for test_network.py * Refactor pytest into sender and receiver scripts * Fix in ServerSocket constructor * bug fixes for tests, and linter formats * - Refactored to indicate current sockets are TCP specific - Simplified many steps by using higher-level APIs - Added some comments and changed README * Created UDP sockets (#36) * Created UDP Sockets * Addressed Comments * Added send() function and override recv() function in client_socket.py, addressed minor changes in socket_wrapper.py * Made requested changes * Updated docstrings * Modify TCP unit tests to remove image encoding + linter and other fixes * fix rebase to main * add UDP unit tests + pytest automated unit tests for TCP & UDP. Refactored UDP class. * linter fixes * use relative imports * rename modules to be lower case * make socket's server_address variable private, fixed logger pytest --------- Co-authored-by: Maxwell Lou <[email protected]> Co-authored-by: Herman Gahra <[email protected]> Co-authored-by: Maxwell Lou <[email protected]>
Created UDP server and client sockets