From 7d6ebabc1b31517262b11778e45ff8e0a2c0ae28 Mon Sep 17 00:00:00 2001
From: Sebastien Bourdeauducq <sb@m-labs.hk>
Date: Mon, 27 Feb 2017 18:37:30 +0800
Subject: [PATCH] reorganize core device communication code

---
 RELEASE_NOTES.rst                             |  2 +
 .../{analyzer.py => comm_analyzer.py}         | 38 +++++++++-
 .../{comm_generic.py => comm_kernel.py}       | 61 +++++++++++----
 .../{comm_dummy.py => comm_kernel_dummy.py}   |  2 +-
 .../coredevice/{moninj.py => comm_moninj.py}  |  4 +-
 artiq/coredevice/comm_tcp.py                  | 76 -------------------
 artiq/dashboard/moninj.py                     |  6 +-
 artiq/examples/drtio/device_db.pyon           |  4 +-
 artiq/examples/master/device_db.pyon          |  4 +-
 artiq/examples/phaser/device_db.pyon          |  4 +-
 artiq/frontend/artiq_coreanalyzer.py          | 44 +++++------
 artiq/gateware/rtio/analyzer.py               |  2 +-
 artiq/protocols/analyzer.py                   | 21 -----
 artiq/test/coredevice/test_analyzer.py        |  6 +-
 artiq/test/coredevice/test_moninj.py          |  2 +-
 .../test/lit/devirtualization/device_db.pyon  |  4 +-
 artiq/test/lit/embedding/device_db.pyon       |  4 +-
 artiq/test/lit/escape/device_db.pyon          |  4 +-
 artiq/test/lit/regression/device_db.pyon      |  4 +-
 19 files changed, 131 insertions(+), 161 deletions(-)
 rename artiq/coredevice/{analyzer.py => comm_analyzer.py} (96%)
 rename artiq/coredevice/{comm_generic.py => comm_kernel.py} (90%)
 rename artiq/coredevice/{comm_dummy.py => comm_kernel_dummy.py} (95%)
 rename artiq/coredevice/{moninj.py => comm_moninj.py} (97%)
 delete mode 100644 artiq/coredevice/comm_tcp.py
 delete mode 100644 artiq/protocols/analyzer.py

diff --git a/RELEASE_NOTES.rst b/RELEASE_NOTES.rst
index d1f4ad35b..585c6bb01 100644
--- a/RELEASE_NOTES.rst
+++ b/RELEASE_NOTES.rst
@@ -24,6 +24,8 @@ Release notes
   the device database.
 * ``int(a, width=b)`` has been removed. Use ``int32(a)`` and ``int64(a)``.
 * The kc705 gateware target has been renamed kc705_dds.
+* ``artiq.coredevice.comm_tcp`` has been renamed ``artiq.coredevice.comm_kernel``,
+  and ``Comm`` has been renamed ``CommKernel``.
 
 
 2.2
diff --git a/artiq/coredevice/analyzer.py b/artiq/coredevice/comm_analyzer.py
similarity index 96%
rename from artiq/coredevice/analyzer.py
rename to artiq/coredevice/comm_analyzer.py
index ea6de22d1..ce611f96f 100644
--- a/artiq/coredevice/analyzer.py
+++ b/artiq/coredevice/comm_analyzer.py
@@ -2,15 +2,49 @@ from operator import itemgetter
 from collections import namedtuple
 from itertools import count
 from contextlib import contextmanager
+from enum import Enum
 import struct
 import logging
-
-from artiq.protocols.analyzer import MessageType, ExceptionType
+import socket
 
 
 logger = logging.getLogger(__name__)
 
 
