Replace the queued communications with deferred delays.

The queued communications was, I believe, intended to ensure that
different parts of the system could issues the commands to the
display asynchronously and not have them trample on one another.
However, it was not effective with my flagship device, and almost
never produced an uncorrupted display.

The queue seems unnecessary, and complicates the logic of the
display communications. It has been removed, so that the operations
all happen when they are requested by clients, rather than being
delayed by an arbitrary amount of time by being queued. Instead,
all the operations have been protected by a mutex, similar to the
prior code, but surrounding the entire operation that must be
sequenced in order.

My own investigation implied that there needed to be a delay
between the bitmap data and the next command sent. It's not clear
why, but with this delay in place, the results were never corrupted.

Inserting the delay immediately after the bitmap data send would
work to allow the data to be sent uncorrupted, but it would force
there to always be a delay even when there would be time between
the bitmap and the next command. Instead the delay is deferred
until we try to send the next command - at which point we will
sleep. This means that the delay is amortised into subsequent
work.

The results are solid on my flagship model with a 0.02s delay. It
is possible that this is high, and it could be reduced. It is
possible that this does not work on model A devices, but I cannot
test this at this time. The necessary changes have been made to
the code, but I just have to hope they work.

Author: gerph

library/display.py

@@ -35,13 +35,11 @@ def __init__(self):
         if CONFIG_DATA["display"]["REVISION"] == "A":
             self.lcd = LcdCommRevA(com_port=CONFIG_DATA['config']['COM_PORT'],
                                    display_width=CONFIG_DATA["display"]["DISPLAY_WIDTH"],
-                                   display_height=CONFIG_DATA["display"]["DISPLAY_HEIGHT"],
-                                   update_queue=config.update_queue)
+                                   display_height=CONFIG_DATA["display"]["DISPLAY_HEIGHT"])
         elif CONFIG_DATA["display"]["REVISION"] == "B":
             self.lcd = LcdCommRevB(com_port=CONFIG_DATA['config']['COM_PORT'],
                                    display_width=CONFIG_DATA["display"]["DISPLAY_WIDTH"],
-                                   display_height=CONFIG_DATA["display"]["DISPLAY_HEIGHT"],
-                                   update_queue=config.update_queue)
+                                   display_height=CONFIG_DATA["display"]["DISPLAY_HEIGHT"])
         else:
             logger.error("Unknown display revision '", CONFIG_DATA["display"]["REVISION"], "'")
 

library/lcd_comm.py

@@ -17,8 +17,11 @@ class Orientation(IntEnum):
 
 
 class LcdComm(ABC):
-    def __init__(self, com_port: str = "AUTO", display_width: int = 320, display_height: int = 480,
-                 update_queue: queue.Queue = None):
+    # Amount of time that we should delay between sending bitmap data and sending the next
+    # command. If commands are issued too quickly after a bitmap, corruption is seen.
+    inter_bitmap_delay = 0.02
+
+    def __init__(self, com_port: str = "AUTO", display_width: int = 320, display_height: int = 480):
         self.lcd_serial = None
 
         # String containing absolute path to serial port e.g. "COM3", "/dev/ttyACM1" or "AUTO" for auto-discovery
