Fix Trend Log ReadRangeACK BY_SEQUENCE where an incorrect FirstSequence is sometimes returned. (#1150)

CHANGELOG.md

@@ -190,6 +190,7 @@ The git repositories are hosted at the following sites:
 
 ### Fixed
 
+* Fixed ReadRangeACK by sequence in Trend Log object. (#1150)
 * Fixed Device Management-Backup and Restore-B functionality to keep
   configuration files during the restore operation. (#1250)
 * Fixed Device Management-Backup and Restore-B Backup_Failure_Timeout

src/bacnet/basic/object/trendlog.c

@@ -298,6 +298,48 @@ bool Trend_Log_Object_Name(
     return status;
 }
 
+/**
+ * @brief Get the total record count for a Trend Log object
+ * @param object_instance - object-instance number of the object
+ * @return total record count
+ */
+uint32_t Trend_Log_Total_Record_Count(uint32_t object_instance)
+{
+    uint32_t total_records = 0;
+    if (object_instance < MAX_TREND_LOGS) {
+        total_records = LogInfo[object_instance].ulTotalRecordCount;
+    }
+    return total_records;
+}
+
+/**
+ * @brief Get the record count for a Trend Log object
+ * @param object_instance - object-instance number of the object
+ * @return record count
+ */
+uint32_t Trend_Log_Record_Count(uint32_t object_instance)
+{
+    uint32_t record_count = 0;
+    if (object_instance < MAX_TREND_LOGS) {
+        record_count = LogInfo[object_instance].ulRecordCount;
+    }
+    return record_count;
+}
+
+/**
+ * @brief Get the buffer size for a Trend Log object
+ * @param object_instance - object-instance number of the object
+ * @return buffer size
+ */
+uint32_t Trend_Log_Buffer_Size(uint32_t object_instance)
+{
+    uint32_t buffer_size = 0;
+    if (object_instance < MAX_TREND_LOGS) {
+        buffer_size = TL_MAX_ENTRIES;
+    }
+    return buffer_size;
+}
+
 /* return the length of the apdu encoded or BACNET_STATUS_ERROR for error or
    BACNET_STATUS_ABORT for abort message */
 int Trend_Log_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata)
@@ -1176,7 +1218,8 @@ int TL_encode_by_sequence(uint8_t *apdu, BACNET_READ_RANGE_DATA *pRequest)
     uint32_t uiSequence = 0; /* Tracking sequence number when encoding */
     uint32_t uiRemaining = 0; /* Amount of unused space in packet */
     uint32_t uiFirstSeq = 0; /* Sequence number for 1st record in log */
-
+    uint32_t total_entries = 0;
+    uint32_t max_fit = 0;
     uint32_t uiBegin = 0; /* Starting Sequence number for request */
     uint32_t uiEnd = 0; /* Ending Sequence number for request */
     bool bWrapReq = false; /* Has request sequence range spanned the max for
@@ -1267,7 +1310,22 @@ int TL_encode_by_sequence(uint8_t *apdu, BACNET_READ_RANGE_DATA *pRequest)
             }
         }
     }
-
+    if (pRequest->Count < 0) {
+        /* adjust uiBegin when Count < 0 and total requested
+           items exceed the maximum encodable items (max_fit).*/
+        if (uiEnd >= uiBegin) {
+            total_entries = uiEnd - uiBegin + 1;
+            max_fit = uiRemaining / TL_MAX_ENC;
+            if ((max_fit > 0) && (total_entries > max_fit)) {
+                /* Adjust beginning index so returned items match z = max_fit */
+                uiBegin = uiEnd - max_fit + 1;
+                /* MORE_ITEMS must be set because
+                   request range not fully delivered */
+                bitstring_set_bit(
+                    &pRequest->ResultFlags, RESULT_FLAG_MORE_ITEMS, true);
+            }
+        }
+    }
     /* We now have a range that lies completely within the log buffer
      * and we need to figure out where that starts in the buffer.
      */

src/bacnet/basic/object/trendlog.h

@@ -136,6 +136,13 @@ BACNET_STACK_EXPORT
 bool Trend_Log_Object_Name(
     uint32_t object_instance, BACNET_CHARACTER_STRING *object_name);
 
+BACNET_STACK_EXPORT
+uint32_t Trend_Log_Total_Record_Count(uint32_t object_instance);
+BACNET_STACK_EXPORT
+uint32_t Trend_Log_Record_Count(uint32_t object_instance);
+BACNET_STACK_EXPORT
+uint32_t Trend_Log_Buffer_Size(uint32_t object_instance);
+
 BACNET_STACK_EXPORT
 int Trend_Log_Read_Property(BACNET_READ_PROPERTY_DATA *rpdata);
 

test/bacnet/basic/object/trendlog/src/main.c

@@ -37,6 +37,109 @@ static void test_Trend_Log_ReadProperty(void)
         Trend_Log_Read_Property, Trend_Log_Write_Property,
         known_fail_property_list);
 }
