From 3690dae27876b52f03bc5d40e942b6f3a5277f22 Mon Sep 17 00:00:00 2001 From: Henrik Palmlund Wahlgren Date: Sun, 17 May 2020 15:23:38 +0200 Subject: [PATCH 1/2] fix: Proper handling of baudrate switching of the serial transport and fixes in command message validation. Fixes #7 --- iec62056_21/client.py | 33 ++++++++++++++++++++++----------- iec62056_21/messages.py | 37 +++++++++++++++++++++++-------------- iec62056_21/transports.py | 8 +++++--- tests/test_data.py | 28 +++++++++++++++------------- 4 files changed, 65 insertions(+), 41 deletions(-) diff --git a/iec62056_21/client.py b/iec62056_21/client.py index 7e3e3a8..756d4ae 100644 --- a/iec62056_21/client.py +++ b/iec62056_21/client.py @@ -55,17 +55,18 @@ def __init__( self.password = password self.battery_powered = battery_powered self.identification = None - self.switchover_baudrate_char = None + self._switchover_baudrate_char = None self.manufacturer_id = None self.use_short_reaction_time = False self.error_parser = error_parser_class() + self._current_baudrate: int = 300 @property def switchover_baudrate(self): """ - Shoirtcut to get the baud rate for the switchover. + Shortcut to get the baud rate for the switchover. """ - return self.BAUDRATES_MODE_C.get(self.switchover_baudrate_char) + return self.BAUDRATES_MODE_C.get(self._switchover_baudrate_char) def read_single_value(self, address, additional_data="1"): """ @@ -76,8 +77,12 @@ def read_single_value(self, address, additional_data="1"): :return: """ # TODO Can't find documentation on why the additional_data of 1 is needed. + # LIS-200 Specific? - request = messages.CommandMessage.for_single_read(address, additional_data) + # TODO: When not using the additional data on an EMH meter we get an ack back. + # a bit later we get the break message. Is the device waiting? + + request = messages.CommandMessage.for_single_read(address) logger.info(f"Sending read request: {request}") self.transport.send(request.to_bytes()) @@ -145,8 +150,8 @@ def startup(self): ident_msg = self.read_identification() # Setting the baudrate to the one propsed by the device. - self.switchover_baudrate_char = str(ident_msg.switchover_baudrate_char) - + self._switchover_baudrate_char = ident_msg.switchover_baudrate_char + print(self.BAUDRATES_MODE_C[self._switchover_baudrate_char]) self.identification = ident_msg.identification self.manufacturer_id = ident_msg.manufacturer @@ -165,8 +170,6 @@ def access_programming_mode(self): self.ack_with_option_select("programming") - self.transport.switch_baudrate(self.switchover_baudrate) - # receive password request pw_req = self.read_response() @@ -178,7 +181,6 @@ def standard_readout(self): """ self.startup() self.ack_with_option_select("readout") - self.transport.switch_baudrate(self.switchover_baudrate) logger.info(f"Reading standard readout from device.") response = self.read_response() return response @@ -201,7 +203,9 @@ def send_break(self): communication. """ logger.info("Sending BREAK message to end communication") - break_msg = messages.CommandMessage(command="B", command_type=0, data_set=None) + break_msg = messages.CommandMessage( + command="B", command_type="0", data_set=None + ) self.transport.send(break_msg.to_bytes()) def ack_with_option_select(self, mode): @@ -213,13 +217,20 @@ def ack_with_option_select(self, mode): :param mode: """ + # TODO: allow the client to suggest a new baudrate to the devices instead of + # the devices proposed one. + mode_char = self.MODE_CONTROL_CHARACTER[mode] + ack_message = messages.AckOptionSelectMessage( - mode_char=mode_char, baud_char=self.switchover_baudrate_char + mode_char=mode_char, baud_char=self._switchover_baudrate_char ) logger.info(f"Sending AckOptionsSelect message: {ack_message}") self.transport.send(ack_message.to_bytes()) self.rest() + self.transport.switch_baudrate( + baud=self.BAUDRATES_MODE_C[self._switchover_baudrate_char] + ) def send_init_request(self): """ diff --git a/iec62056_21/messages.py b/iec62056_21/messages.py index 8e5a564..49a1e47 100644 --- a/iec62056_21/messages.py +++ b/iec62056_21/messages.py @@ -50,7 +50,7 @@ class DataSet(Iec6205621Data): EXCLUDE_CHARS = ["(", ")", "/", "!"] - def __init__(self, address, value, unit=None, no_address=False): + def __init__(self, value: str, address: str = None, unit: str = None): # TODO: in programming mode, protocol mode C the value can be up to 128 chars @@ -58,11 +58,16 @@ def __init__(self, address, value, unit=None, no_address=False): self.value = value self.unit = unit - def to_representation(self): - if self.unit: + def to_representation(self) -> str: + if self.unit is not None and self.address is not None: return f"{self.address}({self.value}*{self.unit})" - else: + elif self.address is not None and self.unit is None: return f"{self.address}({self.value})" + else: + if self.value is None: + return f"()" + else: + return f"({self.value})" @classmethod def from_representation(cls, data_set_string): @@ -90,8 +95,8 @@ def from_representation(cls, data_set_string): def __repr__(self): return ( f"{self.__class__.__name__}(" - f"address={self.address!r}, " f"value={self.value!r}, " + f"address={self.address!r}, " f"unit={self.unit!r}" f")" ) @@ -187,9 +192,11 @@ def __repr__(self): class CommandMessage(Iec6205621Data): allowed_commands = ["P", "W", "R", "E", "B"] - allowed_command_types = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] + allowed_command_types = ["0", "1", "2", "3", "4", "5", "6", "7", "8", "9"] - def __init__(self, command, command_type, data_set): + def __init__( + self, command: str, command_type: str, data_set: typing.Optional[DataSet] + ): self.command = command self.command_type = command_type self.data_set = data_set @@ -219,7 +226,7 @@ def from_representation(cls, string_data): body = _message[3:] command = header[1] - command_type = int(header[2]) + command_type = header[2] data_set = DataSet.from_representation(body[1:-1]) return cls(command, command_type, data_set) @@ -231,12 +238,12 @@ def for_single_read(cls, address, additional_data=None): else: _add_data = "" data_set = DataSet(value=_add_data, address=address) - return cls(command="R", command_type=1, data_set=data_set) + return cls(command="R", command_type="1", data_set=data_set) @classmethod def for_single_write(cls, address, value): data_set = DataSet(value=value, address=address) - return cls(command="W", command_type=1, data_set=data_set) + return cls(command="W", command_type="1", data_set=data_set) def __repr__(self): return ( @@ -339,10 +346,12 @@ def __repr__(self): class IdentificationMessage(Iec6205621Data): - def __init__(self, identification, manufacturer, switchover_baudrate_char): - self.identification = identification - self.manufacturer = manufacturer - self.switchover_baudrate_char = switchover_baudrate_char + def __init__( + self, identification: str, manufacturer: str, switchover_baudrate_char: str + ): + self.identification: str = identification + self.manufacturer: str = manufacturer + self.switchover_baudrate_char: str = switchover_baudrate_char def to_representation(self): return ( diff --git a/iec62056_21/transports.py b/iec62056_21/transports.py index 98caa49..a2da05a 100644 --- a/iec62056_21/transports.py +++ b/iec62056_21/transports.py @@ -53,6 +53,8 @@ def read(self, timeout=None): start_time = time.time() while True: b = self.recv(1) + logger.debug(f"{b!r}") + duration = time.time() - start_time if duration > self.timeout: raise TimeoutError(f"Read in {self.__class__.__name__} timed out") @@ -223,13 +225,13 @@ def __init__(self, port, timeout=10): self.port_name = port self.port = None - def connect(self): + def connect(self, baudrate: int = 300): """ Creates a serial port. """ self.port = serial.Serial( self.port_name, - baudrate=300, + baudrate=baudrate, parity=serial.PARITY_EVEN, stopbits=serial.STOPBITS_ONE, bytesize=serial.SEVENBITS, @@ -270,7 +272,7 @@ def switch_baudrate(self, baud): :param baud: """ - time.sleep(0.5) + logger.info(f"Switching baudrate to: {baud}") self.port = self.port = serial.Serial( self.port_name, baudrate=baud, diff --git a/tests/test_data.py b/tests/test_data.py index 061ea35..44b098d 100644 --- a/tests/test_data.py +++ b/tests/test_data.py @@ -34,14 +34,14 @@ def test_from_string_without_unit(self): ds = messages.DataSet.from_representation(self.data_set_without_unit) assert ds.value == "100" assert ds.address == "3.1.0" - assert ds.unit == None + assert ds.unit is None def test_from_bytes_without_unit(self): ds_bytes = self.data_set_without_unit.encode("latin-1") ds = messages.DataSet.from_representation(self.data_set_without_unit) assert ds.value == "100" assert ds.address == "3.1.0" - assert ds.unit == None + assert ds.unit is None def test_to_string_without_unit(self): ds = messages.DataSet(value="100", address="3.1.0", unit=None) @@ -224,8 +224,8 @@ class TestCommandMessage: def test_command_message_to_representation(self): cm = messages.CommandMessage( command="R", - command_type=1, - data_set=messages.DataSet(address="1.8.0", value=1), + command_type="1", + data_set=messages.DataSet(address="1.8.0", value="1"), ) assert cm.to_representation() == "\x01R1\x021.8.0(1)\x03k" @@ -235,7 +235,7 @@ def test_from_representation(self): cm = messages.CommandMessage.from_representation(data) assert cm.command == "P" - assert cm.command_type == 0 + assert cm.command_type == "0" assert cm.data_set.value == "1234567" assert cm.data_set.address is None assert cm.data_set.unit is None @@ -244,22 +244,24 @@ def test_invalid_command_raises_value_error(self): with pytest.raises(ValueError): cm = messages.CommandMessage( command="X", - command_type=1, - data_set=messages.DataSet(address="1.8.0", value=1), + command_type="1", + data_set=messages.DataSet(address="1.8.0", value="1"), ) def test_invalid_command_type_raises_value_error(self): with pytest.raises(ValueError): cm = messages.CommandMessage( command="R", - command_type=12, - data_set=messages.DataSet(address="1.8.0", value=1), + command_type="12", + data_set=messages.DataSet(address="1.8.0", value="1"), ) def test_to_representation_without_data_set(self): # like the break command - break_msg = messages.CommandMessage(command="B", command_type=0, data_set=None) + break_msg = messages.CommandMessage( + command="B", command_type="0", data_set=None + ) assert break_msg.to_representation() == "\x01B0\x03q" @@ -272,7 +274,7 @@ def test_for_single_read(self): cm = messages.CommandMessage.for_single_read(address="1.8.0") assert cm.command == "R" - assert cm.command_type == 1 + assert cm.command_type == "1" assert cm.data_set.address == "1.8.0" cm = messages.CommandMessage.for_single_read( @@ -280,7 +282,7 @@ def test_for_single_read(self): ) assert cm.command == "R" - assert cm.command_type == 1 + assert cm.command_type == "1" assert cm.data_set.address == "1.8.0" assert cm.data_set.value == "1" @@ -288,7 +290,7 @@ def test_for_single_write(self): cm = messages.CommandMessage.for_single_write(address="1.8.0", value="123") assert cm.command == "W" - assert cm.command_type == 1 + assert cm.command_type == "1" assert cm.data_set.address == "1.8.0" assert cm.data_set.value == "123" From d09071e40c772ea48433c40a95446caf771fc9c0 Mon Sep 17 00:00:00 2001 From: Henrik Palmlund Wahlgren Date: Mon, 18 May 2020 11:24:26 +0200 Subject: [PATCH 2/2] fix: Allowed sending of device address on all transport. Raising exception if a transport requires address and non is given. Added typings to transports.py Fixes #9 --- iec62056_21/client.py | 15 +++---- iec62056_21/exceptions.py | 4 ++ iec62056_21/transports.py | 86 +++++++++++++++++++++++++-------------- tests/test_client.py | 22 ++++++++++ 4 files changed, 90 insertions(+), 37 deletions(-) create mode 100644 tests/test_client.py diff --git a/iec62056_21/client.py b/iec62056_21/client.py index 756d4ae..9220167 100644 --- a/iec62056_21/client.py +++ b/iec62056_21/client.py @@ -61,6 +61,12 @@ def __init__( self.error_parser = error_parser_class() self._current_baudrate: int = 300 + if self.transport.TRANSPORT_REQUIRES_ADDRESS and not self.device_address: + raise exceptions.Iec6205621ClientError( + f"The transported used ({self.transport}) requires a device address " + f"and none was supplied." + ) + @property def switchover_baudrate(self): """ @@ -82,7 +88,7 @@ def read_single_value(self, address, additional_data="1"): # TODO: When not using the additional data on an EMH meter we get an ack back. # a bit later we get the break message. Is the device waiting? - request = messages.CommandMessage.for_single_read(address) + request = messages.CommandMessage.for_single_read(address, additional_data) logger.info(f"Sending read request: {request}") self.transport.send(request.to_bytes()) @@ -151,7 +157,6 @@ def startup(self): # Setting the baudrate to the one propsed by the device. self._switchover_baudrate_char = ident_msg.switchover_baudrate_char - print(self.BAUDRATES_MODE_C[self._switchover_baudrate_char]) self.identification = ident_msg.identification self.manufacturer_id = ident_msg.manufacturer @@ -241,11 +246,7 @@ def send_init_request(self): you want to talk to by adding the address in the request. """ - if self.transport.TRANSPORT_REQUIRES_ADDRESS: - request = messages.RequestMessage(device_address=self.device_address) - else: - request = messages.RequestMessage() - + request = messages.RequestMessage(device_address=self.device_address) logger.info(f"Sending request message: {request}") self.transport.send(request.to_bytes()) self.rest() diff --git a/iec62056_21/exceptions.py b/iec62056_21/exceptions.py index 858a96f..c8eee46 100644 --- a/iec62056_21/exceptions.py +++ b/iec62056_21/exceptions.py @@ -2,6 +2,10 @@ class Iec6205621Exception(Exception): """General IEC62056-21 Exception""" +class Iec6205621ClientError(Iec6205621Exception): + """Client error""" + + class Iec6205621ParseError(Iec6205621Exception): """Error in parsing IEC62056-21 data""" diff --git a/iec62056_21/transports.py b/iec62056_21/transports.py index a2da05a..75ce707 100644 --- a/iec62056_21/transports.py +++ b/iec62056_21/transports.py @@ -1,5 +1,6 @@ import time import logging +from typing import Tuple, Union, Optional import serial import socket @@ -17,18 +18,18 @@ class BaseTransport: Base transport class for IEC 62056-21 communication. """ - TRANSPORT_REQUIRES_ADDRESS = True + TRANSPORT_REQUIRES_ADDRESS: bool = True - def __init__(self, timeout=30): + def __init__(self, timeout: int = 30): self.timeout = timeout - def connect(self): + def connect(self) -> None: raise NotImplemented("Must be defined in subclass") - def disconnect(self): + def disconnect(self) -> None: raise NotImplemented("Must be defined in subclass") - def read(self, timeout=None): + def read(self, timeout: Optional[int] = None) -> bytes: """ Will read a normal readout. Supports both full and partial block readout. When using partial blocks it will recreate the messages as it was not sent with @@ -49,11 +50,10 @@ def read(self, timeout=None): while True: in_data = b"" - duration = 0 + duration: float = 0.0 start_time = time.time() while True: b = self.recv(1) - logger.debug(f"{b!r}") duration = time.time() - start_time if duration > self.timeout: @@ -130,7 +130,12 @@ def read(self, timeout=None): return total_data - def simple_read(self, start_char, end_char, timeout=None): + def simple_read( + self, + start_char: Union[str, bytes], + end_char: Union[str, bytes], + timeout: Optional[int] = None, + ) -> bytes: """ A more flexible read for use with some messages. """ @@ -140,7 +145,7 @@ def simple_read(self, start_char, end_char, timeout=None): in_data = b"" start_char_received = False timeout = timeout or self.timeout - duration = 0 + duration: float = 0.0 start_time = time.time() while True: b = self.recv(1) @@ -167,7 +172,7 @@ def simple_read(self, start_char, end_char, timeout=None): logger.debug(f"Received {in_data!r} over transport: {self.__class__.__name__}") return in_data - def send(self, data: bytes): + def send(self, data: bytes) -> None: """ Will send data over the transport @@ -176,7 +181,7 @@ def send(self, data: bytes): self._send(data) logger.debug(f"Sent {data!r} over transport: {self.__class__.__name__}") - def _send(self, data: bytes): + def _send(self, data: bytes) -> None: """ Transport dependant sending functionality. @@ -184,7 +189,7 @@ def _send(self, data: bytes): """ raise NotImplemented("Must be defined in subclass") - def recv(self, chars): + def recv(self, chars: int) -> bytes: """ Will receive data over the transport. @@ -192,7 +197,7 @@ def recv(self, chars): """ return self._recv(chars) - def _recv(self, chars): + def _recv(self, chars: int) -> bytes: """ Transport dependant sending functionality. @@ -200,7 +205,7 @@ def _recv(self, chars): """ raise NotImplemented("Must be defined in subclass") - def switch_baudrate(self, baud): + def switch_baudrate(self, baud: int) -> None: """ The protocol defines a baudrate switchover process. Though it might not be used in all available transports. @@ -219,13 +224,13 @@ class SerialTransport(BaseTransport): TRANSPORT_REQUIRES_ADDRESS = False - def __init__(self, port, timeout=10): + def __init__(self, port: str, timeout: int = 10): super().__init__(timeout=timeout) - self.port_name = port - self.port = None + self.port_name: str = port + self.port: Optional[serial.Serial] = None - def connect(self, baudrate: int = 300): + def connect(self, baudrate: int = 300) -> None: """ Creates a serial port. """ @@ -242,36 +247,48 @@ def connect(self, baudrate: int = 300): xonxoff=False, ) - def disconnect(self): + def disconnect(self) -> None: """ Closes and removes the serial port. """ + if self.port is None: + raise TransportError("Serial port is closed.") + self.port.close() self.port = None - def _send(self, data: bytes): + def _send(self, data: bytes) -> None: """ Sends data over the serial port. :param data: """ + if self.port is None: + raise TransportError("Serial port is closed.") + self.port.write(data) self.port.flush() - def _recv(self, chars=1): + def _recv(self, chars: int = 1) -> bytes: """ Receives data over the serial port. :param chars: """ + if self.port is None: + raise TransportError("Serial port is closed.") + return self.port.read(chars) - def switch_baudrate(self, baud): + def switch_baudrate(self, baud: int) -> None: """ Creates a new serial port with the correct baudrate. :param baud: """ + if self.port is None: + raise TransportError("Serial port is closed.") + logger.info(f"Switching baudrate to: {baud}") self.port = self.port = serial.Serial( self.port_name, @@ -300,13 +317,13 @@ class TcpTransport(BaseTransport): Transport class for TCP/IP communication. """ - def __init__(self, address, timeout=30): + def __init__(self, address: Tuple[str, int], timeout: int = 30): super().__init__(timeout=timeout) self.address = address - self.socket = self._get_socket() + self.socket: Optional[socket.socket] = self._get_socket() - def connect(self): + def connect(self) -> None: """ Connects the socket to the device network interface. """ @@ -316,34 +333,43 @@ def connect(self): logger.debug(f"Connecting to {self.address}") self.socket.connect(self.address) - def disconnect(self): + def disconnect(self) -> None: """ Closes and removes the socket. """ + if self.socket is None: + raise TransportError("Socket is closed") + self.socket.close() self.socket = None - def _send(self, data: bytes): + def _send(self, data: bytes) -> None: """ Sends data over the socket. :param data: """ + if self.socket is None: + raise TransportError("Socket is closed") + self.socket.sendall(data) - def _recv(self, chars=1): + def _recv(self, chars: int = 1) -> bytes: """ Receives data from the socket. :param chars: """ + if self.socket is None: + raise TransportError("Socket is closed") + try: b = self.socket.recv(chars) except (OSError, IOError, socket.timeout, socket.error) as e: raise TransportError from e return b - def switch_baudrate(self, baud): + def switch_baudrate(self, baud: int) -> None: """ Baudrate has not meaning in TCP/IP so we just dont do anything. @@ -351,7 +377,7 @@ def switch_baudrate(self, baud): """ pass - def _get_socket(self): + def _get_socket(self) -> socket.socket: """ Create a correct socket. """ diff --git a/tests/test_client.py b/tests/test_client.py new file mode 100644 index 0000000..c69c8fc --- /dev/null +++ b/tests/test_client.py @@ -0,0 +1,22 @@ +import pytest +from iec62056_21 import exceptions, client, transports + + +class TestIec6205621Client: + def test_with_no_address_when_required_raises_client_error(self): + with pytest.raises(exceptions.Iec6205621ClientError): + c = client.Iec6205621Client.with_tcp_transport(("192.168.1.1", 5000)) + + def test_can_create_client_with_tcp_transport(self): + c = client.Iec6205621Client.with_tcp_transport( + "192.168.1.1", device_address="00000000" + ) + + def test_no_address_when_required_raises_client_error(self): + trans = transports.TcpTransport(address=("192.168.1.1", 5000)) + with pytest.raises(exceptions.Iec6205621ClientError): + c = client.Iec6205621Client(transport=trans) + + def test_can_create_client_tcp_transport(self): + trans = transports.TcpTransport(address=("192.168.1.1", 5000)) + c = client.Iec6205621Client(transport=trans, device_address="00000000")