From 88b44c647508b45ad89b3e4b8dc145c91d1bf773 Mon Sep 17 00:00:00 2001 From: RobertoRoos Date: Fri, 13 Sep 2024 12:27:20 +0200 Subject: [PATCH 1/5] Added proper string sizing incl. null terminator --- pyads/pyads_ex.py | 11 +++++------ pyads/symbol.py | 4 ++++ tests/test_symbol.py | 2 +- 3 files changed, 10 insertions(+), 7 deletions(-) diff --git a/pyads/pyads_ex.py b/pyads/pyads_ex.py index 6a7e4459..ee80b481 100644 --- a/pyads/pyads_ex.py +++ b/pyads/pyads_ex.py @@ -835,11 +835,10 @@ def adsSyncReadReqEx2( index_group_c = ctypes.c_ulong(index_group) index_offset_c = ctypes.c_ulong(index_offset) - if type_is_string(data_type): - data = (STRING_BUFFER * PLCTYPE_STRING)() - elif type_is_wstring(data_type): + if type_is_wstring(data_type): data = (STRING_BUFFER * ctypes.c_uint8)() else: + # Regular string types already contain size too, rely on this type: data = data_type() data_pointer = ctypes.pointer(data) @@ -861,11 +860,11 @@ def adsSyncReadReqEx2( if error_code: raise ADSError(error_code) - # If we're reading a value of predetermined size (anything but a string or wstring), - # validate that the correct number of bytes were read + # If we're reading a value of predetermined size (anything but wstring, regular + # strings also contain size), validate that the correct number of bytes were read if ( check_length - and not(type_is_string(data_type) or type_is_wstring(data_type)) + and not type_is_wstring(data_type) and bytes_read.value != data_length.value ): raise RuntimeError( diff --git a/pyads/symbol.py b/pyads/symbol.py index 762cc99a..b7d40eb7 100644 --- a/pyads/symbol.py +++ b/pyads/symbol.py @@ -345,6 +345,10 @@ def get_type_from_str(type_str: str) -> Optional[Type[PLCDataType]]: scalar_type = AdsSymbol.get_type_from_str(scalar_type_str) if scalar_type: + if scalar_type == constants.PLCTYPE_STRING: + # E.g. `STRING(80)` actually has a size of 81 including the string + # terminator, so add one to the size + size = size + 1 return scalar_type * size # We allow unmapped types at this point - Instead we will throw an diff --git a/tests/test_symbol.py b/tests/test_symbol.py index d6200a87..fbd84afd 100644 --- a/tests/test_symbol.py +++ b/tests/test_symbol.py @@ -617,7 +617,7 @@ def test_arrays(self): def test_string(self): type_str = 'STRING(80)' # This is how a string might appear plc_type = AdsSymbol.get_type_from_str(type_str) - self.assertSizeOf(plc_type, 1 * 80) + self.assertSizeOf(plc_type, 1 * 81) if __name__ == "__main__": From 7b5b3681dcf437144fb085676f2d71174d57ba1f Mon Sep 17 00:00:00 2001 From: RobertoRoos Date: Fri, 13 Sep 2024 13:12:09 +0200 Subject: [PATCH 2/5] Also updated for WSTRING --- pyads/constants.py | 5 +---- pyads/pyads_ex.py | 27 ++++++++++----------------- pyads/symbol.py | 6 ++---- 3 files changed, 13 insertions(+), 25 deletions(-) diff --git a/pyads/constants.py b/pyads/constants.py index 9ca252f4..f771f82b 100644 --- a/pyads/constants.py +++ b/pyads/constants.py @@ -31,10 +31,6 @@ MAX_ADS_SUB_COMMANDS: int = 500 -class PLCTYPE_WSTRING: - """Special dummy class for handling WSTRING.""" - - # plc data types: PLCTYPE_BOOL = c_bool PLCTYPE_BYTE = c_ubyte @@ -45,6 +41,7 @@ class PLCTYPE_WSTRING: PLCTYPE_REAL = c_float PLCTYPE_SINT = c_int8 PLCTYPE_STRING = c_char +PLCTYPE_WSTRING = c_wchar PLCTYPE_TOD = c_int32 PLCTYPE_UBYTE = c_ubyte PLCTYPE_UDINT = c_uint32 diff --git a/pyads/pyads_ex.py b/pyads/pyads_ex.py index ee80b481..c53d1380 100644 --- a/pyads/pyads_ex.py +++ b/pyads/pyads_ex.py @@ -240,6 +240,11 @@ def type_is_wstring(plc_type: Type) -> bool: if plc_type == PLCTYPE_WSTRING: return True + # If char array + if type(plc_type).__name__ == "PyCArrayType": + if plc_type._type_ == PLCTYPE_WSTRING: + return True + return False @@ -264,13 +269,7 @@ def get_value_from_ctype_data(read_data: Optional[Any], plc_type: Type) -> Any: return read_data.value.decode("utf-8") if type_is_wstring(plc_type): - for ix in range(1, len(read_data), 2): - if (read_data[ix - 1], read_data[ix]) == (0, 0): - null_idx = ix - 1 - break - else: - raise ValueError("No null-terminator found in buffer") - return bytearray(read_data[:null_idx]).decode("utf-16-le") + return read_data.value # ctypes automatically decoded the string for us if type(plc_type).__name__ == "PyCArrayType": return list(read_data) @@ -835,11 +834,9 @@ def adsSyncReadReqEx2( index_group_c = ctypes.c_ulong(index_group) index_offset_c = ctypes.c_ulong(index_offset) - if type_is_wstring(data_type): - data = (STRING_BUFFER * ctypes.c_uint8)() - else: - # Regular string types already contain size too, rely on this type: - data = data_type() + # Strings were handled specifically before, but their sizes are contained and we + # can proceed as normal: + data = data_type() data_pointer = ctypes.pointer(data) data_length = ctypes.c_ulong(ctypes.sizeof(data)) @@ -862,11 +859,7 @@ def adsSyncReadReqEx2( # If we're reading a value of predetermined size (anything but wstring, regular # strings also contain size), validate that the correct number of bytes were read - if ( - check_length - and not type_is_wstring(data_type) - and bytes_read.value != data_length.value - ): + if check_length and bytes_read.value != data_length.value: raise RuntimeError( "Insufficient data (expected {0} bytes, {1} were read).".format( data_length.value, bytes_read.value diff --git a/pyads/symbol.py b/pyads/symbol.py index b7d40eb7..8ee2e907 100644 --- a/pyads/symbol.py +++ b/pyads/symbol.py @@ -299,9 +299,7 @@ def get_type_from_str(type_str: str) -> Optional[Type[PLCDataType]]: # If simple scalar plc_name = "PLCTYPE_" + type_str - # if type is WSTRING just return the PLCTYPE constant - if plc_name.startswith("PLCTYPE_WSTRING"): - return constants.PLCTYPE_WSTRING + # WSTRING used to be captured specifically but is now handled as normal if hasattr(constants, plc_name): # Map e.g. 'LREAL' to 'PLCTYPE_LREAL' directly based on the name @@ -345,7 +343,7 @@ def get_type_from_str(type_str: str) -> Optional[Type[PLCDataType]]: scalar_type = AdsSymbol.get_type_from_str(scalar_type_str) if scalar_type: - if scalar_type == constants.PLCTYPE_STRING: + if scalar_type in [constants.PLCTYPE_STRING, constants.PLCTYPE_WSTRING]: # E.g. `STRING(80)` actually has a size of 81 including the string # terminator, so add one to the size size = size + 1 From c4a4e9a9502579a7f34aac82445d407e58d3d773 Mon Sep 17 00:00:00 2001 From: RobertoRoos Date: Fri, 13 Sep 2024 13:17:09 +0200 Subject: [PATCH 3/5] Fixing unit tests --- pyads/connection.py | 3 ++- pyads/pyads_ex.py | 13 +++++++++---- tests/test_connection_class.py | 10 ++++------ 3 files changed, 15 insertions(+), 11 deletions(-) diff --git a/pyads/connection.py b/pyads/connection.py index bfe4a550..0e8da110 100644 --- a/pyads/connection.py +++ b/pyads/connection.py @@ -50,6 +50,7 @@ ads_type_to_ctype, PLCSimpleDataType, PLCDataType, + STRING_BUFFER, ) from .filetimes import filetime_to_dt from .pyads_ex import ( @@ -445,7 +446,7 @@ def get_all_symbols(self) -> List[AdsSymbol]: symbol_size_msg = self.read( ADSIGRP_SYM_UPLOADINFO2, ADSIOFFS_DEVDATA_ADSSTATE, - PLCTYPE_STRING, + PLCTYPE_STRING * STRING_BUFFER, return_ctypes=True, ) sym_count = struct.unpack("I", symbol_size_msg[0:4])[0] diff --git a/pyads/pyads_ex.py b/pyads/pyads_ex.py index c53d1380..aae756db 100644 --- a/pyads/pyads_ex.py +++ b/pyads/pyads_ex.py @@ -269,7 +269,8 @@ def get_value_from_ctype_data(read_data: Optional[Any], plc_type: Type) -> Any: return read_data.value.decode("utf-8") if type_is_wstring(plc_type): - return read_data.value # ctypes automatically decoded the string for us + # `read_data.value` also exists, but could be wrong - explicitly decode instead: + return bytes(read_data).decode("utf-16-le").rstrip("\x00") if type(plc_type).__name__ == "PyCArrayType": return list(read_data) @@ -857,9 +858,13 @@ def adsSyncReadReqEx2( if error_code: raise ADSError(error_code) - # If we're reading a value of predetermined size (anything but wstring, regular - # strings also contain size), validate that the correct number of bytes were read - if check_length and bytes_read.value != data_length.value: + # If we're reading a value of predetermined size (anything but strings, which can be shorted + # because of null-termination), validate that the correct number of bytes were read + if ( + check_length + and not (type_is_string(data_type) or type_is_wstring(data_type)) + and bytes_read.value != data_length.value + ): raise RuntimeError( "Insufficient data (expected {0} bytes, {1} were read).".format( data_length.value, bytes_read.value diff --git a/tests/test_connection_class.py b/tests/test_connection_class.py index 710eabab..9d1a8557 100644 --- a/tests/test_connection_class.py +++ b/tests/test_connection_class.py @@ -231,11 +231,9 @@ def test_read_string(self): # Assert that the server received the correct command self.assert_command_id(requests[0], constants.ADSCOMMAND_READ) - # The string buffer is 1024 bytes long, this will be filled with \x0F - # and null terminated with \x00 by our test server. The \x00 will get - # chopped off during parsing to python string type - expected_result = "\x0F" * 1023 - self.assertEqual(result, expected_result) + # We are reading only a single character, which the test-server defaults at 0 + expected_result = "\x00" + self.assertEqual(expected_result, result) def test_write_uint(self): value = 100 @@ -1451,7 +1449,7 @@ def test_read_wstring(self): var = PLCVariable( "wstr", expected1.encode("utf-16-le") + b"\x00\x00", - constants.ADST_WSTRING, "WSTRING" + constants.ADST_WSTRING, "WSTRING(80)" ) self.handler.add_variable(var) From 0682e6c8f6123e566543bf36f51784057bf254ee Mon Sep 17 00:00:00 2001 From: RobertoRoos Date: Fri, 13 Sep 2024 14:57:05 +0200 Subject: [PATCH 4/5] Fixed string detection during notification callback --- pyads/connection.py | 9 ++++++--- pyads/pyads_ex.py | 6 +++++- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/pyads/connection.py b/pyads/connection.py index 0e8da110..4e02e937 100644 --- a/pyads/connection.py +++ b/pyads/connection.py @@ -77,6 +77,9 @@ adsSyncDelDeviceNotificationReqEx, adsSyncSetTimeoutEx, ADSError, + get_value_from_ctype_data, + type_is_wstring, + type_is_string, ) from .structs import ( AmsAddr, @@ -1019,9 +1022,9 @@ def parse_notification( addressof(contents) + SAdsNotificationHeader.data.offset ) value: Any - if plc_datatype == PLCTYPE_STRING: - # read only until null-termination character - value = bytearray(data).split(b"\0", 1)[0].decode("utf-8") + if type_is_string(plc_datatype) or type_is_wstring(plc_datatype): + # Re-use string parsing from pyads_ex: (but doesn't work for other types) + value = get_value_from_ctype_data(data, plc_datatype) elif plc_datatype is not None and issubclass(plc_datatype, Structure): value = plc_datatype() diff --git a/pyads/pyads_ex.py b/pyads/pyads_ex.py index aae756db..bec56a94 100644 --- a/pyads/pyads_ex.py +++ b/pyads/pyads_ex.py @@ -266,7 +266,11 @@ def get_value_from_ctype_data(read_data: Optional[Any], plc_type: Type) -> Any: return None if type_is_string(plc_type): - return read_data.value.decode("utf-8") + if hasattr(read_data, "value"): + return read_data.value.decode("utf-8") + return bytes(read_data).decode("utf-8").rstrip("\x00") + # `read_data.value` does not always exist, and without it all the null + # terminators needs to be removed after decoding if type_is_wstring(plc_type): # `read_data.value` also exists, but could be wrong - explicitly decode instead: From 0a259d8e2deea4789cf083728a2bf9287381be2a Mon Sep 17 00:00:00 2001 From: RobertoRoos Date: Fri, 13 Sep 2024 15:20:21 +0200 Subject: [PATCH 5/5] Added case for literal (W)STRING to be handled by a presized buffer instead of a single character --- pyads/connection.py | 3 +-- pyads/pyads_ex.py | 6 +++++- tests/test_connection_class.py | 8 +++++--- 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/pyads/connection.py b/pyads/connection.py index 4e02e937..fd0f8133 100644 --- a/pyads/connection.py +++ b/pyads/connection.py @@ -50,7 +50,6 @@ ads_type_to_ctype, PLCSimpleDataType, PLCDataType, - STRING_BUFFER, ) from .filetimes import filetime_to_dt from .pyads_ex import ( @@ -449,7 +448,7 @@ def get_all_symbols(self) -> List[AdsSymbol]: symbol_size_msg = self.read( ADSIGRP_SYM_UPLOADINFO2, ADSIOFFS_DEVDATA_ADSSTATE, - PLCTYPE_STRING * STRING_BUFFER, + PLCTYPE_STRING, return_ctypes=True, ) sym_count = struct.unpack("I", symbol_size_msg[0:4])[0] diff --git a/pyads/pyads_ex.py b/pyads/pyads_ex.py index bec56a94..b39edb7d 100644 --- a/pyads/pyads_ex.py +++ b/pyads/pyads_ex.py @@ -840,7 +840,11 @@ def adsSyncReadReqEx2( index_offset_c = ctypes.c_ulong(index_offset) # Strings were handled specifically before, but their sizes are contained and we - # can proceed as normal: + # can proceed as normal + if data_type == PLCTYPE_STRING or data_type == PLCTYPE_WSTRING: + # This implies a string of size 1, which is so are we instead use a large fixed + # size buffer: + data_type = data_type * STRING_BUFFER data = data_type() data_pointer = ctypes.pointer(data) diff --git a/tests/test_connection_class.py b/tests/test_connection_class.py index 9d1a8557..d907ec02 100644 --- a/tests/test_connection_class.py +++ b/tests/test_connection_class.py @@ -231,9 +231,11 @@ def test_read_string(self): # Assert that the server received the correct command self.assert_command_id(requests[0], constants.ADSCOMMAND_READ) - # We are reading only a single character, which the test-server defaults at 0 - expected_result = "\x00" - self.assertEqual(expected_result, result) + # The string buffer is 1024 bytes long, this will be filled with \x0F + # and null terminated with \x00 by our test server. The \x00 will get + # chopped off during parsing to python string type + expected_result = "\x0F" * 1023 + self.assertEqual(result, expected_result) def test_write_uint(self): value = 100