Repository: ManiacalLabs/BiblioPixel (python)

Issue:
MIDI note on/off processing is incomplete
MIDI has two ways to represent a note-off: as a note-off command, or as a note-on command with velocity zero.

1. Very often, you do not want to trigger on note-ons if they have velocity zero.  
2. Less often, you want to remap "note off" commands to "note on with velocity zero" commands.

The code handles 2.  We should handle 1.

Added this as a 4.0 issue because it's a gross bug if you use MIDI notes, and the fix is tiny.

Pull request:
Better handling of MIDI note-offs (fix #827)
Another tiny bugfix...

Patch:
diff --git a/bibliopixel/control/address.py b/bibliopixel/control/address.py
index afa21bf..ed8051e 100644
--- a/bibliopixel/control/address.py
+++ b/bibliopixel/control/address.py
@@ -78,6 +78,8 @@ class Address:
             return
 
         self.name, *assignment = name.split('=', 1)
+        assignment = assignment and assignment[0].strip()
+
         self.name = self.name.strip()
 
         try:
@@ -88,16 +90,25 @@ class Address:
         if not self.segments:
             raise ValueError('Empty Addresses are not allowed')
 
-        if assignment:
-            if not self.segments:
-                raise ValueError('Cannot assign to an empty address')
-            if isinstance(self.segments[-1], Address.Call):
-                raise ValueError('Cannot assign to a call operation')
-            assignment = [s.strip() for s in assignment[0].split(',')]
-            assignment = [int(s) if s.isnumeric() else s for s in assignment]
-            self.assignment = tuple(assignment)
-        else:
+        if not assignment:
             self.assignment = ()
+            return
+
+        if isinstance(self.segments[-1], Address.Call):
+            raise ValueError('Cannot assign to a call operation')
+
+        def number(s):
+            try:
+                return int(s)
+            except ValueError:
+                pass
+            try:
+                return float(s)
+            except ValueError:
+                pass
+            return s
+
+        self.assignment = tuple(number(s) for s in assignment.split(','))
 
     def __bool__(self):
         return bool(self.segments)
diff --git a/bibliopixel/control/midi.py b/bibliopixel/control/midi.py
index bd7dc10..b37c857 100644
--- a/bibliopixel/control/midi.py
+++ b/bibliopixel/control/midi.py
@@ -50,7 +50,13 @@ class Midi(control.ExtractedControl, control.ControlLoop):
         'omit': ('port', 'channel'),
     }
 
-    def __init__(self, use_note_off=False, **kwds):
+    def __init__(self, use_note_off=True, **kwds):
+        """
+        :param use_note_off:
+            If False, map note_offs to note_ons with velocity 0
+            If True, map note_ons with velocity 0 to note_offs
+            If None, do not change none_ons or note_offs
+        """
         control.ExtractedControl.__init__(self, **kwds)
         self.use_note_off = use_note_off
 
@@ -77,7 +83,12 @@ class Midi(control.ExtractedControl, control.ControlLoop):
                  '' if len(ports) == 1 else 's', port_names)
         for port, msg in mido.ports.MultiPort(ports, yield_ports=True):
             mdict = dict(vars(msg), port=port)
-            if self.use_note_off or msg.type != 'note_off':
-                yield mdict
-            else:
-                yield dict(mdict, type='note_on', velocity=0)
+
+            if self.use_note_off:
+                if msg.type == 'note_on' and not msg.velocity:
+                    mdict.update(type='note_off')
+            elif self.use_note_off is False:
+                if msg.type == 'note_off':
+                    mdict.update(type='note_on', velocity=0)
+
+            yield mdict


Test patch:
diff --git a/test/bibliopixel/control/address_test.py b/test/bibliopixel/control/address_test.py
index ddba904..c323b48 100644
--- a/test/bibliopixel/control/address_test.py
+++ b/test/bibliopixel/control/address_test.py
@@ -108,3 +108,7 @@ class AddressTest(unittest.TestCase):
         self.attr = None
         Address('.attr = 1').set(self)
         self.assertEqual(self.attr, 1)
+
+        self.attr = None
+        Address('.attr = 1, 2.5, 3').set(self)
+        self.assertEqual(self.attr, (1, 2.5, 3))
diff --git a/test/bibliopixel/control/midi_test.py b/test/bibliopixel/control/midi_test.py
index 40d5e12..981c58f 100644
--- a/test/bibliopixel/control/midi_test.py
+++ b/test/bibliopixel/control/midi_test.py
@@ -4,6 +4,7 @@ from argparse import Namespace
 from test.bibliopixel import patch
 
 C3 = Namespace(type='note_on', note=32, channel=1, velocity=96)
+C3_ZERO = Namespace(type='note_on', note=32, channel=1, velocity=0)
 C3_OFF = Namespace(type='note_off', note=32, channel=1, velocity=0, x=47)
 BC = Namespace(type='control_change', channel=2, control=2, value=10)
 BC3 = Namespace(type='control_change', channel=3, control=2, value=127)
@@ -36,7 +37,8 @@ class FakeMido:
 
 class MidiTest(unittest.TestCase):
     routing = {
-        'note_on': '.note',
+        'note_on': '.note_on',
+        'note_off': '.note_off',
         'control_change': {
             '1': '.cc1',
             '2': '.cc2',
@@ -58,14 +60,26 @@ class MidiTest(unittest.TestCase):
             self.assertEqual(vars(root), expected)
 
     def test_one(self):
-        expected = {'note': (32, fractions.Fraction(96, 127))}
+        expected = {'note_on': (32, fractions.Fraction(96, 127))}
         self.run_test([C3], expected)
 
     def test_accept(self):
         expected = {
             'cc1': 1,
             'cc2': 1,
-            'note': (32, 0),
+            'note_off': (32, 0),
+            'note_on': (32, fractions.Fraction(96, 127)),
             'pitch': fractions.Fraction(-7, 8),
         }
         self.run_test([C3, C3_OFF, BC, BC3, MOD, PB, OTHER], expected)
+
+    def test_use_note_on(self):
+        expected_on = {'note_on': (32, 0)}
+        expected_off = {'note_off': (32, 0)}
+        self.run_test([C3_ZERO], expected_on, use_note_off=False)
+        self.run_test([C3_ZERO], expected_on, use_note_off=None)
+        self.run_test([C3_ZERO], expected_off, use_note_off=True)
+
+        self.run_test([C3_OFF], expected_on, use_note_off=False)
+        self.run_test([C3_OFF], expected_off, use_note_off=None)
+        self.run_test([C3_OFF], expected_off, use_note_off=True)