Make SCP errors more descriptive.

* All SCPError derrived exceptions now include a summary of the offending
  packet (most importantly x, y, p and command!). The SCPPacket object is also
  made accessible from the exc.packet attribute.
* RC errors also now contain a human readable explanation of what the error
  codes mean.
* Instead of having many RC error exception types, there is now one
  FatalReturnCodeError exception type. If differentiating between the types is
  important, the RC is included in the exc.return_code attribute. If required
  in the future, subclasses can (of course) be created for individual return
  codes without breaking backward compatibility.

This change (strictly speaking) breaks backward compatibility as it renames
exceptions of the RC-specific types. @mundya are you using these exceptions
anywhere and is breaking compatibility here a problem?

Author: mossblaser

rig/machine_control/consts.py

@@ -18,14 +18,6 @@
 produce (256 + 8 bytes).
 """
 
-SCP_RC_OK = 0x80
-"""SCP response code which indicates that everything was fine."""
-
-SCP_RC_TIMEOUT = {0x8b, 0x8c, 0x8d, 0x8e}
-"""SCP response codes which should be treated as if they were a packet timing
-out.
-"""
-
 SPINNAKER_RTR_BASE = 0xE1000000  # Unbuffered
 """Base address of router hardware registers."""
 
@@ -93,6 +85,58 @@ class SCPCommands(enum.IntEnum):
     power = 57  # BMP main board power control
 
 
+@add_int_enums_to_docstring
+class SCPReturnCodes(enum.IntEnum):
+    """SCP return codes"""
+    ok = 0x80  # Command completed OK
+    len = 0x81  # Bad packet length (Fatal)
+    sum = 0x82  # Bad checksum (Retryable)
+    cmd = 0x83  # Bad/invalid command (Fatal)
+    arg = 0x84  # Invalid arguments (Fatal)
+    port = 0x85  # Bad port number (Fatal)
+    timeout = 0x86  # Monitor <-> app-core comms timeout (Fatal)
+    route = 0x87  # No P2P route (Fatal)
+    cpu = 0x88  # Bad CPU number (Fatal)
+    dead = 0x89  # SHM dest dead (Fatal)
+    buf = 0x8a  # No free SHM buffers (Fatal)
+    p2p_noreply = 0x8b  # No reply to open (Fatal)
+    p2p_reject = 0x8c  # Open rejected (Fatal)
+    p2p_busy = 0x8d  # Dest busy (Retryable)
+    p2p_timeout = 0x8e  # Eth chip <--> destination comms timeout (Fatal)
+    pkt_tx = 0x8f  # Pkt Tx failed (Fatal)
+
+RETRYABLE_SCP_RETURN_CODES = set([
+    SCPReturnCodes.sum,
+    SCPReturnCodes.p2p_busy,
+])
+"""The set of :py:class:`.SCPReturnCodes` values which indicate a non-fatal
+retryable fault."""
+
+
+FATAL_SCP_RETURN_CODES = {
+    SCPReturnCodes.len: "Bad command length.",
+    SCPReturnCodes.cmd: "Bad/invalid command.",
+    SCPReturnCodes.arg: "Invalid command arguments.",
+    SCPReturnCodes.port: "Bad port number.",
+    SCPReturnCodes.timeout:
+        "Timeout waiting for the application core to respond to "
+        "the monitor core's request.",
+    SCPReturnCodes.route: "No P2P route to the target chip is available.",
+    SCPReturnCodes.cpu: "Bad CPU number.",
+    SCPReturnCodes.dead: "SHM dest dead.",
+    SCPReturnCodes.buf: "No free SHM buffers.",
+    SCPReturnCodes.p2p_noreply:
+        "No response packets from the target reached the "
+        "ethernet connected chip.",
+    SCPReturnCodes.p2p_reject: "The target chip rejected the packet.",
+    SCPReturnCodes.p2p_timeout:
+        "Communications between the ethernet connected chip and target chip "
+        "timedout.",
+    SCPReturnCodes.pkt_tx: "Packet transmission failed.",
+}
+"""The set of fatal SCP errors and a human-readable error."""
+
+
 @add_int_enums_to_docstring
 class DataType(enum.IntEnum):
     """Used to specify the size of data being read to/from a SpiNNaker machine

rig/machine_control/packets.py

@@ -143,6 +143,17 @@ def packed_data(self):
         # Return the SCP header and the rest of the data
         return scp_header + self.data
 
