Remote control receiver for robot control

Question

I have built an RC car and attached a Raspberry Pi to it - in order to control it, I wrote some Java code to listen to commands over the network and make the car move accordingly.

 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
 import java.io.PrintWriter;
 public class Reciever { // note that Receiver is misspelled in this line and throughout the code
 static Process p;
 static PrintWriter out;
 static BufferedReader input;
 static BufferedReader error;
 public static void main(String[] args) {
 String[] ex = {
 "/bin/sh", "-c", "bash"
 };
 try {
 p = Runtime.getRuntime().exec(ex);
 out = new PrintWriter(p.getOutputStream());
 for (int i = 0; i < 4; ++i) {
 out.println("gpio mode " + i + " out");
 }
 input = new BufferedReader(
 new InputStreamReader(p.getInputStream()));
 error = new BufferedReader(
 new InputStreamReader(p.getErrorStream()));
 } catch (IOException e) {
 e.printStackTrace();
 }
 System.out.println("GPIOs setup");
 new Network();
 }
 }

import java.net.ServerSocket;
import java.net.Socket;
public class Network {
 private static ClientListener clientListener;
 private static Thread clientListenerThread;
 Network() {
 clientListener = new ClientListener();
 clientListenerThread = new Thread(clientListener);
 clientListenerThread.start();
 }
 class ClientListener implements Runnable {
 ServerSocket serverSocket = null;
 ClientListener() {}
 @Override
 public void run() {
 try {
 serverSocket = new ServerSocket(9090);
 System.out.println("running network listener");
 } catch (Exception e) {
 e.printStackTrace();
 }
 while (true) {
 try {
 serverSocket.setSoTimeout(1000);
 Socket clientSocket = serverSocket.accept();
 new Thread(new Controller(clientSocket)).start();
 } catch (Exception e) {
 // e.printStackTrace(); // so timeout errors
 }
 }
 }
 }
}

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
import java.net.Socket;
import java.nio.charset.StandardCharsets;
public class Controller implements Runnable {
 Socket s;
 BufferedReader in ;
 public Controller(Socket clientSocket) {
 s = clientSocket;
 try { in = new BufferedReader(new InputStreamReader(s.getInputStream(),
 StandardCharsets.UTF_8.newDecoder()));
 } catch (Exception e) {}
 }
 @Override
 public void run() {
 while (!s.isClosed()) {
 String m = "";
 try {
 while ((m = in .readLine()) != null) {
 System.out.println("recieved " + m);
 switch (m) {
 case "f":
 Reciever.out.println("gpio write 0 1");
 Reciever.out.println("gpio write 1 0");
 Reciever.out.println("gpio write 2 1");
 Reciever.out.println("gpio write 3 0");
 System.out.println("set pins to forward");
 break;
 case "b":
 Reciever.out.println("gpio write 0 0");
 Reciever.out.println("gpio write 1 1");
 Reciever.out.println("gpio write 2 0");
 Reciever.out.println("gpio write 3 1");
 break;
 case "l":
 Reciever.out.println("gpio write 0 0");
 Reciever.out.println("gpio write 1 1");
 Reciever.out.println("gpio write 2 1");
 Reciever.out.println("gpio write 3 0");
 break;
 case "r":
 Reciever.out.println("gpio write 0 1");
 Reciever.out.println("gpio write 1 0");
 Reciever.out.println("gpio write 2 0");
 Reciever.out.println("gpio write 3 1");
 break;
 case "s":
 Reciever.out.println("gpio write 0 0");
 Reciever.out.println("gpio write 1 0");
 Reciever.out.println("gpio write 2 0");
 Reciever.out.println("gpio write 3 0");
 break;
 }
 Reciever.out.flush();
 }
 } catch (IOException e) {
 e.printStackTrace();
 }
 }
 }
}

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStream;
import java.io.InputStreamReader;
import java.io.OutputStreamWriter;
import java.io.PrintWriter;
import java.net.Socket;
import java.nio.charset.StandardCharsets;
import java.util.Scanner;
public class Test {
 static Socket s;
 static PrintWriter output;
 static InputStream input;
 public static void main(String[] args) {
 String url = "192.168.123.14";
 try {
 s = new Socket(url, 9090);
 output = new PrintWriter(new OutputStreamWriter(
 s.getOutputStream(), StandardCharsets.UTF_8), true);
 input = s.getInputStream();
 new Thread(new Writer(s)).start();
 Scanner in = new Scanner(System. in );
 String consoleInput = "";
 while (!s.isClosed()) {
 consoleInput = in .nextLine();
 output.write(consoleInput + '\n');
 output.flush();
 System.out.println("sent " + consoleInput);
 }
 } catch (Exception e) {
 e.printStackTrace();
 }
 }
 static class Writer implements Runnable {
 String str;
 Socket sock;
 BufferedReader input;
 public Writer(Socket s) {
 sock = s;
 try {
 input = new BufferedReader(
 new InputStreamReader(s.getInputStream(),
 StandardCharsets.UTF_8.newDecoder()));
 } catch (IOException e) {
 e.printStackTrace();
 }
 }
 @Override
 public void run() {
 try {
 while ((str = input.readLine()) != null) {
 System.out.println(str);
 }
 } catch (Exception e) {};
 }
 }
}

