Repository: pwitab/iec62056-21

Issue:
Not able to enter programming mode on EMH meter - serial transport
After checking some errors reported by users, #4 I found that it is not possible to enter programming mode on an EMH meter. 

A quick look suggests it is a combined error of the ASCII representation of a command and that we are not preforming a baudrate switch that the meter proposes. (Or telling the meter that we want to stay at our current baudrate.) 

The baudrate issue is only contained to the serial transport as it doesn't have effect on the TCP transport. But the client still needs to call the switch for both of the transport to work.
 

Discussion:

Patch:
diff --git a/iec62056_21/client.py b/iec62056_21/client.py
index 7e3e3a8..9220167 100644
--- a/iec62056_21/client.py
+++ b/iec62056_21/client.py
@@ -55,17 +55,24 @@ class Iec6205621Client:
         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
+
+        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):
         """
-        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,6 +83,10 @@ class Iec6205621Client:
         :return:
         """
         # TODO Can't find documentation on why the additional_data of 1 is needed.
+        #  LIS-200 Specific?
+
+        # 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, additional_data)
         logger.info(f"Sending read request: {request}")
@@ -145,8 +156,7 @@ class Iec6205621Client:
         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
         self.identification = ident_msg.identification
         self.manufacturer_id = ident_msg.manufacturer
 
@@ -165,8 +175,6 @@ class Iec6205621Client:
 
         self.ack_with_option_select("programming")
 
-        self.transport.switch_baudrate(self.switchover_baudrate)
-
         # receive password request
         pw_req = self.read_response()
 
@@ -178,7 +186,6 @@ class Iec6205621Client:
         """
         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 +208,9 @@ class Iec6205621Client:
         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 +222,20 @@ class Iec6205621Client:
 
         :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):
         """
@@ -230,11 +246,7 @@ class Iec6205621Client:
          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/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 @@ class DataSet(Iec6205621Data):
         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 @@ class DataSet(Iec6205621Data):
     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 @@ class ReadoutDataMessage(Iec6205621Data):
 
 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 @@ class CommandMessage(Iec6205621Data):
         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 @@ class CommandMessage(Iec6205621Data):
         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 @@ class AckOptionSelectMessage(Iec6205621Data):
 
 
 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..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,10 +50,11 @@ class BaseTransport:
         while True:
 
             in_data = b""
-            duration = 0
+            duration: float = 0.0
             start_time = time.time()
             while True:
                 b = self.recv(1)
+
                 duration = time.time() - start_time
                 if duration > self.timeout:
                     raise TimeoutError(f"Read in {self.__class__.__name__} timed out")
@@ -128,7 +130,12 @@ class BaseTransport:
 
         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.
         """
@@ -138,7 +145,7 @@ class BaseTransport:
         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)
@@ -165,7 +172,7 @@ class BaseTransport:
         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
 
@@ -174,7 +181,7 @@ class BaseTransport:
         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.
 
@@ -182,7 +189,7 @@ class BaseTransport:
         """
         raise NotImplemented("Must be defined in subclass")
 
-    def recv(self, chars):
+    def recv(self, chars: int) -> bytes:
         """
         Will receive data over the transport.
 
@@ -190,7 +197,7 @@ class BaseTransport:
         """
         return self._recv(chars)
 
-    def _recv(self, chars):
+    def _recv(self, chars: int) -> bytes:
         """
         Transport dependant sending functionality.
 
@@ -198,7 +205,7 @@ class BaseTransport:
         """
         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.
@@ -217,19 +224,19 @@ 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):
+    def connect(self, baudrate: int = 300) -> None:
         """
         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,
@@ -240,37 +247,49 @@ class SerialTransport(BaseTransport):
             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:
         """
-        time.sleep(0.5)
+        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,
             baudrate=baud,
@@ -298,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.
         """
@@ -314,34 +333,43 @@ class TcpTransport(BaseTransport):
         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.
 
@@ -349,7 +377,7 @@ class TcpTransport(BaseTransport):
         """
         pass
 
-    def _get_socket(self):
+    def _get_socket(self) -> socket.socket:
         """
         Create a correct socket.
         """


Test patch:
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")
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 @@ class TestDataSets:
         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 @@ class TestCommandMessage:
         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 @@ class TestCommandMessage:
         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 @@ class TestCommandMessage:
         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 @@ class TestCommandMessage:
         )
 
         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 @@ class TestCommandMessage:
         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"