Repository: ManiacalLabs/BiblioPixel (python)

Issue:
Better handling of DMX offsets
DMX is 1-based - BP is 0-based. 

Our current model is confusing, because we use an offset - that requires an extra +1 which confuses users.  If the lighting designer "sets their board to channel 100", then the offset is -99...

We could easily flip the sign.  But if we tweaked the code to fix this, we'd get the other unintuitive result that offset=0 actually moved everything 1 channel...

The solution involves adding a little functionality, which results in clearer names:

* channel_start (default 1)
* channel_end (default 512)
* pixel_offset (default 0)

This gives us a little extra functionality in specifying exactly the channels we want _and_ it's extremely clear how it works, and how offsets work, and how they interact with channels

I toyed with:

* step (default 1, can be a list)
* scale

but well... not entirely clear how those would work and not worth the extra effort

Pull request description:
New OffsetRange handles begin, end, offset ranges (fix #795)

Patch:
diff --git a/bibliopixel/control/artnet.py b/bibliopixel/control/artnet.py
index 164f48a..d277ac8 100644
--- a/bibliopixel/control/artnet.py
+++ b/bibliopixel/control/artnet.py
@@ -1,6 +1,6 @@
 import collections, copy, queue
 from . import control
-from .. util import artnet_message, log, server_cache, udp
+from .. util import artnet_message, log, offset_range, server_cache, udp
 
 QUEUE_TIMEOUT = 0.1
 
@@ -10,22 +10,14 @@ class ArtNet(control.ExtractedControl):
                  offset=0, **kwds):
         super().__init__(*args, **kwds)
         self.address = ip_address, port
-        self.offset = offset
+        self.offset = offset_range.DMXChannel.make(offset)
 
     def _convert(self, msg):
         msg = artnet_message.bytes_to_message(msg)
         if not msg:
             return
 
