Fix issues #79 #93 #101 #105

Author: Krolken

HISTORY.md

@@ -14,12 +14,20 @@ and this project adheres to [Calendar Versioning](https://calver.org/)
    layer can be disconnected right away.
 
 ### Changed
+* Renamed `TcpTransport` to `IPTransport` to reflect IP-wrapper semantics and kept
+  `TcpTransport` as a backward-compatible alias.
 
 ### Deprecated
 
 ### Removed
 
 ### Fixed
+* Fixed RLRQ context handling so unciphered associations omit user information, and
+  ciphered associations reuse the original proposed xDLMS context from AARQ.
+* Fixed HDLC transport timeout handling so repeated empty serial reads no longer loop
+  forever and now raise a communication timeout error.
+* Implemented encoding and parsing rules for `DateData` and `TimeData`, including
+  support for `datetime` inputs and all-ones wildcard values mapping to `None`.
 
 ### Security
 

README.md

@@ -49,19 +49,20 @@ A simple example of reading invocation counters using a public client:
 
 ```python
 from dlms_cosem.client import DlmsClient
-from dlms_cosem.io import TcpTransport, BlockingTcpIO
+from dlms_cosem.io import IPTransport, BlockingTcpIO
 from dlms_cosem.security import NoSecurityAuthentication
 from dlms_cosem import enumerations, cosem
 
 tcp_io = BlockingTcpIO(host="localhost", port=4059)
-tcp_transport = TcpTransport(io=tcp_io, server_logical_address=1, client_logical_address=16)
-client = DlmsClient(transport=tcp_transport, authentication=NoSecurityAuthentication())
+ip_transport = IPTransport(io=tcp_io, server_logical_address=1, client_logical_address=16)
+client = DlmsClient(transport=ip_transport, authentication=NoSecurityAuthentication())
 with client.session() as dlms_client:
     data = dlms_client.get(
         cosem.CosemAttribute(interface=enumerations.CosemInterface.DATA,
                              instance=cosem.Obis(0, 0, 0x2B, 1, 0), attribute=2, ))
 ```
 
+`TcpTransport` is kept as a backward-compatible alias of `IPTransport`.
 
 Look at the different files in the `examples` folder get a better feel on how to fully
 use the library.

dlms_cosem/connection.py

@@ -209,6 +209,13 @@ class DlmsConnection:
         )
     )
 
+    # Keep a copy of the context proposed in AARQ so the same context can be reused in
+    # RLRQ when ciphering is enabled.
+    proposed_initiate_request: Optional[xdlms.InitiateRequest] = attr.ib(
+        default=None,
+        init=False,
+    )
+
     settings: DlmsConnectionSettings = attr.ib(
         default=DlmsConnectionSettings(),
         converter=attr.converters.default_if_none(factory=DlmsConnectionSettings),
@@ -291,6 +298,7 @@ def send(self, event) -> bytes:
             )
 
         self.state.process_event(event)
+        self.register_proposed_context(event)
         LOG.debug(f"Preparing to send DLMS Request", request=event)
 
         if self.use_protection:
@@ -398,6 +406,39 @@ def next_event(self):
     def clear_buffer(self):
         self.buffer = bytearray()
 
+    @staticmethod
+    def copy_initiate_request(
+        initiate_request: xdlms.InitiateRequest,
+    ) -> xdlms.InitiateRequest:
+        return xdlms.InitiateRequest(
+            proposed_conformance=Conformance(
+                **attr.asdict(initiate_request.proposed_conformance)
+            ),
+            proposed_quality_of_service=initiate_request.proposed_quality_of_service,
+            client_max_receive_pdu_size=initiate_request.client_max_receive_pdu_size,
+            proposed_dlms_version_number=initiate_request.proposed_dlms_version_number,
+            response_allowed=initiate_request.response_allowed,
+            dedicated_key=initiate_request.dedicated_key,
+        )
+
+    def register_proposed_context(self, event: Any) -> None:
+        """
+        Keep track of the context proposed in AARQ so it can be reused in RLRQ.
+        """
+        if not isinstance(event, acse.ApplicationAssociationRequest):
+            return
+
+        if not event.user_information:
+            self.proposed_initiate_request = None
+            return
+
+        if not isinstance(event.user_information.content, xdlms.InitiateRequest):
+            return
+
+        self.proposed_initiate_request = self.copy_initiate_request(
+            event.user_information.content
+        )
+
     @property
     def use_protection(self) -> bool:
         """
@@ -587,19 +628,33 @@ def get_aarq(self) -> acse.ApplicationAssociationRequest:
             user_information=acse.UserInformation(content=initiate_request),
         )
 
-    def get_rlrq(self) -> acse.ReleaseRequest:
+    def get_rlrq_initiate_request(self) -> xdlms.InitiateRequest:
         """