+
+static void test_Trend_Log_ReadRange_BySequence(void)
+{
+    uint8_t apdu[MAX_APDU] = { 0 };
+    BACNET_READ_RANGE_DATA pRequest = { 0 };
+    int len = 0;
+    uint32_t object_instance = 0;
+    int log_index = 0;
+    uint32_t i;
+    uint32_t total_records = 0;
+    uint32_t first_seq = 0;
+
+    Trend_Log_Init();
+    object_instance = Trend_Log_Index_To_Instance(0);
+    log_index = Trend_Log_Instance_To_Index(object_instance);
+
+    /* Populate the log with some entries to make sure it's not empty and we
+     * know the state */
+    for (i = 0; i < 100; i++) {
+        TL_Insert_Status_Rec(log_index, LOG_STATUS_LOG_INTERRUPTED, true);
+    }
+
+    /* Get the current state of the log */
+    total_records = Trend_Log_Total_Record_Count(object_instance);
+    zassert_true(total_records > 0, "Failed to get total record count");
+    /* The first sequence number currently in the log buffer */
+    first_seq = total_records - Trend_Log_Record_Count(object_instance) + 1;
+    zassert_true(first_seq > 0, "Failed to get first sequence number");
+
+    /* Verify BY_SEQUENCE with positive count within range */
+    pRequest.object_type = OBJECT_TRENDLOG;
+    pRequest.object_instance = object_instance;
+    pRequest.object_property = PROP_LOG_BUFFER;
+    pRequest.array_index = BACNET_ARRAY_ALL;
+    pRequest.RequestType = RR_BY_SEQUENCE;
+    pRequest.Range.RefSeqNum = first_seq;
+    pRequest.Count = 10;
+    pRequest.Overhead = 0;
+
+    len = rr_trend_log_encode(apdu, &pRequest);
+    zassert_true(len > 0, "Encoding failed for positive count");
+    zassert_equal(pRequest.ItemCount, 10, "Expected 10 items");
+    zassert_equal(
+        pRequest.FirstSequence, first_seq, "Expected FirstSequence %u, got %u",
+        (unsigned)first_seq, (unsigned)pRequest.FirstSequence);
+
+    /* Verify BY_SEQUENCE with positive count and truncation at end of log */
+    pRequest.Range.RefSeqNum = total_records - 5;
+    pRequest.Count = 10;
+    len = rr_trend_log_encode(apdu, &pRequest);
+    zassert_true(len > 0, "Encoding failed");
+    zassert_equal(pRequest.ItemCount, 6, "Expected 6 items (truncated at end)");
+    zassert_equal(
+        pRequest.FirstSequence, total_records - 5,
+        "Expected FirstSequence %u, got %u", (unsigned)(total_records - 5),
+        (unsigned)pRequest.FirstSequence);
+    zassert_true(
+        bitstring_bit(&pRequest.ResultFlags, RESULT_FLAG_LAST_ITEM),
+        "Expected LAST_ITEM flag");
+
+    /* Verify BY_SEQUENCE with negative count and truncation at beginning of log
+     */
+    pRequest.Range.RefSeqNum = first_seq + 5;
+    pRequest.Count = -10;
+    len = rr_trend_log_encode(apdu, &pRequest);
+    zassert_true(len > 0, "Encoding failed");
+    zassert_equal(
+        pRequest.ItemCount, 6, "Expected 6 items (truncated at start)");
+    zassert_equal(
+        pRequest.FirstSequence, first_seq,
+        "Expected FirstSequence %u (start of log), got %u", (unsigned)first_seq,
+        (unsigned)pRequest.FirstSequence);
+    zassert_true(
+        bitstring_bit(&pRequest.ResultFlags, RESULT_FLAG_FIRST_ITEM),
+        "Expected FIRST_ITEM flag");
+
+    /* Verify BY_SEQUENCE with negative count and APDU capacity truncation (THE
+     * FIX) */
+    /* Requesting everything from the end. This should definitely trigger
+     * truncation. */
+    pRequest.ItemCount = 0;
+    pRequest.Range.RefSeqNum = total_records;
+    pRequest.Count =
+        -(int32_t)total_records; /* Try to get more than will fit */
+
+    len = rr_trend_log_encode(apdu, &pRequest);
+    zassert_true(len > 0, "Encoding failed for negative count");
+    zassert_true(
+        pRequest.ItemCount < total_records, "Expected truncated ItemCount");
+    zassert_true(
+        bitstring_bit(&pRequest.ResultFlags, RESULT_FLAG_MORE_ITEMS),
+        "Expected MORE_ITEMS flag");
+
+    /* THE FIX: FirstSequence should now correctly reflect the shifted uiBegin.
+       uiBegin = uiEnd - max_fit + 1.
+       In the code, uiEnd is RefSeqNum (total_records).
+       So FirstSequence should be total_records - ItemCount + 1 */
+    zassert_equal(
+        pRequest.FirstSequence, total_records - pRequest.ItemCount + 1,
+        "FirstSequence %u does not match calculated start %u",
+        (unsigned)pRequest.FirstSequence,
+        (unsigned)(total_records - pRequest.ItemCount + 1));
+}
 /**
  * @}
  */
@@ -44,7 +147,8 @@ static void test_Trend_Log_ReadProperty(void)
 void test_main(void)
 {
     ztest_test_suite(
-        trendlog_tests, ztest_unit_test(test_Trend_Log_ReadProperty));
+        trendlog_tests, ztest_unit_test(test_Trend_Log_ReadProperty),
+        ztest_unit_test(test_Trend_Log_ReadRange_BySequence));
 
     ztest_run_test_suite(trendlog_tests);
 }