Fix local memory read (Encode and decode space index correctly; Pad with zeroes if last chunk less than 64 bytes). Add a related test case. Add increment option to generate_node_id.

Author: Poikilos

openlcb/conventions.py

@@ -146,6 +146,7 @@ def get_local_ip() -> Optional[str]:
 
 previous_three_octets = None
 initial_three_octets = None
+used_ids = set()
 
 
 def increment_octets(octets: bytearray):
@@ -241,6 +242,8 @@ def generate_node_id_str(id_range_prefix: str, increment: bool = False) -> str:
             python-openlcb (or as otherwise assigned by OpenLCB Group
             which reserves 05.* range). See
             <https://registry.openlcb.org/uniqueidranges>.
+        increment (bool): Increment such as more multiple
+            local IDs (virtual node(s) aside from local Node).
     Returns:
         str: Full 48-bit node ID in dotted hex string notation (Example:
             '05.01.01.4A.B7.19') that is unique (very likely...).
@@ -260,5 +263,10 @@ def generate_node_id_str(id_range_prefix: str, increment: bool = False) -> str:
             " (preferably less to increase likelihood of uniqueness). Got {}"
             .format(id_range_prefix))
     uniqueCount = 6 - len(prefixParts)
-    return ".".join(prefixParts+lastParts[-uniqueCount:])
+    id_str = ".".join(prefixParts+lastParts[-uniqueCount:])
     # ^ negative to keep last uniqueCount pairs
+    if used_ids in used_ids:
+        logger.warning(f"{id_str} was already used. Set increment=True"
+                       " if there is more than one local node!")
+    used_ids.add(id_str)
+    return id_str

openlcb/linklayer.py

@@ -48,7 +48,9 @@ class State(Enum):
     #   (enforced using type(self).__name__ != "LinkLayer" checks in methods)
 
     def __init__(self, physicalLayer: PhysicalLayer, localNodeID):
-        assert isinstance(physicalLayer, PhysicalLayer)  # allows any subclass
+        assert isinstance(physicalLayer, PhysicalLayer), \
+            f"Expected PhysicalLayer/subclass, got a(n) {type(physicalLayer)}"
+        # ^ allows any subclass
         # subclass should check type of localNodeID technically
         self.localNodeID = localNodeID
         self._messageReceivedListeners = []

openlcb/memoryconfigurationheader.py

@@ -51,8 +51,11 @@ def fromMC2ndByte(cls, datagramByte1: int, space: Union[int, None] = None) -> 'M
                 'custom space requires datagramByte1 with last 2 bits 00'
         else:
             # space is None
-            assert datagramByte1 & 0x03 != 0, \
-                'a standard space index must be in last 2 bits datagramByte1'
+            # assert datagramByte1 & 0x03 != 0, \
+            #     ("standard space index req. in low 2 bits of datagramByte1"
+            #      f" but got {hex(datagramByte1)}")
+            # ^ commented to allow indeterminate state,
+            #   so caller can set space later from the later space byte
             space = -1
         # NOTE: Third option is that space isn't known yet
         #   (must be set later, if spaceIsCustom())

openlcb/memorymanager.py

