Implementation of a parser for MIDI events (messages) and alternative send interface #9
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 30 commits
February 27, 2019 01:09
…idi_in and buffering up that data in an internal buffer. Using a new MIDIMessage object with children which represent each MIDI event type as suggested in ticket. Send code left as is for now - must preserve interface as it is in use. adafruit#3
…ow about. Channel filtering NOT YET implemented. adafruit#3
…some variable pauses thrown in to check buffering behaviour. adafruit#3
…on of messages in any order. Adding TimingClock, PolyphonicKeyPressure, ProgramChange and ChannelPressure messages. Removing some unnecessary methods in MIDIUnknownEvent. adafruit#3
… a tuple of channels. Changing constructor to use setters to gain benefit of type/range checks. adafruit#3
…med superfluous but interface needs to be consistent. adafruit#3
… not the same thing. Adding Start and Stop messages. Punting the from_bytes for no data MIDI messages into parent and removing constructors for those as the default constructor does the job. adafruit#3
… or tuple with multiple channels. read_in_port() now returns the message and actual channel (or None). Added SystemExclusive message but implementation of variable length messages is still incomplete. adafruit#3
…w counted by MIDI object. Making buffer append in read_in_port() dependent on receiving some data plus adding a debug print of data. adafruit#3
…er (full) names in messages. adafruit#3
…ssage.from_message_bytes(). adafruit#3
Removing print statement used for temporary debug. adafruit#3
…y in anticipation of splitting out the MIDIMessage classes. adafruit#3
…r to import only what is needed. Adjusting unit tests to match new scheme. adafruit#3
Each MIDIMessage has to supply an as_bytes to support sending - this currently intentionally returns a bytearray for mutability but this is worth reviewing. Test cases for MIDI.send in new file MIDIMessage_unittests.py adafruit#3
…rtialandpreotherchannel1 and test_NoteOn_partialandpreotherchannel2. Fixing test_NoteOn_partialandpreotherchannel with some tweaks to logic in complex MIDIMessage.from_message_bytes(). adafruit#3
…and parse past SysEx larger than input buffer. Re-arranged some of the unit tests for SystemExclusive and added a mal-terminated test. Threw in an empty buffer test too. Adjusted SystemExclusive to handle terminating status byte being passed in databytes. adafruit#3
…urrent behaviour of MIDIMessage.from_message_bytes() which is reasonable here. adafruit#3
…structors to take strings like "C#4" with some unit tests. Adding range checks to data for NoteOn and Note Off with unit tests. adafruit#3
…s passing into MIDI.send(). adafruit#3
…tart and Stop which inherit a new default dataless one from MIDIMessage. adafruit#3
…est to include one with velocity 0. Importing all the messages now. adafruit#3
added 2 commits
April 2, 2019 14:33
…atement which is not quite what pylint thinks it is. adafruit#3
…ldren to be specified as a named (non-positional) arg. This is the variable that is ignored by some children. adafruit#3
tannewt
reviewed
Apr 2, 2019
|
@kevinjwalters I've narrowed down the cryptic Sphinx failure. I almost just pushed the change, but I think it deserves a larger conversation. I'm going to approve my current review, then open another one. |
sommersoft
approved these changes
Apr 3, 2019
sommersoft
requested changes
Apr 4, 2019
1 task
added 11 commits
April 5, 2019 15:10
…set with properties based on @tannewt feedback from adafruit#9. adafruit#3.
…property based on @tannewt feedback from adafruit#9. This changes MIDI.receive() to return just the object and MIDI.send() now has the side-effect of setting the channel on each message. Pushing CHANNELMASK into the parent MIDIMessage as it is no longer used as an indicator of presence/use of channel. MIDIBadEvent now includes the status number in data. Objects are now constructed on messages regardless of matching channel number as input channel test comes afterwards now. Lots of associated changes to unit tests plus a few doc updates. adafruit#3
…based on @tannewt advice. Updating all the examples to explicitly set midi_in or midi_out to the USB ports. Adding test case as constructor now raises an error if midi_in and midi_out are both omitted. This will make sphinx-build work but that appears to be a separate bug. adafruit#13
…e to a peculiar micropython-ism. Leaving test cases for now - these do not (currently) execute in CircuitPython. adafruit#3 adafruit#9
… and channel extraction from messages. adafruit#3 adafruit#9
…member reporting for gc.mem_free(). adafruit#3
sommersoft
approved these changes
Apr 7, 2019
tannewt
reviewed
Apr 8, 2019
adafruit_midi/midi_message.py
Outdated
| @@ -121,6 +121,24 @@ class MIDIMessage: | |||
| # order is more specific masks first | |||
| _statusandmask_to_class = [] | |||
|
|
|||
| def __init__(self, *, channel=None): | |||
| ### TODO - can i kwargs this????? | |||
There was a problem hiding this comment.
I was just pondering if I should/could iterate over the actual args passed and not set the value if it wasn't passed in, e.g. SystemExclusive does not set the value when it calls it. I don't think it's an important issue.
BTW, unrelated, I just threw in some defaults on velocity as it's useful on command line and I think it's 3 bytes per mpy.
(I thought Travis highlighted all the TODO left in the file but maybe I imagined that.)
added 2 commits
April 8, 2019 23:19
…pe() and upper case for exception from MIDIMessage.channel . adafruit#3 adafruit#9
tannewt
approved these changes
Apr 9, 2019
adafruit-adabot
added a commit
to adafruit/Adafruit_CircuitPython_Bundle
that referenced
this pull request
Apr 20, 2019
Updating https://github.com/adafruit/Adafruit_CircuitPython_MIDI to 1.1.0 from 1.0.1: > Merge pull request adafruit/Adafruit_CircuitPython_MIDI#9 from kevinjwalters/master
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've been developing this code and actively using it during that development process. It feels like it's ready to ship or at least get some wider use now.
Some of the coding is kept small as a compromise betweeen code volume and general techniques that would be used when memory is more abundant. I've been actively developing applications alongside this library. There's a balance between a flexible, well-written, high functionality library and being able to write an application that fits in the memory on an M0 board like the popular Circuit Playground Expresss.
Some areas to review:
adafruit_midi.pyhas become anadafruit_mididirectory.1 reduce code and therefore memory (useful for M0 boards),
2 force updates for at least one Adafruit Learn guide (Grand Central USB MIDI Controller in CircuitPython),
3 likely to impact a few users using the existing interface.
4 Remove MIDI constructor defaults for midi_in and midi_out #11 changes force minor updates to existing code so this may affect views on points 2 and 3 above.
as_bytes()currently returning mutable bytearray - there are some pros/cons here with returning immutablebytes().note_parser()takes "C4" as middle C (MIDI note 60) following https://en.wikipedia.org/wiki/Scientific_pitch_notation (Yamaha are the C3 rogues). This also matches the values in Adafruit Learn: Circuit Playground Sound and Music: The Sound of Musicmidi_simpletest.pynow has a partnermidi_simpletest2.pyshowing both send interfaces side by side.raisethem but they are not argument specific to keep those classes lean.There's a fair bit of potential for bugs in parsing. There's 42 unit tests covering a wide range of scenarios, many of them related to parsing to counter this.
I did originally have a look at Python libraries in case something was reusable. I had a bit of a noodle around the code for mido but it all looked too big for something targetting the M0 world. It might be interesting to be able to output the same text format representation it uses in the future but again code (-> memory) bloat will be a factor unless it can be made an optional
import.