+class MessageType(Enum):
+    output = 0b00
+    input = 0b01
+    exception = 0b10
+    stopped = 0b11
+
+
+class ExceptionType(Enum):
+    legacy_reset = 0b000000
+    legacy_reset_falling = 0b000001
+    legacy_reset_phy = 0b000010
+    legacy_reset_phy_falling = 0b000011
+
+    o_underflow_reset = 0b010000
+    o_sequence_error_reset = 0b010001
+    o_collision_reset = 0b010010
+
+    i_overflow_reset = 0b100000
+
+
+def get_analyzer_dump(host, port=1382):
+    sock = socket.create_connection((host, port))
+    try:
+        r = bytes()
+        while True:
+            buf = sock.recv(8192)
+            if not buf:
+                break
+            r += buf
+    finally:
+        sock.close()
+    return r
+
+
 OutputMessage = namedtuple(
     "OutputMessage", "channel timestamp rtio_counter address data")
 
diff --git a/artiq/coredevice/comm_generic.py b/artiq/coredevice/comm_kernel.py
similarity index 90%
rename from artiq/coredevice/comm_generic.py
rename to artiq/coredevice/comm_kernel.py
index d6d049dcd..b6a1a00fc 100644
--- a/artiq/coredevice/comm_generic.py
+++ b/artiq/coredevice/comm_kernel.py
@@ -1,5 +1,7 @@
 import struct
 import logging
+import socket
+import sys
 import traceback
 import numpy
 from enum import Enum
@@ -69,29 +71,60 @@ class RPCReturnValueError(ValueError):
 RPCKeyword = namedtuple('RPCKeyword', ['name', 'value'])
 
 
-class CommGeneric:
-    def __init__(self):
+def set_keepalive(sock, after_idle, interval, max_fails):
+    if sys.platform.startswith("linux"):
+        sock.setsockopt(socket.SOL_SOCKET, socket.SO_KEEPALIVE, 1)
+        sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPIDLE, after_idle)
+        sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPINTVL, interval)
+        sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPCNT, max_fails)
+    elif sys.platform.startswith("win") or sys.platform.startswith("cygwin"):
+        # setting max_fails is not supported, typically ends up being 5 or 10
+        # depending on Windows version
+        sock.ioctl(socket.SIO_KEEPALIVE_VALS,
+                   (1, after_idle*1000, interval*1000))
+    else:
+        logger.warning("TCP keepalive not supported on platform '%s', ignored",
+                       sys.platform)
+
+
+def initialize_connection(host, port):
+    sock = socket.create_connection((host, port), 5.0)
+    sock.settimeout(None)
+    set_keepalive(sock, 3, 2, 3)
+    logger.debug("connected to host %s on port %d", host, port)
+    return sock
+
+
+class CommKernel:
+    def __init__(self, dmgr, host, port=1381):
         self._read_type = None
+        self.host = host
+        self.port = port
 
     def open(self):
-        """Opens the communication channel.
-        Must do nothing if already opened."""
-        raise NotImplementedError
+        if hasattr(self, "socket"):
+            return
+        self.socket = initialize_connection(self.host, self.port)
+        self.socket.sendall(b"ARTIQ coredev\n")
 
     def close(self):
-        """Closes the communication channel.
-        Must do nothing if already closed."""
-        raise NotImplementedError
+        if not hasattr(self, "socket"):
+            return
+        self.socket.close()
+        del self.socket
+        logger.debug("disconnected")
 
     def read(self, length):
-        """Reads exactly length bytes from the communication channel.
-        The channel is assumed to be opened."""
-        raise NotImplementedError
+        r = bytes()
+        while len(r) < length:
+            rn = self.socket.recv(min(8192, length - len(r)))
+            if not rn:
+                raise ConnectionResetError("Connection closed")
+            r += rn
+        return r
 
     def write(self, data):
-        """Writes exactly length bytes to the communication channel.
-        The channel is assumed to be opened."""
-        raise NotImplementedError
+        self.socket.sendall(data)
 
     #
     # Reader interface
