Testing: retry problems within bacnet4j

[robert bouwens]

uses case: unplug the network cable and re-connect and watch what happens. before this 'action' you make sure that there will be bacnet traffic.
no traffic no fun :shock:
output:

com.serotonin.bacnet4j.exception.BACnetTimeoutException: Timeout while waiting for segment part: invokeId=37, sequenceId=1
at com.serotonin.bacnet4j.npdu.ip.IpMessageControl.receiveSegmented(IpMessageControl.java:710)
at com.serotonin.bacnet4j.npdu.ip.IpMessageControl.waitForAck(IpMessageControl.java:680)
at com.serotonin.bacnet4j.npdu.ip.IpMessageControl.send(IpMessageControl.java:294)
at com.serotonin.bacnet4j.npdu.ip.IpMessageControl.send(IpMessageControl.java:238)
at com.serotonin.bacnet4j.LocalDevice.send(LocalDevice.java:425)
at com.serotonin.bacnet4j.LocalDevice.send(LocalDevice.java:415)
at com.serotonin.bacnet4j.LocalDevice.send(LocalDevice.java:408)
at com.sauter_controls.jbacnet.debug.Perfom_Action4Debug.perfom(Perfom_Action4Debug.java:247)
at com.sauter_controls.jbacnet.debug.Perfom_Action4Debug.<init>(Perfom_Action4Debug.java:84)
at com.sauter_controls.jbacnet.debug.Debug4Sauter$1$3.run(Debug4Sauter.java:209)
at java.lang.Thread.run(Thread.java:619) 

the regular retry loop, no segmentation in use, seems to work, but not with segmented answers.
the timeout exception should be named differently because the meaning is: not all segments received. could be a timeout because a modern pc is way too fast for a few devices ... but in my case a hard disconnect and reconnect within the given segment timeouts.

robert

[robert bouwens]

this is actually my first project dealing with java.
in the past i learned that throwing exceptions is expensive when doing the stack unwinding.
i'm happy - one throw() less :)

in the procedure:

    private AckAPDU send(Key key, int timeout, APDU[] apdu) throws BACnetException {
        byte[][] data = new byte[apdu.length][];
        for (int i = 0; i < data.length; i++)
            data* = createMessageData(apdu*, false, key.getNetwork());

        AckAPDU response = null;
        int attempts = retries + 1;

        // The retry loop.
        while (true) {
            for (int i = 0; i < data.length; i++)
                sendImpl(data*, key.getPeer());

            response = waitForAck(key, timeout, false);
            if (response == null) {
                attempts--;

                if (attempts > 0)
                    // Try again
                    continue;

                // Give up
                throw new BACnetTimeoutException("Timeout while waiting for response for id " + key.getInvokeId());
            }

            // We got the response
            break;
        }

        return response;
    }

you see the retry loop - which is correct.
let's change the two procedure to make the upper code with the retry loop working.
the next procedure should return null when segments are missed.

    private AckAPDU waitForAck(Key key, long timeout, boolean throwTimeout) throws BACnetException {
        AckAPDU ack = waitingRoom.getAck(key, timeout, throwTimeout);

        if (!(ack instanceof ComplexACK))
            return ack;

        ComplexACK firstPart = (ComplexACK) ack;
        if (firstPart.isSegmentedMessage())
        {
            if ( false == receiveSegmented(key, firstPart))
            {
            	return null;
            }
        }

        firstPart.parseServiceData();
        return firstPart;
    }

in case the segment extracting fails it returns null and the caller knows: nothing received and iterates the retry loop.

    private boolean receiveSegmented(Key key, Segmentable firstPart) throws BACnetException {
        boolean server = !key.isFromServer();
        InetSocketAddress peer = key.getPeer();
        byte id = firstPart.getInvokeId();
        int window = firstPart.getProposedWindowSize();
        int lastSeq = firstPart.getSequenceNumber() & 0xff;
        System.out.println("Receiving segmented: window="+ window);

        // Send a segment ack. Go with whatever window size was proposed.
        sendImpl(new SegmentACK(false, server, id, lastSeq, window, true), false, peer, key.getNetwork());

        // The loop for receiving windows of request parts.
        Segmentable segment;
        SegmentWindow segmentWindow = new SegmentWindow(window, lastSeq + 1);
        while (true) {
            segment = segmentWindow.getSegment(lastSeq + 1);
            if (segment == null) {
                // Wait for the next part of the message to arrive.
                segment = waitingRoom.getSegmentable(key, segTimeout * 4, false);

                if (segment == null) {
                	// jose bitsch's test - do not throw an exception
                    // We timed out waiting for a segment.
                    if (segmentWindow.isEmpty())
                    {
                        // We didn't receive anything -->NO!!! do not throw a timeout exception.
                    	return false;
                    	// throwing an exception is wrong because the retry loop
//                        throw new BACnetTimeoutException("Timeout while waiting for segment part: invokeId=" + id
//                                + ", sequenceId=" + (lastSeq + 1));
                    }

                    // Return a NAK with the last sequence id received in order and start over.
                   System.out.println("Send NAK: lastSeq="+ lastSeq);
                   sendImpl(new SegmentACK(true, server, id, lastSeq, window, true), false, peer, key.getNetwork());
                    segmentWindow.clear(lastSeq + 1);
                }
                else if (segmentWindow.fitsInWindow(segment))
                    segmentWindow.setSegment(segment);

                continue;
            }

            // We have the required segment. Append the part to the first part.
            System.out.println("Received segment "+ segment.getSequenceNumber());
            firstPart.appendServiceData(segment.getServiceData());
            lastSeq++;

            // Do we need to send an ack?
            if (!segment.isMoreFollows() || segmentWindow.isLastSegment(lastSeq)) {
                // Return an acknowledgement
                sendImpl(new SegmentACK(false, server, id, lastSeq, window, segment.isMoreFollows()), false, peer, key
                        .getNetwork());
                segmentWindow.clear(lastSeq + 1);
            }

            if (!segment.isMoreFollows())
                // We're done.
                break;
        }
        System.out.println("Finished receiving segmented");
        return true;
    }

the receivesegmented returns false instead of throwing an exception when not catching all segments.

done!

robert

[mlohbihler]

Throwing exceptions is fine in exception cases. What is to be avoided is throwing exceptions in normal cases. I'd say that a segment timeout should be more of an exception case, and so it is ok to throw. If segment timeouts are common, you need a better network.