+    def __repr__(self):
+        """Produce a human-redaable summary of (the most important parts of)
+        the packet."""
+        return ("<{} x: {}, y: {}, cpu: {}, "
+                "cmd_rc: {}, arg1: {}, arg2: {}, arg3: {}, "
+                "data: {}>".format(self.__class__.__name__,
+                                   self.dest_x, self.dest_y, self.dest_cpu,
+                                   self.cmd_rc,
+                                   self.arg1, self.arg2, self.arg3,
+                                   repr(self.data)))
+
 
 def _unpack_sdp_into_packet(packet, bytestring):
     """Unpack the SDP header from a bytestring into a packet.

rig/machine_control/scp_connection.py

@@ -50,7 +50,6 @@ def __new__(cls, x, y, p, cmd, arg1=0, arg2=0, arg3=0, data=b'',
 class SCPConnection(object):
     """Implements the SCP protocol for communicating with a SpiNNaker chip.
     """
-    error_codes = {}
 
     def __init__(self, spinnaker_host, port=consts.SCP_PORT,
                  n_tries=5, timeout=0.5):
@@ -82,15 +81,6 @@ def __init__(self, spinnaker_host, port=consts.SCP_PORT,
         # Sequence values
         self.seq = seqs()
 
-    @classmethod
-    def _register_error(cls, cmd_rc):
-        """Register an Exception class as belonging to a certain CMD_RC value.
-        """
-        def err_(err):
-            cls.error_codes[cmd_rc] = err
-            return err
-        return err_
-
     def send_scp(self, buffer_size, x, y, p, cmd, arg1=0, arg2=0, arg3=0,
                  data=b'', expected_args=3, timeout=0.0):
         """Transmit a packet to the SpiNNaker machine and block until an