diff --git a/artiq/coredevice/comm_dummy.py b/artiq/coredevice/comm_kernel_dummy.py
similarity index 95%
rename from artiq/coredevice/comm_dummy.py
rename to artiq/coredevice/comm_kernel_dummy.py
index 5c20d7dc2..c9dae5200 100644
--- a/artiq/coredevice/comm_dummy.py
+++ b/artiq/coredevice/comm_kernel_dummy.py
@@ -1,7 +1,7 @@
 from operator import itemgetter
 
 
-class Comm:
+class CommKernel:
     def __init__(self, dmgr):
         super().__init__()
 
diff --git a/artiq/coredevice/moninj.py b/artiq/coredevice/comm_moninj.py
similarity index 97%
rename from artiq/coredevice/moninj.py
rename to artiq/coredevice/comm_moninj.py
index c6e181ac3..25fd11ba0 100644
--- a/artiq/coredevice/moninj.py
+++ b/artiq/coredevice/comm_moninj.py
@@ -4,7 +4,7 @@ import struct
 from enum import Enum
 
 
-__all__ = ["TTLProbe", "TTLOverride", "MonInjComm"]
+__all__ = ["TTLProbe", "TTLOverride", "CommMonInj"]
 
 
 logger = logging.getLogger(__name__)
@@ -21,7 +21,7 @@ class TTLOverride(Enum):
     oe = 2
 
 
-class MonInjComm:
+class CommMonInj:
     def __init__(self, monitor_cb, injection_status_cb, disconnect_cb=None):
         self.monitor_cb = monitor_cb
         self.injection_status_cb = injection_status_cb
diff --git a/artiq/coredevice/comm_tcp.py b/artiq/coredevice/comm_tcp.py
deleted file mode 100644
index 9d0a22703..000000000
--- a/artiq/coredevice/comm_tcp.py
+++ /dev/null
@@ -1,76 +0,0 @@
-import logging
-import socket
-import sys
-
-from artiq.coredevice.comm_generic import CommGeneric
-
-
-logger = logging.getLogger(__name__)
-
-
-def set_keepalive(sock, after_idle, interval, max_fails):
-    if sys.platform.startswith("linux"):
-        sock.setsockopt(socket.SOL_SOCKET, socket.SO_KEEPALIVE, 1)
-        sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPIDLE, after_idle)
-        sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPINTVL, interval)
-        sock.setsockopt(socket.IPPROTO_TCP, socket.TCP_KEEPCNT, max_fails)
-    elif sys.platform.startswith("win") or sys.platform.startswith("cygwin"):
-        # setting max_fails is not supported, typically ends up being 5 or 10
-        # depending on Windows version
-        sock.ioctl(socket.SIO_KEEPALIVE_VALS,
-                   (1, after_idle*1000, interval*1000))
-    else:
-        logger.warning("TCP keepalive not supported on platform '%s', ignored",
-                       sys.platform)
-
-
-def initialize_connection(host, port):
-    sock = socket.create_connection((host, port), 5.0)
-    sock.settimeout(None)
-    set_keepalive(sock, 3, 2, 3)
-    logger.debug("connected to host %s on port %d", host, port)
-    return sock
-
-
-class Comm(CommGeneric):
-    def __init__(self, dmgr, host, port=1381, port_analyzer=1382):
-        super().__init__()
-        self.host = host
-        self.port = port
-        self.port_analyzer = port_analyzer
-
-    def open(self):
-        if hasattr(self, "socket"):
-            return
-        self.socket = initialize_connection(self.host, self.port)
-        self.socket.sendall(b"ARTIQ coredev\n")
-
-    def close(self):
-        if not hasattr(self, "socket"):
-            return
-        self.socket.close()
-        del self.socket
-        logger.debug("disconnected")
-
-    def read(self, length):
-        r = bytes()
-        while len(r) < length:
-            rn = self.socket.recv(min(8192, length - len(r)))
-            if not rn:
-                raise ConnectionResetError("Connection closed")
-            r += rn
-        return r
-
-    def write(self, data):
-        self.socket.sendall(data)
-
-    def get_analyzer_dump(self):
-        sock = initialize_connection(self.host, self.port_analyzer)
-        r = bytes()
-        while True:
-            buf = sock.recv(8192)
-            if not buf:
-                break
-            r += buf
-        sock.close()
-        return r
diff --git a/artiq/dashboard/moninj.py b/artiq/dashboard/moninj.py
index 723e6d47a..42150e5e0 100644
--- a/artiq/dashboard/moninj.py
+++ b/artiq/dashboard/moninj.py
@@ -4,7 +4,7 @@ import logging
 from PyQt5 import QtCore, QtWidgets, QtGui
 
 from artiq.protocols.sync_struct import Subscriber
