10
\$\begingroup\$

Push a button and an LED lights up1 and sends a signal to another computer that plays a song.

First project on the Raspberry pi, really simple but definitely fun and educational to implement. Also, second time using python and sockets so I'd especially appreciate input on anything I do inefficiently.2

  1. Mostly feedback to ensure that the program is running/button is active.
  2. I know it's straightforward, but this site has shown me that even the simplest things leave room to be executed much more elegantly.

SongServer.java:

import java.io.BufferedReader;
import java.io.File;
import java.io.IOException;
import java.io.InputStreamReader;
import java.io.PrintWriter;
import java.net.ServerSocket;
import java.net.Socket;
import java.util.Date;
import java.awt.Desktop;

public class SongServer {
    final static int PORT = 9001;

    public static void main(String[] args) {
        try(ServerSocket server = new ServerSocket(PORT)) {
            System.out.println(new Date() + "\nServer online.");

            while(true) {
                new Thread(new ClientHandler(server.accept())).start();
            }
        } catch (IOException ioe) {
            ioe.printStackTrace();
        }
    }

    private static class ClientHandler implements Runnable {
        private Socket socket;
        private BufferedReader in;
        private PrintWriter out;

        ClientHandler(Socket socket) {
            this.socket = socket;
            System.out.println("Client connected!");
        }

        @Override
        public void run() {
            try {
                in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
                out = new PrintWriter(socket.getOutputStream(), true);

                out.println("Connection success");
                while (true) {
                    String input = in.readLine();
                    if (input == null || input.trim().isEmpty()) {
                        continue;
                    }
                    System.out.println(input);

                    if (input.startsWith("play")) {
                        Desktop.getDesktop().open(new File("uptown_funk.mp3"));
                    }
                }
            } catch (Exception e) {
                System.out.println("Client disconnected!");
            }
        }
    }
}

funk_button.py:

import RPi.GPIO as GPIO
import socket

funk_socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
funk_socket.connect(("10.0.0.18", 9001))
sent = False

button = 17
led = 22

GPIO.setwarnings(False)
GPIO.setmode(GPIO.BCM)
GPIO.setup(led, GPIO.OUT)
GPIO.setup(button, GPIO.IN)

while True:
    if GPIO.input(button):
        GPIO.output(led, GPIO.LOW)
    else:
        GPIO.output(led, GPIO.HIGH)
        if not sent:
            funk_socket.send(bytearray("play\r\n", "utf-8"))
            sent = True
\$\endgroup\$
3
\$\begingroup\$

Keep trying with those resources

Seeing that the other answers mainly focused on the Python part, I'll focus on the Java part.


public static void main(String[] args) {
    try(ServerSocket server = new ServerSocket(PORT)) {

Good job using try-with-resources here. Try-with-resources is good because it automatically closes any resources declared in the resources part and frees up the system resources. It's very helpful.

However, you aren't using it in all of your code:

        try {
            in = new BufferedReader(new InputStreamReader(socket.getInputStream()));
            out = new PrintWriter(socket.getOutputStream(), true);

in and out are both resources. Therefore, you should be closing them after you are done with this whole try/catch part. This can be done easily by putting both in and out in the resources part of this try statement.


Why is this multi-threaded?

Your server spawns a new thread every time a client connections. All a client is going to really do is start playing a music file on the machine that is running the server.

This means that having multiple clients asking for the server to play the song locally would not make any sense because the machine would just have a bunch of audio files overlapping each other.

Instead, I recommend that you ignore any other client connections until the one you are focusing on is done. Perhaps you can somehow communicate to the client that it is busy working with another client at the time. And that leads to my next point...


Handling a greedy client

What if a client asks to play the song as it is still playing? The audio on the server machine would start overlapping. To handle this, I recommend one of these two options:

  1. Tell the client that it is busy playing a song (this is the continuation of the above point. Instead of blocking the client, you could just tell it that it was currently busy along with the rest of the clients).

  2. You could restart the song. Every time the server receives a message to play the song as it is currently playing, the server could just stop the song and restart it from the beginning.


Saving the file

Desktop.getDesktop().open(new File("uptown_funk.mp3"));

Each time a client wants this computer to play the music, it creates a new File. I'm not sure how much of a speed impact this has, but it may be a lot faster to just create the file once, store it someplace safe, and then open it whenever it is needed.

That way, you aren't creating a new File every time.


Exception handling

} catch (Exception e) {

You've probably heard this many times before, but just handling the normal Exception is a bad practice; you should explicitly handle the specific exceptions that could occur.


GPIO practices

Clean up, clean up...

At the end of your program, you need to clean up the GPIO. Otherwise, system resources are not freed and you are at risk of accidentally shorting out the Pi's pins.

At the end of your program, simple insert this:

GPIO.cleanup()

Seeing that your client is constantly running and watching the pins and has no specific end point, you can use the atexit library to help you:

import atexit
...
atexit.register(GPIO.cleanup)

Use GPIO.BOARD

Right now, when setting up your pins, you use GPIO.BCM:

GPIO.setmode(GPIO.BCM)

GPIO.BCM is very system dependent. Someone using this code on another machine would have to modify the code just to change which pins its going to use so the code works with their machine.

Using GPIO.BOARD, you can specify which pin to use and that will be the pin to use for all models of the Raspberry Pi. That is much more portable.


Conclusion

GPIO documention.

\$\endgroup\$
3
\$\begingroup\$

Looks good as a proof of concept. I recommend to modularize python code, that is

  • Make socket interface into a function

  • Make GPIO interface into a function

  • Have a

    if __name__ == '__main__'
    

    clause to call them (directly or via a main) function.

I would also handle failures to connect (retry maybe?); a total fun is to implement service discovery via pybonjour or zeroconf.

\$\endgroup\$
2
\$\begingroup\$

Just a brief note about these constants:

button = 17
led = 22

If they're constants they should be in UPPER_SNAKE_CASE to denote them as such in Python style. Also, where do the numbers come from? I'm not sure if it's a Raspberri Pi thing but where possible unclear arbitrary numbers benefit from a brief comment explaining their meaning.

\$\endgroup\$
  • \$\begingroup\$ They refer to pin numbers; might be worth commenting if it's not clear that they're a pin number, otherwise possibly useful to note pin's features/location since the numbering is weird in some numbering schemes. \$\endgroup\$ – hexafraction Oct 13 '15 at 10:26

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.