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

Boring Network Tool #34

Open
wants to merge 12 commits into
base: master
Choose a base branch
from
Open

Boring Network Tool #34

wants to merge 12 commits into from

Conversation

mjswen0923
Copy link
Collaborator

No description provided.

@mjswen0923 mjswen0923 added the enhancement New feature or request label Mar 21, 2021
@mjswen0923 mjswen0923 requested a review from Burke-Daniel March 21, 2021 00:03
Copy link
Contributor

@colton-smith colton-smith left a comment

Choose a reason for hiding this comment

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

Functionality works great, I tested the UDP and TCP functionality as well as the changing the frequency. Most requested changes are style-related or just comments pointing out helpful python features that could help.

There are some important comments about the blocking behaviour of the sendUDP method, that's the most important one I'd say.

Let me know if you have any questions about these comments!

Footnote: In the future, there is a branch naming convention to follow that can be found on the website

Tools/boringNetworkTool/README.MD Outdated Show resolved Hide resolved
Tools/boringNetworkTool/main.py Show resolved Hide resolved
Tools/boringNetworkTool/main.py Outdated Show resolved Hide resolved
Tools/boringNetworkTool/main.py Outdated Show resolved Hide resolved
Tools/boringNetworkTool/sending_config.json Outdated Show resolved Hide resolved
Tools/boringNetworkTool/sending_config.json Outdated Show resolved Hide resolved
Tools/boringNetworkTool/udp_server.py Outdated Show resolved Hide resolved
Tools/boringNetworkTool/tcp_server.py Outdated Show resolved Hide resolved
Tools/boringNetworkTool/udp_server.py Outdated Show resolved Hide resolved
s.sendto(bytes(message.toString(), 'utf-8'), (host, port))
print("message sent:" + message.toString())
# receive data from client (data, addr)
d = s.recvfrom(1024)
Copy link
Contributor

Choose a reason for hiding this comment

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

This blocks, with UDP I don't think we will always want to wait for a reply from the server before sending the next packet. Potentially, we could have an option in the config to await a reply.

Where it is a debug tool, we will usually just want it to send data continuously as well. This could be another option in the config (bool sendContinuous), that if true the script just continuously calls sendUDP or sendTCP.

I tried a while loop, it worked fine for UDP but there was an error when calling send TCP in a while loop (the server reset the connection or something)

image

Copy link
Collaborator

@Burke-Daniel Burke-Daniel left a comment

Choose a reason for hiding this comment

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

Couple things to look at, but this is super nice and really clean. Great work!

sys.exit()


def send_with_tcp(msgconfig, host, port):
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be nice for TCP to have the option for the network tool to act as either the server or the client

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you did this, you might be able to eliminate the tcp_server.py script altogether and use 2 instances of main.py, one configured as server and one configured as client, to test

"config": {
"ip": "localhost",
"port": 8888,
"useTCP": false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it might be better if this is commsType, or something similar, and then it parses for TCP or UDP as the value. If you make the option to act as the tcp server, you could then have a setting called servePort which would determine that.

Comment on lines +7 to +12
class Message:
def __init__(self, data):
self.data = data

def to_string(self) -> str:
return "%s \n" % str(self.data)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like this a lot, very clean

Comment on lines +1 to +4
import socket
import sys
import time
import json
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably want to order these in alphabetical order

s.sendto(bytes(message.to_string(), 'utf-8'), (host, port))
print("message sent: %s" % message.to_string())
# receive data from client (data, addr)
if msgconfig["config"]["sendContinuous"] is False:
Copy link
Collaborator

Choose a reason for hiding this comment

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

We are probably gonna want to properly send the messages repetitively indefinitely

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants