Blocking and async UDP communication with an IR camera

Question

I'm writing a Java application to control a custom IR research camera that will be installed on an aircraft. The camera connects to the the computer using UDP sockets via a direct LAN cable. The camera does not initiate any communications; it just replies to commands, 5-character strings, sent to it (the camera code is in C BTW). There are about 16 commands most of which reply more or less instantly with the string ACKNO if the command is recognized, 3 reply with very short data strings, and the last replies with 480 packets of 640 bytes: the image data.

My intent is to send a command and either get the reply buffer(s) or a time out. It must be blocking.

I last worked on this code about 5 months ago and then had to put it on hold while I wrote the interface for the user and the camera was still being built. I'm self taught in Java (my title is Ecologist). I've included code for sending a command and waiting for an ACKNO or time out response. It appears complicated and somewhat verbose to me so I'd like someone else to look at it and comment it or point me in a better direction. I'm using Java 8.

How it might be used in a method:

if (!waitForACK(waitMS, filter).isSuccess) { 
    // do something
    return "Failed to receive ACK.";
}

Top code:

private SendAndWaitForACK sendAndWaitForACK = null;
sendAndWaitForACK = new SendAndWaitForACK();

public Result waitForACK(int waitMS, CameraCommands cmd) {
    result.isSuccess = false;
    result.dataBytebuffer = ackByteBuffer;
    sendAndWaitForACK.setCommand(cmd); // set the comand
    Future<?> future = executor.submit(sendAndWaitForACK);
    try {
        result = (Result) future.get(waitMS, TimeUnit.MILLISECONDS);
    } catch (Exception e) {

    } finally {

    }

    return result;
}


private class SendAndWaitForACK implements Callable<Result> {
    private CameraCommands cmd;

    @Override
    public Result call() throws Exception {
        // does not return on timeout
        result.dataBytebuffer = postCommand(cmd);
        result.isSuccess = true;
        return result;
    }

    /**
     * setCommand must be called prior to use.
     * 
     * @param cmd
     *            : CameraCommands
     */
    public void setCommand(CameraCommands cmd) {
        this.cmd = cmd;
    }
}    

This is the method that actually deals with the socket.

/**
 * postCommand configures the ackByteBuffer prior to writing it to the
 * camera. It is invoked by postCommand call method.
 * 
 * @return ackByteBuffer : tByteBuffer The image.
 * @throws IOException
 *           
 */
    private ByteBuffer postCommand(CameraCommands command) throws IOException {
    ackByteBuffer.rewind();
    writeCommand(command); // to UDP Socket
    // blocking read reply from UDP Socket
    datagramChannel.read(ackByteBuffer);
    return ackByteBuffer;

}

Answer 1

Constructors and Generics

It is customary to use constructors to set values for instances in Java. This code:

private class SendAndWaitForACK implements Callable<Result> {
    private CameraCommands cmd;

    @Override
    public Result call() throws Exception {
        // does not return on timeout
        result.dataBytebuffer = postCommand(cmd);
        result.isSuccess = true;
        return result;
    }

    /**
     * setCommand must be called prior to use.
     * 
     * @param cmd
     *            : CameraCommands
     */
    public void setCommand(CameraCommands cmd) {
        this.cmd = cmd;
    }
}

Would more typically be written as:

private static class SendAndWaitForACK implements Callable<Result> {
    private final CameraCommands cmd;

    SendAndWaitForACK(CameraCommands cmd) {
        this.cmd = cmd;
    }

    @Override
    public Result call() throws Exception {
        // does not return on timeout
        result.dataBytebuffer = postCommand(cmd);
        result.isSuccess = true;
        return result;
    }

}

Note that I have replaced the setter with the constructor. I have also made the class static, which saves a tiny amount of memory, but it is good practice because it makes Java GC easier, and has some other benefits.

You will need to change the way you use that class, and your calling code will change from:

Future<?> future = executor.submit(sendAndWaitForACK);

to

Future<Result> future = executor.submit(new SendAndWaitForACK(cmd));

Note, the SendAndWaitForACK implements Callable<Result>, so it will produce a Future<Result> when submitted. As a result, I have changed the generic type of the Future to Future<Result>. This also means that you no longer need the explicit cast and it can simply be:

result = future.get(waitMS, TimeUnit.MILLISECONDS);

Network

Note that your class does not do a clean handling of the network. When you submit a Callable you start a thread, and you run the Callable on that thread. Then, in your code, you set a timeout to wait for the Callable to complete. If the callable does not complete within that specified time, you get a timeout. This does not stop the Callable from running in the other thread. All that happens is that the callable keeps running, and eventually it gets something back from the camera... and, what it gets back, may be something belonging to another command sent later.

Fundamentally, your protocol/communication is a little flawed in this regard. As long as things work as expected, you won't notice it, but, if things go screwy, then your code will make the screwy situation even more screwy.

I strongly recommend, in cases like this, to not use threading at all. At least, don't use multiple threads to communicate over a single socket.

One of the benefits of UDP is the easy way to build simple protocols on top of it.

If this were me, I would:

• have a single class that manages the socket, and protocol.
• have a thread on that class that checks periodically to see that the camera is there, and working (perhaps every second or so, a 'heartbeat').
• that class has a ConcurrentBlockingQueue on the input side that takes a SendAndWaitForACK
• (I would probably make the 'heartbeat' one of these SendAndWaitForACK instances)
• When a callable is queued, the class will give that command a unique number, and it will 'transmit' that command every 10 or so milliseconds, until it gets a response, or until the timeout happens
• when it gets the response (or timeout), it will notify the caller.

I would make the camera-side of the system return the unique ID for the command as part of the response. That way you can make sure you are getting the right data back.

Doing things this way means that:

• only one thread accesses the network socket.
• only one command can be 'in transit' at any one time.
• you do not need to worry about threading or concurrency on the socket.
• you always have a 'state' of the camera.

Obviously, that recommendation is very different to what you have at the moment.... but, bottom line, is that almost all UDP-based communication channels will have some form of simple protocol established above the raw network layer.