-        if self.offset == 0:
-            data = bytes(msg.data)
-        else:
-            pad = bytes(0 for i in range(abs(self.offset)))
-            if self.offset < 0:
-                data = bytes(msg.data[-self.offset:]) + pad
-            else:
-                data = pad + bytes(msg.data[:-self.offset])
-
+        data = bytes(self.offset.read_from(msg.data))
         msg = collections.OrderedDict((
             ('type', 'dmx'),
             ('net', msg.net),
diff --git a/bibliopixel/drivers/artnet.py b/bibliopixel/drivers/artnet.py
index d242c9d..705f03c 100644
--- a/bibliopixel/drivers/artnet.py
+++ b/bibliopixel/drivers/artnet.py
@@ -1,6 +1,6 @@
 import copy, ctypes, enum
 from . server_driver import ServerDriver
-from .. util import artnet_message, log, server_cache, udp
+from .. util import artnet_message, log, offset_range, server_cache, udp
 
 
 class ArtNet(ServerDriver):
@@ -15,7 +15,7 @@ class ArtNet(ServerDriver):
         """
         super().__init__(*args, address=(ip_address, port), **kwds)
         self.filter_dupes = filter_dupes
-        self.offset = offset
+        self.offset = offset_range.DMXChannel(offset)
         self.msg = artnet_message.dmx_message()
         self.last_message = None
 
@@ -27,18 +27,7 @@ class ArtNet(ServerDriver):
             self.last_message = msg
 
     def _copy_buffer_into_data(self):
-        buf, offset, data = self._buf, self.offset, self.msg.data
-        if offset < 0:
-            buf = buf[-offset:]
-            offset = 0
-
-        delta = len(data) - len(buf) - offset
-        if delta <= 0:
-            # data is shorter
-            data[:] = buf[offset:offset + len(data)]
-        else:
-            # data is longer
-            data[:len(buf) - offset] = buf[offset:]
+        self.offset.copy_to(self._buf, self.msg.data)
 
     def _on_positions(self):
         pass
diff --git a/bibliopixel/drivers/network_receiver.py b/bibliopixel/drivers/network_receiver.py
index 7505644..dd5b6be 100644
--- a/bibliopixel/drivers/network_receiver.py
+++ b/bibliopixel/drivers/network_receiver.py
@@ -1,14 +1,8 @@
-import threading
-import os
+import os, threading
 
-try:
-    import SocketServer
-except:
-    import socketserver as SocketServer
+import socketserver as SocketServer
 from .. drivers.return_codes import RETURN_CODES
 from . network import CMDTYPE
-
-os.sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__))))
 from .. util import log
 
 
diff --git a/bibliopixel/project/construct.py b/bibliopixel/project/construct.py
index 0a4e32f..a084488 100644
--- a/bibliopixel/project/construct.py
+++ b/bibliopixel/project/construct.py
@@ -17,7 +17,15 @@ def to_type(d):
     fn = load.load_if_filename(d)
     if fn:
         return fn
-    return {'typename': d} if isinstance(d, str) else copy.deepcopy(d)
+    if isinstance(d, str):
+        return {'typename': d}
+
+    result = copy.deepcopy(d)
+
+    # 'datatype' always overrides 'typename'
+    if 'datatype' in result:
+        result.pop('typename', None)
+    return result
 
 
 def to_type_constructor(value, python_path=None):
@@ -46,6 +54,7 @@ def to_type_constructor(value, python_path=None):
         try:
             value['datatype'] = importer.import_symbol(
                 r, python_path=python_path)
+            del value['typename']
         except Exception as e:
             value['_exception'] = e
 
diff --git a/bibliopixel/project/merge.py b/bibliopixel/project/merge.py
index dab9034..e367e9d 100644
--- a/bibliopixel/project/merge.py
+++ b/bibliopixel/project/merge.py
@@ -16,7 +16,7 @@ DEFAULT_PROJECT = collections.OrderedDict((
     ('shape', ()),
     ('layout', {}),
     ('run', {}),
-    ('animation', {}),
+    ('animation', {'typename': 'animation'}),
     ('controls', []),
 ))
 PROJECT_SECTIONS = tuple(DEFAULT_PROJECT.keys())
@@ -26,6 +26,7 @@ NOT_MERGEABLE = (
     'typename')
 
 SECTION_ISNT_DICT_ERROR = 'Project section "%s" is %s, should be dictionary'
+UNKNOWN_SECTION_ERROR = 'There is no Project section named "%s"'
 
 
 def merge(*projects):
@@ -40,6 +41,9 @@ def merge(*projects):
                 result[name] = section
                 continue
 
+            if name not in DEFAULT_PROJECT:
+                raise ValueError(UNKNOWN_SECTION_ERROR % name)
+
             if section and not isinstance(section, (dict, str)):
                 cname = section.__class__.__name__
                 raise ValueError(SECTION_ISNT_DICT_ERROR % (name, cname))
@@ -52,5 +56,7 @@ def merge(*projects):
                     section['run'] = adesc.get('run', {})
 
             result.setdefault(name, {}).update(construct.to_type(section))
+            if 'datatype' in result[name]:
+                result[name].pop('typename', None)
 
     return result
diff --git a/bibliopixel/project/project.py b/bibliopixel/project/project.py
index 6ec62b2..b3121cd 100644
--- a/bibliopixel/project/project.py
+++ b/bibliopixel/project/project.py
@@ -4,6 +4,7 @@ from . import (
 from .. util import exception, json, log
 
 EVENT_QUEUE_MAXSIZE = 1000
+NO_DATATYPE_ERROR = 'No "datatype" field in section "%s"'
 
 
 class Project:
@@ -13,7 +14,11 @@ class Project:
     def pre_recursion(desc):
         return cleanup.cleanup(desc)
 
-    def construct_child(self, datatype, typename=None, **kwds):
+    def construct_child(self, section_name, datatype=None, typename=None,
+                        **kwds):
+        if not datatype:
+            raise ValueError(NO_DATATYPE_ERROR % section_name)
+
         construct = getattr(datatype, 'construct', None)
         if construct:
             return construct(self, **kwds)
@@ -26,13 +31,13 @@ class Project:
         :param int event_queue_maxsize: maxsize parameter to queue.Queue.
             0 means an unbounded queue.
         """
-        def post(desc):
-            exception = desc.get('_exception')
-            if exception:
-                raise exception
-            return self.construct_child(**desc)
-
         def create(root, name):
+            def post(desc):
+                exception = desc.get('_exception')
+                if exception:
+                    raise exception
+                return self.construct_child(name, **desc)
+
             with exception.add('Unable to create ' + name):
                 return recurse.recurse(
                     root,
@@ -43,10 +48,10 @@ class Project:
         attributes.check(kwds, 'project')
         self.path = path
         layout = layout or cleanup.cleanup_layout(animation)
-        self.maker = self.construct_child(**maker)
+        self.maker = self.construct_child('maker', **maker)
         self.drivers = [create(d, 'drivers') for d in drivers]
         with exception.add('Unable to create layout'):
-            self.layout = self.construct_child(**layout)
+            self.layout = self.construct_child('layout', **layout)
 
         self.animation = create(animation, 'animation')
 
diff --git a/bibliopixel/util/offset_range.py b/bibliopixel/util/offset_range.py
new file mode 100644
index 0000000..9231c91
--- /dev/null
+++ b/bibliopixel/util/offset_range.py
@@ -0,0 +1,83 @@
+"""
+Handle DMX and MIDI channel offsets
+"""
+
+
+class OffsetRange:
+    def __init__(self, offset=0, begin=None, end=None):
+        """
+        Unlike a `range`, an OffsetRange includes both its begin *and* its end,
+        so it's closer to how regular people think of a range - for example
+        that DMX channels are in the range 1-512.
+        """
+
+        self.begin = self.BEGIN if begin is None else begin
+        self.end = self.END if end is None else end
+
+        if not (self.BEGIN <= self.begin <= self.end <= self.END):
+            raise ValueError('Bad range: FAILED %s <= %s <=  %s <= %s' %
+                             (self.BEGIN, self.begin, self.end, self.END))
+
+        self.offset = offset
+
+    def full_range(self):
+        return self.END - self.BEGIN + 1
+
+    def index(self, i, length=None):
+        """Return an integer index or None"""
+        if self.begin <= i <= self.end:
+            index = i - self.BEGIN - self.offset
+            if length is None:
+                length = self.full_range()
+            else:
+                length = min(length, self.full_range())
+
+            if 0 <= index < length:
+                return index
+
+    def read_from(self, data, pad=0):
+        """
+        Returns a generator with the elements "data" taken by offset, restricted
+        by self.begin and self.end, and padded on either end by `pad` to get
+        back to the original length of `data`
+        """
+        for i in range(self.BEGIN, self.END + 1):
+            index = self.index(i, len(data))
+            yield pad if index is None else data[index]
+
+    def copy_to(self, source, target):
+        if not target:
+            return
+
+        offset = self.offset
+        if offset < 0:
+            source = source[-offset:]
+            offset = 0
+
+        begin = min(self.begin - self.BEGIN, len(target) - 1)
+        end = min(self.end - self.BEGIN, len(target) - 1)
+        length = min(len(source) - offset, end - begin + 1)
+
+        target[begin:begin + length] = source[offset:offset + length]
+
+    @classmethod
+    def make(cls, x=0):
+        try:
+            return cls(**x)
+        except TypeError:
+            return cls(offset=x)
+
+
+class MidiChannel(OffsetRange):
+    BEGIN = 1
+    END = 16
+
+
+class MidiNote(OffsetRange):
+    BEGIN = 0
+    END = 127
+
+
+class DMXChannel(OffsetRange):
+    BEGIN = 1
+    END = 512


Test patch:
diff --git a/test/bibliopixel/project/bad_project_test.py b/test/bibliopixel/project/bad_project_test.py
index b70e6a4..6cefa8b 100644
--- a/test/bibliopixel/project/bad_project_test.py
+++ b/test/bibliopixel/project/bad_project_test.py
@@ -6,10 +6,8 @@ from . make import make
 
 PYTHON_FILE = 'driver = {"a": "b"}'
 MISSING_LAYOUT = '{"animation": "off", "driver": "dummy"}'
-MISSING_ANIMATION = '{"layout": "matrix", "driver": "dummy"}'
 
 
-# This one works, but prints a warning.
 BAD_SECTION = """
 {
     "bad_section": {"foo": true},
@@ -19,13 +17,33 @@ BAD_SECTION = """
 }
 """
 
+BAD_SECTION2 = """
+{
+    "verbose": true,
+    "driver": "dummy",
+    "layout": "matrix",
+    "animation": "off"
+}
+"""
+
+MISSING_DATATYPE = """
+{
+    "driver": {
+         "width": 16,
+         "height": 16
+    },
+    "layout": "matrix",
+    "animation": "off"
+}
+"""
+
 BAD_DRIVER_ATTRIBUTE = """
 {
     "driver": {
          "typename": "simpixel",
          "width": 16,
          "height": 16,
-        "bad_attribute": 16
+         "bad_attribute": 16
     },
     "layout": "matrix",
     "animation": "off"
@@ -80,7 +98,24 @@ class BadProjectTest(unittest.TestCase):
 
         self.assertEquals(
             e.exception.args,
-            ('Unknown attribute for project: "bad_section"',))
+            ('There is no Project section named "bad_section"',))
+
+    def test_bad_section2(self):
+        # See https://github.com/ManiacalLabs/BiblioPixel/issues/787
+        with self.assertRaises(ValueError) as e:
+            make(BAD_SECTION2)
+
+        self.assertEquals(
+            e.exception.args,
+            ('There is no Project section named "verbose"',))
+
+    def test_missing_datatype(self):
+        with self.assertRaises(ValueError) as e:
+            make(MISSING_DATATYPE)
+        self.assertEquals(
+            e.exception.args,
+            ('Unable to create drivers',
+             'No "datatype" field in section "drivers"'))
 
     def test_bad_driver_attribute(self):
         with self.assertRaises(ValueError) as e:
@@ -117,16 +152,9 @@ class BadProjectTest(unittest.TestCase):
             e.exception.args,
             ('Missing "layout" section',))
 
-    def test_missing_animation(self):
-        with self.assertRaises(ValueError) as e:
-            make(MISSING_ANIMATION)
-        self.assertEquals(
-            e.exception.args,
-            ('Missing "animation" section',))
-
     def test_missing_everything(self):
         with self.assertRaises(ValueError) as e:
             make('{}')
         self.assertEquals(
             e.exception.args,
-            ('Missing "animation" section',))
+            ('Missing "layout" section',))
diff --git a/test/bibliopixel/project/cleanup_test.py b/test/bibliopixel/project/cleanup_test.py
index d529d10..9b3cdab 100644
--- a/test/bibliopixel/project/cleanup_test.py
+++ b/test/bibliopixel/project/cleanup_test.py
@@ -10,7 +10,6 @@ BASE = {
         'run': {},
         'datatype': OffAnim,
         'name': 'Off',
-        'typename': 'off',
     },
     'controls': [],
     'drivers': [{'typename': 'simpixel'}],
