-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[synthio] add a simple MidiTrack implementation #4447
Conversation
99b6c9e
to
3ad6e4d
Compare
This looks neat! I wonder whether we'll want to place it in its own separate module, like we did with audiomp3. Good find on space savings :) |
Isn't it too simple for a module of its own? |
It should definitely be in a module of it's own because 546 can be the difference to fitting things in. Why do this in C at all? Can't Python do it? The RTTTL library is very similar but in Python: https://github.com/adafruit/Adafruit_CircuitPython_RTTTL/ Perhaps there is a lower level API we need to add. I highly doubt we need to do any parsing in C though. |
The main difference is that
It would be nice to be able to implement AudioSample classes in Python, but I thought there was a conscious decision to disallow any form of concurrency in Python code? And |
Yup! True.
Ya, we don't really want to add concurrency because of all of it's pitfalls. I wonder if a better way to do this would be to have a |
Filed this issue: #4467 |
There you go :) Essentially the same code, but taking MIDI stream as input. |
51cd048
to
59875f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That's awesome. One question about the API. I think we'll wait to wait until after 6.2.0 to merge this. That will give us time to refine the API if we need to.
//| """Simple square-wave MIDI synth""" | ||
//| | ||
//| def __init__(self, buffer: ReadableBuffer, tempo: int, *, sample_rate: int = 11025) -> None: | ||
//| """Create a MidiTrack from the given stream of MIDI events. Only "Note On" and "Note Off" events |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about not taking a buffer
and having a write
method instead that queues data up? That way you can pipe midi from BLE or USB directly into this object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see your point, but:
- that would make looping (or even passing the same AudioSample to
play()
repeatedly) impossible; - the protocol for
audiosample_reset_buffer_fun
has no way to report failure if the AudioSample is not rewindable; - if the queued MIDI stream depletes during playback, the playback will stop, instead of waiting for new input. For real-time input, when the MIDI events arrive at the same speed that they're played, this would happen after every event!
Perhaps a new protocol, e.g. AudioStream
, should be defined alongside AudioSample
, and Audio*Out
implementations will need to be updated to handle the new kind of input -- e.g. a new method play_stream
which wouldn't take a loop
parameter. Then synthio
can have MidiStream
alongside MidiTrack
, with much of the implementation shared. Similarly, there can then be RawStream
alongside RawSample
.
What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are all really good points. I'd like to get @jepler's thoughts on it too since he has thought about MP3 streaming some.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tyomitch This all looks good. Would you mind putting your comment above in an issue so we can think about that for the future? I'll merge this now.
I haven't had time to look into that with any detail. (or test it) I think the main thing is putting it in the right place, module-wise. How to handle repeat=True I think we need to look at, but the time developing 7.0 may give us an opportunity -- and may end up changing how this works in the process. |
Sorry for forgetting to update the PR title; it no longer targets |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Looks like it needs a translation merge. Sorry about that! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update!
Looks like three boards are out of space. The simplest fix is to disable the module for those builds. |
region `FLASH_FIRMWARE' overflowed by 228 bytes
Done! #4583 took care of two boards; the third one had to have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome! Thank you for this!
for QBasic-compatible music strings
(https://en.wikibooks.org/wiki/QBasic/Appendix#PLAY)
The music parser is WIP and comments are welcome
before the input validation and missing features
are added; in particular, would microbit syntax
(https://microbit-micropython.readthedocs.io/en/v1.0.1/tutorials/music.html#wolfgang-amadeus-microbit)
be preferable?