Repository navigation
fix: make ACTIVE_COV_SUBSCRIPTIONS readable (wire decoder into ReadProperty, decode multi-entry lists) - #91
Open
chrismoorhouse wants to merge 2 commits into
Conversation
decodeCovSubscription was only reachable via bacappDecodeContextApplicationData, which no service calls, so reading the Device object's ACTIVE_COV_SUBSCRIPTIONS property always failed with INVALID_DECODING: the generic application-tag decoder cannot parse the context-tagged BACnetCOVSubscription structure. Decode the list entries in ReadProperty.decodeAcknowledge, following the same pattern as the weekly/exception schedule and calendar date list special cases, and export decodeCovSubscription from asn1.
decodeCovSubscription treated any byte after timeRemaining that was not a closing tag as the optional covIncrement [4]. In a list with multiple subscriptions the next byte is the opening tag [0] of the next entry, so decoding failed with INVALID_DECODING for any device reporting more than one active subscription. Require the context tag to actually be 4 before consuming covIncrement.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Closing-tag handling, tag validation, and regression test coverage need correction.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Fixes decoding of Device ACTIVE_COV_SUBSCRIPTIONS, including multi-entry lists.
Changes:
- Adds COV subscription handling to ReadProperty acknowledgements.
- Exports and corrects the COV subscription decoder.
| File | Description |
|---|---|
src/lib/services/ReadProperty.ts |
Decodes COV subscription lists. |
src/lib/asn1.ts |
Exports the decoder and recognizes optional COV increments. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+1958
to
1965
| if ( | ||
| len < apduLen && | ||
| !isClosingTag(buffer[offset + len]) && | ||
| decodeIsContextTag(buffer, offset + len, 4) | ||
| ) { | ||
| result = decodeTagNumberAndValue(buffer, offset + len) | ||
| len += result.len | ||
| if (result.tagNumber !== 4) return undefined |
Comment on lines
+259
to
+260
| len += result.len | ||
| } |
| value: result.value, | ||
| } as ApplicationData) | ||
| len += result.len | ||
| } else if (property.id === PropertyIdentifier.ACTIVE_COV_SUBSCRIPTIONS) { |
This branch has not been deployed
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
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.
Summary
Fixes #90.
Reading the Device object's
ACTIVE_COV_SUBSCRIPTIONSproperty (152) always failed withINVALID_DECODINGfor two independent reasons, fixed in two commits:decodeCovSubscription()was only reachable viabacappDecodeContextApplicationData(), which no service calls.ReadProperty.decodeAcknowledge()now decodes the list entries directly, following the same special-case pattern already used forWEEKLY_SCHEDULE,EXCEPTION_SCHEDULE,EFFECTIVE_PERIODandDATE_LIST;decodeCovSubscriptionis now exported fromasn1.time-remaining [3], the decoder treated any non-closing-tag byte as the optionalcov-increment [4]— but in a multi-entry list that byte is the opening tag[0]of the next subscription, so decoding failed whenever a device reported two or more subscriptions. The decoder now only consumescov-incrementwhen the context tag is actually 4.Verification
npm run buildcleannpm run test:unit— 204/204 passbacappEncodeApplicationData(tag 111) decode sequentially with correct process ids, object ids and lengthsrouter-mstp:readProperty(..., 152)now returns the full subscription list (previouslyINVALID_DECODING), enabling enumeration and cleanup of stale COV subscriptions