-from artiq.coredevice.moninj import *
+from artiq.coredevice.comm_moninj import *
 from artiq.gui.tools import LayoutWidget
 from artiq.gui.flowlayout import FlowLayout
 
@@ -197,7 +197,7 @@ class _DeviceManager:
         try:
             if v["type"] == "local":
                 widget = None
-                if v["module"] == "artiq.coredevice.comm_tcp":
+                if k == "comm":
                     self.core_addr = v["arguments"]["host"]
                     self.new_core_addr.set()
                 elif v["module"] == "artiq.coredevice.ttl":
@@ -300,7 +300,7 @@ class _DeviceManager:
             if self.core_connection is not None:
                 await self.core_connection.close()
                 self.core_connection = None
-            new_core_connection = MonInjComm(self.monitor_cb, self.injection_status_cb,
+            new_core_connection = CommMonInj(self.monitor_cb, self.injection_status_cb,
                     lambda: logger.error("lost connection to core device moninj"))
             try:
                 await new_core_connection.connect(self.core_addr, 1383)
diff --git a/artiq/examples/drtio/device_db.pyon b/artiq/examples/drtio/device_db.pyon
index 269a40ba4..380c2dd39 100644
--- a/artiq/examples/drtio/device_db.pyon
+++ b/artiq/examples/drtio/device_db.pyon
@@ -5,8 +5,8 @@
 {
     "comm": {
         "type": "local",
-        "module": "artiq.coredevice.comm_tcp",
-        "class": "Comm",
+        "module": "artiq.coredevice.comm_kernel",
+        "class": "CommKernel",
         "arguments": {"host": "kc705.lab.m-labs.hk"}
     },
     "core": {
diff --git a/artiq/examples/master/device_db.pyon b/artiq/examples/master/device_db.pyon
index ec9bc6083..71dede55d 100644
--- a/artiq/examples/master/device_db.pyon
+++ b/artiq/examples/master/device_db.pyon
@@ -5,8 +5,8 @@
 {
     "comm": {
         "type": "local",
-        "module": "artiq.coredevice.comm_tcp",
-        "class": "Comm",
+        "module": "artiq.coredevice.comm_kernel",
+        "class": "CommKernel",
         "arguments": {"host": "kc705.lab.m-labs.hk"}
     },
     "core": {
diff --git a/artiq/examples/phaser/device_db.pyon b/artiq/examples/phaser/device_db.pyon
index 5bfaabf9f..208762478 100644
--- a/artiq/examples/phaser/device_db.pyon
+++ b/artiq/examples/phaser/device_db.pyon
@@ -3,8 +3,8 @@
 {
     "comm": {
         "type": "local",
-        "module": "artiq.coredevice.comm_tcp",
-        "class": "Comm",
+        "module": "artiq.coredevice.comm_kernel",
+        "class": "CommKernel",
         "arguments": {"host": "kc705aux.lab.m-labs.hk"}
     },
     "core": {
diff --git a/artiq/frontend/artiq_coreanalyzer.py b/artiq/frontend/artiq_coreanalyzer.py
index ec1b4406d..4f7626ea9 100755
--- a/artiq/frontend/artiq_coreanalyzer.py
+++ b/artiq/frontend/artiq_coreanalyzer.py
@@ -6,7 +6,8 @@ import sys
 from artiq.tools import verbosity_args, init_logger
 from artiq.master.databases import DeviceDB
 from artiq.master.worker_db import DeviceManager
-from artiq.coredevice.analyzer import decode_dump, decoded_dump_to_vcd
+from artiq.coredevice.comm_analyzer import (get_analyzer_dump,
+                                            decode_dump, decoded_dump_to_vcd)
 
 
 def get_argparser():
@@ -38,28 +39,25 @@ def main():
         sys.exit(1)
 
     device_mgr = DeviceManager(DeviceDB(args.device_db))
-    try:
-        if args.read_dump:
-            with open(args.read_dump, "rb") as f:
-                dump = f.read()
-        else:
-            comm = device_mgr.get("comm")
-            dump = comm.get_analyzer_dump()
-        decoded_dump = decode_dump(dump)
-        if args.print_decoded:
-            print("Log channel:", decoded_dump.log_channel)
-            print("DDS one-hot:", decoded_dump.dds_onehot_sel)
-            for message in decoded_dump.messages:
-                print(message)
-        if args.write_vcd:
-            with open(args.write_vcd, "w") as f:
-                decoded_dump_to_vcd(f, device_mgr.get_device_db(),
-                                    decoded_dump)
-        if args.write_dump:
-            with open(args.write_dump, "wb") as f:
-                f.write(dump)
-    finally:
-        device_mgr.close_devices()        
+    if args.read_dump:
+        with open(args.read_dump, "rb") as f:
+            dump = f.read()
+    else:
+        core_addr = device_mgr.get_desc("comm")["arguments"]["host"]
+        dump = get_analyzer_dump(core_addr)
+    decoded_dump = decode_dump(dump)
+    if args.print_decoded:
+        print("Log channel:", decoded_dump.log_channel)
+        print("DDS one-hot:", decoded_dump.dds_onehot_sel)
+        for message in decoded_dump.messages:
+            print(message)
+    if args.write_vcd:
+        with open(args.write_vcd, "w") as f:
+            decoded_dump_to_vcd(f, device_mgr.get_device_db(),
+                                decoded_dump)
+    if args.write_dump:
+        with open(args.write_dump, "wb") as f:
+            f.write(dump)
 
 
 if __name__ == "__main__":
diff --git a/artiq/gateware/rtio/analyzer.py b/artiq/gateware/rtio/analyzer.py
index 0794f35bc..56fe80aa7 100644
--- a/artiq/gateware/rtio/analyzer.py
+++ b/artiq/gateware/rtio/analyzer.py
@@ -3,7 +3,7 @@ from migen.genlib.record import Record, layout_len
 from misoc.interconnect.csr import *
 from misoc.interconnect import stream
 
-from artiq.protocols.analyzer import MessageType, ExceptionType
+from artiq.coredevice.comm_analyzer import MessageType, ExceptionType
 
 
 __all__ = ["Analyzer"]
diff --git a/artiq/protocols/analyzer.py b/artiq/protocols/analyzer.py
deleted file mode 100644
index 0941e697e..000000000
--- a/artiq/protocols/analyzer.py
+++ /dev/null
@@ -1,21 +0,0 @@
-from enum import Enum
-
-
-class MessageType(Enum):
-    output = 0b00
-    input = 0b01
-    exception = 0b10
-    stopped = 0b11
-
-
-class ExceptionType(Enum):
-    legacy_reset = 0b000000
-    legacy_reset_falling = 0b000001
-    legacy_reset_phy = 0b000010
-    legacy_reset_phy_falling = 0b000011
-
-    o_underflow_reset = 0b010000
-    o_sequence_error_reset = 0b010001
-    o_collision_reset = 0b010010
-
-    i_overflow_reset = 0b100000
diff --git a/artiq/test/coredevice/test_analyzer.py b/artiq/test/coredevice/test_analyzer.py
index 496354b36..a7fd84780 100644
--- a/artiq/test/coredevice/test_analyzer.py
+++ b/artiq/test/coredevice/test_analyzer.py
@@ -1,7 +1,7 @@
 from artiq.experiment import *
-from artiq.coredevice.analyzer import (decode_dump, StoppedMessage,
-                                       OutputMessage, InputMessage,
-                                       _extract_log_chars)
+from artiq.coredevice.comm_analyzer import (decode_dump, StoppedMessage,
+                                            OutputMessage, InputMessage,
+                                           _extract_log_chars)
 from artiq.test.hardware_testbench import ExperimentCase
 
 
diff --git a/artiq/test/coredevice/test_moninj.py b/artiq/test/coredevice/test_moninj.py
index 23e27e098..aaf47d87f 100644
--- a/artiq/test/coredevice/test_moninj.py
+++ b/artiq/test/coredevice/test_moninj.py
@@ -21,7 +21,7 @@ class MonInjTest(ExperimentCase):
 
         loop = asyncio.get_event_loop()
         try:
-            moninj_comm = MonInjComm(monitor_cb, injection_status_cb)
+            moninj_comm = CommMonInj(monitor_cb, injection_status_cb)
             loop.run_until_complete(moninj_comm.connect(core_host))
             try:
                 moninj_comm.get_injection_status(loop_out_channel, TTLOverride.en.value)
diff --git a/artiq/test/lit/devirtualization/device_db.pyon b/artiq/test/lit/devirtualization/device_db.pyon
index 02d4e06bb..eabdbcc0a 100644
--- a/artiq/test/lit/devirtualization/device_db.pyon
+++ b/artiq/test/lit/devirtualization/device_db.pyon
@@ -1,8 +1,8 @@
 {
     "comm": {
         "type": "local",
-        "module": "artiq.coredevice.comm_dummy",
-        "class": "Comm",
+        "module": "artiq.coredevice.comm_kernel_dummy",
+        "class": "CommKernel",
         "arguments": {}
     },
     "core": {
diff --git a/artiq/test/lit/embedding/device_db.pyon b/artiq/test/lit/embedding/device_db.pyon
index 02d4e06bb..eabdbcc0a 100644
--- a/artiq/test/lit/embedding/device_db.pyon
+++ b/artiq/test/lit/embedding/device_db.pyon
@@ -1,8 +1,8 @@
 {
     "comm": {
         "type": "local",
-        "module": "artiq.coredevice.comm_dummy",
-        "class": "Comm",
+        "module": "artiq.coredevice.comm_kernel_dummy",
+        "class": "CommKernel",
         "arguments": {}
     },
     "core": {
diff --git a/artiq/test/lit/escape/device_db.pyon b/artiq/test/lit/escape/device_db.pyon
index 02d4e06bb..eabdbcc0a 100644
--- a/artiq/test/lit/escape/device_db.pyon
+++ b/artiq/test/lit/escape/device_db.pyon
@@ -1,8 +1,8 @@
 {
     "comm": {
         "type": "local",
-        "module": "artiq.coredevice.comm_dummy",
-        "class": "Comm",
+        "module": "artiq.coredevice.comm_kernel_dummy",
+        "class": "CommKernel",
         "arguments": {}
     },
     "core": {
diff --git a/artiq/test/lit/regression/device_db.pyon b/artiq/test/lit/regression/device_db.pyon
index 02d4e06bb..eabdbcc0a 100644
--- a/artiq/test/lit/regression/device_db.pyon
+++ b/artiq/test/lit/regression/device_db.pyon
@@ -1,8 +1,8 @@
 {
     "comm": {
         "type": "local",
-        "module": "artiq.coredevice.comm_dummy",
-        "class": "Comm",
+        "module": "artiq.coredevice.comm_kernel_dummy",
+        "class": "CommKernel",
         "arguments": {}
     },
     "core": {