Skip to content

Commit 8074b54

Browse files
CopilotrobertsLando
Copilot
and
robertsLando
authored
fix: decode special log records in ReadRange without status flags (#77)
Fixes ReadRange decoding when TREND_LOG contains special log records (e.g., `log-interrupted`, `time-change`) that don't have status flags. ## Problem When reading a `TREND_LOG` that contains a special log record, records after it were silently missing from the result. The issue was that `decodeRange` unconditionally consumed the status flags bytes after the log-datum value, but special records don't have status flags. This caused the decoder to read into the bytes of the next record and lose sync. ## Solution Updated `decodeRange` in `src/lib/asn1.ts` to: - Validate closing tag 1 before consuming it - Check for context tag 2 presence before decoding status flags (status flags are **optional for ALL record types** per ASHRAE 135 §12.25) - Add support for log-status choice (context tag 0 - special records like `log-disabled`, `buffer-purged`, `log-interrupted`) - Add support for time-change choice (context tag 9 - clock adjustment records) - Add `isLogStatus`, `logStatus`, and `isTimeChange` fields to differentiate special records ## Type Changes - Extracted `LogRecord`, `LogStatusFlags`, `LogRecordStatusFlags` interfaces to `types.ts` - Updated `ReadRangeAcknowledge.values` to use `LogRecord[]` instead of `any[]` - Added `LogRecordValue` union type to narrow `LogRecord.value` - Added ASHRAE 135 §12.25 standard references to code comments ## Testing - Added unit test for special log-status records without status flags (mixed with normal records) - Added unit test for time-change records without status flags - All 204 unit tests pass - Linting passes - Build passes - CodeQL security scan - no issues found <!-- START COPILOT ORIGINAL PROMPT --> <details> <summary>Original prompt</summary> > > ---- > > *This section details on the original issue you should resolve* > > <issue_title>ReadRange bug when response contains a special log record</issue_title> > <issue_description>Hello > > When reading a `TREND_LOG` that contains a special log record (e.g. `log-interrupted`), records after it are silently missing from the result — no error, no warning. > > I spent some time debugging this with Wireshark and found that the raw UDP packet is perfectly valid and complete. The issue is in `decodeRange`. > > ### What's happening > > Special log records don't have status flags. A normal record ends with: > ``` > 1f 2a 04 00 ← closing tag 1, opening tag 2, BIT_STRING, closing tag 2 > ``` > A special record ends with just: > ``` > 1f ← closing tag 1, nothing after > ``` > > But `decodeRange` unconditionally consumes 2 tags after the datum value without validating their tag numbers. So it reads into the bytes of the next record, shifts the offset by an unpredictable amount (depends on the byte values that follow), and loses sync for everything after. > > The behavior is non-deterministic — sometimes records after the special one are silently dropped, sometimes the buffer appears truncated. Took a while to figure out why the same code would behave differently depending on which records were in the response. > > ### Reproduction > > ```javascript > const response = await client.readRange( > { address: '10.x.x.x:47808' }, > { type: bacnet.ObjectType.TREND_LOG, instance: 100101 }, > startIndex, 20, > {} > ); > // response contains fewer records than expected > // no error thrown > // Wireshark shows the full response is received correctly > > Thanks so much for the patch ;-) > > Thomas</issue_description> > > ## Comments on the Issue (you are @copilot in this section) > > <comments> > <comment_new><author>@robertsLando</author><body> > Hi Thomas and thanks for your issue! WOuld you like to submit a PR? </body></comment_new> > </comments> > </details> <!-- START COPILOT CODING AGENT SUFFIX --> - Fixes #76 <!-- START COPILOT CODING AGENT TIPS --> --- 💬 Send tasks to Copilot coding agent from [Slack](https://gh.io/cca-slack-docs) and [Teams](https://gh.io/cca-teams-docs) to turn conversations into code. Copilot posts an update in your thread when it's finished. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: robertsLando <11502495+robertsLando@users.noreply.github.com>
1 parent 3d5d609 commit 8074b54

4 files changed

Lines changed: 402 additions & 29 deletions

File tree

‎src/lib/asn1.ts‎

Lines changed: 105 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,8 @@ import {
3131
BACNetDateValue,
3232
BACNetEncodableAppData,
3333
BACNetRawDate,
34+
LogRecord,
35+
LogRecordValue,
3436
} from './types'
3537
import {
3638
CharacterStringEncoding,
@@ -2421,15 +2423,23 @@ export const decodeCalendarDatelist = (
24212423
return { len, value: result }
24222424
}
24232425

2426+
/**
2427+
* Decodes LogRecord sequence from ReadRange ACK payload.
2428+
* Per ASHRAE 135 §12.25, LogRecord ::= SEQUENCE {
2429+
* timestamp [0] BACnetDateTime,
2430+
* logDatum [1] CHOICE { log-status [0], boolean-value [1], real-value [2], ... },
2431+
* statusFlags [2] BACnetStatusFlags OPTIONAL -- NOT present for log-status choice
2432+
* }
2433+
*/
24242434
export const decodeRange = (
24252435
buffer: Buffer,
24262436
offset: number,
24272437
maxOffset: number,
2428-
): Decode<any[]> | undefined => {
2438+
): Decode<LogRecord[]> | undefined => {
24292439
// The payload for readRange ACK is expected to start with opening tag 0.
24302440
if (!decodeIsOpeningTagNumber(buffer, offset, 0)) return undefined
24312441
let len = 0
2432-
const result: any[] = []
2442+
const result: LogRecord[] = []
24332443

24342444
while (
24352445
offset + len < maxOffset &&
@@ -2474,58 +2484,106 @@ export const decodeRange = (
24742484
}
24752485
len += 2
24762486

2477-
// value payload (context specific)
2487+
// log-datum [1] CHOICE per ASHRAE 135 §12.25:
2488+
// [0] log-status (BACnetLogStatus bitstring - special records)
2489+
// [1] boolean-value
2490+
// [2] real-value
2491+
// [3] enum-value
2492+
// [4] unsigned-value
2493+
// [5] signed-value
2494+
// [6] bitstring-value
2495+
// [7] null-value
2496+
// [8] failure (BACnetError)
2497+
// [9] time-change (REAL - seconds delta for clock adjustment)
2498+
// [10] any-value (ABSTRACT-SYNTAX.&Type)
24782499
tag = decodeTagNumberAndValue(buffer, offset + len)
24792500
len += tag.len
24802501
let value: ApplicationData | undefined
2481-
if (tag.tagNumber === 2 && tag.value === 4) {
2502+
let isLogStatus = false
2503+
let isTimeChange = false
2504+
if (tag.tagNumber === 0) {
2505+
// log-status choice: BACnetLogStatus bitstring per ASHRAE 135 §12.25
2506+
// Special log records (log-disabled, buffer-purged, log-interrupted)
2507+
value = bacappDecodeData(
2508+
buffer,
2509+
offset + len,
2510+
maxOffset,
2511+
ApplicationTag.BIT_STRING,
2512+
tag.value,
2513+
)
2514+
isLogStatus = true
2515+
} else if (tag.tagNumber === 2) {
2516+
// real-value
24822517
value = bacappDecodeData(
24832518
buffer,
24842519
offset + len,
24852520
maxOffset,
24862521
ApplicationTag.REAL,
24872522
tag.value,
24882523
)
2489-
} else if (tag.tagNumber === 4 && tag.value === 1) {
2524+
} else if (tag.tagNumber === 3) {
2525+
// enum-value
24902526
value = bacappDecodeData(
24912527
buffer,
24922528
offset + len,
24932529
maxOffset,
24942530
ApplicationTag.ENUMERATED,
24952531
tag.value,
24962532
)
2497-
} else if (tag.tagNumber === 3 && tag.value === 1) {
2533+
} else if (tag.tagNumber === 4) {
2534+
// unsigned-value
24982535
value = bacappDecodeData(
24992536
buffer,
25002537
offset + len,
25012538
maxOffset,
2502-
ApplicationTag.ENUMERATED,
2539+
ApplicationTag.UNSIGNED_INTEGER,
2540+
tag.value,
2541+
)
2542+
} else if (tag.tagNumber === 9) {
2543+
// time-change: REAL value representing seconds the clock changed
2544+
// Per ASHRAE 135 §12.25, time-change records do not have status flags
2545+
value = bacappDecodeData(
2546+
buffer,
2547+
offset + len,
2548+
maxOffset,
2549+
ApplicationTag.REAL,
25032550
tag.value,
25042551
)
2552+
isTimeChange = true
25052553
}
25062554
if (!value) return undefined
25072555
len += value.len
25082556

2509-
// closing tag 1 + opening tag 2
2510-
tag = decodeTagNumberAndValue(buffer, offset + len)
2511-
len += tag.len
2557+
// closing tag 1 is required
2558+
if (!decodeIsClosingTagNumber(buffer, offset + len, 1)) {
2559+
return undefined
2560+
}
25122561
tag = decodeTagNumberAndValue(buffer, offset + len)
25132562
len += tag.len
25142563

2515-
// status flags
2516-
const status = bacappDecodeData(
2517-
buffer,
2518-
offset + len,
2519-
maxOffset,
2520-
ApplicationTag.BIT_STRING,
2521-
tag.value,
2522-
)
2523-
if (!status) return undefined
2524-
len += status.len
2564+
// context tag 2 (status flags) is OPTIONAL for ALL log record types.
2565+
// Per ASHRAE 135 §12.25 and bacnet-stack, status flags are only encoded
2566+
// when the ucStatus byte has bit 7 set. This means any record type
2567+
// (log-status, time-change, real-value, etc.) may or may not have status flags.
2568+
let status: ApplicationData | undefined
2569+
if (decodeIsContextTag(buffer, offset + len, 2)) {
2570+
tag = decodeTagNumberAndValue(buffer, offset + len)
2571+
len += tag.len
2572+
2573+
// status flags bitstring
2574+
status = bacappDecodeData(
2575+
buffer,
2576+
offset + len,
2577+
maxOffset,
2578+
ApplicationTag.BIT_STRING,
2579+
tag.value,
2580+
)
2581+
if (!status) return undefined
2582+
len += status.len
2583+
}
25252584

25262585
const d = date.value as Date
25272586
const t = time.value as Date
2528-
const statusBits = status.value as { value: number[] }
25292587
const timestamp = new Date(
25302588
d.getFullYear(),
25312589
d.getMonth(),
@@ -2535,16 +2593,37 @@ export const decodeRange = (
25352593
t.getSeconds(),
25362594
t.getMilliseconds(),
25372595
)
2538-
result.push({
2596+
2597+
const record: LogRecord = {
25392598
timestamp,
2540-
value: value.value,
2541-
status: {
2599+
value: value.value as LogRecordValue,
2600+
}
2601+
2602+
if (isLogStatus) {
2603+
record.isLogStatus = true
2604+
const logStatusBits = value.value as BACNetBitString
2605+
record.logStatus = {
2606+
log_disabled: ((logStatusBits.value[0] >> 0) & 1) !== 0,
2607+
buffer_purged: ((logStatusBits.value[0] >> 1) & 1) !== 0,
2608+
log_interrupted: ((logStatusBits.value[0] >> 2) & 1) !== 0,
2609+
}
2610+
}
2611+
2612+
if (isTimeChange) {
2613+
record.isTimeChange = true
2614+
}
2615+
2616+
if (status) {
2617+
const statusBits = status.value as BACNetBitString
2618+
record.status = {
25422619
out_of_service: ((statusBits.value[0] >> 0) & 1) !== 0,
25432620
overridden: ((statusBits.value[0] >> 1) & 1) !== 0,
25442621
fault: ((statusBits.value[0] >> 2) & 1) !== 0,
25452622
in_alarm: ((statusBits.value[0] >> 3) & 1) !== 0,
2546-
},
2547-
})
2623+
}
2624+
}
2625+
2626+
result.push(record)
25482627
}
25492628

25502629
if (offset + len >= maxOffset) return undefined

‎src/lib/services/WriteProperty.ts‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,10 @@ export default class WriteProperty extends BacnetService {
108108
)
109109
}
110110

111-
private static writeDateBytes(buffer: EncodeBuffer, value: BACNetDateValue) {
111+
private static writeDateBytes(
112+
buffer: EncodeBuffer,
113+
value: BACNetDateValue,
114+
) {
112115
if (WriteProperty.isRawDate(value)) {
113116
WriteProperty.validateRawDateByte('year', value.year, 0, 255)
114117
if (value.month !== 0xff) {
@@ -170,7 +173,8 @@ export default class WriteProperty extends BacnetService {
170173
if (timeValue == null) {
171174
throw new Error(`${errorPrefix} time is required`)
172175
}
173-
const normalized = timeValue instanceof Date ? timeValue : new Date(timeValue)
176+
const normalized =
177+
timeValue instanceof Date ? timeValue : new Date(timeValue)
174178
if (Number.isNaN(normalized.getTime())) {
175179
throw new Error(`${errorPrefix} time is invalid`)
176180
}

‎src/lib/types.ts‎

Lines changed: 72 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -742,13 +742,84 @@ export interface ReadRangePayload extends BasicServicePayload {
742742
values: BACNetAppData[]
743743
}
744744

745+
/**
746+
* BACnetLogStatus bitstring per ASHRAE 135 §12.25.
747+
* Represents the log-status choice for special log records.
748+
* These are status-only records without actual data values.
749+
*/
750+
export interface LogStatusFlags {
751+
log_disabled: boolean
752+
buffer_purged: boolean
753+
log_interrupted: boolean
754+
}
755+
756+
/**
757+
* BACnetStatusFlags per ASHRAE 135 §12.25.
758+
* Represents the status flags for a normal log record.
759+
*/
760+
export interface LogRecordStatusFlags {
761+
out_of_service: boolean
762+
overridden: boolean
763+
fault: boolean
764+
in_alarm: boolean
765+
}
766+
767+
/**
768+
* Union type for log record values per ASHRAE 135 §12.25.
769+
* log-datum CHOICE can contain various types depending on the logged property.
770+
*/
771+
export type LogRecordValue =
772+
| number // REAL, ENUMERATED, UNSIGNED_INTEGER, SIGNED_INTEGER
773+
| boolean // BOOLEAN
774+
| BACNetBitString // log-status bitstring
775+
| null // NULL
776+
| string // CHARACTER_STRING
777+
| BACNetObjectID // OBJECTIDENTIFIER
778+
779+
/**
780+
* LogRecord per ASHRAE 135 §12.25.
781+
* Represents a single log record from a TREND_LOG.
782+
* Can be either a normal data record, a special log-status record, or a time-change record.
783+
*/
784+
export interface LogRecord {
785+
/** Timestamp when the record was logged */
786+
timestamp: Date
787+
/**
788+
* The logged value. For normal records, this is the actual data (number, boolean, etc.).
789+
* For log-status records, this is a BACNetBitString.
790+
* For time-change records, this is the number of seconds the clock changed (REAL).
791+
*/
792+
value: LogRecordValue
793+
/**
794+
* True if this is a special log-status record (log-disabled, buffer-purged, log-interrupted).
795+
* Undefined for normal data records and time-change records.
796+
*/
797+
isLogStatus?: boolean
798+
/**
799+
* True if this is a time-change record (clock adjustment).
800+
* Per ASHRAE 135 §12.25, time-change records contain a REAL value representing
801+
* the number of seconds the clock changed (positive or negative).
802+
* Undefined for normal data records and log-status records.
803+
*/
804+
isTimeChange?: boolean
805+
/**
806+
* Present only for log-status records. Indicates which special status applies.
807+
*/
808+
logStatus?: LogStatusFlags
809+
/**
810+
* Present only when status flags are encoded. Per ASHRAE 135 §12.25,
811+
* status flags are optional for ALL log record types.
812+
*/
813+
status?: LogRecordStatusFlags
814+
}
815+
745816
export interface ReadRangeAcknowledge {
746817
objectId: BACNetObjectID
747818
property: BACNetPropertyID
748819
resultFlag: BACNetBitString
749820
itemCount: number
750821
rangeBuffer: Buffer
751-
values?: any[]
822+
values?: LogRecord[]
752823
len: number
753824
}
754825

0 commit comments

Comments
 (0)