-        Returns a ReleaseRequestApdu to release the current association if one should be used.
+        Return the original context proposed in AARQ when available.
         """
 
-        initiate_request = xdlms.InitiateRequest(
+        if self.proposed_initiate_request is not None:
+            return self.copy_initiate_request(self.proposed_initiate_request)
+
+        return xdlms.InitiateRequest(
             proposed_conformance=self.conformance,
             client_max_receive_pdu_size=self.max_pdu_size,
         )
 
+    def get_rlrq(self) -> acse.ReleaseRequest:
+        """
+        Returns a ReleaseRequestApdu to release the current association if one should be used.
+        """
+
+        user_information = None
+
+        if self.use_protection:
+            initiate_request = self.get_rlrq_initiate_request()
+            user_information = acse.UserInformation(content=initiate_request)
+
         return acse.ReleaseRequest(
             reason=enums.ReleaseRequestReason.NORMAL,
-            user_information=acse.UserInformation(content=initiate_request),
+            user_information=user_information,
         )
 
     def update_negotiated_parameters(

dlms_cosem/dlms_data.py

@@ -10,6 +10,40 @@
 VARIABLE_LENGTH = -1
 
 
+def convert_datetime_to_date(
+    in_value: Optional[Union[datetime.datetime, datetime.date]],
+) -> Optional[datetime.date]:
+    if in_value is None:
+        return None
+
+    if isinstance(in_value, datetime.datetime):
+        return in_value.date()
+
+    if isinstance(in_value, datetime.date):
+        return in_value
+
+    raise ValueError(
+        f"DateData.value can only be datetime.date, datetime.datetime or None. Got {type(in_value)!r}"
+    )
+
+
+def convert_datetime_to_time(
+    in_value: Optional[Union[datetime.datetime, datetime.time]],
+) -> Optional[datetime.time]:
+    if in_value is None:
+        return None
+
+    if isinstance(in_value, datetime.datetime):
+        return in_value.time()
+
+    if isinstance(in_value, datetime.time):
+        return in_value
+
+    raise ValueError(
+        f"TimeData.value can only be datetime.time, datetime.datetime or None. Got {type(in_value)!r}"
+    )
+
+
 class AbstractDlmsData(abc.ABC):
     @classmethod
     @abc.abstractmethod
@@ -365,12 +399,26 @@ class DateData(BaseDlmsData):
     TAG = 26
     LENGTH = 5
 
+    value: Optional[datetime.date] = attr.ib(
+        default=None, converter=convert_datetime_to_date
+    )
+
     @classmethod
     def from_bytes(cls, bytes_data: bytes):
         if len(bytes_data) != cls.LENGTH:
             raise ValueError(f"Date should be 5 bytes long, got {len(bytes_data)}")
+
+        if bytes_data == b"\xff\xff\xff\xff\xff":
+            return cls(None)
+
         return cls(time.date_from_bytes(bytes_data))
 
+    def value_to_bytes(self) -> bytes:
+        if self.value is None:
+            return b"\xff\xff\xff\xff\xff"
+
+        return time.date_to_bytes(self.value)
+
 
 @attr.s(auto_attribs=True)
 class TimeData(BaseDlmsData):
@@ -379,12 +427,26 @@ class TimeData(BaseDlmsData):
     TAG = 27
     LENGTH = 4
 
+    value: Optional[datetime.time] = attr.ib(
+        default=None, converter=convert_datetime_to_time
+    )
+
     @classmethod
     def from_bytes(cls, bytes_data: bytes):
         if len(bytes_data) != cls.LENGTH:
             raise ValueError(f"Time should be 4 bytes long, got {len(bytes_data)}")
+
+        if bytes_data == b"\xff\xff\xff\xff":
+            return cls(None)
+
         return cls(time.time_from_bytes(bytes_data))
 
+    def value_to_bytes(self) -> bytes:
+        if self.value is None:
+            return b"\xff\xff\xff\xff"
+
+        return time.time_to_bytes(self.value)
+
 
 @attr.s(auto_attribs=True)
 class DontCareData(BaseDlmsData):

dlms_cosem/io.py

@@ -2,6 +2,7 @@
 
 import socket
 import sys
+import time
 from typing import Optional, Tuple
 
 from dlms_cosem.hdlc import connection, address, state, frames
@@ -192,9 +193,11 @@ def recv_until(self, end: bytes) -> bytes:
 
 
 @attr.s(auto_attribs=True)
-class TcpTransport:
+class IPTransport:
     """
