sdo.client: Add missing abort messages (#594)

SDO abort messages were sent in some cases when a response times out
or has an unexpected server command specifier. But not consistently,
thus the following cases are now added:

    Mismatched scs:
        ReadableStream, after upload segment request
        WritableStream, after download initiate request
        WritableStream, after expedited download request
        WritableStream, after download segment request

    Toggle bit mismatch (reports as not toggled):
        ReadableStream, after upload segment request

* Consistently format abort code literals.

* Fix typo (grammatical error in exception message).

* Avoid duplicate SDO abort messages on timeout in _block_ack()
   and _end_upload().

* Add docstring to read_response() and detail exceptions.

Co-authored-by: samsam <[email protected]>

Author: acolomb

canopen/sdo/client.py

@@ -65,6 +65,13 @@ def send_request(self, request):
                 break
 
     def read_response(self):
+        """Wait for an SDO response and handle timeout or remote abort.
+
+        :raises canopen.SdoAbortedError:
+            When receiving an SDO abort response from the server.
+        :raises canopen.SdoCommunicationError:
+            After timeout with no response received.
+        """
         try:
             response = self.responses.get(
                 block=True, timeout=self.RESPONSE_TIMEOUT)
@@ -89,11 +96,11 @@ def request_response(self, sdo_request):
             except SdoCommunicationError as e:
                 retries_left -= 1
                 if not retries_left:
-                    self.abort(0x5040000)
+                    self.abort(0x0504_0000)
                     raise
                 logger.warning(str(e))
 
-    def abort(self, abort_code=0x08000000):
+    def abort(self, abort_code=0x0800_0000):
         """Abort current transfer."""
         request = bytearray(8)
         request[0] = REQUEST_ABORTED
@@ -302,8 +309,10 @@ def read(self, size=-1):
         response = self.sdo_client.request_response(request)
         res_command, = struct.unpack_from("B", response)
         if res_command & 0xE0 != RESPONSE_SEGMENT_UPLOAD:
+            self.sdo_client.abort(0x0504_0001)
             raise SdoCommunicationError(f"Unexpected response 0x{res_command:02X}")
         if res_command & TOGGLE_BIT != self._toggle:
+            self.sdo_client.abort(0x0503_0000)
             raise SdoCommunicationError("Toggle bit mismatch")
         length = 7 - ((res_command >> 1) & 0x7)
         if res_command & NO_MORE_DATA:
@@ -362,6 +371,7 @@ def __init__(self, sdo_client, index, subindex=0, size=None, force_segment=False
             response = sdo_client.request_response(request)
             res_command, = struct.unpack_from("B", response)
             if res_command != RESPONSE_DOWNLOAD:
+                self.sdo_client.abort(0x0504_0001)
                 raise SdoCommunicationError(
                     f"Unexpected response 0x{res_command:02X}")
         else:
@@ -390,6 +400,7 @@ def write(self, b):
             response = self.sdo_client.request_response(request)
             res_command, = struct.unpack_from("B", response)
             if res_command & 0xE0 != RESPONSE_DOWNLOAD:
+                self.sdo_client.abort(0x0504_0001)
                 raise SdoCommunicationError(
                     f"Unexpected response 0x{res_command:02X}")
             bytes_sent = len(b)
@@ -414,6 +425,7 @@ def write(self, b):
             response = self.sdo_client.request_response(request)
             res_command, = struct.unpack("B", response[0:1])
             if res_command & 0xE0 != RESPONSE_SEGMENT_DOWNLOAD:
+                self.sdo_client.abort(0x0504_0001)
                 raise SdoCommunicationError(
                     f"Unexpected response 0x{res_command:02X} "
                     f"(expected 0x{RESPONSE_SEGMENT_DOWNLOAD:02X})")
@@ -487,7 +499,7 @@ def __init__(self, sdo_client, index, subindex=0, request_crc_support=True):
         res_command, res_index, res_subindex = SDO_STRUCT.unpack_from(response)
         if res_command & 0xE0 != RESPONSE_BLOCK_UPLOAD:
             self._error = True
-            self.sdo_client.abort(0x05040001)
+            self.sdo_client.abort(0x0504_0001)
             raise SdoCommunicationError(f"Unexpected response 0x{res_command:02X}")
         # Check that the message is for us
         if res_index != index or res_subindex != subindex:
@@ -544,7 +556,7 @@ def read(self, size=-1):
             if self._done:
                 if self._server_crc != self._crc.final():
                     self._error = True
-                    self.sdo_client.abort(0x05040004)
+                    self.sdo_client.abort(0x0504_0004)
                     raise SdoCommunicationError("CRC is not OK")
                 logger.info("CRC is OK")
         self.pos += len(data)
@@ -564,8 +576,8 @@ def _retransmit(self):
                 self._ackseq = seqno
                 return response
         self._error = True
-        self.sdo_client.abort(0x05040000)
-        raise SdoCommunicationError("Some data were lost and could not be retransmitted")
+        self.sdo_client.abort(0x0504_0000)
+        raise SdoCommunicationError("Some data was lost and could not be retransmitted")
 
     def _ack_block(self):
         request = bytearray(8)
@@ -576,15 +588,19 @@ def _ack_block(self):
         self._ackseq = 0
 
     def _end_upload(self):
-        response = self.sdo_client.read_response()
+        try:
+            response = self.sdo_client.read_response()
+        except SdoCommunicationError:
+            self.abort(0x0504_0000)
+            raise
         res_command, self._server_crc = struct.unpack_from("<BH", response)
         if res_command & 0xE0 != RESPONSE_BLOCK_UPLOAD:
             self._error = True
-            self.sdo_client.abort(0x05040001)
+            self.sdo_client.abort(0x0504_0001)
             raise SdoCommunicationError(f"Unexpected response 0x{res_command:02X}")
         if res_command & 0x3 != END_BLOCK_TRANSFER:
             self._error = True
-            self.sdo_client.abort(0x05040001)
+            self.sdo_client.abort(0x0504_0001)
             raise SdoCommunicationError("Server did not end transfer as expected")
         # Return number of bytes not used in last message
         return (res_command >> 2) & 0x7
@@ -654,7 +670,7 @@ def __init__(self, sdo_client, index, subindex=0, size=None, request_crc_support
         response = sdo_client.request_response(request)
         res_command, res_index, res_subindex = SDO_STRUCT.unpack_from(response)
         if res_command & 0xE0 != RESPONSE_BLOCK_DOWNLOAD:
-            self.sdo_client.abort(0x05040001)
+            self.sdo_client.abort(0x0504_0001)
             raise SdoCommunicationError(
                 f"Unexpected response 0x{res_command:02X}")
         # Check that the message is for us
@@ -736,14 +752,18 @@ def tell(self):
 
     def _block_ack(self):
         logger.debug("Waiting for acknowledgement of last block...")
-        response = self.sdo_client.read_response()
+        try:
+            response = self.sdo_client.read_response()
+        except SdoCommunicationError:
+            self.sdo_client.abort(0x0504_0000)
+            raise
         res_command, ackseq, blksize = struct.unpack_from("BBB", response)
         if res_command & 0xE0 != RESPONSE_BLOCK_DOWNLOAD:
-            self.sdo_client.abort(0x05040001)
+            self.sdo_client.abort(0x0504_0001)
             raise SdoCommunicationError(
                 f"Unexpected response 0x{res_command:02X}")
         if res_command & 0x3 != BLOCK_TRANSFER_RESPONSE:
-            self.sdo_client.abort(0x05040001)
+            self.sdo_client.abort(0x0504_0001)
             raise SdoCommunicationError("Server did not respond with a "
                                         "block download response")
         if ackseq != self._blksize: