From 3d8687334608d072544ce3b94fedeaa5c99a95e8 Mon Sep 17 00:00:00 2001 From: Steve Karg Date: Sat, 21 Sep 2024 09:26:09 -0500 Subject: [PATCH] Bugfix/read property multiple client errors (#765) * Fixed variable data type for boolean in RPM structure. * Fixed RPM error handling to use callback. * Fixed bacrpm app example when not enough command line parameters are used * Fixed empty-list EPICS printing. * Fixed RPM-Ack processing for end of list-of-results * Added minimal handling for segmentation-not-supported during RPM of object properties. --- apps/readpropm/main.c | 5 +- src/bacnet/bacapp.c | 5 +- src/bacnet/basic/client/bac-discover.c | 48 +++++++++++++---- src/bacnet/basic/client/bac-rw.c | 22 +++++++- src/bacnet/basic/service/h_rpm_a.c | 11 ++-- src/bacnet/rpm.c | 73 +++++++++++++++++++++----- 6 files changed, 133 insertions(+), 31 deletions(-) diff --git a/apps/readpropm/main.c b/apps/readpropm/main.c index 2f92b9e6..5b7bf244 100644 --- a/apps/readpropm/main.c +++ b/apps/readpropm/main.c @@ -496,7 +496,10 @@ int main(int argc, char *argv[]) } } } - if (tag_value_arg != 0) { + if (!Read_Access_Data) { + print_usage(filename); + return 1; + } else if (tag_value_arg != 0) { fprintf(stderr, "Error: not enough object property triples.\n"); return 1; } diff --git a/src/bacnet/bacapp.c b/src/bacnet/bacapp.c index 777cdc88..10fc7ba9 100644 --- a/src/bacnet/bacapp.c +++ b/src/bacnet/bacapp.c @@ -3278,6 +3278,9 @@ int bacapp_snprintf_value( str, str_len, &value->type.Shed_Level); break; #endif + case BACNET_APPLICATION_TAG_EMPTYLIST: + ret_val = bacapp_snprintf(str, str_len, "{}"); + break; default: ret_val = bacapp_snprintf( str, str_len, "UnknownType(tag=%d)", value->tag); @@ -3922,7 +3925,7 @@ void bacapp_value_list_init(BACNET_APPLICATION_DATA_VALUE *value, size_t count) if (value && count) { for (i = 0; i < count; i++) { value->tag = BACNET_APPLICATION_TAG_NULL; - value->context_specific = 0; + value->context_specific = false; value->context_tag = 0; if ((i + 1) < count) { value->next = value + 1; diff --git a/src/bacnet/basic/client/bac-discover.c b/src/bacnet/basic/client/bac-discover.c index c5817088..d611777f 100644 --- a/src/bacnet/basic/client/bac-discover.c +++ b/src/bacnet/basic/client/bac-discover.c @@ -677,16 +677,20 @@ static void bacnet_device_object_property_add( /** * @brief Handle the error from a ReadProperty or ReadPropertyMultiple * @param device_id - device instance number where data originated - * @param error_code - BACnet Error code + * @param rp_data - ReadProperty data structure + * @param device_data - Pointer to the device data structure */ static void Device_Error_Handler( uint32_t device_id, - BACNET_ERROR_CODE error_code, + BACNET_READ_PROPERTY_DATA *rp_data, BACNET_DEVICE_DATA *device_data) { + bool status = false; + if (device_data) { debug_printf( - "%u - %s\n", device_id, bactext_error_code_name((int)error_code)); + "%u - %s\n", device_id, + bactext_error_code_name((int)rp_data->error_code)); switch (device_data->Discovery_State) { case BACNET_DISCOVER_STATE_OBJECT_LIST_REQUEST: /* resend request */ @@ -697,12 +701,38 @@ static void Device_Error_Handler( BACNET_DISCOVER_STATE_OBJECT_LIST_RESPONSE; break; case BACNET_DISCOVER_STATE_OBJECT_GET_PROPERTY_REQUEST: - /* resend request */ - if (device_data->Object_List_Index != 0) { - device_data->Object_List_Index--; + if (rp_data->error_code == ERROR_CODE_TIMEOUT) { + /* resend request */ + if (device_data->Object_List_Index != 0) { + device_data->Object_List_Index--; + } + device_data->Discovery_State = + BACNET_DISCOVER_STATE_OBJECT_GET_PROPERTY_RESPONSE; + } else if ( + (rp_data->error_code == + ERROR_CODE_ABORT_SEGMENTATION_NOT_SUPPORTED) && + (rp_data->object_property == PROP_ALL)) { + /* fallback to ReadProperty required properties */ + /* FIXME: fill a property-list with properties + and use FSM to ReadProperty of each. + For now, read the object-name property */ + status = bacnet_read_property_queue( + device_id, rp_data->object_type, + rp_data->object_instance, PROP_OBJECT_NAME, + BACNET_ARRAY_ALL); + if (status) { + device_data->Discovery_State = + BACNET_DISCOVER_STATE_OBJECT_GET_PROPERTY_REQUEST; + } else { + /* skip */ + device_data->Discovery_State = + BACNET_DISCOVER_STATE_OBJECT_GET_PROPERTY_RESPONSE; + } + } else { + /* skip */ + device_data->Discovery_State = + BACNET_DISCOVER_STATE_OBJECT_GET_PROPERTY_RESPONSE; } - device_data->Discovery_State = - BACNET_DISCOVER_STATE_OBJECT_GET_PROPERTY_RESPONSE; break; default: break; @@ -733,7 +763,7 @@ static void bacnet_read_property_reply( return; } if (rp_data->error_code != ERROR_CODE_SUCCESS) { - Device_Error_Handler(device_id, rp_data->error_code, device_data); + Device_Error_Handler(device_id, rp_data, device_data); } else if (value) { bacnet_device_object_property_add( device_id, rp_data, value, device_data); diff --git a/src/bacnet/basic/client/bac-rw.c b/src/bacnet/basic/client/bac-rw.c index 07c0fb87..8abe4b48 100644 --- a/src/bacnet/basic/client/bac-rw.c +++ b/src/bacnet/basic/client/bac-rw.c @@ -211,15 +211,29 @@ static void bacnet_read_property_ack_process( BACNET_ARRAY_INDEX array_index = 0; if (rp_data) { + value = &Target_Decoded_Property_Value; + /* check for property error */ if (rp_data->error_code != ERROR_CODE_SUCCESS) { if (bacnet_read_write_value_callback) { bacnet_read_write_value_callback(device_id, rp_data, NULL); } + return; + } + /* check for empty list */ + if (rp_data->application_data_len == 0) { + bacapp_value_list_init(value, 1); + value->tag = BACNET_APPLICATION_TAG_EMPTYLIST; + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_SUCCESS; + if (bacnet_read_write_value_callback) { + bacnet_read_write_value_callback(device_id, rp_data, value); + } + return; } apdu = rp_data->application_data; apdu_len = rp_data->application_data_len; while (apdu_len) { - value = &Target_Decoded_Property_Value; + bacapp_value_list_init(value, 1); len = bacapp_decode_known_property( apdu, (unsigned)apdu_len, value, rp_data->object_type, rp_data->object_property); @@ -251,7 +265,11 @@ static void bacnet_read_property_ack_process( } } else { rp_data->error_class = ERROR_CLASS_SERVICES; - rp_data->error_code = ERROR_CODE_SUCCESS; + if (len < 0) { + rp_data->error_code = ERROR_CODE_OTHER; + } else { + rp_data->error_code = ERROR_CODE_SUCCESS; + } if (bacnet_read_write_value_callback) { bacnet_read_write_value_callback(device_id, rp_data, NULL); } diff --git a/src/bacnet/basic/service/h_rpm_a.c b/src/bacnet/basic/service/h_rpm_a.c index 19ed95dc..525a8910 100644 --- a/src/bacnet/basic/service/h_rpm_a.c +++ b/src/bacnet/basic/service/h_rpm_a.c @@ -103,18 +103,19 @@ int rpm_ack_decode_service_request( decoded_len++; apdu_len--; apdu++; - /* note: if this is an array, there will be - more than one element to decode */ value = calloc(1, sizeof(BACNET_APPLICATION_DATA_VALUE)); rpm_property->value = value; - - /* Special case for an empty array - we decode it as null */ if (apdu_len && decode_is_closing_tag_number(apdu, 4)) { - bacapp_value_list_init(value, 1); + /* Special case for an empty array or list */ + if (value) { + bacapp_value_list_init(value, 1); + value->tag = BACNET_APPLICATION_TAG_EMPTYLIST; + } decoded_len++; apdu_len--; apdu++; } else { + /* one or more (array or list) elements to decode */ while (value && (apdu_len > 0)) { len = bacapp_decode_known_property( apdu, (unsigned)apdu_len, value, diff --git a/src/bacnet/rpm.c b/src/bacnet/rpm.c index 078e69c8..f3f123a6 100644 --- a/src/bacnet/rpm.c +++ b/src/bacnet/rpm.c @@ -663,7 +663,7 @@ void rpm_ack_object_property_process( read_property_ack_process callback) { int len = 0; - uint16_t application_data_len; + int application_data_len; uint32_t error_value = 0; /* decoded error value */ if (!apdu) { @@ -674,21 +674,45 @@ void rpm_ack_object_property_process( } while (apdu_len) { /* object-identifier [0] BACnetObjectIdentifier */ - /* list-of-results [1] SEQUENCE OF SEQUENCE */ + /* list-of-results [1] SEQUENCE OF SEQUENCE */ len = rpm_ack_decode_object_id( apdu, apdu_len, &rp_data->object_type, &rp_data->object_instance); if (len <= 0) { /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } return; } apdu_len -= len; apdu += len; while (apdu_len) { + if (bacnet_is_closing_tag_number(apdu, apdu_len, 1, &len)) { + /* end of list-of-results [1] SEQUENCE OF SEQUENCE */ + apdu_len -= len; + if (apdu_len > 0) { + /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } + return; + } + break; + } len = rpm_ack_decode_object_property( apdu, apdu_len, &rp_data->object_property, &rp_data->array_index); if (len <= 0) { /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } return; } apdu_len -= len; @@ -696,20 +720,33 @@ void rpm_ack_object_property_process( if (bacnet_is_opening_tag_number(apdu, apdu_len, 4, &len)) { application_data_len = bacnet_enclosed_data_length(apdu, apdu_len); + if (application_data_len < 0) { + /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } + return; + } /* propertyValue */ apdu_len -= len; apdu += len; - if (application_data_len) { - rp_data->application_data_len = application_data_len; - rp_data->application_data = apdu; - apdu_len -= application_data_len; - apdu += application_data_len; - } + /* fill the RP application data */ + rp_data->application_data_len = application_data_len; + rp_data->application_data = apdu; + apdu_len -= application_data_len; + apdu += application_data_len; if (bacnet_is_closing_tag_number(apdu, apdu_len, 4, &len)) { apdu_len -= len; apdu += len; } else { /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } return; } rp_data->error_class = ERROR_CLASS_PROPERTY; @@ -729,6 +766,11 @@ void rpm_ack_object_property_process( apdu += len; } else { /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } return; } len = bacnet_enumerated_application_decode( @@ -739,6 +781,11 @@ void rpm_ack_object_property_process( apdu += len; } else { /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } return; } if (bacnet_is_closing_tag_number(apdu, apdu_len, 5, &len)) { @@ -746,6 +793,11 @@ void rpm_ack_object_property_process( apdu += len; } else { /* malformed */ + rp_data->error_class = ERROR_CLASS_SERVICES; + rp_data->error_code = ERROR_CODE_INVALID_TAG; + if (callback) { + callback(device_id, rp_data); + } return; } if (callback) { @@ -753,11 +805,6 @@ void rpm_ack_object_property_process( } } } - len = rpm_decode_object_end(apdu, apdu_len); - if (len) { - apdu_len -= len; - apdu += len; - } } } #endif