[GitHub] [tvm] areusch commented on a change in pull request #8236: [microTVM] Add wait to QEMU Setup

[GitBox <gi...@apache.org>]

areusch commented on a change in pull request #8236:
URL: https://github.com/apache/tvm/pull/8236#discussion_r649371406



##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -703,6 +709,21 @@ def write(self, data, timeout_sec):
             raise TransportClosedError()
         return self.fd_transport.write(data, timeout_sec)
 
+    def _qemu_check_stdout(self):
+        for line in self.proc.stdout:
+            line = str(line)
+            _LOG.debug(line)
+            if "[QEMU] CPU" in line:
+                self._queue.put(self._qemu_ready_msg)
+                return
+
+    def _wait_for_qemu(self):
+        threading.Thread(target=self._qemu_check_stdout, daemon=True).start()
+        while True:
+            item = self._queue.get()

Review comment:
       add a timeout here, even if it's long. also add an assert message to the timeout

##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -703,6 +709,21 @@ def write(self, data, timeout_sec):
             raise TransportClosedError()
         return self.fd_transport.write(data, timeout_sec)
 
+    def _qemu_check_stdout(self):
+        for line in self.proc.stdout:
+            line = str(line)
+            _LOG.debug(line)
+            if "[QEMU] CPU" in line:
+                self._queue.put(self._qemu_ready_msg)

Review comment:
       i think we need to push an indicator of success (QEMU startup message found) or failure (didn't find that message; probably `make` error'd out). rather than push Qready, you could push e.g. True in this case and False if the loop exits (e.g. in a finally block with an "if i didn't push anything yet" check).

##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -703,6 +709,21 @@ def write(self, data, timeout_sec):
             raise TransportClosedError()
         return self.fd_transport.write(data, timeout_sec)
 
+    def _qemu_check_stdout(self):
+        for line in self.proc.stdout:
+            line = str(line)
+            _LOG.debug(line)
+            if "[QEMU] CPU" in line:
+                self._queue.put(self._qemu_ready_msg)
+                return

Review comment:
       don't return here--doing so could freeze the subprocess. instead, continue to read the output so that the pipe is drained.

##########
File path: python/tvm/micro/contrib/zephyr.py
##########
@@ -630,6 +632,8 @@ def __init__(self, base_dir, startup_timeout_sec=5.0, qemu_debugger=None, **kwar
         self.fd_transport = None
         self.pipe_dir = None
         self.qemu_debugger = qemu_debugger
+        self._queue = queue.Queue()
+        self._qemu_ready_msg = "Qready"

Review comment:
       if you leave this in here, promote it to a constant and add a comment explaining its function




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org