@@ -176,12 +166,13 @@ def send_scp_burst(self, buffer_size, window_size,
         class TransmittedPacket(object):
             """A packet which has been transmitted and still awaits a response.
             """
-            __slots__ = ["callback", "packet", "n_tries",
+            __slots__ = ["callback", "packet", "bytestring", "n_tries",
                          "timeout", "timeout_time"]
 
             def __init__(self, callback, packet, timeout):
                 self.callback = callback
                 self.packet = packet
+                self.bytestring = packet.bytestring
                 self.n_tries = 1
                 self.timeout = timeout
                 self.timeout_time = time.time() + self.timeout
@@ -226,12 +217,12 @@ def __init__(self, callback, packet, timeout):
                     # expecting a response for it and can retransmit it if
                     # necessary.
                     outstanding_packets[seq] = TransmittedPacket(
-                        args.callback, packet.bytestring,
+                        args.callback, packet,
                         self.default_timeout + args.timeout
                     )
 
                     # Actually send the packet
-                    self.sock.send(outstanding_packets[seq].packet)
+                    self.sock.send(outstanding_packets[seq].bytestring)
 
             # Listen on the socket for an acknowledgement packet, there may not
             # be one.
@@ -260,18 +251,19 @@ def __init__(self, callback, packet, timeout):
                                              consts.SDP_HEADER_LENGTH + 2)
 
                 # If the code is an error then we respond immediately
-                if rc != consts.SCP_RC_OK:
-                    if rc in consts.SCP_RC_TIMEOUT:
+                if rc != consts.SCPReturnCodes.ok:
+                    if rc in consts.RETRYABLE_SCP_RETURN_CODES:
                         # If the error is timeout related then treat the packet
                         # as though it timed out, just discard.  This avoids us
                         # hammering the board when it's most vulnerable.
                         pass
-                    elif rc in self.error_codes:
-                        raise self.error_codes[rc]
                     else:
-                        raise SCPError(
-                            "Unhandled exception code {:#2x}".format(rc)
-                        )
+                        # For all other errors, we'll just fall over
+                        # immediately.
+                        packet = outstanding_packets.get(seq)
+                        if packet is not None:
+                            packet = packet.packet
+                        raise FatalReturnCodeError(rc, packet)
                 else:
                     # Look up the sequence index of packet in the list of
                     # outstanding packets.  We may have already processed a
@@ -294,10 +286,13 @@ def __init__(self, callback, packet, timeout):
                     # the given number of times then raise a timeout error for
                     # it.
                     if outstanding.n_tries >= self.n_tries:
-                        raise TimeoutError(self.n_tries)
+                        raise TimeoutError(
+                            "No response after {} attempts.".format(
+                                self.n_tries),
+                            outstanding.packet)
 
                     # Otherwise we retransmit it
-                    self.sock.send(outstanding.packet)
+                    self.sock.send(outstanding.bytestring)
                     outstanding.n_tries += 1
                     outstanding.timeout_time = (current_time +
                                                 outstanding.timeout)
@@ -425,34 +420,46 @@ def seqs(mask=0xffff):
 
 
 class SCPError(IOError):
-    """Base Error for SCP return codes."""
-    pass
+    """Base Error for SCP return codes.
 
-
-class TimeoutError(SCPError):
-    """Raised when an SCP is not acknowledged within the given period of time.
+    Attributes
+    ----------
+    packet : :py:class:`rig.machine_control.packets.SCPPacket`
+        The packet being processed when the error occurred. May be None if no
+        specific packet was involved.
     """
-    pass
 
+    def __init__(self, message, packet=None):
+        self.packet = packet
+        if self.packet is not None:
+            message = "{} (Packet: {})".format(message, packet)
 
-@SCPConnection._register_error(0x81)
-class BadPacketLengthError(SCPError):
-    """Raised when an SCP packet is an incorrect length."""
-    pass
+        super(SCPError, self).__init__(message)
 
 
-@SCPConnection._register_error(0x83)
-class InvalidCommandError(SCPError):
-    """Raised when an SCP packet contains an invalid command code."""
+class TimeoutError(SCPError):
+    """Raised when an SCP is not acknowledged within the given period of time.
+    """
     pass
 
 
-@SCPConnection._register_error(0x84)
-class InvalidArgsError(SCPError):
-    """Raised when an SCP packet has an invalid argument."""
-    pass
+class FatalReturnCodeError(SCPError):
+    """Raised when an SCP command returns with an error which is connsidered
+    fatal.
 
+    Attributes
+    ----------
+    return_code : :py:class:`rig.machine_control.consts.SCPReturnCodes` or int
+        The return code (will be a raw integer if the code is unrecognised).
+    """
 
-@SCPConnection._register_error(0x87)
-class NoRouteError(SCPError):
-    """Raised when there is no route to the requested core."""
+    def __init__(self, return_code=None, packet=None):
+        try:
+            self.return_code = consts.SCPReturnCodes(return_code)
+            message = "RC_{}: {}".format(
+                self.return_code.name.upper(),
+                consts.FATAL_SCP_RETURN_CODES[self.return_code])
+        except ValueError:
+            self.return_code = return_code
+            message = "Unrecognised return code {:#2X}."
+        super(FatalReturnCodeError, self).__init__(message, packet)

tests/machine_control/test_packets.py

@@ -1,5 +1,7 @@
 from rig.machine_control.packets import SDPPacket, SCPPacket
 
+import sys
+
 
 class TestSDPPacket(object):
     """Test SDPPacket representations."""
@@ -263,3 +265,15 @@ def test_from_bytestring_2_args_short(self):
         # Check that the bytestring this packet creates is the same as the one
         # we specified before.
         assert scp_packet.bytestring == packet
+
+    def test_repr(self):
+        """Test the string representation of an SCP packet makes sense."""
+        scp_packet = SCPPacket(dest_x=10, dest_y=20, dest_cpu=3,
+                               cmd_rc=2, arg1=123, arg2=456,
+                               data=b"foobar")
+        # Note: Python 2 does not have the "b" prefix on byte strings
+        assert repr(scp_packet) == (
+            "<SCPPacket x: 10, y: 20, cpu: 3, "
+            "cmd_rc: 2, arg1: 123, arg2: 456, arg3: None, "
+            "data: {}'foobar'>".format(
+                "b" if sys.version_info >= (3, 0) else ""))

tests/machine_control/test_scp_connection.py

@@ -4,9 +4,12 @@
 import struct
 import time
 
-from rig.machine_control.consts import SCPCommands, DataType, SDP_HEADER_LENGTH
+from rig.machine_control.consts import \
+    SCPCommands, DataType, SDP_HEADER_LENGTH, RETRYABLE_SCP_RETURN_CODES, \
+    FATAL_SCP_RETURN_CODES
 from rig.machine_control.packets import SCPPacket
-from rig.machine_control.scp_connection import SCPConnection, scpcall
+from rig.machine_control.scp_connection import \
+    SCPConnection, scpcall, FatalReturnCodeError
 from rig.machine_control import scp_connection
 
 
@@ -193,10 +196,11 @@ def mock_select(rlist, wlist, xlist, timeout):
         # times we specified.
         assert mock_conn.sock.send.call_count == mock_conn.n_tries
 
-    @pytest.mark.parametrize("err_code", [0x8b, 0x8c, 0x8d, 0x8e])
-    def test_single_packet_fails_with_RC_P2P_ERROR(self, mock_conn, err_code):
+    @pytest.mark.parametrize("err_code", RETRYABLE_SCP_RETURN_CODES)
+    def test_single_packet_fails_with_retryable_error(self, mock_conn,
+                                                      err_code):
         """Test correct operation for transmitting a single packet which is
-        always acknowledged with one of the RC_P2P error codes.
+        always acknowledged with one of the retryable error codes.
         """
         # Create a packet to send
         packets = [scpcall(3, 5, 0, 12)]
@@ -344,14 +348,9 @@ def recv(self, *args):
                 mock.patch("select.select", new=mock_select):
             mock_conn.send_scp_burst(512, 8, packets())
 
-    @pytest.mark.parametrize(
-        "rc, error",
-        [(0x81, scp_connection.BadPacketLengthError),
-         (0x83, scp_connection.InvalidCommandError),
-         (0x84, scp_connection.InvalidArgsError),
-         (0x87, scp_connection.NoRouteError),
-         (0x00, Exception)])
-    def test_errors(self, mock_conn, rc, error):
+    @pytest.mark.parametrize("rc",
+                             list(map(int, FATAL_SCP_RETURN_CODES)) + [0x00])
+    def test_errors(self, mock_conn, rc):
         """Test that errors are raised when error RCs are returned."""
         # Create an object which returns a packet with an error code
         class ReturnPacket(object):
@@ -374,7 +373,7 @@ def __call__(self, packet):
 
         # Send an SCP command and check that the correct error is raised
         packets = [scpcall(3, 5, 0, 12)]
-        with pytest.raises(error), \
+        with pytest.raises(FatalReturnCodeError), \
                 mock.patch("select.select", new=mock_select):
             mock_conn.send_scp_burst(256, 1, iter(packets))