Repository: ManiacalLabs/BiblioPixel
Pull request #103 (2017-02-13 17:16:22)

First half of "extracting threading".
So this is getting pretty long, and it's all refactorings, so let me get it out now and I can keep going on top of it.

This area in the code is delicate and I don't feel my battery of tests adequately tests it so I made a lot of very small, obvious moves.  Probably best to read one commit at a time as I suspect that each one is very uncontroversial.

The plan is to get all the code regarding threading into one file and I'm about two-thirds of the way there.  I tried hard to avoid anything other than that, but I tweaked the logic in one place to make that refactoring easier, and I found a tiny bug I fixed in passing.



Patch:
diff --git a/bibliopixel/animation/base.py b/bibliopixel/animation/base.py
index 6b6b01e..1dda839 100644
--- a/bibliopixel/animation/base.py
+++ b/bibliopixel/animation/base.py
@@ -6,15 +6,14 @@ class BaseAnimation(object):
 
     def __init__(self, led):
         self._led = led
+        self.thread_strategy = led.thread_strategy
+        self.thread_strategy.set_animation(self)
+
         self.animComplete = False
         self._step = 0
         self._timeRef = 0
         self._internalDelay = None
-        self._thread = None
         self._callback = None
-        self._stopEvent = threading.Event()
-        self._stopEvent.clear()
-        self._led._threadedAnim = False
         self._free_run = False
 
     def _msTime(self):
@@ -26,25 +25,15 @@ class BaseAnimation(object):
     def step(self, amt=1):
         raise RuntimeError("Base class step() called. This shouldn't happen")
 
-    def stopThread(self, wait=False):
-        if self._thread:
-            self._stopEvent.set()
-
-            if wait:
-                self._thread.join()
-
     def cleanup(self):
-        self.stopThread(wait=True)
+        self.thread_strategy.stop_animation_thread(wait=True)
         self._led.all_off()
         self._led.push_to_driver()
-        self._led.waitForUpdate()
-
-    def stopped(self):
-        return not (self._thread and self._thread.isAlive())
+        self.thread_strategy.wait_for_update()
 
     def _run(self, amt, fps, sleep, max_steps, until_complete,
              max_cycles, seconds):
-        self.preRun()
+        self.preRun(amt)
 
         # calculate sleep time base on desired Frames per Second
         if fps:
@@ -60,7 +49,7 @@ class BaseAnimation(object):
         cycle_count = 0
         self.animComplete = False
 
-        while (not self._stopEvent.isSet() and
+        while (not self.thread_strategy.animation_stop_event.isSet() and
                ((max_steps == 0 and not until_complete) or
                 (max_steps > 0 and cur_step < max_steps) or
                 (max_steps == 0 and until_complete and not self.animComplete))):
@@ -91,21 +80,7 @@ class BaseAnimation(object):
                     cycle_count += 1
                     self.animComplete = False
 
-            stepTime = int(mid - start)
-            if self._led._threadedUpdate:
-                updateTime = int(self._led.lastThreadedUpdate())
-                totalTime = updateTime
-            else:
-                updateTime = int(now - mid)
-                totalTime = stepTime + updateTime
-
-            if self._led._threadedUpdate:
-                log.debug(
-                    "Frame: %sms / Update Max: %sms", stepTime, updateTime)
-            else:
-                log.debug("%sms/%sfps / Frame: %sms / Update: %sms",
-                          totalTime, int(1000 / max(totalTime, 1)), stepTime,
-                          updateTime)
+            self.thread_strategy.report_framerate(start, mid, now)
 
             if sleep:
                 diff = (self._msTime() - self._timeRef)
@@ -113,10 +88,7 @@ class BaseAnimation(object):
                 if t == 0:
                     log.warning(
                         "Frame-time of %dms set, but took %dms!", sleep, diff)
-                if self._led._threadedAnim:
-                    self._stopEvent.wait(t)
-                else:
-                    time.sleep(t)
+                self.thread_strategy.animation_wait(t)
             cur_step += 1
 
         self._callback and self._callback(self)
@@ -125,12 +97,6 @@ class BaseAnimation(object):
             max_cycles=0, threaded=False, joinThread=False, callback=None,
             seconds=None):
 
