Merge pull request #49 from Nakakiyo092/feature/refactor

Refactoring

Author: Nakakiyo092

src/reader.py

@@ -103,7 +103,7 @@
 }
 
 
-def get_argparser():
+def _get_argparser():
     """Get the command line argument parser."""
 
     parser = argparse.ArgumentParser(
@@ -128,7 +128,7 @@ def get_argparser():
     return parser
 
 
-def create_bus(args):
+def _create_bus(args):
     """Create and return a CAN bus, or None if initialization fails."""
     try:
         if args.devicename == "virtual":
@@ -158,94 +158,98 @@ def create_bus(args):
         return None
 
 
-def read_all_dids(args, bus, notifier):
-    """Read all EDR DIDs via 11bits functional, 11bits physical, and 29bits addresses."""
-
-    # Abbreviated name
-    func = isotp.TargetAddressType.Functional
-    phys = isotp.TargetAddressType.Physical
-
-    # Read with 11bits functional address (See GB39732-2020 for the address values)
-    # rxid=0x700 is a dummy; the functional broadcast (txid=0x7DF) does not listen on a fixed ID.
-    tx_addr = isotp.Address(isotp.AddressingMode.Normal_11bits, txid=_TX_FUNC_11BIT, rxid=0x700)
-    rx_addrs = []
-    # Pre-allocate one receive stack per plausible physical CAN ID pair in the 0x700–0x7FF range.
-    # GB39732-2020 physical pairs follow the convention: ECU TX = tester TX + 8
+def _build_11func(bus, notifier, params):
+    """11bits functional: emit-only tx_stack on 0x7DF + per-ECU rx_stacks (0x700-0x7FF)."""
+    # rxid=0x700 is a placeholder to satisfy Address validation; no ECU transmits on it.
+    tx_addr = isotp.Address(
+        isotp.AddressingMode.Normal_11bits, txid=_TX_FUNC_11BIT, rxid=0x700
+    )
+    tx_stack = isotp.NotifierBasedCanStack(
+        bus=bus, notifier=notifier, address=tx_addr, params=params
+    )
+    # Pre-allocate one rx stack per plausible per-ECU physical pair (DESIGN.md library-gap),
+    # so the FF and the subsequent FC/CFs are received on the responder's own pair.
+    rx_stacks = []
     for i in range(0x100 - 0x8):
-        rx_addrs.append(isotp.Address(
-            isotp.AddressingMode.Normal_11bits, txid=0x700 + i, rxid=0x700 + i + 8))
+        rx_addr = isotp.Address(
+            isotp.AddressingMode.Normal_11bits, txid=0x700 + i, rxid=0x700 + i + 8
+        )
+        rx_stacks.append(isotp.NotifierBasedCanStack(
+            bus=bus, notifier=notifier, address=rx_addr, params=params
+        ))
+    return tx_stack, rx_stacks, isotp.TargetAddressType.Functional, "11bits functional address"
 
-    try:
-        for did in _EDR_DID_LIST:
-            payload = read_did(did, bus, notifier, tx_addr, rx_addrs, func, _ISOTP_PARAMS, args.timeout)
-            output_data(payload)
-    except Exception as err:
-        print(err)
 
-    # Read with 11bits physical address (See GB39732-2020 for the address values)
-    tx_addr = isotp.Address(isotp.AddressingMode.Normal_11bits, txid=_TX_PHYS_11BIT, rxid=_RX_PHYS_11BIT)
-    rx_addrs = []
+def _build_11phys(bus, notifier, params):
+    """11bits physical: single symmetric stack used for both send and receive."""
+    addr = isotp.Address(
+        isotp.AddressingMode.Normal_11bits, txid=_TX_PHYS_11BIT, rxid=_RX_PHYS_11BIT
+    )
+    stack = isotp.NotifierBasedCanStack(
+        bus=bus, notifier=notifier, address=addr, params=params
+    )
+    return stack, [stack], isotp.TargetAddressType.Physical, "11bits physical address"
 
-    try:
-        for did in _EDR_DID_LIST:
-            payload = read_did(did, bus, notifier, tx_addr, rx_addrs, phys, _ISOTP_PARAMS, args.timeout)
-            output_data(payload)
-    except Exception as err:
-        print(err)
 
-    # Read with 29bits address (See GB39732-2020 for the address values)
+def _build_29bit(bus, notifier, params):
+    """29bits NormalFixed: emit-only broadcast tx_stack + per-ECU rx_stacks."""
     tx_addr = isotp.Address(
         isotp.AddressingMode.NormalFixed_29bits,
         target_address=_BROADCAST_29BIT,
-        source_address=_TESTER_ADDR
+        source_address=_TESTER_ADDR,
+    )
+    tx_stack = isotp.NotifierBasedCanStack(
+        bus=bus, notifier=notifier, address=tx_addr, params=params
     )
-    rx_addrs = []
+    # Cover every plausible responder address (excluding the OBD functional broadcast).
+    rx_stacks = []
     for i in range(0xF0):
-        if i != _OBD_FUNC_ADDR:
-            rx_addrs.append(isotp.Address(
-                isotp.AddressingMode.NormalFixed_29bits,
-                target_address=i,
-                source_address=_TESTER_ADDR
-            ))
-
-    try:
-        for did in _EDR_DID_LIST:
-            payload = read_did(did, bus, notifier, tx_addr, rx_addrs, func, _ISOTP_PARAMS, args.timeout)
-            output_data(payload)
-    except Exception as err:
-        print(err)
+        if i == _OBD_FUNC_ADDR:
+            continue
+        rx_addr = isotp.Address(
+            isotp.AddressingMode.NormalFixed_29bits,
+            target_address=i,
+            source_address=_TESTER_ADDR,
+        )
+        rx_stacks.append(isotp.NotifierBasedCanStack(
+            bus=bus, notifier=notifier, address=rx_addr, params=params
+        ))
+    return tx_stack, rx_stacks, isotp.TargetAddressType.Functional, "29bits address"
 
 
-def read_did(did, bus, notifier, tx_addr, rx_addrs, addr_type, isotp_params,
-             timeout=_DEFAULT_TIMEOUT_S) -> bytearray | None:
+def _read_all_dids(args, bus, notifier):
+    """Read all EDR DIDs via 11bits functional, 11bits physical, and 29bits addresses."""
+    for builder in (_build_11func, _build_11phys, _build_29bit):
+        try:
+            tx_stack, rx_stacks, addr_type, mode_label = builder(
+                bus, notifier, _ISOTP_PARAMS
+            )
+            # The 11bits physical builder returns the same instance as tx_stack and
+            # rx_stacks[0]; set() dedupes so start() / stop() run once per stack.
+            # python-can-isotp's TransportLayer is designed for long-lived stacks:
+            # construct once per scheme, reuse across DIDs, tear down at the end.
+            all_stacks = {tx_stack, *rx_stacks}
+            for s in all_stacks:
+                s.start()
+            try:
+                for did in _EDR_DID_LIST:
+                    payload = _read_did(
+                        did, tx_stack, rx_stacks, addr_type, mode_label, args.timeout
+                    )
+                    _output_data(payload)
+            finally:
+                for s in all_stacks:
+                    s.stop()
+        except Exception as err:
+            print(err)
+
+
+def _read_did(did, tx_stack, rx_stacks, addr_type, mode_label,
+              timeout=_DEFAULT_TIMEOUT_S) -> bytearray | None:
     """Read one data by identifier (DID) from the target ECU."""
 
     print("")
-    if tx_addr.is_tx_29bits():
-        print("Reading data id", hex(did), "with 29bits address.")
-    elif addr_type == isotp.TargetAddressType.Functional:
-        print("Reading data id", hex(did), "with 11bits functional address.")
-    else:
-        print("Reading data id", hex(did), "with 11bits physical address.")
-
-    # Setup ISOTP stacks
-    tx_stack = isotp.NotifierBasedCanStack(
-        bus=bus,
-        notifier=notifier,
-        address=tx_addr,
-        params=isotp_params
-    )
-    rx_stacks = []
-    # In Physical addressing, request and response share the same address pair (tx_addr),
-    # so rx_addrs can be void. tx_stack is included in rx_stacks to receive the ECU's response.
-    rx_stacks.append(tx_stack)
-    for rx_addr in rx_addrs:
-        rx_stack = isotp.NotifierBasedCanStack(
-            bus=bus, notifier=notifier,
-            address=rx_addr,
-            params=isotp_params
-        )
-        rx_stacks.append(rx_stack)
+    print(f"Reading data id {hex(did)} with {mode_label}.")
 
     # Build the UDS ReadDataByIdentifier request payload.
     # didconfig maps DID to a string codec; udsoncan requires it even though we
@@ -262,46 +266,35 @@ def read_did(did, bus, notifier, tx_addr, rx_addrs, addr_type, isotp_params,
         data=bytes([(did >> 8) & 0xFF, did & 0xFF])  # DID encoded as big-endian 2-byte
     )
 
-    # Start stacks
-    for rx_stack in rx_stacks:
-        rx_stack.start()
-
-    # Send request
+    # Send request. Stacks are started/stopped by the caller (_read_all_dids)
+    # once per addressing scheme, not per DID.
     tx_stack.send(request.get_payload(), addr_type)
 
     try:
-        # Wait for response
-        waiting = True
-        start_time = time.time()
-        while waiting:
-            # Response timeout
-            if time.time() - start_time > timeout:
-                payload = None
-                break
-
-            # Check response for all stacks
+        # Wait for response.
+        # Non-blocking recv(): a sweep over hundreds of rx_stacks takes microseconds
+        # when empty, so the user-supplied timeout is honored within ~1 ms granularity.
+        # monotonic() is immune to wall-clock adjustments mid-wait.
+        payload = None
+        deadline = time.monotonic() + timeout
+        while payload is None and time.monotonic() < deadline:
             for rx_stack in rx_stacks:
-                # TODO: #42
-                payload = rx_stack.recv(block=True, timeout=0.01)
-                if payload is not None:
-                    # Compare only the header portion of the received payload against
-                    # the expected positive-response bytes. The remainder is data.
-                    if payload[:len(response)] == response.get_payload():
-                        # Positive response
-                        waiting = False
-                        break
-                    else:
-                        # No Negative response handling. See the DESIGN.md.
-                        pass
-
+                received = rx_stack.recv(block=False)
+                if received is None:
+                    continue
+                # Compare only the header portion of the received payload against
+                # the expected positive-response bytes. The remainder is data.
+                if received[:len(response)] == response.get_payload():
+                    # Positive response
+                    payload = received
+                    break
+                # Non-matching payload (e.g., negative response). See DESIGN.md.
+            if payload is None:
+                time.sleep(0.001)
     except Exception as err:
         print(err)
         return None
 
-    # Stop stacks
-    for rx_stack in rx_stacks:
-        rx_stack.stop()
-
     if payload is not None:
         print(len(payload), "bytes of data received.")
     else:
@@ -310,7 +303,7 @@ def read_did(did, bus, notifier, tx_addr, rx_addrs, addr_type, isotp_params,
     return payload
 
 
-def output_data(payload) -> None:
+def _output_data(payload) -> None:
     """Output the data to a CSV file according to the format defined in the 'format' folder."""
 
     # Get target did from payload
@@ -379,7 +372,7 @@ def output_data(payload) -> None:
         return
 
 
-def copy_readme():
+def _copy_readme():
     """Copy the README file from the format folder to the result folder."""
     try:
         shutil.copy("format/README.md", "result/README.md")
@@ -397,11 +390,11 @@ def main():
     """Main process."""
 
     # Parse command line arguments
-    argparser = get_argparser()
+    argparser = _get_argparser()
     args = argparser.parse_args()
 
     # Setup and start a CAN bus
-    bus = create_bus(args)
+    bus = _create_bus(args)
     if bus is None:
         return
 
@@ -413,14 +406,14 @@ def main():
 
     try:
         # Read all EDR DIDs
-        read_all_dids(args, bus, notifier)
+        _read_all_dids(args, bus, notifier)
     finally:
         # Shutdown the CAN bus
         notifier.stop()
         bus.shutdown()
 
     # Copy the README file
-    copy_readme()
+    _copy_readme()
 
 
 if __name__ == "__main__":