fix: encode the Error PDU without spurious context tags#202
Merged
Conversation
Since 3.x (2f23aba) Services.EncodeError wrapped the error-class and error-code enumerations in context tags by default, so every Error PDU sent by ErrorResponse was malformed: the BACnet-Error-PDU carries the plain Error production - two application-tagged enumerations with no enclosing context tags (ASHRAE 135 20.1.7). Foreign stacks mis-decoded every error this library returned (bacnet-stack displayed "services: success" instead of "object: unknown-object"); the tolerant DecodeError hid the regression from same-stack round-trips. The wrapping is now opt-in and explicit at the call sites whose ASN.1 actually requires it: ConfirmedPrivateTransfer-Error error-type [0] and the trend-log failure log-datum [8]. DecodeError stays tolerant so errors from peers running affected versions still decode. The regression tests assert raw wire bytes on purpose - verifying the encoder through the tolerant decoder is how this went unnoticed.
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.
Fixes the malformed Error PDU described in #199: since 3.x,
Services.EncodeErrorwrapped the error class/code pair in context tags by default, so every error sent byErrorResponseviolated ASHRAE 135 §20.1.7 (the BACnet-Error-PDU carries the plainErrorproduction — two application-tagged enumerations, no enclosing context tags). Foreign stacks mis-decoded every error this library returned; the tolerantDecodeErrorhid it from C#↔C# round-trips. 2.0.4 was correct — the regression came in with 2f23aba, whose tag-wrapping is genuinely needed only for the trend-logfailurelog-datum.Changes
Services.EncodeError:tagNumberis now nullable and opt-in — plain encoding by default.Errorpass their tags explicitly:EncodePrivateTransferError(error-type [0]) andEncodeLogRecord(failurelog-datum[8]).ErrorResponsegets the plain encoding without a call-site change.DecodeErrorstays tolerant (accepts both plain and legacy-wrapped forms), so errors from peers running affected 3.x/4.0-beta versions still decode.Verification
ErrorPduTests: plain production bytes, full Error-PDU bytes, explicit-tag wrapping, plain+legacy decode).bacrpvia router)analog-value 999(unknown object)BACnet Error: services: successBACnet Error: object: unknown-objectBACnet Error: services: successBACnet Error: property: unknown-propertyIndependent of #198 (no shared files apart from a trivial CHANGELOG adjacency).
Closes #199.