@@ -31,13 +34,11 @@ def __init__(self, com_port: str = "AUTO", display_width: int = 320, display_hei
         # Display height in default orientation (portrait)
         self.display_height = display_height
 
-        # Queue containing the serial requests to send to the screen. An external thread should run to process requests
-        # on the queue. If you want serial requests to be done in sequence, set it to None
-        self.update_queue = update_queue
+        # Last time we sent bitmap data
+        self.last_bitmap_time = 0
 
-        # Mutex to protect the queue in case a thread want to add multiple requests (e.g. image data) that should not be
-        # mixed with other requests in-between
-        self.update_queue_mutex = threading.Lock()
+        # Mutex that must be held over any requests that must be kept together.
+        self.com_mutex = threading.Lock()
 
     def get_width(self) -> int:
         if self.orientation == Orientation.PORTRAIT or self.orientation == Orientation.REVERSE_PORTRAIT:

library/lcd_comm_rev_a.py

@@ -19,9 +19,8 @@ class Command(IntEnum):
 
 
 class LcdCommRevA(LcdComm):
-    def __init__(self, com_port: str = "AUTO", display_width: int = 320, display_height: int = 480,
-                 update_queue: queue.Queue = None):
-        LcdComm.__init__(self, com_port, display_width, display_height, update_queue)
+    def __init__(self, com_port: str = "AUTO", display_width: int = 320, display_height: int = 480):
+        super().__init__(com_port, display_width, display_height)
         self.openSerial()
 
     def __del__(self):
@@ -38,7 +37,13 @@ def auto_detect_com_port():
 
         return auto_com_port
 
-    def SendCommand(self, cmd: Command, x: int, y: int, ex: int, ey: int, bypass_queue: bool = False):
+    def SendCommand(self, cmd: Command, x: int, y: int, ex: int, ey: int):
+
+        # Commands must be sent at least 'inter_bitmap_delay' after the bitmap data.
+        delay = (self.last_bitmap_time + self.inter_bitmap_delay) - time.time()
+        if delay > 0:
+            time.sleep(delay)
+
         byteBuffer = bytearray(6)
         byteBuffer[0] = (x >> 2)
         byteBuffer[1] = (((x & 3) << 6) + (y >> 4))
@@ -48,35 +53,37 @@ def SendCommand(self, cmd: Command, x: int, y: int, ex: int, ey: int, bypass_que
         byteBuffer[5] = cmd
 
         # If no queue for async requests, or if asked explicitly to do the request sequentially: do request now
-        if not self.update_queue or bypass_queue:
-            self.WriteData(byteBuffer)
-        else:
-            # Lock queue mutex then queue the request
-            with self.update_queue_mutex:
-                self.update_queue.put((self.WriteData, [byteBuffer]))
+        self.WriteData(byteBuffer)
 
     def InitializeComm(self):
         # HW revision A does not need init commands
         pass
 
     def Reset(self):
         logger.info("Display reset (COM port may change)...")
-        # Reset command bypasses queue because it is run when queue threads are not yet started
-        self.SendCommand(Command.RESET, 0, 0, 0, 0, bypass_queue=True)
-        # Wait for display reset then reconnect
-        time.sleep(1)
-        self.openSerial()
+
+        with self.com_mutex:
+            self.SendCommand(Command.RESET, 0, 0, 0, 0)
+
+            # NOTE: Shouldn't we close serial before we try to open it again ?
+
+            # Wait for display reset then reconnect
+            time.sleep(1)
+            self.openSerial()
 
     def Clear(self):
         self.SetOrientation(Orientation.PORTRAIT)  # Bug: orientation needs to be PORTRAIT before clearing
-        self.SendCommand(Command.CLEAR, 0, 0, 0, 0)
+        with self.com_mutex:
+            self.SendCommand(Command.CLEAR, 0, 0, 0, 0)
         self.SetOrientation()  # Restore default orientation
 
     def ScreenOff(self):
-        self.SendCommand(Command.SCREEN_OFF, 0, 0, 0, 0)
+        with self.com_mutex:
+            self.SendCommand(Command.SCREEN_OFF, 0, 0, 0, 0)
 
     def ScreenOn(self):
-        self.SendCommand(Command.SCREEN_ON, 0, 0, 0, 0)
+        with self.com_mutex:
+            self.SendCommand(Command.SCREEN_ON, 0, 0, 0, 0)
 
     def SetBrightness(self, level: int = 25):
         assert 0 <= level <= 100, 'Brightness level must be [0-100]'
@@ -86,7 +93,8 @@ def SetBrightness(self, level: int = 25):
         level_absolute = int(255 - ((level / 100) * 255))
 
         # Level : 0 (brightest) - 255 (darkest)
-        self.SendCommand(Command.SET_BRIGHTNESS, level_absolute, 0, 0, 0)
+        with self.com_mutex:
+            self.SendCommand(Command.SET_BRIGHTNESS, level_absolute, 0, 0, 0)
 
     def SetBackplateLedColor(self, led_color: tuple[int, int, int] = (255, 255, 255)):
         logger.info("HW revision A does not support backplate LED color setting")
@@ -112,7 +120,8 @@ def SetOrientation(self, orientation: Orientation = Orientation.PORTRAIT):
         byteBuffer[8] = (width & 255)
         byteBuffer[9] = (height >> 8)
         byteBuffer[10] = (height & 255)
-        self.lcd_serial.write(bytes(byteBuffer))
+        with self.com_mutex:
+            self.lcd_serial.write(bytes(byteBuffer))
 
     def DisplayPILImage(
             self,
@@ -141,13 +150,12 @@ def DisplayPILImage(
         (x0, y0) = (x, y)
         (x1, y1) = (x + image_width - 1, y + image_height - 1)
 
-        self.SendCommand(Command.DISPLAY_BITMAP, x0, y0, x1, y1)
+        with self.com_mutex:
+            self.SendCommand(Command.DISPLAY_BITMAP, x0, y0, x1, y1)
 
-        pix = image.load()
-        line = bytes()
+            pix = image.load()
+            line = bytes()
 
-        # Lock queue mutex then queue all the requests for the image data
-        with self.update_queue_mutex:
             for h in range(image_height):
                 for w in range(image_width):
                     R = pix[w, h][0] >> 3
@@ -159,9 +167,13 @@ def DisplayPILImage(
 
                     # Send image data by multiple of DISPLAY_WIDTH bytes
                     if len(line) >= self.get_width() * 8:
-                        self.SendLine(line)
+                        self.WriteLine(line)
                         line = bytes()
 
             # Write last line if needed
             if len(line) > 0:
-                self.SendLine(line)
+                self.WriteLine(line)
+
+            # There must be a short period between the last write of the bitmap data and the next
+            # command. This seems to be around 0.02s on the flagship device.
+            self.last_bitmap_time = time.time()

library/lcd_comm_rev_b.py

@@ -1,4 +1,5 @@
 import struct
+import time
 
 from serial.tools.list_ports import comports
 
@@ -30,9 +31,8 @@ class SubRevision(IntEnum):
 
 
 class LcdCommRevB(LcdComm):
-    def __init__(self, com_port: str = "AUTO", display_width: int = 320, display_height: int = 480,
-                 update_queue: queue.Queue = None):
-        LcdComm.__init__(self, com_port, display_width, display_height, update_queue)
+    def __init__(self, com_port: str = "AUTO", display_width: int = 320, display_height: int = 480):
+        super().__init__(com_port, display_width, display_height)
         self.openSerial()
         self.sub_revision = SubRevision.A01  # Run a Hello command to detect correct sub-rev.
 
@@ -56,7 +56,13 @@ def auto_detect_com_port():
 
         return auto_com_port
 
-    def SendCommand(self, cmd: Command, payload=None, bypass_queue: bool = False):
+    def SendCommand(self, cmd: Command, payload=None):
+
+        # Commands must be sent at least 'inter_bitmap_delay' after the bitmap data.
+        delay = (self.last_bitmap_time + self.inter_bitmap_delay) - time.time()
+        if delay > 0:
+            time.sleep(delay)
+
         # New protocol (10 byte packets, framed with the command, 8 data bytes inside)
         if payload is None:
             payload = [0] * 8
@@ -75,20 +81,15 @@ def SendCommand(self, cmd: Command, payload=None, bypass_queue: bool = False):
         byteBuffer[8] = payload[7]
         byteBuffer[9] = cmd
 
-        # If no queue for async requests, or if asked explicitly to do the request sequentially: do request now
-        if not self.update_queue or bypass_queue:
-            self.WriteData(byteBuffer)
-        else:
-            # Lock queue mutex then queue the request
-            with self.update_queue_mutex:
-                self.update_queue.put((self.WriteData, [byteBuffer]))
+        self.WriteData(byteBuffer)
 
     def Hello(self):
         hello = [ord('H'), ord('E'), ord('L'), ord('L'), ord('O')]
 
         # This command reads LCD answer on serial link, so it bypasses the queue
-        self.SendCommand(Command.HELLO, payload=hello, bypass_queue=True)
-        response = self.lcd_serial.read(10)
+        with self.com_mutex:
+            self.SendCommand(Command.HELLO, payload=hello)
+            response = self.lcd_serial.read(10)
 
         if len(response) != 10:
             logger.warning("Device not recognised (short response to HELLO)")
@@ -138,29 +139,31 @@ def SetBrightness(self, level_user: int = 25):
 
         if self.is_brightness_range():
             # Brightness scales from 0 to 255, with 255 being the brightest and 0 being the darkest.
-            # Convert our brightness % to an absolute value.
             level = int((level_user / 100) * 255)
         else:
             # Brightness is 1 (off) or 0 (full brightness)
             logger.info("Your display does not support custom brightness level")
             level = 1 if level_user == 0 else 0
 
-        self.SendCommand(Command.SET_BRIGHTNESS, payload=[level])
+        with self.com_mutex:
+            self.SendCommand(Command.SET_BRIGHTNESS, payload=[level])
 
     def SetBackplateLedColor(self, led_color: tuple[int, int, int] = (255, 255, 255)):
         if self.is_flagship():
-            self.SendCommand(Command.SET_LIGHTING, payload=led_color)
+            with self.com_mutex:
+                self.SendCommand(Command.SET_LIGHTING, payload=led_color)
         else:
             logger.info("Only HW revision 'flagship' supports backplate LED color setting")
 
     def SetOrientation(self, orientation: Orientation = Orientation.PORTRAIT, new_width: int = 320, new_height: int = 480):
         # In revision B, basic orientations (portrait / landscape) are managed by the display
         # The reverse orientations (reverse portrait / reverse landscape) are software-managed
         self.orientation = orientation
-        if self.orientation == Orientation.PORTRAIT or self.orientation == Orientation.REVERSE_PORTRAIT:
-            self.SendCommand(Command.SET_ORIENTATION, payload=[OrientationValueRevB.ORIENTATION_PORTRAIT])
-        else:
-            self.SendCommand(Command.SET_ORIENTATION, payload=[OrientationValueRevB.ORIENTATION_LANDSCAPE])
+        with self.com_mutex:
+            if self.orientation == Orientation.PORTRAIT or self.orientation == Orientation.REVERSE_PORTRAIT:
+                self.SendCommand(Command.SET_ORIENTATION, payload=[OrientationValueRevB.ORIENTATION_PORTRAIT])
+            else:
+                self.SendCommand(Command.SET_ORIENTATION, payload=[OrientationValueRevB.ORIENTATION_LANDSCAPE])
 
     def DisplayPILImage(
             self,
@@ -193,16 +196,17 @@ def DisplayPILImage(
             (x0, y0) = (self.get_width() - x - image_width, self.get_height() - y - image_height)
             (x1, y1) = (self.get_width() - x - 1, self.get_height() - y - 1)
 
-        self.SendCommand(Command.DISPLAY_BITMAP,
-                         payload=[(x0 >> 8) & 255, x0 & 255,
-                                  (y0 >> 8) & 255, y0 & 255,
-                                  (x1 >> 8) & 255, x1 & 255,
-                                  (y1 >> 8) & 255, y1 & 255])
+        # Do the image load outside the mutex in case it takes a long time
         pix = image.load()
-        line = bytes()
 
-        # Lock queue mutex then queue all the requests for the image data
-        with self.update_queue_mutex:
+        with self.com_mutex:
+            self.SendCommand(Command.DISPLAY_BITMAP,
+                             payload=[(x0 >> 8) & 255, x0 & 255,
+                                      (y0 >> 8) & 255, y0 & 255,
+                                      (x1 >> 8) & 255, x1 & 255,
+                                      (y1 >> 8) & 255, y1 & 255])
+            line = bytes()
+
             for h in range(image_height):
                 for w in range(image_width):
                     if self.orientation == Orientation.PORTRAIT or self.orientation == Orientation.LANDSCAPE:
@@ -227,9 +231,13 @@ def DisplayPILImage(
 
                     # Send image data by multiple of DISPLAY_WIDTH bytes
                     if len(line) >= self.get_width() * 8:
-                        self.SendLine(line)
+                        self.WriteLine(line)
                         line = bytes()
 
             # Write last line if needed
             if len(line) > 0:
-                self.SendLine(line)
+                self.WriteLine(line)
+
+            # There must be a short period between the last write of the bitmap data and the next
+            # command. This seems to be around 0.02s on the flagship device.
+            self.last_bitmap_time = time.time()