diff --git a/test/bibliopixel/project/recurse_test.py b/test/bibliopixel/project/recurse_test.py
index 8d0d992..cda5031 100644
--- a/test/bibliopixel/project/recurse_test.py
+++ b/test/bibliopixel/project/recurse_test.py
@@ -35,7 +35,6 @@ class RecurseTest(unittest.TestCase):
             'bing': 'bang',
         }
         expected = {
-            'typename': '%s.%s' % (Tester.__module__, Tester.__name__),
             'datatype': Tester,
             'foo': {'datatype': RecurseTest, 'foo': 'bar', 'color': colors.Red},
             'bing': 'post-bang',
diff --git a/test/bibliopixel/util/offset_range_test.py b/test/bibliopixel/util/offset_range_test.py
new file mode 100644
index 0000000..887b2f6
--- /dev/null
+++ b/test/bibliopixel/util/offset_range_test.py
@@ -0,0 +1,114 @@
+import unittest
+from bibliopixel.util import offset_range
+
+
+class OffsetRangeTest(unittest.TestCase):
+    def test_empty(self):
+        dmx = offset_range.DMXChannel.make()
+
+        self.assertEqual(dmx.index(0), None)
+        self.assertEqual(dmx.index(1), 0)
+        self.assertEqual(dmx.index(2), 1)
+        self.assertEqual(dmx.index(511), 510)
+        self.assertEqual(dmx.index(512), 511)
+        self.assertEqual(dmx.index(513), None)
+
+        l256 = list(range(256))
+        r = list(dmx.read_from(l256))
+        self.assertEqual(r, l256 + ([0] * 256))
+
+        target = [23] * 128
+        dmx.copy_to(l256, target)
+        self.assertEqual(target, list(range(128)))
+
+    def test_positive_offset(self):
+        midi = offset_range.MidiChannel(offset=4)
+        self.assertEqual(midi.index(0), None)
+        self.assertEqual(midi.index(1), None)
+        self.assertEqual(midi.index(4), None)
+        self.assertEqual(midi.index(5), 0)
+        self.assertEqual(midi.index(6), 1)
+        self.assertEqual(midi.index(15), 10)
+        self.assertEqual(midi.index(16), 11)
+        self.assertEqual(midi.index(16), 11)
+        self.assertEqual(midi.index(17), None)
+
+        expected = [-1, -1, -1, -1] + list(range(12))
+        actual = list(midi.read_from(range(16), pad=-1))
+        self.assertEqual(expected, actual)
+
+        target = [100] * 100
+        midi.copy_to(list(range(16)), target)
+        expected = list(range(4, 16)) + [100] * 88
+        self.assertEqual(target, expected)
+
+    def test_negative_offset(self):
+        midi = offset_range.MidiChannel(-4)
+        self.assertEqual(midi.index(0), None)
+        self.assertEqual(midi.index(1), 4)
+        self.assertEqual(midi.index(2), 5)
+        self.assertEqual(midi.index(12), 15)
+        self.assertEqual(midi.index(13), None)
+
+        actual = list(midi.read_from(range(16), pad=-1))
+        expected = list(range(4, 16)) + [-1, -1, -1, -1]
+        self.assertEqual(expected, actual)
+
+        target = [100] * 8
+        midi.copy_to(list(range(16)), target)
+
+        expected = [4, 5, 6, 7, 8, 9, 10, 11]
+        self.assertEqual(target, expected)
+
+    def test_begin_end_offset(self):
+        midi = offset_range.MidiChannel(offset=-5, begin=6, end=8)
+        self.assertEqual(midi.index(0), None)
+        self.assertEqual(midi.index(4), None)
+        self.assertEqual(midi.index(5), None)
+        self.assertEqual(midi.index(6), 10)
+        self.assertEqual(midi.index(7), 11)
+        self.assertEqual(midi.index(8), 12)
+        self.assertEqual(midi.index(9), None)
+        self.assertEqual(midi.index(10), None)
+
+        actual = list(midi.read_from(range(16)))
+        expected = [0, 0, 0, 0, 0, 10, 11, 12, 0, 0, 0, 0, 0, 0, 0, 0]
+
+        self.assertEqual(expected, actual)
+
+        target = [100] * 24
+        midi.copy_to(list(range(7)), target)
+        expected = 5 * [100] + [5, 6] + 17 * [100]
+        self.assertEqual(target, expected)
+
+        target = [100] * 24
+        midi.copy_to(list(range(8)), target)
+        expected = 5 * [100] + [5, 6, 7] + 16 * [100]
+        self.assertEqual(target, expected)
+
+        target = [100] * 24
+        midi.copy_to(list(range(9)), target)
+        expected = 5 * [100] + [5, 6, 7] + 16 * [100]
+        self.assertEqual(target, expected)
+
+    def test_errors(self):
+        with self.assertRaises(ValueError):
+            offset_range.MidiChannel(begin=0)
+
+        offset_range.MidiChannel(begin=1)
+        offset_range.MidiChannel(begin=16)
+
+        with self.assertRaises(ValueError):
+            offset_range.MidiChannel(begin=17)
+
+        with self.assertRaises(ValueError):
+            offset_range.MidiChannel(end=0)
+
+        offset_range.MidiChannel(end=1)
+        offset_range.MidiChannel(end=16)
+
+        with self.assertRaises(ValueError):
+            offset_range.MidiChannel(end=17)
+
+        with self.assertRaises(ValueError):
+            offset_range.MidiChannel(begin=2, end=1)