6
\$\begingroup\$

I'm currently building a small plane with Raspberry Pi Zero W. I wrote a small code which uses UDP to control it.

Here I'm resending packets regularly in a thread because of the joystick input (it gives a lot of values when used and time.sleep() in a pygame event loop does bad things). Am I doing it the right way?

I'm also sending three bytes only per packets (which is enough for the control), but should I use power of 2 packet size? And why?

import socket
import struct
import time
import threading

S_TYPE = "BBB"


class Server:
    def __init__(self, ip="", port=8080, timeout=-1):
        self.ip = ip
        self.port = port
        self.sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
        self.sock.bind((ip, port))
        if (timeout > 0):
            self.sock.settimeout(timeout)

    def recv(self):
        data = self.sock.recv(struct.calcsize(S_TYPE))
        res = struct.unpack("BBB", data)
        return (res)


class Client:
    def __init__(self, ip, port=8080, interval=100):
        self.ip = ip
        self.port = port
        self.data = (0, 0, 0)
        self.interval = interval
        self.sock = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
        self.thread = threading.Thread(target=self.send_loop)
        self.thread.daemon = True
        self.lock = threading.Lock()
        self.running = True

    def send(self, data_bytes):
        self.sock.sendto(data_bytes, (self.ip, self.port))

    def send_loop(self):
        '''Send loop is used to regulary send data in a thread'''
        while (self.running):
            self.lock.acquire()
            data_bytes = struct.pack(S_TYPE, *self.data)
            self.lock.release()
            self.send(data_bytes)
            time.sleep(self.interval / 1000.0)

    def update(self, a, b, c):
        '''Update data which is sent'''
        self.lock.acquire()
        self.data = (a, b, c)
        self.lock.release()

    def start(self):
        self.thread.start()

    def stop(self):
        # __del__ wouldn't be called with a thread running
        self.running = False

EDIT: do you see any problem not using a lock for self.running?

\$\endgroup\$
4
\$\begingroup\$

Client

In __init__ you should have

    self.running = False

and start should look like this:

def start(self):
    if not self.running:
        self.running = True
        self.thread.start()

Instead of

self.lock.acquire()
self.data = (a, b, c)
self.lock.release()

it's safer and more elegant to write

with self.lock:
  self.data = (a, b, c)

Parentheses aren't necessary in

while (self.running):
\$\endgroup\$
4
\$\begingroup\$

Some notes:

  • data as a name has been criticised elsewhere; I would simply say that any variable or property should be self-explanatory within context. a, b, and c should also have more descriptive names.
  • You don't need self.ip and self.port in the server - they are only used in the constructor. YAGNI.
  • You should default timeout to None and forward it directly to the socket configuration. Making your code have different timeout semantics from the socket library is probably just going to cause confusion.
  • Why return (res) instead of return res?
\$\endgroup\$
1
  • \$\begingroup\$ IP and port of the server might be useful. On the other hand, you can probably obtain them from the bound socket. EDIT OK, you win. \$\endgroup\$
    – kyrill
    Apr 1 '17 at 21:39
3
\$\begingroup\$

You extracted "BBB" into a pseudo-const, which is good. However, you missed one:

res = struct.unpack("BBB", data)

This obviously should've been:

res = struct.unpack(S_TYPE, data)

If you aren't sure whether you replaced them all, Ctrl + F works in pretty much all editors. Often there's even a search and replace :-)

\$\endgroup\$
2
  • \$\begingroup\$ You could also make it even more obvious and add it to the constructor as it's only used inside the class. \$\endgroup\$ Apr 3 '17 at 17:54
  • \$\begingroup\$ @Dex'ter it is used in two classes \$\endgroup\$
    – mou
    Apr 3 '17 at 19:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.