patternjavaMinor

Keeping track of byte count in a binary protocol handler

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

keepingprotocoltrackbytebinarycounthandler

Problem

I am putting together a fairly simple server that listens for a connection then creates this thread - textbook Java code - then accepts data on that connection.

I am following a protocol that the manufacturer has laid out for start of message (0xfd) and EOM (0xfe) as below. I then simply populate a byte array with using a bytecounter.

I think this is the simplest doing so, and it seems to work fine. Is there any possible problem with taking this approach? I want to be sure using a bytecounter with an array is acceptable. I can't see anyway that bytecounter could get out of sync or anything like that. I like to keeps things simple. Is there anything wrong with this code with regard to stuffing the inbyte[] array?

```
public void run() {

try {

boolean socketalive = true;

MSG_ID = 0;

disIn = new DataInputStream( in );
disOut = new DataOutputStream ( out );

String msg = null;

StringBuilder hexforlog;

byte[] c ;

int i = 0;
int bytecounter = 0;

byte[] inbyte ;

while( true ){

i = 0;
bytecounter = 0;

inbyte = new byte[1024];

byte b ;

try{

/ read from network until end of message byte is received /
while( ( b = disIn.readByte() ) != (byte)0xfe ){

/ checking for start of message /
if( b == (byte)0xfd ){

inbyte = new byte[1024];
inbyte[0] = b;
bytecounter = 1;

}
else {

inbyte[bytecounter] = b;
bytecounter++;

}

}

}catch ( java.io.EOFException ioef ){
System.out.println("EOF recieved bytes" );
break;
}

/test for link verification /

if( bytecounter == 15 ){

byte SOURCEbyte = inbyte[2];
byte DESTbyte = inbyte[3];

/ swap bytes per protocol /

Solution

Some variables, such as socketAlive and hexforlog, aren't being used.

Another quirk I noticed is that you don't really take advantage of the capabilities of DataInputStream. If you just want to read a byte at a time, any InputStream can accomplish that. DataInputStream gives you the ability, for example, to read two bytes and interpret it as an unsigned 16-bit number using .readUnsignedShort(). I'm guessing that that is what your twoBytesToInt() does. Hopefully, the protocol uses big endian numbers so that you can take advantage of .readUnsignedShort().

Your loop is structured such that any 0xfe byte in the input stream will be treated as the end of a packet. That means that no 0xfe byte can ever appear in the payload of a message. Most binary protocols are not designed that way — you should obey the message length fields instead.

Anyway, your code is one long function! It desperately needs some kind of abstraction. bytecounter and all of the concerns surrounding it can go away! The code proposed below compiles, but obviously I can't test it.

Packet.java: This is just a "dumb" struct.

public class Packet {
    public byte som,
                packetClass,    // "class" is a Java keyword
                source,
                dest,
                msgPrgNumber,
                spare1,
                spare2,
                spare3;
    public int  length,         // 2-byte unsigned short
                numMsgs;        // 2-byte unsigned short

    public static class Message {
        public int msgId,
                   length;
        public byte[] payload;
    }
}

PacketInputStream.java: This constructs Packets from the data stream.

import java.io.*;

public class PacketInputStream {

    //////////////////////////////////////////////////////////////////////

    public static class MalformedPacketException extends Exception {
        public MalformedPacketException(String errMsg) {
            super(errMsg);
        }
    }

    //////////////////////////////////////////////////////////////////////

    private DataInputStream in;
    private Packet packet;
    private int messageNumber;

    public PacketInputStream(DataInputStream in) {
        this.in = in;
    }

    /**
     * Reads a 12-byte packet header.
     */
    public Packet readHeader() throws IOException {
        this.packet = new Packet();
        this.messageNumber = 0;
        do {
            this.packet.som       = in.readByte();
        } while (this.packet.som != 0xfd);
        this.packet.packetClass   = in.readByte();
        this.packet.source        = in.readByte();
        this.packet.dest          = in.readByte();
        this.packet.msgPrgNumber  = in.readByte();
        this.packet.spare1        = in.readByte();
        this.packet.spare2        = in.readByte();
        this.packet.spare3        = in.readByte();
        this.packet.length        = in.readUnsignedShort();
        this.packet.numMsgs       = in.readUnsignedShort();
        return this.packet;
    }

    public boolean hasMoreMessages() {
        return this.packet != null && this.messageNumber < this.packet.numMsgs;
    }

    public int getMessageNumber() {
        return this.messageNumber;
    }

    public Packet.Message readMessage() throws IOException {
        Packet.Message msg = new Packet.Message();
        msg.msgId   = in.readUnsignedShort();
        msg.length  = in.readUnsignedShort();
        msg.payload = new byte[msg.length];
        in.read(msg.payload);
        return msg;
    }

    /**
     * Requires the next input byte to be the end-of-packet marker.
     */
    public void readEndOfPacket() throws IOException, MalformedPacketException {
        byte b;
        if ((b = in.readByte()) != 0xfe) {
            throw new MalformedPacketException(String.format("Expected 0xfe, got 0x%02x", b));
        }
    }
}

PacketOutputStream is left as an exercise for the reader.

Consumer:

```
private void processMessage(Packet.Message msg) {
switch (msg.msgId) {
case 50:
this.processMessage50(msg);
break;
case 70:
this.processMessage70(msg);
break;
}
}

