Python UDP class to control drone

Question

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?

Answer 1

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

Answer 2

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?

• kyrill: 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.

Answer 3

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

• Grajdeanu Alex: You could also make it even more obvious and add it to the constructor as it's only used inside the class.
• mou: @Dex'ter it is used in two classes