current community

your communities

more stack exchange communities

Stack Exchange
tour help
Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.
up vote 2 down vote favorite

I use a thread to read from a UDP socket. Afterwards the string gets parsed. In another thread I send to every connected client via this socket. I would like to keep latency and resources for running the script as low as possible.

It would be nice to add a time-out.

Every attempt to read should be cancelled if longer than e.g. 20 ms, to keep the response time for other clients low, because I believe that the current attempt to read from this socket is blocking the loop. I was reading that some people use select() is there an advantage?

def trnm_thr():                                                         # trnm_thr() sends commands to Arduino
  global udp_clients
  udp_client = None

  while pySerial is not None:
    if not pySerial.writable():
      continue

    try:
      udp_msg, udp_client = udp_sock.recvfrom(512)                      # Wait for UDP packet from ground station
      if udp_client is not None: udp_clients.add(udp_client)            # Add new client to client list
      if not udp_msg: continue                                          # If message is empty continue without parsing
    except socket.timeout:
      logging.error("Write timeout on socket")                          # Log the problem
    else:        
      try:
        p = json.loads(udp_msg)                                         # parse JSON string from socket
      except (ValueError, KeyError, TypeError):
        logging.debug("JSON format error: " + udp_msg.strip() )
      else:
        # Serial device stuff ..
share|improve this question
    
Context –  200_success♦ Jun 30 at 16:59

3 Answers 3

up vote 3 down vote accepted

First a quick question: Does this code run? Currently I see no declaration for pySerial or udp_sock. If the interpreter hit any line in your code using either of those two variables, a NameError would have thrown.

In order to review the valid content you have, I am going to assume this was a copy-paste error.

I have a few implementation comments:

  1. If you wanted to set a timeout, you can use the aptly named socket.settimeout() function. I imagine this should suffice for your purposes. You could look into the socket.setblocking and select as this SO post says. However, unless you are listening with multiple sockets per thread, you shouldn't really have to worry whether a socket is blocking or not.
  2. Instead of using try ... else and creating another level of indentation, use continue in your except blocks.
  3. Don't use global unless absolutely necessary (which will practically be never). A simple fix would be to pass udp_clients to your function.

Now, onto some style comments.

  1. I enjoy reading and understanding 'low-level' (sockets, OS stuff, etc.) code like this. However, for some reason, most of the code that I read that is this low-level, has one glaring problem: the writers truncate EVERYTHING: a simple socket becomes sock, an address becomes addr, etc.

    Many of these names have become conventionalized through constant and consistent use. However, this goes against the general Python convention that its better to be too verbose instead of too terse. Instead of sock take the time to type the two extra characters to make socket.

    Also, make sure your variable/function names describe what they hold/do. The name p tells us nothing about what it holds and your function name trnm_thr is so condensed I have no clue what it is supposed to do. Don't sacrifice clarity and readability for conciseness.

  2. Don't use inline statements after if statements. This breaks the flow of the program and can throw readers off. The same goes for inline comments.

  3. Technically your indentation level is fine. However, Pythonic convention is to use 4 spaces.

  4. Be as specific with your except blocks as possible. Unfortunately, I don't know enough about what errors json.loads throws, so I cannot suggest anything better for:

    except (ValueError, KeyError, TypeError):

    than the general case mentioned above.

Here is a PEP8 compliant version of your code (with my other recommendations):

def do_something_with_sockets(udp_clients, py_serial, udp_socket):
    udp_client = None

    udp_socket.settimeout(.02)
    while py_serial is not None:
        if not py_serial.writable():
            continue

        try:
            udp_message, udp_client = udp_socket.recvfrom(512)
        except socket.timeout:
            logging.error("Write timeout on socket")
            continue

        if udp_client is not None:
            udp_clients.add(udp_client)
        if not udp_message:
            continue

        try:
            json_data = json.loads(udp_message)  
        except (ValueError, KeyError, TypeError):
            logging.debug('JSON format error: {}'.format(udp_message.strip()))
            continue

        # Serial device stuff ..
share|improve this answer
    
I think this question is a follow-up on a previous question. –  200_success♦ Jun 30 at 17:01
up vote 4 down vote

I would instead use a single thread for both tasks and employ an epoll selector on the sockets with a certain timeout. To the best of my knowledge, this is more or less what popular networking frameworks (e.g., NodeJS, Nginx, Lighttpd, etc.) do. I always found waiting on a socket for an undetermined amount of time in a separate thread a waste of resources. That being said, YMMV.

share|improve this answer
up vote 1 down vote

This is my version, using select. Maybe it is possible to make it easier somehow.

def trnm_thr():                                                         # trnm_thr() sends commands to Arduino and is a multiplexed socket server
  global udp_clients
  udp_client = None

  try:
    while pySerial is not None:
      if not pySerial.writable():
        continue

      udp_read, udp_write, oob = select.select([udp_server] + udp_clients, [], [])

      for sock in udp_read: 
        if sock is udp_server: 
          client, addr = server.accept() 
          udp_clients.add(client) 
        else: 
          udp_msg = sock.recv(512) 
          if udp_msg: 
            try: 
              p = json.loads(udp_msg)                                     # parse JSON string from socket
            except (ValueError, KeyError, TypeError):
              logging.debug("JSON format error: " + udp_msg.strip() )
            else:
              make_command(p)
          else: 
            sock.close() 
            udp_clients.remove(sock)
  finally:
    for client in udp_clients: 
        client.close() 
    udp_server.close()
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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