public void run() {
try (DataInputStream dataIn = new DataInputStream(in);
DataOutputStream dataOut = new DataOutputStream(out)) {
PacketInputStream packIn = new PacketInputStream(dataIn);
PacketOutputStream packOut = new PacketOutputStream(dataOut);

while (true) {
Packet p = packIn.readHeader();

// Read one message before writing the acknowledgement,
// because that's what the original code did. You
// could simplify this by acknowledging immediately
// after reading the header instead.
Packet.Message msg = packIn.readMessage();
packOut.writeAck(p);
this.processMessage(msg);

while (packIn.hasMoreMessages()) {
this.process

Code Snippets

public class Packet {
    public byte som,
                packetClass,    // "class" is a Java keyword
                source,
                dest,
                msgPrgNumber,
                spare1,
                spare2,
                spare3;
    public int  length,         // 2-byte unsigned short
                numMsgs;        // 2-byte unsigned short

    public static class Message {
        public int msgId,
                   length;
        public byte[] payload;
    }
}

import java.io.*;

public class PacketInputStream {

    //////////////////////////////////////////////////////////////////////

    public static class MalformedPacketException extends Exception {
        public MalformedPacketException(String errMsg) {
            super(errMsg);
        }
    }

    //////////////////////////////////////////////////////////////////////

    private DataInputStream in;
    private Packet packet;
    private int messageNumber;

    public PacketInputStream(DataInputStream in) {
        this.in = in;
    }

    /**
     * Reads a 12-byte packet header.
     */
    public Packet readHeader() throws IOException {
        this.packet = new Packet();
        this.messageNumber = 0;
        do {
            this.packet.som       = in.readByte();
        } while (this.packet.som != 0xfd);
        this.packet.packetClass   = in.readByte();
        this.packet.source        = in.readByte();
        this.packet.dest          = in.readByte();
        this.packet.msgPrgNumber  = in.readByte();
        this.packet.spare1        = in.readByte();
        this.packet.spare2        = in.readByte();
        this.packet.spare3        = in.readByte();
        this.packet.length        = in.readUnsignedShort();
        this.packet.numMsgs       = in.readUnsignedShort();
        return this.packet;
    }

    public boolean hasMoreMessages() {
        return this.packet != null && this.messageNumber < this.packet.numMsgs;
    }

    public int getMessageNumber() {
        return this.messageNumber;
    }

    public Packet.Message readMessage() throws IOException {
        Packet.Message msg = new Packet.Message();
        msg.msgId   = in.readUnsignedShort();
        msg.length  = in.readUnsignedShort();
        msg.payload = new byte[msg.length];
        in.read(msg.payload);
        return msg;
    }

    /**
     * Requires the next input byte to be the end-of-packet marker.
     */
    public void readEndOfPacket() throws IOException, MalformedPacketException {
        byte b;
        if ((b = in.readByte()) != 0xfe) {
            throw new MalformedPacketException(String.format("Expected 0xfe, got 0x%02x", b));
        }
    }
}

private void processMessage(Packet.Message msg) {
    switch (msg.msgId) {
      case 50:
        this.processMessage50(msg);
        break;
      case 70:
        this.processMessage70(msg);
        break;
    }
}

public void run() {
    try (DataInputStream  dataIn   = new DataInputStream(in);
         DataOutputStream dataOut  = new DataOutputStream(out)) {
        PacketInputStream  packIn  = new PacketInputStream(dataIn);
        PacketOutputStream packOut = new PacketOutputStream(dataOut);

        while (true) {
            Packet p = packIn.readHeader();

            // Read one message before writing the acknowledgement,
            // because that's what the original code did.  You
            // could simplify this by acknowledging immediately
            // after reading the header instead.
            Packet.Message msg = packIn.readMessage();
            packOut.writeAck(p);
            this.processMessage(msg);

            while (packIn.hasMoreMessages()) {
                this.processMessage(packIn.readMessage());
            }
            packIn.readEndOfPacket();
        }
    } catch (PacketInputStream.MalformedPacketException mpe) {
        mpe.printStackTrace();
    } catch (IOException ioe) {
        ioe.printStackTrace();
    }
}

Context

StackExchange Code Review Q#47396, answer score: 4

Revisions (0)

No revisions yet.