-        self._callback = callback
-        self._led._threadedAnim = threaded
-        if self._led._threadedAnim:
-            self._stopEvent.clear()
-        self._callback = callback
-
         def run():
             try:
                 self._run(amt, fps, sleep, max_steps, until_complete,
@@ -138,19 +104,8 @@ class BaseAnimation(object):
             finally:
                 self.cleanup()
 
-        if self._led._threadedAnim:
-            def target():
-                # TODO: no testpath exercises this code...
-                log.debug('Starting thread...')
-                run()
-                log.debug('Thread Complete')
-
-            self._thread = threading.Thread(target=target, daemon=True)
-            self._thread.start()
-            if joinThread:
-                self._thread.join()
-        else:
-            run()
+        self._callback = callback
+        self.thread_strategy.run_animation(run, threaded, joinThread)
 
     RUN_PARAMS = [{
         "id": "amt",
diff --git a/bibliopixel/animation/queue.py b/bibliopixel/animation/queue.py
index cee8d68..f2709ac 100644
--- a/bibliopixel/animation/queue.py
+++ b/bibliopixel/animation/queue.py
@@ -16,8 +16,8 @@ class AnimationQueue(BaseAnimation):
     def stopThread(self, wait=False):
         for a, r in self.anims:
             # a bit of a hack. they aren't threaded, but stops them anyway
-            a._stopEvent.set()
-        super().stopThread(wait)
+            a.thread_strategy.animation_stop_event.set()
+        self.thread_strategy.stop_animation_thread(wait)
 
     def addAnim(self, anim, amt=1, fps=None, max_steps=0, until_complete=False,
                 max_cycles=0, seconds=None):
diff --git a/bibliopixel/led/base.py b/bibliopixel/led/base.py
index a62eb37..19ab025 100644
--- a/bibliopixel/led/base.py
+++ b/bibliopixel/led/base.py
@@ -1,6 +1,6 @@
 import time
-
 from .. import colors, data_maker, util
+from .. threads.thread_strategy import ThreadStrategy
 
 
 class LEDBase(object):
@@ -27,16 +27,7 @@ class LEDBase(object):
         self._frameGenTime = 0
         self._frameTotalTime = None
 
-        self._threadedUpdate = threadedUpdate
-
-        if self._threadedUpdate:
-            self._updateThread = util.UpdateThread(self.drivers)
-            self._updateThread.start()
-
-        self._waitingBrightness = False
-        self._waitingBrightnessValue = None
-        self._threadedAnim = False
-
+        self.thread_strategy = ThreadStrategy(threadedUpdate, self)
         self.set_brightness(masterBrightness)
 
     def set_layout(self, layout):
@@ -67,27 +58,10 @@ class LEDBase(object):
             result.append([x, 0, 0])
         return result
 
-    def waitForUpdate(self):
-        if self._threadedUpdate:
-            while all([d._thread.sending() for d in self.drivers]):
-                time.sleep(0.000001)
-
-    def doBrightness(self):
-        if self._waitingBrightness:
-            self.do_set_brightness(self._waitingBrightnessValue)
-
     def push_to_driver(self):
         """Push the current pixel state to the driver"""
-        self.waitForUpdate()
-        self.doBrightness()
-
-        if self._threadedUpdate:
-            self._updateThread.update_colors()
-        else:
-            for d in self.drivers:
-                d.update_colors()
-            for d in self.drivers:
-                d.sync()
+        # This is overridden elsewhere.
+        self.thread_strategy.push_to_driver()
 
     def lastThreadedUpdate(self):
         return max([d.lastUpdate for d in self.drivers])
@@ -111,7 +85,7 @@ class LEDBase(object):
 
     # Set the master brightness for the LEDs 0 - 255
     def _do_set_brightness(self, bright):
-        self.waitForUpdate()
+        self.thread_strategy.wait_for_update()
         result = True
         for d in self.drivers:
             if not d.set_brightness(bright):
@@ -126,22 +100,8 @@ class LEDBase(object):
         else:
             self.masterBrightness = 255
 
-        self._waitingBrightnessValue = None
-        self._waitingBrightness = False
-
     def set_brightness(self, bright):
-        """Sets the master brightness scaling, 0 - 255
-
-        If the driver supports it the brightness will be sent to the receiver directly.
-        """
-        if bright > 255 or bright < 0:
-            raise ValueError('Brightness must be between 0 and 255')
-
-        if self._threadedAnim:
-            self._waitingBrightnessValue = bright
-            self._waitingBrightness = True
-        else:
-            self._do_set_brightness(bright)
+        self.thread_strategy.set_brightness(bright)
 
     # Set single pixel to RGB value
     def setRGB(self, pixel, r, g, b):
diff --git a/bibliopixel/threads/__init__.py b/bibliopixel/threads/__init__.py
new file mode 100644
index 0000000..e69de29
diff --git a/bibliopixel/util/compose_events.py b/bibliopixel/threads/compose_events.py
similarity index 100%
rename from bibliopixel/util/compose_events.py
rename to bibliopixel/threads/compose_events.py
diff --git a/bibliopixel/util/producer_consumer.py b/bibliopixel/threads/producer_consumer.py
similarity index 100%
rename from bibliopixel/util/producer_consumer.py
rename to bibliopixel/threads/producer_consumer.py
diff --git a/bibliopixel/util/task_thread.py b/bibliopixel/threads/task_thread.py
similarity index 100%
rename from bibliopixel/util/task_thread.py
rename to bibliopixel/threads/task_thread.py
diff --git a/bibliopixel/threads/thread_strategy.py b/bibliopixel/threads/thread_strategy.py
new file mode 100644
index 0000000..106c6c5
--- /dev/null
+++ b/bibliopixel/threads/thread_strategy.py
@@ -0,0 +1,105 @@
+import threading, time
+
+from .. import log
+from . import update_thread
+
+
+class ThreadStrategy(object):
+    def __init__(self, enabled, led):
+        self.enabled = enabled
+        self.led = led
+        self.use_animation_thread = False
+        self.animation_thread = None
+        self.animation_stop_event = threading.Event()
+
+        self.waiting_brightness = None
+
+        if enabled:
+            self.update_thread = update_thread.UpdateThread(led.drivers)
+            self.update_thread.start()
+
+    def update_colors(self):
+        if self.enabled:
+            self.update_thread.update_colors()
+        else:
+            for d in self.led.drivers:
+                d.update_colors()
+            for d in self.led.drivers:
+                d.sync()
+
+    def wait_for_update(self):
+        if self.enabled:
+            while all([d._thread.sending() for d in self.led.drivers]):
+                time.sleep(0.000001)
+
+    def set_brightness(self, brightness):
+        """Sets the master brightness scaling, 0 - 255
+
+        If the driver supports it the brightness will be sent to the receiver
+        directly.
+        """
+        if brightness > 255 or brightness < 0:
+            raise ValueError('Brightness must be between 0 and 255')
+
+        if self.use_animation_thread:
+            self.waiting_brightness = brightness
+        else:
+            self.led._do_set_brightness(brightness)
+
+    def push_to_driver(self):
+        """Push the current pixel state to the driver"""
+        self.wait_for_update()
+        if self.waiting_brightness is not None:
+            self.led.do_set_brightness(self.waiting_brightness)
+            self.waiting_brightness = None
+        self.update_colors()
+
+    def set_animation(self, animation):
+        self.animation = animation
+
+    def report_framerate(self, start, mid, now):
+        stepTime = int(mid - start)
+        if self.enabled:
+            updateTime = int(self.led.lastThreadedUpdate())
+            log.debug(
+                "Frame: %sms / Update Max: %sms", stepTime, updateTime)
+        else:
+            updateTime = int(now - mid)
+            totalTime = stepTime + updateTime
+            log.debug("%sms/%sfps / Frame: %sms / Update: %sms",
+                      totalTime, int(1000 / max(totalTime, 1)), stepTime,
+                      updateTime)
+
+    def stop_animation_thread(self, wait=False):
+        if self.animation_thread:
+            self.animation_stop_event.set()
+
+            if wait:
+                self.animation_thread.join()
+
+    def animation_stopped(self):
+        return not (self.animation_thread and self.animation_thread.isAlive())
+
+    def animation_wait(self, t):
+        if self.use_animation_thread:
+            self.animation_stop_event.wait(t)
+        else:
+            time.sleep(t)
+
+    def run_animation(self, run, threaded, join_thread):
+        self.use_animation_thread = threaded
+        if self.use_animation_thread:
+            self.animation_stop_event.clear()
+
+            def target():
+                # TODO: no testpath exercises this code...
+                log.debug('Starting thread...')
+                run()
+                log.debug('Thread Complete')
+
+            self.animation_thread = threading.Thread(target=target, daemon=True)
+            self.animation_thread.start()
+            if join_thread:
+                self.animation_thread.join()
+        else:
+            run()
diff --git a/bibliopixel/util/threads.py b/bibliopixel/threads/threads.py
similarity index 100%
rename from bibliopixel/util/threads.py
rename to bibliopixel/threads/threads.py
diff --git a/bibliopixel/util/update_thread.py b/bibliopixel/threads/update_thread.py
similarity index 100%
rename from bibliopixel/util/update_thread.py
rename to bibliopixel/threads/update_thread.py
diff --git a/bibliopixel/util/__init__.py b/bibliopixel/util/__init__.py
index aa7a71e..9fe23d5 100644
--- a/bibliopixel/util/__init__.py
+++ b/bibliopixel/util/__init__.py
@@ -1,5 +1,4 @@
 from . util import generate_header, even_dist, pointOnCircle, genVector
-from . update_thread import UpdateThread
 from . attribute_dict import AttributeDict
 
 d = AttributeDict


Test patch:
diff --git a/test/compose_events_test.py b/test/compose_events_test.py
index 8ca6751..8c64659 100644
--- a/test/compose_events_test.py
+++ b/test/compose_events_test.py
@@ -1,6 +1,6 @@
 import threading, unittest
 
-from bibliopixel.util import compose_events
+from bibliopixel.threads import compose_events
 
 
 class ComposeEventTest(unittest.TestCase):
diff --git a/test/producer_consumer_test.py b/test/producer_consumer_test.py
index 564456e..0741e26 100644
--- a/test/producer_consumer_test.py
+++ b/test/producer_consumer_test.py
@@ -1,6 +1,6 @@
 import random, threading, time, unittest
 
-from bibliopixel.util import producer_consumer
+from bibliopixel.threads import producer_consumer
 
 
 class ProducerConsumerTest(unittest.TestCase):