-    A TCP transport.
+    A DLMS IP-wrapper transport.
+
+    Can be used over any IP-based IO implementation (for example TCP or UDP).
     """
 
     client_logical_address: int
@@ -245,6 +248,10 @@ def recv_response(self) -> bytes:
         return data
 
 
+# Backward compatible alias.
+TcpTransport = IPTransport
+
+
 @attr.s(auto_attribs=True)
 class HdlcTransport:
     """
@@ -334,13 +341,24 @@ def next_event(self):
         :return:
         """
 
+        timeout_at = time.monotonic() + self.timeout
+
         while True:
             # If we already have a complete event buffered internally, just
             # return that. Otherwise, read some data, add it to the internal
             # buffer, and then try again.
             event = self.hdlc_connection.next_event()
             if event is state.NEED_DATA:
-                self.hdlc_connection.receive_data(self.recv_frame())
+                if time.monotonic() >= timeout_at:
+                    raise exceptions.CommunicationError(
+                        f"Timed out waiting for HDLC response after {self.timeout} seconds"
+                    )
+
+                frame_data = self.recv_frame()
+                if not frame_data:
+                    continue
+
+                self.hdlc_connection.receive_data(frame_data)
                 continue
             return event
 

tests/test_blocking_tcp_transport.py

@@ -2,7 +2,7 @@
 
 import pytest
 
-from dlms_cosem.io import BlockingTcpIO, TcpTransport
+from dlms_cosem.io import BlockingTcpIO, IPTransport, TcpTransport
 from dlms_cosem.exceptions import CommunicationError
 
 
@@ -18,7 +18,7 @@ def test_can_connect(self):
         server_socket.bind((self.host, self.port))
         server_socket.listen(1)
         io = BlockingTcpIO(host=self.host, port=self.port)
-        transport = TcpTransport(
+        transport = IPTransport(
             self.client_logical_address, self.server_logical_address, io
         )
         transport.connect()
@@ -30,7 +30,7 @@ def test_connect_on_connected_raises(self):
         server_socket.listen(1)
 
         io = BlockingTcpIO(host=self.host, port=self.port)
-        transport = TcpTransport(
+        transport = IPTransport(
             self.client_logical_address, self.server_logical_address, io
         )
         transport.connect()
@@ -39,7 +39,7 @@ def test_connect_on_connected_raises(self):
 
     def test_cant_connect_raises_communications_error(self):
         io = BlockingTcpIO(host=self.host, port=self.port)
-        transport = TcpTransport(
+        transport = IPTransport(
             self.client_logical_address, self.server_logical_address, io
         )
         with pytest.raises(CommunicationError):
@@ -51,7 +51,7 @@ def test_disconnect(self):
         server_socket.listen(1)
 
         io = BlockingTcpIO(host=self.host, port=self.port)
-        transport = TcpTransport(
+        transport = IPTransport(
             self.client_logical_address, self.server_logical_address, io
         )
         transport.connect()
@@ -64,10 +64,14 @@ def test_disconnect_is_noop_if_disconnected(self):
         server_socket.listen(1)
 
         io = BlockingTcpIO(host=self.host, port=self.port)
-        transport = TcpTransport(
+        transport = IPTransport(
             self.client_logical_address, self.server_logical_address, io
         )
         transport.connect()
         transport.disconnect()
         transport.disconnect()
         assert transport.io.tcp_socket is None
+
+
+def test_tcp_transport_is_kept_as_alias_for_backwards_compatibility():
+    assert TcpTransport is IPTransport