@@ -104,6 +104,9 @@ def setData(self, address: int, data: Union[bytearray, bytes],
         else:
             self._data[address:end] = data
 
+    def size(self) -> int:
+        return len(self._data)
+
 
 class MemoryManager:
     def __init__(self):

openlcb/memoryreadjob.py

@@ -63,7 +63,8 @@ def echoS(message: str):
 
         if isinstance(space, MemorySpace):
             space = space.value
-        echoS("Requesting memory read. Please wait...")
+        echoS("")
+        echoS(f"Requesting memory read (space={space}). Please wait...")
         # read 64 bytes from the CDI space starting at address zero
         self.memMemo = MemoryReadMemo(farNodeID, 64, space, 0,
                                       self.memoryReadFail,
@@ -85,9 +86,9 @@ def memoryReadSuccess(self, memo):
         if len(memo.data) == 64 and 0 not in memo.data:
             # save content
             self.resultingCDI += memo.data
-            logger.debug(
+            logger.info(
                 f"[{memo.address}] successful read"
-                f" {Convert.arrayToString(memo.data, len(memo.data))}"
+                f" `{Convert.arrayToString(memo.data, len(memo.data))}`"
                 "; next = address + 64")
             # update the address
             memo.address = memo.address+64
@@ -120,8 +121,11 @@ def memoryReadSuccess(self, memo):
             memo.done = True
             # done
 
-    def memoryReadFail(self, memo):
-        print("memory read failed: {}".format(memo.data))
+    def memoryReadFail(self, memo: MemoryReadMemo):
+        assert isinstance(memo, MemoryReadMemo)
+        print(f"memory read failed: id={memo.nodeID}"
+              f" data={memo.data} space={memo.space} address={memo.address}"
+              f" error={memo.error} code={memo.errorCode}")
         self.failed = True
 
     def processXML(self, content: str) :

openlcb/memoryservice.py

@@ -43,6 +43,7 @@
 from openlcb.convert import Convert
 # from openlcb.localnode import LocalNode  # circular import
 from openlcb.memoryconfigurationheader import MemoryConfigurationHeader
+from openlcb.memoryspace import MemorySpace
 from openlcb.memoryspaceindex import MemorySpaceIndex
 from openlcb.memorymanager import MemoryManager
 from openlcb.nodeid import NodeID
@@ -328,13 +329,17 @@ def parseReplyDatagram(memo: Union[MemoryReadMemo, MemoryWriteMemo],
     )
     offset = 6
     error = None
-    assert mcHeader.spaceIndex is not MemorySpaceIndex.Uninitialized
-    if mcHeader.spaceIndex is MemorySpaceIndex.Custom:
+    if mcHeader.spaceIndex in (MemorySpaceIndex.Custom,
+                               MemorySpaceIndex.Uninitialized):
         # mcHeader.customSpace = memo.space
         mcHeader.customSpace = dmemo.data[6]
         offset = 7
+    else:
+        assert mcHeader.customSpace is None, \
+            "fromMC2ndByte should not set customSpace in this case"
     memo.error = None
     memo.errorCode = None
+    print(f"reply datagram: {mcHeader.spaceIndex} customSpace={mcHeader.customSpace}")
     if (dmemo.data[1] & 0x08 == 0):
         # ok reply
         return
@@ -625,42 +630,55 @@ def datagramReceivedListener(self, dmemo: DatagramReadMemo) -> bool:
             self.spaceLengthCallback(address)
             self.spaceLengthCallback = None
         elif mcOp is MCOp.Read_Command:
-            # assert dmemo.data[1] in TWO_BIT_PARAMS[MCOp.Read_Command.value], \
+            # assert dmemo.data[1] in TWO_BIT_PARAMS[MCOp.Read_Command.value],\
             #     "self-test failed (bad constant(s))"
             mcHeader = MemoryConfigurationHeader.fromMC2ndByte(dmemo.data[1])
             addressBytes = dmemo.data[2:6]
             address = struct.unpack(">I", addressBytes)[0]
             # ^ [0] since always returns list even when reading 1 value.
             # ^ capital assumes unsigned, "I" assumes 32-bit (4 bytes)
-            space = dmemo.data[1] & 0b00000011
+            spaceIndex = dmemo.data[1] & 0b00000011
             offset = 0
+            space = None
             if mcHeader.spaceIsCustom():
-                assert space == 0
+                assert spaceIndex == 0
                 space = dmemo.data[6]
                 offset = 1
             size = dmemo.data[6+offset]  # requested read count
             datagramBytes = bytearray([0x20, MCOp.Read_Reply.value])
             # ^ byte1 (2nd) changed to error below if applicable
-            assert isinstance(space, int), \
-                (f"Logic missing, space should be number here,"
-                    f" got {emit_cast(space)}")
+            assert isinstance(spaceIndex, int), \
+                (f"Logic missing, spaceIndex should be number here,"
+                    f" got {emit_cast(spaceIndex)}")
             assert isinstance(address, int)
             datagramBytes += addressBytes
-            assert space is not None, \
-                f"space not computed from datagram: {dmemo.data}"
             if mcHeader.spaceIsCustom():
+                assert space is not None
                 datagramBytes.append(space)
                 assert len(datagramBytes) == 7, "space goes in index [6]"
             else:
+                space = MemorySpace.fromIndex(MemorySpaceIndex(spaceIndex))
+                assert space is not None
+                space = space.value
                 spaceIndex = (space & 0b00000011)
-                assert spaceIndex == space
                 assert space is not None
                 datagramBytes[1] = \
                     datagramBytes[1] | spaceIndex
                 assert len(datagramBytes) == 6, "should not have space in [6]"
+            assert space is not None, \
+                f"space not computed from datagram: {dmemo.data}"
             payload = None
             try:
-                payload = self.memory.getSlice(space, address, size)
+                segment = self.memory.getStorage(space)
+                if segment is None:
+                    raise KeyError(f"space {space} is not valid")
+                if address >= segment.size():
+                    raise IndexError(
+                        f"address {address} past end of {hex(space)}")
+                payload = self.memory.getSlice(space, address, size,
+                                               force=True)
+                # ^ force=True because reading past end is normal
+                #   (pad with zeroes to indicate end)
             except (IndexError, KeyError) as ex:
                 # address out of range (See Segment's getSlice)
                 datagramBytes[1] = MCOp.Read_Reply_Failure.value
@@ -698,6 +716,10 @@ def datagramReceivedListener(self, dmemo: DatagramReadMemo) -> bool:
                 dmemo.srcID,
                 datagramBytes
             )
+            # hexStrings = []
+            # for b in datagramBytes:
+            #     hexStrings.append(hex(b))
+            # print(f"Sending read reply: {hexStrings}")
             self.service.sendDatagram(requestedMemoryMemo)
         else:
             logger.error("Did not expect reply of type 0x{:02X}"

openlcb/nodeid.py

@@ -1,6 +1,10 @@
+from logging import getLogger
+
 from openlcb import emit_cast
 from openlcb.conventions import generate_node_id_str
 
+logger = getLogger(__name__)
+
 
 class NodeID:
     """A 6-byte (48-bit) Node ID.
@@ -88,7 +92,7 @@ def __hash__(self):
         return hash(self.value)
 
 
-def generate_node_id(id_range_prefix):
+def generate_node_id(id_range_prefix, increment=False):
     """Generate a unique NodeID for the session to ensure each
     instance (even of python-openlcb on same device) or
     locally-generated virtual node is unique.
@@ -99,7 +103,9 @@ def generate_node_id(id_range_prefix):
             python-openlcb (or as otherwise assigned by OpenLCB Group
             which reserves 05.* range). See
             <https://registry.openlcb.org/uniqueidranges>.
+        increment (bool): Increment such as more multiple
+            local IDs (virtual node(s) aside from local Node).
     Returns:
         NodeID: A NodeID that is unique (very likely...).
     """
-    return NodeID(generate_node_id_str(id_range_prefix))
+    return NodeID(generate_node_id_str(id_range_prefix, increment=increment))

openlcb/openlcbnetwork.py

@@ -368,7 +368,8 @@ def _fireStatus(self, status,
             print("OpenLCBNetwork callback_msg({})".format(repr(status)))
             callback(CDIMemo(status=status))
         else:
-            logger.warning("No callback, but set status: {}".format(status))
+            logger.warning(
+                f"[OpenLCBNetwork] No callback, but set status: {status}")
 
     def _memoryReadSuccess(self, memo: MemoryReadMemo, force_end=False):
         """Handle a successful read

openlcb/physicallayer.py

@@ -226,15 +226,18 @@ def encodeFrameAsData(self, frame) -> Union[bytearray, bytes]:
 
     def physicalLayerUp(self):
         """abstract method"""
-        raise NotImplementedError("Each subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement physicalLayerUp.")
 
     def physicalLayerRestart(self):
         """abstract method"""
-        raise NotImplementedError("Each subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement physicalLayerRestart.")
 
     def physicalLayerDown(self):
         """abstract method"""
-        raise NotImplementedError("Each subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement physicalLayerDown.")
 
     def handleData(self, data: Union[bytes, bytearray]) -> int:
         """abstract method (accept data, return # of frames created)"""

openlcb/portinterface.py

@@ -69,7 +69,8 @@ def setListeners(self, onReadyToSend, onReadyToReceive):
 
     def _settimeout(self, seconds):
         """Abstract method. Return: implementation-specific or None."""
-        raise NotImplementedError("Subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement _settimeout.")
 
     def settimeout(self, seconds):
         return self._settimeout(seconds)
@@ -79,7 +80,8 @@ def _connect(self, host: Any, port: Any, device: Any = None):
         See connect for details.
         raise exception on failure to prevent self._open = True.
         """
-        raise NotImplementedError("Subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement _connect.")
 
     def connect(self, host, port, device=None):
         """Connect to a port.
@@ -109,7 +111,8 @@ def connectLocal(self, port):
 
     def _send(self, data: Union[bytes, bytearray]) -> None:
         """Abstract method. Return: implementation-specific or None"""
-        raise NotImplementedError("Subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement _send.")
 
     def send(self, data: Union[bytes, bytearray]) -> None:
         """
@@ -132,7 +135,8 @@ def send(self, data: Union[bytes, bytearray]) -> None:
 
     def _receive(self) -> Union[bytearray, bytes, None]:
         """Abstract method. Return (bytes): data"""
-        raise NotImplementedError("Subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement _receive.")
 
     def receive(self) -> Union[bytearray, bytes, None]:
         self._setBusy("receive")
@@ -147,7 +151,8 @@ def receive(self) -> Union[bytearray, bytes, None]:
 
     def _close(self) -> None:
         """Abstract method. Return: implementation-specific or None"""
-        raise NotImplementedError("Subclass must implement this.")
+        raise NotImplementedError(
+            f"{type(self).__name__} subclass must implement _close.")
 
     def setOpen(self, is_open):
         if self._open != is_open: