fix: segmented response conformance and routed I-Am replies#198
Merged
Conversation
…mented responses The segment encoder opened its encode window at offset + GetMaxApdu() regardless of the physical transport buffer (default maxPayload 1472 vs the 1476-octet B/IP APDU), throwing IndexOutOfRangeException on the first segmented response. It also sized segments only by the local transport limit, ignoring the requester max-APDU-length-accepted that ASHRAE 135 5.2.1.2 requires a responder to honor. Segments are now sized to the smallest of the transport limit, the physical buffer (5.2.1.2 (a), a local matter) and the requester max-APDU (5.2.1.2 (c)). The requester value is captured automatically when GetSegmentBuffer is called inside a request event handler, or can be passed explicitly via the new GetSegmentBuffer(maxSegments, requesterMaxAdpu) overload. EncodeBuffer flags NotEnoughBuffer instead of writing past the end of a fixed buffer.
Bits 1-0 of the ComplexACK type octet are reserved and shall be set to zero (ASHRAE 135 20.1.5); the server flag exists only in SegmentACK PDUs. Strict peers could discard segmented responses carrying the stray bit. This aligns the wire format with YABE and bacnet-stack.
Iam() used the receiver address as both the NPDU destination and the datalink destination, so a reply to a Who-Is forwarded by a BACnet router was delivered to the router itself with no routing information and never reached the requester. ASHRAE 135 16.10.4 requires the I-Am response to be sent such that the originator of the Who-Is receives it. When the receiver address carries a RoutedSource (SNET/SADR of the forwarded request), the I-Am is now NPDU-addressed to that original source network/address and the frame goes back through the router that forwarded the request - the same behaviour as the reference bacnet-stack implementation (Send_I_Am_Unicast, "must direct back to src->net to meet BTL tests").
BacnetIpUdpProtocolTransport accepts a maxApdu constructor parameter to lower the advertised and sent APDU size. The 1476-octet default predates UDP/IP overhead, so full-size frames exceed a 1500-byte MTU and rely on IP fragmentation; MAX_APDU1024 keeps every frame, including segmented responses, below the MTU (ASHRAE 135 5.2.1.2 (b): the APDU is limited by what the internetwork can convey). MaxAdpuLength drives both what I-Am advertises and how responses are segmented, so the endpoint stays self-consistent.
This was referenced Jul 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Four conformance/robustness fixes in the segmented-response encoder, I-Am addressing and the BACnet/IP transport, found while interop-testing this library against bacnet-stack 1.5.0 through a BACnet router (two isolated BACnet/IP networks, DNET 1 ↔ DNET 2).
Changes
fix): the segment encoder opened its window atoffset + GetMaxApdu()regardless of the physical transport buffer (defaultmaxPayload: 1472vs the 1476-octet B/IP APDU), throwingIndexOutOfRangeExceptionon the first segmented response — and sized segments only by the local transport limit, ignoring the requester's max-APDU-length-accepted. Per ASHRAE 135 §5.2.1.2 the APDU shall be the smallest of the local limit (a), what the internetwork conveys (b) and what the peer accepts (c); for ComplexACKs (c) comes from the request header. Segments now honor all of it; the requester value is captured automatically whenGetSegmentBuffer(maxSegments)is called inside a request event handler (thread-static, so existing handlers become conformant with zero changes), or passed explicitly via the newGetSegmentBuffer(maxSegments, requesterMaxAdpu)overload.EncodeBufferadditionally reportsNotEnoughBufferinstead of writing out of bounds.fix): segmented ComplexACKs carried theSERVERflag in the PDU type octet. §20.1.5:reserved [3] Unsigned (0..3) -- shall be set to zero; the server flag exists only in SegmentACK PDUs. YABE and bacnet-stack both encode these bits as zero. (Checked all published addenda through 135-2024cu — the bits remain unclaimed, so clearing them is also forward-compatible.)fix):Iam()used the receiver address as both NPDU destination and datalink destination, so a reply to a Who-Is forwarded by a router died at the router. §16.10.4: the I-Am "shall be sent in such a manner that the BACnet-user that sent the Who-Is will receive the resulting I-Am". With aRoutedSourceon the receiver, the I-Am is now NPDU-addressed to the original source and sent back through the router — the same behaviour as bacnet-stack's BTL-testedSend_I_Am_Unicast.new):BacnetIpUdpProtocolTransportgets amaxApduconstructor parameter. The 1476 default predates UDP/IP overhead, so full-size frames exceed a 1500-byte MTU and rely on IP fragmentation;MAX_APDU1024keeps every frame under the MTU.MaxAdpuLengthdrives both the I-Am advertisement and segment sizing, so the endpoint stays self-consistent.Verification
Unit tests: 8 new (
EncodeBufferTests,BacnetClientSegmentationTestswith a full segmented round-trip over an in-memory transport,BacnetClientIamTests). Each commit builds and passes the suite independently (128/128 → 131/131).Container before/after matrix — executed against the branch tip (
dotnet packof commit0d34744→ MinVer4.0.0-beta.1.14) vs the published4.0.0-beta.1from nuget.org. The library version under test is printed by the device on startup in every run.flowchart LR C["client container<br/>bacnet-stack 1.5.0<br/>bacrpr: ReadProperty client<br/>+ segment reassembly"] R["router container<br/>bacnet-stack 1.5.0 router app<br/>172.28.1.10 / 172.28.2.10"] D["device container<br/>BACnet .NET, this PR vs beta.1<br/>device 1234, 800 analog-values"] C <-->|"bacnet-net1 bridge<br/>172.28.1.0/24<br/>BACnet DNET 1, udp/47808"| R R <-->|"bacnet-net2 bridge<br/>172.28.2.0/24<br/>BACnet DNET 2, udp/47808"| DThe two Docker bridge networks are isolated from each other, so the only path between client and device is the BACnet router container — the tests cannot pass via direct IP.
4.0.0-beta.1(before)4.0.0-beta.1.14= this PR (after)IndexOutOfRangeException, client getsBACnet Error: services: other0x3D/0x39(reserved bit set)0x3C/0x38ReasmReqds +4, FragCreates +4(with the oldmaxPayload: 1600workaround)maxApdu: 1024→ 4 segments ≤1019 B, all counters +0APDU Timeout!(I-Am dies at the router)Note: the before-rows for fixes 1 (480), 2 and 4 required the old
maxPayload: 1600workaround so beta.1 could segment at all — on beta.1 one must choose between crashing (fix 1) and IP fragmentation (fix 4).Raw console output — before (published 4.0.0-beta.1)
Raw console output — after (this PR, 4.0.0-beta.1.14)
Also compared against YABE trunk (r642) and downstream forks: none solve these; YABE's unicast reply pattern and bacnet-stack's
Send_I_Am_Unicast("must direct back to src->net to meet BTL tests") match the approach taken here.Notes for review
ProcessConfirmedServiceRequestcovers the universal synchronous-handler pattern; handlers that respond asynchronously should use the explicit overload. The alternative — addingmaxAdputo every per-service delegate — would break every handler signature.WhoIs,IHave, …); consult the peer'sMax_APDU_Length_AcceptedDevice property for >1476 APDUs if BACnet/SC (Addendum 135-2016bj) support is ever added.