To control it, it receives the messages "f" (forward), "b" (backwards), "r" (right), "l" (left), and "s" (stop), and sets the GPIO pins that control the electric motors accordingly (it has two motors, one on that controls the left wheels (pins 0 and 1) and one that controls the right wheels (pins 2 and 3)

It relies on the wiring Pi utility for controlling the GPIO pins, and communicates with it via a shell environment. There isn't much of a practical point for writing it in Java (and having to deal with the shell stuff vs importing a library in C for the functions), but the next part is to write an android app that will send the relevant messages, and since that would be written in Java, I felt like writing this in Java as well (not that there couldn't be inter-language communication over the network..)

At some point I might like to move to a Bluetooth communication instead, so that it doesn't rely on the two devices being on the same LAN.

Although it works as-is right now, there seems to be a leak of some sort as it gradually becomes less responsive (not sure of if it's over time or over lots of commands), but the quick & dirty way of fixing it was to just re-launch it via crontab on the Pi every so often.

I don't know of a good way for testing this outside of on a raspberry pi with the "wiring pi" utility installed, as the outputs are physical pins turning on and off.

• Legato: This sounds like a fun/awesome project. I hope you get some fine answers.
• Ismael Miguel: You misspelled the title
• user2813274: @IsmaelMiguel fixed the title, but it seems I misspelled it all over my code as well.. time for a re-factor!
• Ismael Miguel: @user2813274 You should warn the other users about it
• user2813274: @IsmaelMiguel added a comment in the original code explaining it

Answer 1

Most of the properties of your classes are missing scope identifiers.

For example, these properties of Receiver

static Process p;
static PrintWriter out;
static BufferedReader input;
static BufferedReader error;

should have scope identifiers. Judging by your use for these properties, I'd say you should give out a public modifier, and the rest private modifiers.

---

Instead of giving the messages single letter string names, I recommend creating an enumerated type that stores the types of messages.

Here is what I came up with:

public enum MOVEMENT {
 FORWARD([1,0,1,0]), 
 BACKWARD([0,1,0,1]),
 RIGHT([1,0,0,1]), 
 LEFT([0,1,1,0]), 
 STOP([0,0,0,0])
 public final int[] signal;
 private MOVEMENT(signal) {
 this.signal = signal;
 }
}

Notice something about signal? The 4 numbers in signal are the second number of the 4 times you are logging to Reciever. I didn't include the first number since the first number was the same for each option.

Now, to further simplify the switch statement, we are going to want to create a function that sends the 4 signals over to Reciever that uses our new enum.

Here is what I came up with:

private void sendToReciever(MOVEMENT movement) {
 for(int i = 0; i < movement.signal.length; i++) {
 Reciever.out.println("gpio write 0 " + movement.signal[i]);
 }
}

Now, your switch statement becomes:

switch (m) {
 case "f":
 sendToReciever(MOVEMENT.FORWARD);
 break;
 case "b":
 sendToReciever(MOVEMENT.BACKWARDS);
 break;
 case "l":
 sendToReciever(MOVEMENT.LEFT);
 break;
 case "r":
 sendToReciever(MOVEMENT.RIGHT);
 break;
 case "s":
 sendToReciever(MOVEMENT.STOP);
 break;
}

---

From .run of ClientListener of Network:

try {
 serverSocket = new ServerSocket(9090);
 System.out.println("running network listener");
} catch (Exception e) {
 e.printStackTrace();
}
while (true) {

1. You shouldn't just be catching exception.

Catch a more specific exception. Looking at the documentation for ServerSocket, the exception that would be raised for how you are opening up the ServerSocket would be an IOException.

2. You should exit the function.

If there was some sort of error while making ServerSocket, the rest of your code isn't going to work. At this point, you just exit the function and return something like -1 to symoblize an error.

---

Add JavaDoc to your functions and classes.

---

try {
 serverSocket.setSoTimeout(1000);
 Socket clientSocket = serverSocket.accept();
 new Thread(new Controller(clientSocket)).start();
} catch (Exception e) {
 // e.printStackTrace(); // so timeout errors
}

1. You should move serverSocket.setSoTimeout(1000); outside of the try statement because you don't need to redefine the timeout time every loop.

2. There are two kinds of errors that could happen here: a timeout error, and an IOException from serverSocket.accept.

---

My guess on why this gets sluggish is because you have an infinite loop in Network that creates a new thread every loop.

• user2813274: Any ideas as to why it would become sluggish after a while?
• SirPython: @user2813274 I edited my answer to include more information.
• user2813274: Ah, that seems like an obvious reason that I missed, thanks!