Refactor/property lists member function (#609)

* Refactor property lists member function for WriteProperty default case.

* Refactor time-value object unit testing

* Added test for unsupported property to common property test

.github/workflows/zephyr.yml

@@ -1,6 +1,12 @@
 name: Zephyr OS CMake
 
-on: [push, pull_request]
+on:
+  push:
+    branches:
+      - master
+  pull_request:
+    branches:
+      - '*'
 
 jobs:
   build:

src/bacnet/basic/object/ai.c

@@ -66,34 +66,6 @@ void Analog_Input_Property_Lists(
     return;
 }
 
-/**
- * @brief Determine if the object property is a member of this object instance
- * @param object_instance - object-instance number of the object
- * @param object_property - object-property to be checked
- * @return true if the property is a member of this object instance
- */
-static bool Property_List_Member(
-    uint32_t object_instance, int object_property)
-{
-    bool found = false;
-    const int *pRequired = NULL;
-    const int *pOptional = NULL;
-    const int *pProprietary = NULL;
-
-    (void)object_instance;
-    Analog_Input_Property_Lists(
-        &pRequired, &pOptional, &pProprietary);
-    found = property_list_member(pRequired, object_property);
-    if (!found) {
-        found = property_list_member(pOptional, object_property);
-    }
-    if (!found) {
-        found = property_list_member(pProprietary, object_property);
-    }
-
-    return found;
-}
-
 void Analog_Input_Init(void)
 {
     unsigned i;
@@ -903,8 +875,9 @@ bool Analog_Input_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
             break;
 #endif
         default:
-            if (Property_List_Member(
+            if (property_lists_member(
-                    wp_data->object_instance, wp_data->object_property)) {
+                Properties_Required, Properties_Optional, 
+                Properties_Proprietary, wp_data->object_property)) {
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
             } else {

src/bacnet/basic/object/av.c

@@ -86,34 +86,6 @@ void Analog_Value_Property_Lists(
     return;
 }
 
-/**
- * @brief Determine if the object property is a member of this object instance
- * @param object_instance - object-instance number of the object
- * @param object_property - object-property to be checked
- * @return true if the property is a member of this object instance
- */
-static bool Property_List_Member(
-    uint32_t object_instance, int object_property)
-{
-    bool found = false;
-    const int *pRequired = NULL;
-    const int *pOptional = NULL;
-    const int *pProprietary = NULL;
-
-    (void)object_instance;
-    Analog_Value_Property_Lists(
-        &pRequired, &pOptional, &pProprietary);
-    found = property_list_member(pRequired, object_property);
-    if (!found) {
-        found = property_list_member(pOptional, object_property);
-    }
-    if (!found) {
-        found = property_list_member(pProprietary, object_property);
-    }
-
-    return found;
-}
-
 /**
  * Initialize the analog values.
  */
@@ -1026,8 +998,11 @@ bool Analog_Value_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
             break;
 #endif
         default:
-            if (Property_List_Member(
+            if (property_lists_member(
-                    wp_data->object_instance, wp_data->object_property)) {
+                Analog_Value_Properties_Required, 
+                Analog_Value_Properties_Optional, 
+                Analog_Value_Properties_Proprietary, 
+                wp_data->object_property)) {
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
             } else {

src/bacnet/basic/object/calendar.c

@@ -466,34 +466,6 @@ bool Calendar_Description_Set(uint32_t object_instance, char *new_name)
     return status;
 }
 
-/**
- * @brief Determine if the object property is a member of this object instance
- * @param object_instance - object-instance number of the object
- * @param object_property - object-property to be checked
- * @return true if the property is a member of this object instance
- */
-static bool Property_List_Member(
-    uint32_t object_instance, int object_property)
-{
-    bool found = false;
-    const int *pRequired = NULL;
-    const int *pOptional = NULL;
-    const int *pProprietary = NULL;
-
-    (void)object_instance;
-    Calendar_Property_Lists(
-        &pRequired, &pOptional, &pProprietary);
-    found = property_list_member(pRequired, object_property);
-    if (!found) {
-        found = property_list_member(pOptional, object_property);
-    }
-    if (!found) {
-        found = property_list_member(pProprietary, object_property);
-    }
-
-    return found;
-}
-
 /**
  * @brief Determine if the object property is a BACnetARRAY property
  * @param object_property - object-property to be checked
@@ -637,8 +609,11 @@ bool Calendar_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
                 &wp_data->error_code);
             break;
         default:
-            if (Property_List_Member(
+            if (property_lists_member(
-                    wp_data->object_instance, wp_data->object_property)) {
+                Calendar_Properties_Required, 
+                Calendar_Properties_Optional, 
+                Calendar_Properties_Proprietary, 
+                wp_data->object_property)) {
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
             } else {

src/bacnet/basic/object/command.c

@@ -380,34 +380,6 @@ void Command_Property_Lists(
     return;
 }
 
-/**
- * @brief Determine if the object property is a member of this object instance
- * @param object_instance - object-instance number of the object
- * @param object_property - object-property to be checked
- * @return true if the property is a member of this object instance
- */
-static bool Property_List_Member(
-    uint32_t object_instance, int object_property)
-{
-    bool found = false;
-    const int *pRequired = NULL;
-    const int *pOptional = NULL;
-    const int *pProprietary = NULL;
-
-    (void)object_instance;
-    Command_Property_Lists(
-        &pRequired, &pOptional, &pProprietary);
-    found = property_list_member(pRequired, object_property);
-    if (!found) {
-        found = property_list_member(pOptional, object_property);
-    }
-    if (!found) {
-        found = property_list_member(pProprietary, object_property);
-    }
-
-    return found;
-}
-
 /**
  * Initializes the Command object data
  */
@@ -828,8 +800,9 @@ bool Command_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
             }
             break;
         default:
-            if (Property_List_Member(
+            if (property_lists_member(
-                    wp_data->object_instance, wp_data->object_property)) {
+                Command_Properties_Required, Command_Properties_Optional, 
+                Command_Properties_Proprietary, wp_data->object_property)) {
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
             } else {

src/bacnet/basic/object/lsz.c

@@ -92,32 +92,6 @@ void Life_Safety_Zone_Property_Lists(
     return;
 }
 
-/**
- * @brief Determine if the object property is a member of this object instance
- * @param object_instance - object-instance number of the object
- * @param object_property - object-property to be checked
- * @return true if the property is a member of this object instance
- */
-static bool Property_List_Member(uint32_t object_instance, int object_property)
-{
-    bool found = false;
-    const int *pRequired = NULL;
-    const int *pOptional = NULL;
-    const int *pProprietary = NULL;
-
-    (void)object_instance;
-    Life_Safety_Zone_Property_Lists(&pRequired, &pOptional, &pProprietary);
-    found = property_list_member(pRequired, object_property);
-    if (!found) {
-        found = property_list_member(pOptional, object_property);
-    }
-    if (!found) {
-        found = property_list_member(pProprietary, object_property);
-    }
-
-    return found;
-}
-
 /**
  * @brief Determines if a given object instance is valid
  * @param  object_instance - object-instance number of the object
@@ -832,8 +806,11 @@ bool Life_Safety_Zone_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
             status = Life_Safety_Zone_Members_Write(wp_data);
             break;
         default:
-            if (Property_List_Member(
+            if (property_lists_member(
-                    wp_data->object_instance, wp_data->object_property)) {
+                Life_Safety_Zone_Properties_Required, 
+                Life_Safety_Zone_Properties_Optional, 
+                Life_Safety_Zone_Properties_Proprietary, 
+                wp_data->object_property)) {
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
             } else {

src/bacnet/basic/object/schedule.c

@@ -354,24 +354,16 @@ bool Schedule_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
                     wp_data->object_instance, value.type.Boolean);
             }
             break;
-
+        default:
-        case PROP_OBJECT_IDENTIFIER:
+            if (property_lists_member(
-        case PROP_OBJECT_NAME:
+                Schedule_Properties_Required, Schedule_Properties_Optional, 
-        case PROP_OBJECT_TYPE:
+                Schedule_Properties_Proprietary, wp_data->object_property)) {
-        case PROP_PRESENT_VALUE:
-        case PROP_EFFECTIVE_PERIOD:
-        case PROP_WEEKLY_SCHEDULE:
-        case PROP_SCHEDULE_DEFAULT:
-        case PROP_LIST_OF_OBJECT_PROPERTY_REFERENCES:
-        case PROP_PRIORITY_FOR_WRITING:
-        case PROP_STATUS_FLAGS:
-        case PROP_RELIABILITY:
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
-            break;
+            } else {
-        default:
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_UNKNOWN_PROPERTY;
+            }
             break;
     }
 

src/bacnet/basic/object/time_value.c

@@ -391,34 +391,6 @@ bool Time_Value_Description_Set(uint32_t object_instance, char *new_name)
     return status;
 }
 
-/**
- * @brief Determine if the object property is a member of this object instance
- * @param object_instance - object-instance number of the object
- * @param object_property - object-property to be checked
- * @return true if the property is a member of this object instance
- */
-static bool Property_List_Member(
-    uint32_t object_instance, int object_property)
-{
-    bool found = false;
-    const int *pRequired = NULL;
-    const int *pOptional = NULL;
-    const int *pProprietary = NULL;
-
-    (void)object_instance;
-    Time_Value_Property_Lists(
-        &pRequired, &pOptional, &pProprietary);
-    found = property_list_member(pRequired, object_property);
-    if (!found) {
-        found = property_list_member(pOptional, object_property);
-    }
-    if (!found) {
-        found = property_list_member(pProprietary, object_property);
-    }
-
-    return found;
-}
-
 /**
  * @brief Determine if the object property is a BACnetARRAY property
  * @param object_property - object-property to be checked
@@ -564,8 +536,9 @@ bool Time_Value_Write_Property(BACNET_WRITE_PROPERTY_DATA *wp_data)
             }
             break;
         default:
-            if (Property_List_Member(
+            if (property_lists_member(
-                    wp_data->object_instance, wp_data->object_property)) {
+                Time_Value_Properties_Required, Time_Value_Properties_Optional, 
+                Time_Value_Properties_Proprietary, wp_data->object_property)) {
                 wp_data->error_class = ERROR_CLASS_PROPERTY;
                 wp_data->error_code = ERROR_CODE_WRITE_ACCESS_DENIED;
             } else {

src/bacnet/proplist.c

@@ -85,6 +85,33 @@ bool property_list_member(const int *pList, int object_property)
     return status;
 }
 
+/**
+ * @brief Determine if the object property is a member of any of the lists
+ * @param pRequired - array of type 'int' that is a list of BACnet properties
+ * @param pOptional - array of type 'int' that is a list of BACnet properties
+ * @param pProprietary - array of type 'int' that is a list of BACnet properties
+ * @param object_property - object-property to be checked
+ * @return true if the property is a member of any of these lists
+ */
+bool property_lists_member(
+    const int *pRequired,
+    const int *pOptional,
+    const int *pProprietary,
+    int object_property)
+{
+    bool found = false;
+
+    found = property_list_member(pRequired, object_property);
+    if (!found) {
+        found = property_list_member(pOptional, object_property);
+    }
+    if (!found) {
+        found = property_list_member(pProprietary, object_property);
+    }
+
+    return found;
+}
+
 /**
  * ReadProperty handler for this property.  For the given ReadProperty
  * data, the application_data is loaded or the error flags are set.

src/bacnet/proplist.h

@@ -55,6 +55,12 @@ extern "C" {
         const int *pList,
         int object_property);
     BACNET_STACK_EXPORT
+    bool property_lists_member(
+        const int *pRequired,
+        const int *pOptional,
+        const int *pProprietary,
+        int object_property);
+    BACNET_STACK_EXPORT
     int property_list_encode(
         BACNET_READ_PROPERTY_DATA * rpdata,
         const int *pListRequired,

test/bacnet/basic/object/property_test.c

@@ -170,6 +170,7 @@ void bacnet_object_properties_read_write_test(BACNET_OBJECT_TYPE object_type,
     const int *pProprietary = NULL;
     unsigned count = 0;
     int len = 0;
+    bool status = false;
 
     /* ReadProperty parameters */
     rpdata.application_data = &apdu[0];
@@ -207,4 +208,13 @@ void bacnet_object_properties_read_write_test(BACNET_OBJECT_TYPE object_type,
             &wpdata, write_property, skip_fail_property_list);
         pProprietary++;
     }
+    /* check for unsupported property - use ALL */
+    rpdata.object_property = PROP_ALL;
+    rpdata.array_index = BACNET_ARRAY_ALL;
+    len = read_property(&rpdata);
+    zassert_equal(len, BACNET_STATUS_ERROR, NULL);
+    wpdata.object_property = PROP_ALL;
+    wpdata.array_index = BACNET_ARRAY_ALL;
+    status = write_property(&wpdata);
+    zassert_false(status, NULL);
 }

test/bacnet/basic/object/time_value/CMakeLists.txt

@@ -27,6 +27,7 @@ add_compile_definitions(
 
 include_directories(
 	${SRC_DIR}
+	${TST_DIR}/bacnet/basic/object
 	${TST_DIR}/ztest/include
 	)
 
@@ -64,6 +65,7 @@ add_executable(${PROJECT_NAME}
 	./src/main.c
 	./stubs.c
 	../mock/device_mock.c
+	../property_test.c
 	${ZTST_DIR}/ztest_mock.c
 	${ZTST_DIR}/ztest.c
 	)

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

@@ -10,6 +10,7 @@
 #include <zephyr/ztest.h>
 #include <bacnet/basic/object/time_value.h>
 #include <bacnet/bactext.h>
+#include <property_test.h>
 
 /**
  * @addtogroup bacnet_tests
@@ -25,111 +26,26 @@ ZTEST(bacnet_tv, testTimeValue)
 static void testTimeValue(void)
 #endif
 {
-    uint8_t apdu[MAX_APDU] = { 0 };
-    int len = 0, test_len = 0;
-    BACNET_READ_PROPERTY_DATA rpdata = { 0 };
-    BACNET_WRITE_PROPERTY_DATA wpdata = { 0 };
-    BACNET_APPLICATION_DATA_VALUE value = {0};
-    const int *pRequired = NULL;
-    const int *pOptional = NULL;
-    const int *pProprietary = NULL;
-    const uint32_t instance = 123;
-    uint32_t test_instance = 0;
     bool status = false;
-    unsigned index;
+    unsigned count = 0;
+    uint32_t object_instance = 0;
+    const int skip_fail_property_list[] = { -1 };
 
     Time_Value_Init();
-    test_instance = Time_Value_Create(instance);
+    object_instance = Time_Value_Create(BACNET_MAX_INSTANCE);
-    zassert_equal(test_instance, instance, NULL);
+    count = Time_Value_Count();
-    status = Time_Value_Valid_Instance(instance);
+    zassert_true(count > 0, NULL);
-    zassert_true(status, NULL);
+    object_instance = Time_Value_Index_To_Instance(0);
-    index = Time_Value_Instance_To_Index(instance);
+    bacnet_object_properties_read_write_test(
-    zassert_equal(index, 0, NULL);
+        OBJECT_TIME_VALUE,
-
+        object_instance,
-    rpdata.application_data = &apdu[0];
+        Time_Value_Property_Lists,
-    rpdata.application_data_len = sizeof(apdu);
+        Time_Value_Read_Property,
-    rpdata.object_type = OBJECT_TIME_VALUE;
+        Time_Value_Write_Property,
-    rpdata.object_instance = instance;
+        skip_fail_property_list);
-    rpdata.array_index = BACNET_ARRAY_ALL;
-
-    Time_Value_Property_Lists(
-        &pRequired, &pOptional, &pProprietary);
-    while ((*pRequired) >= 0) {
-        rpdata.object_property = *pRequired;
-        rpdata.array_index = BACNET_ARRAY_ALL;
-        len = Time_Value_Read_Property(&rpdata);
-        zassert_not_equal(len, BACNET_STATUS_ERROR,
-            "property '%s': failed to ReadProperty!\n",
-            bactext_property_name(rpdata.object_property));
-        if (len >= 0) {
-            test_len = bacapp_decode_known_property(rpdata.application_data,
-                len, &value, rpdata.object_type, rpdata.object_property);
-            if (rpdata.object_property != PROP_PRIORITY_ARRAY) {
-                zassert_equal(len, test_len,
-                    "property '%s': failed to decode!\n",
-                    bactext_property_name(rpdata.object_property));
-            }
-            /* check WriteProperty properties */
-            wpdata.object_type = rpdata.object_type;
-            wpdata.object_instance = rpdata.object_instance;
-            wpdata.object_property = rpdata.object_property;
-            wpdata.array_index = rpdata.array_index;
-            memcpy(&wpdata.application_data, rpdata.application_data, MAX_APDU);
-            wpdata.application_data_len = len;
-            wpdata.error_code = ERROR_CODE_SUCCESS;
-            status = Time_Value_Write_Property(&wpdata);
-            if (!status) {
-                /* verify WriteProperty property is known */
-                zassert_not_equal(wpdata.error_code,
-                    ERROR_CODE_UNKNOWN_PROPERTY,
-                    "property '%s': WriteProperty Unknown!\n",
-                    bactext_property_name(rpdata.object_property));
-            }
-        }
-        pRequired++;
-    }
-    while ((*pOptional) != -1) {
-        rpdata.object_property = *pOptional;
-        rpdata.array_index = BACNET_ARRAY_ALL;
-        len = Time_Value_Read_Property(&rpdata);
-        zassert_not_equal(len, BACNET_STATUS_ERROR,
-            "property '%s': failed to ReadProperty!\n",
-            bactext_property_name(rpdata.object_property));
-        if (len > 0) {
-            test_len = bacapp_decode_application_data(rpdata.application_data,
-                (uint8_t)rpdata.application_data_len, &value);
-            zassert_equal(len, test_len, "property '%s': failed to decode!\n",
-                bactext_property_name(rpdata.object_property));
-            /* check WriteProperty properties */
-            wpdata.object_type = rpdata.object_type;
-            wpdata.object_instance = rpdata.object_instance;
-            wpdata.object_property = rpdata.object_property;
-            wpdata.array_index = rpdata.array_index;
-            memcpy(&wpdata.application_data, rpdata.application_data, MAX_APDU);
-            wpdata.application_data_len = len;
-            wpdata.error_code = ERROR_CODE_SUCCESS;
-            status = Time_Value_Write_Property(&wpdata);
-            if (!status) {
-                /* verify WriteProperty property is known */
-                zassert_not_equal(wpdata.error_code,
-                    ERROR_CODE_UNKNOWN_PROPERTY,
-                    "property '%s': WriteProperty Unknown!\n",
-                    bactext_property_name(rpdata.object_property));
-            }
-        }
-        pOptional++;
-    }
-    /* check for unsupported property - use ALL */
-    rpdata.object_property = PROP_ALL;
-    len = Time_Value_Read_Property(&rpdata);
-    zassert_equal(len, BACNET_STATUS_ERROR, NULL);
-    status = Time_Value_Write_Property(&wpdata);
-    zassert_false(status, NULL);
     /* check the delete function */
-    status = Time_Value_Delete(instance);
+    status = Time_Value_Delete(object_instance);
     zassert_true(status, NULL);
-
-    return;
 }
 /**
  * @}

zephyr/tests/bacnet/basic/object/time_value/CMakeLists.txt

@@ -7,26 +7,31 @@ string(REGEX REPLACE
   "/zephyr/tests/[a-zA-Z_/-]*$" ""
   BACNET_BASE
   ${CMAKE_CURRENT_SOURCE_DIR})
-string(REGEX REPLACE
-  "/zephyr/tests/" "/src/"
-  BACNET_SRC_PATH
-  ${CMAKE_CURRENT_SOURCE_DIR})
 string(REGEX REPLACE
   "/zephyr/tests/" "/test/"
   BACNET_TEST_PATH
   ${CMAKE_CURRENT_SOURCE_DIR})
+string(REGEX REPLACE
+  "/zephyr/tests/" "/src/"
+  BACNET_SRC_PATH
+  ${CMAKE_CURRENT_SOURCE_DIR})
 get_filename_component(BACNET_NAME ${BACNET_BASE} NAME)
 
 # Update include path for this module
 list(APPEND BACNET_INCLUDE ${BACNET_BASE}/src)
 
+set(TEST_OBJECT_SRC ${BACNET_BASE}/test/bacnet/basic/object)
+list(APPEND TEST_OBJECT_INCLUDE ${TEST_OBJECT_SRC})
+
 if(BOARD STREQUAL unit_testing)
   file(RELATIVE_PATH BACNET_INCLUDE $ENV{ZEPHYR_BASE} ${BACNET_BASE}/src)
-  list(APPEND INCLUDE ${BACNET_INCLUDE})
+file(RELATIVE_PATH TEST_OBJECT_INCLUDE $ENV{ZEPHYR_BASE} ${TEST_OBJECT_SRC})
+  list(APPEND INCLUDE ${BACNET_INCLUDE} ${TEST_OBJECT_INCLUDE})
   list(APPEND SOURCES
     ${BACNET_SRC_PATH}.c
     ${BACNET_TEST_PATH}/src/main.c
     ${BACNET_TEST_PATH}/stubs.c
+    ${TEST_OBJECT_SRC}/property_test.c
     ${BACNET_TEST_PATH}/../mock/device_mock.c
     )
 
@@ -51,11 +56,12 @@ if(BOARD STREQUAL unit_testing)
     ${BACNET_SRC}/lighting.c
     ${BACNET_SRC}/proplist.c
     ${BACNET_SRC}/wp.c
+    ${BACNET_SRC}/bactimevalue.c
     ${BACNET_SRC}/hostnport.c
-    ${BACNET_SRC}/calendar_entry.c
     ${BACNET_SRC}/dailyschedule.c
-    ${BACNET_SRC}/special_event.c
     ${BACNET_SRC}/weeklyschedule.c
+    ${BACNET_SRC}/calendar_entry.c
+    ${BACNET_SRC}/special_event.c
     ${BACNET_SRC}/basic/sys/bigend.c
     ${BACNET_SRC}/bactimevalue.c
     )
@@ -67,9 +73,12 @@ else()
   find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
   project(${BACNET_NAME})
 
-  target_include_directories(app PRIVATE ${BACNET_INCLUDE})
+  target_include_directories(app PRIVATE 
+    ${BACNET_INCLUDE}
+    ${TEST_OBJECT_INCLUDE})
   target_sources(app PRIVATE
     ${BACNET_TEST_PATH}/src/main.c
+    ${TEST_OBJECT_SRC}/property_test.c
     )
 endif()