Patch for UDP 7 segments bug

[japearson]

Hi Matthew,

We found a bug where com.serotonin.bacnet4j.npdu.ip.IpMessageControl.sendResponse(InetSocketAddress, Network, ConfirmedRequest, AcknowledgementService) was limiting the number of UDP packets to 7 segments instead of more than 64. We need to have at least 2000 BACnet Objects on a local device and it was complaining about too many segments.

I have created a patch for this, how should I give this to you?

For my own convenience I have converted your CVS repository to GIT, so if you like I could put the repository up on Github, then I could fork your repository and create pull request. Otherwise I can just upload a patch in the [url=http://sourceforge.net/tracker/?group_id=224576&atid=1062318]Patches section of sourceforge

Let me know what you prefer.

Cheers,

-Joel

[mlohbihler]

Not sure what you mean. This is the code that i have (lines 274 and 275):

            if (segmentsRequired > request.getMaxSegmentsAccepted() || segmentsRequired > 128)
                throw new BACnetException("Response too big to send to device; too many segments required");

BACnet4J will respond with up to 128 segments unless limited further by the max accepted by the client.

[japearson]

@mlohbihler said:

Not sure what you mean. This is the code that i have (lines 274 and 275):

           if (segmentsRequired > request.getMaxSegmentsAccepted() || segmentsRequired > 128)
               throw new BACnetException("Response too big to send to device; too many segments required");

BACnet4J will respond with up to 128 segments unless limited further by the max accepted by the client.

That is true, however request.getMaxSegmentsAccepted() is a maximum of number 7 and not 128.

Look at lines 240 - 258 of ConfirmedRequest, maxSegmentsAccepted is set directly from the byte queue, which works for everything except the one use of getMaxSegmentsAccepted. It needs to work exactly like maxApduLengthAccepted, which is the solution I came up with. I created an enum that maps between the integer/byte value and the actual meaning of that value.

    ConfirmedRequest(ServicesSupported servicesSupported, ByteQueue queue) throws BACnetException {
        byte b = queue.pop();
        segmentedMessage = (b & 8) != 0;
        moreFollows = (b & 4) != 0;
        segmentedResponseAccepted = (b & 2) != 0;

        b = queue.pop();
        maxSegmentsAccepted = (b & 0x70) >> 4;
        maxApduLengthAccepted = MaxApduLength.valueOf((byte) (b & 0xf));
        invokeId = queue.pop();
        if (segmentedMessage) {
            sequenceNumber = queue.popU1B();
            proposedWindowSize = queue.popU1B();
        }
        serviceChoice = queue.pop();
        serviceData = new ByteQueue(queue.popAll());

        ConfirmedRequestService.checkConfirmedRequestService(servicesSupported, serviceChoice);
    }

Lines 191-193 show that no conversion is taking place, it's still the original byte/integer value.

    public int getMaxSegmentsAccepted() {
        return maxSegmentsAccepted;
    }

[mlohbihler]

Ah, i see what you mean. The code is using the encoded value as the actual max segments. I've checked in a fix for this into the SF repo. I created an enum for the max segments like there is for the max APDU length. Thanks for pointing this out.

[japearson]

Thanks that now breaks the AnnexFEncodingTest though, I also wrote a test that tries to use 4000 objects as well, should I submit that as a patch for you?

[mlohbihler]

I noticed that too and committed a fix just after.

[mlohbihler]

Re test, sure, send it along.

[japearson]

Ok done: https://sourceforge.net/tracker/?func=detail&aid=3562579&group_id=224576&atid=1062318

[mlohbihler]

Patch has been applied. Thanks.

[japearson]

Hi Matthew,

You missed the MORE_THAN_64 segments in the valueOf function, which makes bacnet4j pretty unusable

This is the diff of what's missing:

diff --git a/src/com/serotonin/bacnet4j/enums/MaxSegments.java b/src/com/serotonin/bacnet4j/enums/MaxSegments.java
index 0845c74..362e210 100644
--- a/src/com/serotonin/bacnet4j/enums/MaxSegments.java
+++ b/src/com/serotonin/bacnet4j/enums/MaxSegments.java
@@ -67,6 +67,8 @@ public enum MaxSegments {
             return UP_TO_32;
         if (id == UP_TO_64.id)
             return UP_TO_64;
+        if (id == MORE_THAN_64.id)
+            return MORE_THAN_64;
 
         throw new IllegalArgumentException("Unknown id: " + id);
     }

Cheers,

-Joel

[mlohbihler]

Indeed. A fix has been checked in.